refactor: flatten directory structure to single minichain package#19
refactor: flatten directory structure to single minichain package#19SIDDHANTCOOKIE wants to merge 5 commits intoStabilityNexus:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughConsolidates public exports into Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
SIDDHANTCOOKIE
force-pushed
the
feature/flatten-structure
branch
from
592e7a2 to
59b0093
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
main.py (1)
80-92:⚠️ Potential issue | 🟠 Major
stateandchain.stateare now two different, permanently diverged objects —claim_noncereads stale data.
Blockchain()(line 81) creates its own internalState()instance. The standalonestate = State()on line 80 is never mutated by any transaction or block addition;state.get_account(address)will always return a freshly-initialized account withnonce = 0.
claim_nonceonly produces correct nonces by coincidence:max(account_nonce=0, local_nonce)reduces topending_nonce_map[address], which is kept in sync withchain.stateviasync_nonce. Ifpending_nonce_maphas no entry for an address (first call before anysync_noncefor that address), the function returns0regardless of whatchain.stateactually holds for that address's nonce.The AI summary confirms this was previously
Blockchain(state)— the two objects were the same. That sharing was lost in this PR.The fix is to remove
stateand close overchaininstead:🐛 Proposed fix
async def node_loop(): logger.info("Starting MiniChain Node with Smart Contracts") - state = State() chain = Blockchain() mempool = Mempool() pending_nonce_map = {} def claim_nonce(address): - account = state.get_account(address) + account = chain.state.get_account(address) account_nonce = account.get("nonce", 0) if account else 0 local_nonce = pending_nonce_map.get(address, account_nonce) next_nonce = max(account_nonce, local_nonce) pending_nonce_map[address] = next_nonce + 1 return next_nonce🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main.py` around lines 80 - 92, The standalone State() instance is stale and claim_nonce reads from it; remove the unused state variable and have claim_nonce consult the blockchain's canonical state (chain.state) instead of state; update any uses of state.get_account(...) inside claim_nonce to use chain.state.get_account(...), ensure pending_nonce_map and sync_nonce continue to coordinate with chain.state, and delete the now-unused State() instantiation to avoid diverged state objects.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.gitignore:
- Around line 328-332: Update the Python ignore block in .gitignore to cover
common artifacts missing from the current entries (__pycache__/, *.py[cod],
*$py.class, *.so): add virtualenv folders (e.g., .venv/, venv/, env/),
build/dist metadata (build/, dist/, *.egg-info/, pip-wheel-metadata/), test and
cache outputs (.pytest_cache/, .mypy_cache/, .cache/, .eggs/), coverage and
report files (.coverage, htmlcov/), and packaging helpers (eggs, wheel caches);
modify the Python block around the existing patterns so these extra patterns are
included together for clearer maintenance.
In `@main.py`:
- Line 94: Replace the unused lambda parameter name to follow convention: change
the anonymous callback passed to P2PNetwork from "lambda x: None" to "lambda _:
None" so the unused argument is deliberately named as a discard and silences
ARG005; update the instantiation at the P2PNetwork call site accordingly.
---
Outside diff comments:
In `@main.py`:
- Around line 80-92: The standalone State() instance is stale and claim_nonce
reads from it; remove the unused state variable and have claim_nonce consult the
blockchain's canonical state (chain.state) instead of state; update any uses of
state.get_account(...) inside claim_nonce to use chain.state.get_account(...),
ensure pending_nonce_map and sync_nonce continue to coordinate with chain.state,
and delete the now-unused State() instantiation to avoid diverged state objects.
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.gitignore:
- Around line 327-332: The Python ignore block currently includes __pycache__/,
*.py[cod], *$py.class, and *.so but should be expanded to cover common
virtualenvs, build artifacts and test/coverage outputs; update the .gitignore by
adding patterns for virtual environments (.venv/, venv/, env/, ENV/), build/dist
and packaging metadata (build/, dist/, *.egg-info/, pip-wheel-metadata/),
testing and coverage caches (.pytest_cache/, .coverage, coverage.xml, htmlcov/),
and tool caches (mypy cache: .mypy_cache/, pip cache if needed) so files
produced by functions/tools referenced in the repo are not accidentally
committed. Ensure the new entries are placed near the existing Python block for
clarity and do not remove the current patterns (__pycache__/, *.py[cod],
*$py.class, *.so).
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@main.py`:
- Around line 85-91: The function claim_nonce lacks a return type annotation;
add an explicit return type of int to its signature (i.e., annotate claim_nonce
with "-> int") so the function declaration reflects that it always returns an
integer and satisfies Ruff ANN202; update the def line for claim_nonce
accordingly and run linters to confirm the warning is resolved.
- Around line 128-131: The finally block in main.py that follows await
_run_node(...) is a no-op and can leak resources opened by network.start();
implement P2PNetwork.stop() to perform async cleanup and then call await
network.stop() in that finally block (or if stop() may not exist yet, attempt a
guarded await network.stop() and fall back to cancelling/closing any network
attributes/tasks), ensuring network.start()/network.stop() are paired; update
the P2PNetwork class to provide an async stop method and wire it into the main
finally to always release connections/tasks on exceptions or KeyboardInterrupt.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@main.py`:
- Line 93: P2PNetwork is being constructed with a dummy lambda to satisfy a
callable check then immediately reassigned to the real handler, which defeats
validation; update P2PNetwork.__init__ to accept an optional handler_callback:
allow None as a valid initial value, store it (preferably as a private attribute
like self._handler_callback) and provide a setter or register_handler(method) to
validate callables when assigning later, then change the caller (where network =
P2PNetwork(lambda _: None)) to pass None or omit the argument and register the
real callback via the new setter (or property) instead of reassigning the public
attribute directly.
In `@minichain/p2p.py`:
- Around line 32-37: The stop() method currently drops the pubsub reference
(self.pubsub = None) without closing it; update stop() to gracefully shut down
any real pubsub: if self.pubsub exists, call its shutdown method (try common
names like close(), stop(), or async aclose()/shutdown() as applicable), await
it when it returns a coroutine, catch and log exceptions from that shutdown
call, then set self.pubsub = None; use the stop() function and self.pubsub
identifiers when modifying the code.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@minichain/p2p.py`:
- Around line 28-31: The register_handler method currently raises ValueError
when handler_callback is not callable; change that to raise TypeError instead to
follow stdlib conventions for wrong argument types. Locate register_handler in
the class (the method checking callable(handler_callback)), replace the raised
exception from ValueError("handler_callback must be callable") to TypeError with
the same or similar message, ensuring any callers/tests expecting ValueError are
updated if necessary.
- Around line 53-54: In the except block that catches exceptions during pubsub
shutdown (the block logging "Network: Error shutting down pubsub"), replace the
call to logger.error(...) with logger.exception(...) so the full traceback is
captured; keep the existing message text and exception variable/context but use
logger.exception in the same except Exception as e handler (within the pubsub
shutdown logic) to include stack traces for debugging.
- Around line 113-114: The handler registration currently allows any callable
but handle_message unconditionally awaits self._handler_callback which causes a
TypeError for sync functions; update register_handler to validate the callable
is a coroutine function (use inspect.iscoroutinefunction or
asyncio.iscoroutinefunction) and raise a clear TypeError/ValueError if not,
ensuring only async handlers are accepted; update the register_handler
signature/type hints to reflect coroutine-only callbacks and keep handle_message
unchanged to safely await _handler_callback.
- Around line 49-52: Move the local import out of the stop() body by adding
"import asyncio" (and "import inspect") at the top of the module, and replace
the asyncio.iscoroutine check with inspect.isawaitable for the shutdown result:
call shutdown_meth(), assign to res, and if inspect.isawaitable(res) then await
res; remove the in-function "import asyncio" and ensure you reference
shutdown_meth and stop() exactly as in the diff.
| def register_handler(self, handler_callback): | ||
| if not callable(handler_callback): | ||
| raise ValueError("handler_callback must be callable") | ||
| self.handler_callback = handler_callback | ||
| self.pubsub = None # Will be set in real implementation | ||
| self._handler_callback = handler_callback |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
ValueError should be TypeError for a callable-type guard.
Raising ValueError for a wrong-argument-type is semantically incorrect; TypeError is the stdlib convention (e.g., threading.Thread(target=42) raises TypeError).
♻️ Proposed fix
def register_handler(self, handler_callback):
- if not callable(handler_callback):
- raise ValueError("handler_callback must be callable")
+ if not callable(handler_callback):
+ raise TypeError("handler_callback must be callable")
self._handler_callback = handler_callback🧰 Tools
🪛 Ruff (0.15.1)
[warning] 30-30: Prefer TypeError exception for invalid type
(TRY004)
[warning] 30-30: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@minichain/p2p.py` around lines 28 - 31, The register_handler method currently
raises ValueError when handler_callback is not callable; change that to raise
TypeError instead to follow stdlib conventions for wrong argument types. Locate
register_handler in the class (the method checking callable(handler_callback)),
replace the raised exception from ValueError("handler_callback must be
callable") to TypeError with the same or similar message, ensuring any
callers/tests expecting ValueError are updated if necessary.
| import asyncio | ||
| res = shutdown_meth() | ||
| if asyncio.iscoroutine(res): | ||
| await res |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Move import asyncio to the top of the file and prefer inspect.isawaitable over asyncio.iscoroutine.
Two issues:
- Importing inside the method body is non-idiomatic and runs on every
stop()call;asynciois a stdlib module with no circular-import risk. asyncio.iscoroutine(res)only matches coroutine objects; it missesasyncio.Future,asyncio.Task, and any custom__await__implementor.inspect.isawaitable(res)is the correct, broader guard.
♻️ Proposed fix
At the top of the file:
import json
import logging
+import inspectIn stop():
- if shutdown_meth:
- import asyncio
- res = shutdown_meth()
- if asyncio.iscoroutine(res):
- await res
+ if shutdown_meth:
+ res = shutdown_meth()
+ if inspect.isawaitable(res):
+ await res🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@minichain/p2p.py` around lines 49 - 52, Move the local import out of the
stop() body by adding "import asyncio" (and "import inspect") at the top of the
module, and replace the asyncio.iscoroutine check with inspect.isawaitable for
the shutdown result: call shutdown_meth(), assign to res, and if
inspect.isawaitable(res) then await res; remove the in-function "import asyncio"
and ensure you reference shutdown_meth and stop() exactly as in the diff.
| except Exception as e: | ||
| logger.error("Network: Error shutting down pubsub: %s", e) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use logger.exception instead of logger.error to capture the full traceback.
logger.error logs only the formatted message string; logger.exception automatically appends the current exception's traceback, which is far more useful when diagnosing unexpected shutdown failures. (Ruff TRY400)
♻️ Proposed fix
- except Exception as e:
- logger.error("Network: Error shutting down pubsub: %s", e)
+ except Exception:
+ logger.exception("Network: Error shutting down pubsub")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| except Exception as e: | |
| logger.error("Network: Error shutting down pubsub: %s", e) | |
| try: | |
| await self.pubsub.close() | |
| except Exception: | |
| logger.exception("Network: Error shutting down pubsub") | |
| finally: | |
| self.pubsub = None |
🧰 Tools
🪛 Ruff (0.15.1)
[warning] 53-53: Do not catch blind exception: Exception
(BLE001)
[warning] 54-54: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@minichain/p2p.py` around lines 53 - 54, In the except block that catches
exceptions during pubsub shutdown (the block logging "Network: Error shutting
down pubsub"), replace the call to logger.error(...) with logger.exception(...)
so the full traceback is captured; keep the existing message text and exception
variable/context but use logger.exception in the same except Exception as e
handler (within the pubsub shutdown logic) to include stack traces for
debugging.
| if self._handler_callback: | ||
| await self._handler_callback(data) |
There was a problem hiding this comment.
await self._handler_callback(data) silently fails for sync callbacks.
register_handler accepts any callable, but handle_message unconditionally awaits the result. A plain (non-async) callable registered as the handler will cause TypeError: object ... can't be used in 'await' expression, which is silently swallowed by the outer except Exception block with a misleading log message. Enforce that only coroutine functions can be registered.
🛡️ Proposed fix in register_handler
+import inspect
def register_handler(self, handler_callback):
- if not callable(handler_callback):
- raise TypeError("handler_callback must be callable")
+ if not inspect.iscoroutinefunction(handler_callback):
+ raise TypeError("handler_callback must be an async (coroutine) function")
self._handler_callback = handler_callback🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@minichain/p2p.py` around lines 113 - 114, The handler registration currently
allows any callable but handle_message unconditionally awaits
self._handler_callback which causes a TypeError for sync functions; update
register_handler to validate the callable is a coroutine function (use
inspect.iscoroutinefunction or asyncio.iscoroutinefunction) and raise a clear
TypeError/ValueError if not, ensuring only async handlers are accepted; update
the register_handler signature/type hints to reflect coroutine-only callbacks
and keep handle_message unchanged to safely await _handler_callback.
1 participant
Description
Hey @Zahnentferner
This PR is a direct follow-up to the recent discussions on discord and addresses the feedback regarding the project's folder structure:
To improve minimalism and make the codebase easier for new developers to explore, this PR flattens the architecture by consolidating the single-file packages into a single cohesive
minichaindirectory.Changes Made:
consensus/,core/, [network/] and [node/] into a singleminichain/directory.tests/directory to import directly from the unifiedminichainpackage.Addressed Issues:
Resolves maintainer request on discord to flatten the directory structure.
Screenshots/Recordings:
Additional Notes:
Checklist
AI Usage Disclosure
Check one of the checkboxes below:
I have used the following AI models and tools: N/A
Summary by CodeRabbit
Refactor
Tests
Chores