Feat/contract transfers#101
Open
SIDDHANTCOOKIE wants to merge 4 commits into
Open
Conversation
Contributor
WalkthroughThe PR introduces contract ChangesBlockchain node feature expansion
Sequence Diagram(s)sequenceDiagram
participant Client
participant JSONRPCServer
participant ExternalPeer
participant P2PNetwork
participant make_network_handler
participant Blockchain
participant Mempool
rect rgba(100, 149, 237, 0.5)
Note over ExternalPeer,Mempool: Chain sync via P2P
ExternalPeer->>P2PNetwork: block (index ahead of local tip)
P2PNetwork->>make_network_handler: dispatch block msg
make_network_handler->>P2PNetwork: broadcast_chain_request()
P2PNetwork->>ExternalPeer: chain_request {}
ExternalPeer->>P2PNetwork: chain_response {blocks: [...]}
P2PNetwork->>make_network_handler: dispatch chain_response
make_network_handler->>Blockchain: resolve_conflicts(new_chain_list)
Blockchain-->>make_network_handler: (True, orphans)
make_network_handler->>Mempool: add each orphan tx
end
rect rgba(60, 179, 113, 0.5)
Note over Client,P2PNetwork: RPC transaction submission
Client->>JSONRPCServer: POST / mc_sendTransaction(payload)
JSONRPCServer->>Mempool: add_transaction(tx)
JSONRPCServer->>P2PNetwork: broadcast_transaction(tx) [async task]
JSONRPCServer-->>Client: {result: tx_id}
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_rpc.py (1)
27-63: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd a regression test for non-object JSON-RPC requests.
Please add a case like
json=1(or batch entry with a primitive) and assert{"error":{"code":-32600}}. This protects the invalid-request path from returning 500s.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_rpc.py` around lines 27 - 63, Add a new test function following the same pattern as the existing test functions (test_rpc_blockNumber, test_rpc_getBlockByNumber, test_rpc_invalid_method) that sends a non-object JSON-RPC request to verify error handling for invalid request formats. The test should send a primitive value like the integer 1 as the JSON payload using json=1 in the post request, assert that the response status is 200, and verify that the response contains an error object with error code -32600 (Invalid Request) to ensure the invalid-request path does not return 500 errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@main.py`:
- Around line 440-441: The RPC server startup uses asyncio.create_task which can
hide failures and does not await the actual startup. Replace the create_task
call that assigns to rpc_task with an await of rpc_server.start() directly, and
store only the server reference if needed. At shutdown (around line 469 where
the task is currently canceled), replace the task cancellation with an explicit
await rpc_server.stop() call to reliably stop the RPC server instead of just
canceling the task.
- Around line 169-174: The chain response is being broadcasted to all peers via
network._broadcast_raw() when it should only be sent to the specific peer that
requested it. Replace the asyncio.create_task(network._broadcast_raw(payload))
call with a unicast method that sends the chain_response payload only to the
requesting peer connection. You will need to capture the requesting peer's
connection identifier from the message context and use an appropriate network
method to send the response back to only that peer rather than broadcasting to
all connected peers.
In `@minichain/chain.py`:
- Around line 188-190: Remove the unused variables state_snapshot and
original_chain from the reorg validation logic. These variables are captured at
the beginning but never actually used for rollback purposes since all validation
operations work with temp_state instead, and self.state is only replaced upon
successful reorg completion. Simply delete the lines that assign to
state_snapshot and original_chain to clean up unnecessary variable assignments.
In `@minichain/contract.py`:
- Around line 48-58: The address validation in the transfer_out function only
checks if the address is a string type but does not validate the actual format
of the address. Add additional validation to ensure the address has a valid
format by checking for non-empty strings and any other format requirements (such
as proper hex character format or minimum length). This validation should occur
after the type check for the address parameter and before appending the transfer
to the transfers list.
In `@minichain/mempool.py`:
- Around line 65-68: The comment in the get_transactions_for_block method
incorrectly states that the retrieval is O(1), but the list slicing operation
that creates a new list from self._list[:self.transactions_per_block] is
actually O(k) where k equals transactions_per_block. Update the comment to
accurately reflect that this is an O(k) operation, while noting it remains
efficient compared to alternative approaches.
In `@minichain/p2p.py`:
- Around line 410-413: The send_chain_response method currently serializes all
blocks from blocks_dicts into a single unbounded payload, which can create
oversized sync frames and degrade performance as the chain grows. Refactor this
method to implement chunked or paginated responses by defining explicit maximum
limits (either by block count or payload size in bytes), and send multiple
chain_response messages if the total blocks exceed the limit. Process
blocks_dicts in batches, serializing and sending each batch separately to ensure
each payload stays within the defined maximum bounds.
In `@minichain/rpc.py`:
- Around line 17-23: The `start` method creates `runner` (AppRunner) and `site`
(TCPSite) as local variables that go out of scope when the method returns,
making it impossible to cleanly shutdown the server later. Store these objects
as instance attributes by assigning them to `self.runner` and `self.site`
respectively, then create a new async method (e.g., `stop` or `shutdown`) that
properly cleans up these resources by calling their respective cleanup methods
to ensure proper lifecycle management and prevent resource leaks.
- Around line 40-41: The error response on line 41 calls `req.get("id")` without
verifying that `req` is a dict, but the condition on line 40 can be true when
`req` is not a dict (first part of the OR condition), causing an AttributeError
when non-dict objects don't have a get method. Fix this by checking if req is a
dict before calling `.get("id")`, otherwise use None as the id value in the
error response to safely handle non-dict request objects.
- Line 4: The aiohttp module is imported at the top of the file (from aiohttp
import web) and used throughout the code (web.Application, web.AppRunner,
web.TCPSite, web.post), but aiohttp is not declared as a dependency in the
project's requirements.txt file. Add aiohttp to the requirements.txt file to
ensure the dependency is properly declared and installed when the project is set
up, preventing ImportError at runtime.
In `@tests/test_contract_transfers.py`:
- Line 34: Update the comment that states "plus 10 fee" to reflect the actual
fee value of 1000 used in the transaction. Change the comment text from "Sender
sent 100 to contract, plus 10 fee" to "Sender sent 100 to contract, plus 1000
fee" to match the fee=1000 parameter in the actual code.
In `@tests/test_reorg.py`:
- Line 2: The import statement for os is duplicated in the test_reorg.py file,
appearing on both line 2 and line 11. Remove one of these duplicate import os
statements, keeping only a single import os declaration at the top of the file
to avoid redundancy and maintain clean import organization.
---
Outside diff comments:
In `@tests/test_rpc.py`:
- Around line 27-63: Add a new test function following the same pattern as the
existing test functions (test_rpc_blockNumber, test_rpc_getBlockByNumber,
test_rpc_invalid_method) that sends a non-object JSON-RPC request to verify
error handling for invalid request formats. The test should send a primitive
value like the integer 1 as the JSON payload using json=1 in the post request,
assert that the response status is 200, and verify that the response contains an
error object with error code -32600 (Invalid Request) to ensure the
invalid-request path does not return 500 errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ba4720fa-9556-48f2-9038-4968a5cbe31e
📒 Files selected for processing (10)
main.pyminichain/chain.pyminichain/contract.pyminichain/mempool.pyminichain/p2p.pyminichain/rpc.pyminichain/state.pytests/test_contract_transfers.pytests/test_reorg.pytests/test_rpc.py
Comment on lines
+169
to
+174
| elif msg_type == "chain_request": | ||
| logger.info("📡 Peer requested chain sync. Broadcasting our chain...") | ||
| blocks_dicts = [b.to_dict() for b in chain.chain] | ||
| payload = {"type": "chain_response", "data": {"blocks": blocks_dicts}} | ||
| asyncio.create_task(network._broadcast_raw(payload)) | ||
|
|
Contributor
There was a problem hiding this comment.
Avoid broadcasting chain_response to all peers for a single chain_request.
At Line 173, one requester triggers a full-chain broadcast to every peer, which creates amplification and unnecessary reorg parsing load. This should be a unicast reply to the requesting connection.
🧰 Tools
🪛 Ruff (0.15.17)
[warning] 173-173: Store a reference to the return value of asyncio.create_task
(RUF006)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@main.py` around lines 169 - 174, The chain response is being broadcasted to
all peers via network._broadcast_raw() when it should only be sent to the
specific peer that requested it. Replace the
asyncio.create_task(network._broadcast_raw(payload)) call with a unicast method
that sends the chain_response payload only to the requesting peer connection.
You will need to capture the requesting peer's connection identifier from the
message context and use an appropriate network method to send the response back
to only that peer rather than broadcasting to all connected peers.
Comment on lines
+440
to
+441
| rpc_port = 8545 + (port - 9000) | ||
| rpc_task = asyncio.create_task(rpc_server.start(host="127.0.0.1", port=rpc_port)) |
Contributor
There was a problem hiding this comment.
Do not fire-and-forget RPC startup; await startup and stop explicitly.
At Line 441, create_task can hide startup/bind failures. At Line 469, canceling that task is not a reliable server shutdown mechanism. Start the RPC server with await and call an explicit await rpc_server.stop() during shutdown.
Also applies to: 468-469
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@main.py` around lines 440 - 441, The RPC server startup uses
asyncio.create_task which can hide failures and does not await the actual
startup. Replace the create_task call that assigns to rpc_task with an await of
rpc_server.start() directly, and store only the server reference if needed. At
shutdown (around line 469 where the task is currently canceled), replace the
task cancellation with an explicit await rpc_server.stop() call to reliably stop
the RPC server instead of just canceling the task.
Comment on lines
+188
to
+190
| # 2. Snapshot current state and chain in case reorg fails validation | ||
| state_snapshot = self.state.snapshot() | ||
| original_chain = list(self.chain) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Remove unused snapshot variables.
state_snapshot is captured but never used for rollback since all validation operates on temp_state. The original state is preserved naturally because self.state is only replaced upon successful reorg at line 234.
🧹 Suggested cleanup
logger.info("Incoming chain is heavier (%s > %s). Attempting reorg...", new_work, current_work)
- # 2. Snapshot current state and chain in case reorg fails validation
- state_snapshot = self.state.snapshot()
+ # 2. Preserve reference to original chain for orphan calculation
original_chain = list(self.chain)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@minichain/chain.py` around lines 188 - 190, Remove the unused variables
state_snapshot and original_chain from the reorg validation logic. These
variables are captured at the beginning but never actually used for rollback
purposes since all validation operations work with temp_state instead, and
self.state is only replaced upon successful reorg completion. Simply delete the
lines that assign to state_snapshot and original_chain to clean up unnecessary
variable assignments.
Comment on lines
+48
to
+58
| transfers = [] | ||
|
|
||
| def transfer_out(address, amount): | ||
| if not isinstance(amount, int) or amount <= 0: | ||
| raise ValueError("Invalid transfer amount") | ||
| if not isinstance(address, str): | ||
| raise ValueError("Invalid address type") | ||
| transfers.append({"to": address, "amount": amount}) | ||
|
|
||
| globals_dict["__builtins__"]["transfer_out"] = transfer_out | ||
|
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Consider validating address format in transfer_out.
The address validation only checks for str type but does not validate the format (e.g., length, hex characters). This allows contracts to transfer to invalid or malformed addresses like empty strings. While State.get_account() will create accounts for any string, this could lead to funds being sent to unretrievable addresses.
🛡️ Suggested validation
def transfer_out(address, amount):
if not isinstance(amount, int) or amount <= 0:
raise ValueError("Invalid transfer amount")
if not isinstance(address, str):
- raise ValueError("Invalid address type")
+ raise TypeError("Invalid address type")
+ if len(address) < 40 or not all(c in '0123456789abcdefABCDEF' for c in address):
+ raise ValueError("Invalid address format")
transfers.append({"to": address, "amount": amount})📝 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.
Suggested change
| transfers = [] | |
| def transfer_out(address, amount): | |
| if not isinstance(amount, int) or amount <= 0: | |
| raise ValueError("Invalid transfer amount") | |
| if not isinstance(address, str): | |
| raise ValueError("Invalid address type") | |
| transfers.append({"to": address, "amount": amount}) | |
| globals_dict["__builtins__"]["transfer_out"] = transfer_out | |
| transfers = [] | |
| def transfer_out(address, amount): | |
| if not isinstance(amount, int) or amount <= 0: | |
| raise ValueError("Invalid transfer amount") | |
| if not isinstance(address, str): | |
| raise TypeError("Invalid address type") | |
| if len(address) < 40 or not all(c in '0123456789abcdefABCDEF' for c in address): | |
| raise ValueError("Invalid address format") | |
| transfers.append({"to": address, "amount": amount}) | |
| globals_dict["__builtins__"]["transfer_out"] = transfer_out |
🧰 Tools
🪛 Ruff (0.15.17)
[warning] 50-50: Missing return type annotation for private function transfer_out
Add return type annotation: None
(ANN202)
[warning] 52-52: Abstract raise to an inner function
(TRY301)
[warning] 52-52: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 54-54: Abstract raise to an inner function
(TRY301)
[warning] 54-54: Prefer TypeError exception for invalid type
(TRY004)
[warning] 54-54: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@minichain/contract.py` around lines 48 - 58, The address validation in the
transfer_out function only checks if the address is a string type but does not
validate the actual format of the address. Add additional validation to ensure
the address has a valid format by checking for non-empty strings and any other
format requirements (such as proper hex character format or minimum length).
This validation should occur after the type check for the address parameter and
before appending the transfer to the transfers list.
Comment on lines
+65
to
+68
| def get_transactions_for_block(self): | ||
| with self._lock: | ||
| # O(1) retrieval! The list is strictly ordered upon insertion. | ||
| return list(self._list[:self.transactions_per_block]) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Comment inaccuracy: slice is O(k), not O(1).
The retrieval is O(k) where k = transactions_per_block, not strictly O(1). This is still efficient and much better than the prior implementation, but the comment is slightly misleading.
📝 Suggested fix
def get_transactions_for_block(self):
with self._lock:
- # O(1) retrieval! The list is strictly ordered upon insertion.
+ # O(k) retrieval where k = transactions_per_block. The list is strictly ordered upon insertion.
return list(self._list[:self.transactions_per_block])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@minichain/mempool.py` around lines 65 - 68, The comment in the
get_transactions_for_block method incorrectly states that the retrieval is O(1),
but the list slicing operation that creates a new list from
self._list[:self.transactions_per_block] is actually O(k) where k equals
transactions_per_block. Update the comment to accurately reflect that this is an
O(k) operation, while noting it remains efficient compared to alternative
approaches.
| import logging | ||
| import json | ||
| import asyncio | ||
| from aiohttp import web |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Read-only verification: locate dependency manifests and check whether aiohttp is declared.
set -euo pipefail
echo "== Candidate manifest files =="
fd -i 'pyproject.toml|requirements.*|setup.py|Pipfile|Pipfile.lock|poetry.lock' -t f
echo
echo "== Occurrences of aiohttp in manifests =="
rg -n --iglob 'pyproject.toml' --iglob 'requirements*' --iglob 'setup.py' --iglob 'Pipfile*' --iglob 'poetry.lock' '\baiohttp\b|install_requires|dependencies'
echo
echo "== aiohttp imports in code =="
rg -n '\bfrom aiohttp import\b|\bimport aiohttp\b'Repository: StabilityNexus/MiniChain
Length of output: 159
🏁 Script executed:
cat requirements.txtRepository: StabilityNexus/MiniChain
Length of output: 94
🏁 Script executed:
head -20 minichain/rpc.pyRepository: StabilityNexus/MiniChain
Length of output: 651
🏁 Script executed:
fd -type f -name '*.txt' -o -name '*.toml' -o -name 'setup.py' -o -name 'setup.cfg' | head -20Repository: StabilityNexus/MiniChain
Length of output: 239
🏁 Script executed:
find . -maxdepth 2 -type f \( -name 'setup.py' -o -name 'setup.cfg' -o -name 'pyproject.toml' \)Repository: StabilityNexus/MiniChain
Length of output: 50
Add aiohttp to requirements.txt
The code imports aiohttp at line 4 and uses it throughout the file (web.Application, web.AppRunner, web.TCPSite, web.post), but the dependency is not declared in requirements.txt. This will cause ImportError at runtime if aiohttp is not separately installed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@minichain/rpc.py` at line 4, The aiohttp module is imported at the top of the
file (from aiohttp import web) and used throughout the code (web.Application,
web.AppRunner, web.TCPSite, web.post), but aiohttp is not declared as a
dependency in the project's requirements.txt file. Add aiohttp to the
requirements.txt file to ensure the dependency is properly declared and
installed when the project is set up, preventing ImportError at runtime.
Comment on lines
+17
to
+23
| async def start(self, host="127.0.0.1", port=8545): | ||
| runner = web.AppRunner(self.app) | ||
| await runner.setup() | ||
| site = web.TCPSite(runner, host, port) | ||
| await site.start() | ||
| logger.info("🚀 JSON-RPC Server running on http://%s:%d", host, port) | ||
|
|
Contributor
There was a problem hiding this comment.
Persist RPC runner/site and add explicit shutdown API.
At Lines 17-23, AppRunner/TCPSite are local-only, so the server has no reliable stop path. This leaks lifecycle control and makes shutdown coordination fragile.
Suggested fix
class JSONRPCServer:
def __init__(self, chain, mempool, network):
self.chain = chain
self.mempool = mempool
self.network = network
self.app = web.Application()
self.app.add_routes([web.post('/', self.handle_rpc)])
+ self._runner = None
+ self._site = None
async def start(self, host="127.0.0.1", port=8545):
- runner = web.AppRunner(self.app)
- await runner.setup()
- site = web.TCPSite(runner, host, port)
- await site.start()
+ self._runner = web.AppRunner(self.app)
+ await self._runner.setup()
+ self._site = web.TCPSite(self._runner, host, port)
+ await self._site.start()
logger.info("🚀 JSON-RPC Server running on http://%s:%d", host, port)
+
+ async def stop(self):
+ if self._runner is not None:
+ await self._runner.cleanup()
+ self._runner = None
+ self._site = None📝 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.
Suggested change
| async def start(self, host="127.0.0.1", port=8545): | |
| runner = web.AppRunner(self.app) | |
| await runner.setup() | |
| site = web.TCPSite(runner, host, port) | |
| await site.start() | |
| logger.info("🚀 JSON-RPC Server running on http://%s:%d", host, port) | |
| class JSONRPCServer: | |
| def __init__(self, chain, mempool, network): | |
| self.chain = chain | |
| self.mempool = mempool | |
| self.network = network | |
| self.app = web.Application() | |
| self.app.add_routes([web.post('/', self.handle_rpc)]) | |
| self._runner = None | |
| self._site = None | |
| async def start(self, host="127.0.0.1", port=8545): | |
| self._runner = web.AppRunner(self.app) | |
| await self._runner.setup() | |
| self._site = web.TCPSite(self._runner, host, port) | |
| await self._site.start() | |
| logger.info("🚀 JSON-RPC Server running on http://%s:%d", host, port) | |
| async def stop(self): | |
| if self._runner is not None: | |
| await self._runner.cleanup() | |
| self._runner = None | |
| self._site = None |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@minichain/rpc.py` around lines 17 - 23, The `start` method creates `runner`
(AppRunner) and `site` (TCPSite) as local variables that go out of scope when
the method returns, making it impossible to cleanly shutdown the server later.
Store these objects as instance attributes by assigning them to `self.runner`
and `self.site` respectively, then create a new async method (e.g., `stop` or
`shutdown`) that properly cleans up these resources by calling their respective
cleanup methods to ensure proper lifecycle management and prevent resource
leaks.
Comment on lines
+40
to
+41
| if not isinstance(req, dict) or "method" not in req or req.get("jsonrpc") != "2.0": | ||
| return {"jsonrpc": "2.0", "error": {"code": -32600, "message": "Invalid Request"}, "id": req.get("id")} |
Contributor
There was a problem hiding this comment.
Fix invalid-request handling crash for non-dict JSON-RPC entries.
At Line 41, req.get("id") is executed even when req is not a dict, which raises before your error response path and returns 500 instead of -32600.
Suggested fix
async def _process_single(self, req):
- if not isinstance(req, dict) or "method" not in req or req.get("jsonrpc") != "2.0":
- return {"jsonrpc": "2.0", "error": {"code": -32600, "message": "Invalid Request"}, "id": req.get("id")}
+ req_id = req.get("id") if isinstance(req, dict) else None
+ if not isinstance(req, dict) or "method" not in req or req.get("jsonrpc") != "2.0":
+ return {"jsonrpc": "2.0", "error": {"code": -32600, "message": "Invalid Request"}, "id": req_id}📝 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.
Suggested change
| if not isinstance(req, dict) or "method" not in req or req.get("jsonrpc") != "2.0": | |
| return {"jsonrpc": "2.0", "error": {"code": -32600, "message": "Invalid Request"}, "id": req.get("id")} | |
| req_id = req.get("id") if isinstance(req, dict) else None | |
| if not isinstance(req, dict) or "method" not in req or req.get("jsonrpc") != "2.0": | |
| return {"jsonrpc": "2.0", "error": {"code": -32600, "message": "Invalid Request"}, "id": req_id} |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@minichain/rpc.py` around lines 40 - 41, The error response on line 41 calls
`req.get("id")` without verifying that `req` is a dict, but the condition on
line 40 can be true when `req` is not a dict (first part of the OR condition),
causing an AttributeError when non-dict objects don't have a get method. Fix
this by checking if req is a dict before calling `.get("id")`, otherwise use
None as the id value in the error response to safely handle non-dict request
objects.
| self.assertEqual(receipt.status, 1) | ||
| contract_addr = receipt.contract_address | ||
|
|
||
| # Sender sent 100 to contract, plus 10 fee |
Contributor
There was a problem hiding this comment.
Minor comment typo: fee is 1000, not 10.
The comment says "plus 10 fee" but the transaction uses fee=1000.
📝 Suggested fix
- # Sender sent 100 to contract, plus 10 fee
+ # Sender sent 100 to contract (fee=1000 is separate)📝 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.
Suggested change
| # Sender sent 100 to contract, plus 10 fee | |
| # Sender sent 100 to contract (fee=1000 is separate) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_contract_transfers.py` at line 34, Update the comment that states
"plus 10 fee" to reflect the actual fee value of 1000 used in the transaction.
Change the comment text from "Sender sent 100 to contract, plus 10 fee" to
"Sender sent 100 to contract, plus 1000 fee" to match the fee=1000 parameter in
the actual code.
| @@ -0,0 +1,117 @@ | |||
| import pytest | |||
| import os | |||
Contributor
There was a problem hiding this comment.
Remove duplicate import os.
os is imported on both line 2 and line 11.
🧹 Suggested fix
import pytest
import os
import json
import time
from minichain.chain import Blockchain
from minichain.transaction import Transaction
from minichain.mempool import Mempool
import sys
-import os
sys.path.insert(0, os.path.abspath(os.path.join(os.path.dirname(__file__), '..')))
from main import mine_and_process_blockAlso applies to: 10-11
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_reorg.py` at line 2, The import statement for os is duplicated in
the test_reorg.py file, appearing on both line 2 and line 11. Remove one of
these duplicate import os statements, keeping only a single import os
declaration at the top of the file to avoid redundancy and maintain clean import
organization.
|
@SIDDHANTCOOKIE LGTM |
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
2 participants
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: #94
transfer_out(address, amount)built-in to the sandbox environment.State.apply_transactionto validate and process contract transfers.test_contract_transfers.pycovering successful transfers, insufficient balance failures, and immediate transfer of incomingmsg['value'].Screenshots/Recordings:
TODO: If applicable, add screenshots or recordings that demonstrate the interface before and after the changes.
Additional Notes:
AI Usage Disclosure:
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.
Check one of the checkboxes below:
I have used the following AI models and tools: TODO
Checklist
Summary by CodeRabbit
New Features
Tests