fix: address CodeRabbit comments for pr #46#54
Open
SIDDHANTCOOKIE wants to merge 3 commits intoStabilityNexus:mainfrom
Open
fix: address CodeRabbit comments for pr #46#54SIDDHANTCOOKIE wants to merge 3 commits intoStabilityNexus:mainfrom
SIDDHANTCOOKIE wants to merge 3 commits intoStabilityNexus:mainfrom
Conversation
Contributor
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 34-35: TRUSTED_PEERS is defined as an empty set so the trust check
using peer_addr in TRUSTED_PEERS will always fail; either populate it from
configuration or document intent. Fix by adding configuration parsing to
populate TRUSTED_PEERS (e.g., read a comma-separated env var like TRUSTED_PEERS
or add an argparse option --trusted-peer/--trusted-peers and split into a set)
and ensure LOCALHOST_PEERS remains separate; update the trust check logic that
references peer_addr to consult the populated TRUSTED_PEERS set. If emptiness is
intentional for a demo, instead add a clear comment above the TRUSTED_PEERS
declaration stating that behavior.
- Around line 94-99: The code's peer_host extracted from peer_addr (used in
is_trusted/is_localhost checks) can include IPv6 brackets (e.g., "[::1]") so
comparisons against TRUSTED_PEERS/LOCALHOST_PEERS fail; update the logic around
peer_addr/peer_host (used where peer_host is derived and in the
is_trusted/is_localhost checks inside the block guarded by chain.state.accounts)
to strip surrounding "[" and "]" if present (e.g., if peer_host.startswith("[")
and peer_host.endswith("]") then peer_host = peer_host[1:-1]) before membership
tests and before logging the rejected peer via logger.warning so bracketed IPv6
peers are recognized as localhost/trusted.
In `@minichain/p2p.py`:
- Around line 162-170: The except block that swallows exceptions during writer
cleanup should log the exception at DEBUG to aid troubleshooting; update the
except Exception: handler around writer.close() / await writer.wait_closed() to
call the module or instance logger (e.g., self.logger.debug(...) or
logging.getLogger(__name__).debug(...)) and include the exception information
and context (reader, writer, or peer identifier) without changing the existing
removal behavior of pair from self._peers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b02a162c-3021-492c-8796-749e3df5518a
📒 Files selected for processing (2)
main.pyminichain/p2p.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 98-100: The check rejecting syncs when chain.state.accounts is
truthy blocks bootstrap for nodes pre-funded by run_node(); either defer the
initial funding in run_node() until after peer bootstrap completes, or relax the
sync guard in the peer handling code to allow bootstrapping when the only
existing account is the node's self-funded local wallet; locate the funding call
in run_node() and move it to run after the bootstrap routine, or update the
conditional around logger.warning("🔒 Rejected sync from untrusted peer %s",
peer_addr) to treat the case where chain.state.accounts contains only the local
wallet (or when a --fund flag is present) as bootstrappable so remote seeds
aren’t rejected.
- Line 157: Help text uses the incorrect placeholder "connect <host:port>" —
update the help string to match the CLI format used elsewhere by changing the
placeholder to "connect <host>:<port>" (locate the help/menu string in main.py
where the line contains "connect <host:port>" and replace it so the documented
syntax matches the actual command format).
- Around line 103-108: The payload["accounts"] handling is unsafe: validate that
payload is a dict and that payload.get("accounts") returns a mapping before
iterating; ensure each account value (acc) is a dict before mutating
chain.state.accounts or calling acc.get. In the block using remote_accounts,
first coerce or reject non-dict accounts (skip entries where not isinstance(acc,
dict) or addr not a str), accumulate only valid entries, then update
chain.state.accounts with those valid entries and use logger.info to report
synced accounts and optionally warn about skipped/malformed entries; reference
the variables and methods remote_accounts, payload, chain.state.accounts, addr,
acc, and logger.info when making the changes.
Contributor
There was a problem hiding this comment.
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)
306-311: 🧹 Nitpick | 🔵 TrivialConsider renaming to avoid shadowing the
hostparameter.Line 308 shadows the
hostparameter with a local variable. While not a bug (the original value was used at line 303), this reduces readability.♻️ Suggested refactor for clarity
if connect_to: try: - host, peer_port = connect_to.rsplit(":", 1) - await network.connect_to_peer(host, int(peer_port)) + peer_host, peer_port = connect_to.rsplit(":", 1) + await network.connect_to_peer(peer_host, int(peer_port)) except ValueError: logger.error("Invalid --connect format. Use host:port")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main.py` around lines 306 - 311, The local variable created from connect_to.rsplit(":", 1) shadows the outer parameter named host; rename that local variable (e.g., to peer_host or remote_host) and update its use in the network.connect_to_peer call so you don't reuse the parameter name — adjust the try block where connect_to is parsed and the await network.connect_to_peer(host, int(peer_port)) invocation accordingly (leave the outer host parameter unchanged).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@main.py`:
- Around line 306-311: The local variable created from connect_to.rsplit(":", 1)
shadows the outer parameter named host; rename that local variable (e.g., to
peer_host or remote_host) and update its use in the network.connect_to_peer call
so you don't reuse the parameter name — adjust the try block where connect_to is
parsed and the await network.connect_to_peer(host, int(peer_port)) invocation
accordingly (leave the outer host parameter unchanged).
SIDDHANTCOOKIE
changed the title
SIDDHANTCOOKIE
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addressed Issues:
This PR applies the CodeRabbit-requested fixes in
main.pyandminichain/p2p.pywith minimal, targeted changes.Screenshots/Recordings:
Additional Notes:
connectcommand to display success/failure message based onconnect_to_peer(...)return value.P2PNetwork.set_on_peer_connected(callback)network._on_peer_connected = ...to setterP2PNetwork.start(..., host="127.0.0.1")--hostCLI arg throughmain.pyto allow explicit external binding_listen_tasksbefore clearingStreamWriters, awaitwait_closed(), then remove peer tuplesChecklist
AI Usage Disclosure
Check one of the checkboxes below:
I have used the following AI models and tools: N/A
We encourage contributors to use AI tools responsibly when creating Pull Requests. While AI can be a valuable aid, it is essential to ensure that your contributions meet the task requirements, build successfully, include relevant tests, and pass all linters. Submissions that do not meet these standards may be closed without warning to maintain the quality and integrity of the project. Please take the time to understand the changes you are proposing and their impact. AI slop is strongly discouraged and may lead to banning and blocking. Do not spam our repos with AI slop.
Summary by CodeRabbit
New Features
Bug Fixes
Chores