feat : bounded difficulty adjustment for PoW consensus#29
Closed
arunabha003 wants to merge 21 commits intoStabilityNexus:mainfrom
Closed
feat : bounded difficulty adjustment for PoW consensus#29arunabha003 wants to merge 21 commits intoStabilityNexus:mainfrom
arunabha003 wants to merge 21 commits intoStabilityNexus:mainfrom
Conversation
Contributor
WalkthroughThis pull request establishes the foundational architecture and development infrastructure for a blockchain project in Python. It introduces project configuration (Makefile, pyproject.toml, CI/CD workflow), cryptographic primitives, core blockchain structures (transactions, blocks, headers), consensus mechanisms (Proof-of-Work mining and difficulty adjustment), state management, and a comprehensive test suite. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Crypto
participant Transaction
participant Block
participant Consensus
participant State
User->>Crypto: generate_key_pair()
Crypto-->>User: (signing_key, verify_key)
User->>Transaction: create(sender, recipient, amount, ...)
User->>Transaction: sign(signing_key)
Transaction->>Crypto: serialize_verify_key()
Transaction->>Crypto: sign_message(signing_bytes, signing_key)
Transaction-->>User: signed_transaction
User->>Block: create(transactions=[...])
Block->>Transaction: transaction_id() for each
Block->>Crypto: compute_merkle_root(tx_ids)
Block->>Block: update_header_merkle_root()
Block-->>User: unsigned_block
User->>Consensus: mine_block_header(block.header, target)
Consensus->>Crypto: blake2b_digest(header_bytes)
Consensus->>Consensus: is_valid_pow(digest, target)
Consensus-->>User: (nonce, digest)
User->>State: apply_block(block)
State->>Transaction: verify() for each
State->>State: apply_transaction(tx)
State-->>User: state_updated or error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
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)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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: 28
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 15-18: The Setup Python step (name: Setup Python, uses:
actions/setup-python@v5) lacks pip caching; add the built-in pip cache by adding
cache: 'pip' under the with block for actions/setup-python@v5 so dependencies
are cached between CI runs and not re-downloaded every time.
In @.gitignore:
- Around line 338-340: The .gitignore currently contains a broad "docs/" entry
which can hide hand-authored documentation; replace this broad pattern with a
targeted ignore (for example change "docs/" to "docs/_build/" or similar
generated-output paths) or narrow it and add explicit negations for source docs
(e.g., keep "docs/*.md" or "!docs/**/README.md") so that generated artifacts are
ignored but authored docs in the docs tree remain trackable; update the entry in
the .gitignore where "docs/" appears.
In `@Makefile`:
- Line 3: Add a standard clean target to the Makefile and include it (and
optionally all) in the .PHONY list: add a target named clean that removes Python
and build artifacts such as __pycache__, .pytest_cache, dist/, build/, and any
*.egg-info/ directories (use rm -rf on those patterns or a find -name approach),
and update the existing .PHONY line (which currently lists install dev-install
test lint format start-node) to include clean (and add all if you want the
checkmake suggestion honored); ensure the clean target is runnable with make
clean and does not depend on other targets.
In `@pyproject.toml`:
- Around line 38-39: The Ruff config currently restricts selected rules to
["E","F","I"], which omits RUF and PT checks causing missed issues like RUF059
(unused unpacked variable `signing_key` in tests/test_crypto.py) and PT017
(assert in except in tests/test_genesis.py); update the `select` array in the
`[tool.ruff.lint]` section to include "RUF" and "PT" so CI will surface those
warnings (ensure the entry becomes e.g. ["E","F","I","RUF","PT"] and commit the
pyproject.toml change).
In `@README.md`:
- Around line 53-71: The README repository layout is missing the two new
modules; update the listed tree under the project root to include
src/minichain/merkle.py and src/minichain/serialization.py so the README
accurately reflects the codebase (look for the existing src/minichain/ listing
in README.md and add lines for merkle.py and serialization.py alongside
crypto.py, transaction.py, etc.).
In `@src/minichain/block.py`:
- Around line 29-30: Public method docstring inconsistency: add brief docstrings
to Block.hash_hex (and other public methods on the Block class) to match the
existing BlockHeader.hash() docstring style. Update the Block class methods such
as hash_hex to include a one-line docstring describing what the method returns
(e.g., "Return the block hash as a hex string") and do the same for any other
public methods on Block to keep the API surface consistent with
BlockHeader.hash().
In `@src/minichain/consensus.py`:
- Around line 38-75: Add a defensive verification in
compute_next_difficulty_target to ensure the chain segment is contiguous: after
computing tip = chain[-1] and start_header = chain[-(adjustment_interval + 1)],
check that start_header.block_height == tip.block_height - adjustment_interval
and raise a ValueError (with a clear message) if not; this uses the existing
symbols tip and start_header to detect misordered or partial chains early and
preserves all other logic and validations (including validate_difficulty_target
calls).
- Around line 1-11: Replace the typing.Sequence import with the modern
collections.abc.Sequence import in this module: update the import statement that
currently brings in Sequence from typing so it imports Sequence from
collections.abc instead (affects the symbol Sequence used in
src/minichain/consensus.py).
In `@src/minichain/crypto.py`:
- Around line 21-24: The check in _require_nacl() is fragile because it tests
for the string "blake2b" in globals(); change it to use an explicit boolean
sentinel (e.g., _NACL_AVAILABLE) that is set to False by default and set to True
in the nacl import block when imports succeed, then update _require_nacl() to
raise the RuntimeError if not _NACL_AVAILABLE (include _NACL_IMPORT_ERROR as the
exception cause as before); reference symbols: _require_nacl,
_NACL_IMPORT_ERROR, and introduce _NACL_AVAILABLE.
In `@src/minichain/genesis.py`:
- Around line 56-73: apply_genesis_block currently only checks structural
constraints but does not enforce that the provided block was built from the same
GenesisConfig; inside apply_genesis_block add explicit consistency checks
comparing block.header.difficulty_target to config.difficulty_target,
block.header.timestamp to config.timestamp, and block.header.version to
config.version (and raise ValueError with a clear message on mismatch) so
callers cannot apply a genesis block whose header fields disagree with the
supplied config.
- Around line 20-38: GenesisConfig.validate currently only checks
difficulty_target > 0; add an upper-bound check against the consensus maximum
(MAX_TARGET) so genesis rejects values > MAX_TARGET early: import MAX_TARGET
from consensus (or reference the same constant) and add a conditional in
GenesisConfig.validate that raises ValueError("Genesis difficulty_target must be
<= MAX_TARGET") (or similar message) when self.difficulty_target > MAX_TARGET to
match consensus.validate_difficulty_target behavior and fail fast.
In `@src/minichain/merkle.py`:
- Line 17: The current list comprehension level = [bytes(leaf) for leaf in
leaves] silently treats integers and other non-bytes types incorrectly; change
it to explicitly validate and normalize inputs: iterate over leaves, for each
item if isinstance(leaf, (bytes, bytearray, memoryview)) convert to bytes(leaf),
otherwise raise a TypeError with a clear message (e.g., "Merkle leaves must be
bytes-like, got <type> for leaf at index N"); update the code around the level
variable (where leaves are handled) and add a unit test to assert that non-bytes
inputs raise the error.
- Around line 8-9: _hash_pair currently concatenates left and right raw bytes
causing no domain separation between leaves and internal nodes; change hashing
to be domain-separated by prefixing a single distinguishing byte (e.g. 0x00 for
leaf, 0x01 for internal node) before hashing with blake2b_digest. Update
compute_merkle_root to create leaf hashes using blake2b_digest(b"\x00" +
leaf_data) and change _hash_pair to call blake2b_digest(b"\x01" + left + right)
so internal nodes and leaves cannot collide; ensure any code that builds the
initial "level" of hashes uses the new leaf-prefixing behavior consistently.
In `@src/minichain/serialization.py`:
- Line 6: Replace the deprecated typing.Mapping import with the
collections.abc.Mapping import: update the import statement that currently
references Mapping from typing (the top-level import in this module) so that Any
still comes from typing while Mapping is imported from collections.abc to match
Python 3.11 usage.
- Around line 47-56: serialize_canonical currently calls json.dumps(...,
sort_keys=True) which overrides the insertion order produced by _to_field_map
and makes the *_FIELD_ORDER constants misleading; fix by removing sort_keys=True
so json preserves the order returned by _to_field_map (thus honoring
TRANSACTION_FIELD_ORDER and other field-order constants) and update
serialize_canonical's docstring to state that canonical bytes follow the
provided field_order; ensure tests/verifications that rely on byte-level
ordering (e.g., anywhere using serialize_canonical) still pass after this
change.
In `@src/minichain/state.py`:
- Around line 45-67: The transaction handler apply_transaction currently debits
sender by transaction.amount + transaction.fee but only credits recipient with
transaction.amount, effectively burning the fee; update apply_transaction to
credit transaction.fee to the block producer/miner account (retrieve via
self.get_account(miner_address) or a passed-in miner argument) — i.e., after
decrementing sender and incrementing recipient, increment miner_account.balance
by transaction.fee (use the existing Transaction.fee symbol and sender/recipient
objects), or if burning fees is intentional add a one-line comment above
apply_transaction explaining that fees are burned by design.
- Around line 29-43: get_account currently mutates State on read by inserting an
empty Account for unknown addresses; change it to return a non-persistent
default (e.g., a temporary Account or None) without modifying self.accounts, and
ensure callers that intend to create/persist accounts (notably apply_transaction
and any commit path) call set_account or explicitly assign into self.accounts
after validation succeeds; keep State.copy and apply_block behavior intact so
rollback still works, and update apply_transaction to only create the
recipient/sender in self.accounts after nonce/balance checks pass.
In `@src/minichain/storage.py`:
- Line 1: The module-level placeholder docstring in src/minichain/storage.py
lacks traceability; update that docstring to include a clear TODO and an issue
reference (e.g., "TODO: implement persistent storage — see ISSUE-<number>" or a
URL) so future work is tracked; modify the top-level docstring in the storage.py
module to mention the intended work, a target owner or ticket ID, and optionally
an expected milestone.
In `@src/minichain/transaction.py`:
- Around line 80-86: The sign method performs an unguarded attribute access on
signing_key (accessing signing_key.verify_key) while the parameter is typed as
object; tighten the type and add a runtime guard: change the annotation to
nacl.signing.SigningKey (or import SigningKey) and/or check
isinstance(signing_key, SigningKey) (or hasattr(signing_key, "verify_key")) at
the start of sign, and raise a ValueError with a clear message if the check
fails; then proceed to use signing_key.verify_key,
serialize_verify_key(verify_key) and sign_message(...) as before so callers get
a ValueError consistent with the method contract instead of an AttributeError.
- Around line 97-100: In verify(), don't catch all exceptions from
deserialize_verify_key; restrict the except to only the expected errors (e.g.,
replace "except Exception:" with "except (ValueError, TypeError):") so
deserialize_verify_key/ nacl.signing.VerifyKey failures are handled but
programming errors still surface; keep the existing return False in that except
block and leave RuntimeError from _require_nacl() untouched.
In `@tests/test_block.py`:
- Around line 49-51: The test test_block_hash_is_deterministic currently
compares block.hash() to itself which is trivial; change it to construct two
independent Block instances with identical fixed inputs (use _make_block(...) or
a new helper that accepts deterministic parameters like fixed
key/nonce/timestamp) and assert that block1.hash() == block2.hash(), or
alternatively rename the test to indicate it only checks stability (e.g.,
test_block_hash_stable) if you intend to keep the single-object check; update
references to _make_block and block.hash() accordingly.
In `@tests/test_consensus.py`:
- Around line 76-96: Add "import pytest" and replace the try/except/else blocks
in tests test_mining_honors_stop_event and
test_mining_raises_when_nonce_range_exhausted with pytest.raises context
managers: use pytest.raises(MiningInterrupted) as excinfo for
test_mining_honors_stop_event and assert "interrupted" in
str(excinfo.value).lower(), and use pytest.raises(RuntimeError) as excinfo for
test_mining_raises_when_nonce_range_exhausted and assert "No valid nonce found"
in str(excinfo.value) after calling mine_block_header (refer to the test
functions and the mine_block_header call and MiningInterrupted symbol to locate
the changes).
In `@tests/test_crypto.py`:
- Line 41: The unpacked variable signing_key returned from generate_key_pair()
is unused; rename it to _ (i.e., use "_, verify_key = generate_key_pair()") to
satisfy the unused-variable lint (RUF059) and make it clear only verify_key is
needed; update the call site where generate_key_pair() is used in
tests/test_crypto.py accordingly.
In `@tests/test_genesis.py`:
- Around line 60-85: Replace the fragile try/except/else blocks in
test_genesis_requires_empty_state and
test_genesis_block_rejects_wrong_previous_hash with pytest.raises(ValueError)
context managers: wrap the apply_genesis_block(state, block, config) call in
"with pytest.raises(ValueError) as excinfo:" and then assert the expected
substring using "assert 'empty state' in str(excinfo.value)" and "assert
'previous_hash' in str(excinfo.value)" respectively; ensure pytest is imported
at top of the test file if not already.
In `@tests/test_scaffold.py`:
- Around line 25-26: Remove the redundant assertion that checks "imported is not
None" after calling importlib.import_module; importlib.import_module will raise
ModuleNotFoundError on failure and never return None, so delete the line "assert
imported is not None" (the import call
"importlib.import_module(f\"minichain.{module}\")" can remain to exercise the
import).
In `@tests/test_serialization.py`:
- Around line 68-69: The parametrize call uses a string for argnames which
triggers Ruff PT006; update the pytest.mark.parametrize invocation so the first
argument is an explicit tuple of names (e.g., ("payload", "serializer",
"expected")) instead of the single string "payload,serializer,expected" in the
pytest.mark.parametrize decorator in tests/test_serialization.py.
In `@tests/test_state.py`:
- Around line 154-157: The test wrongly uses State.get_account (which
auto-creates accounts) to assert rollback removed recipient; update the
assertions to avoid get_account and instead assert the recipient is absent from
the internal store (e.g., assert recipient_address not in state.accounts or
state.accounts.get(recipient_address) is None) so the rollback check is
meaningful; keep existing checks for sender using
state.get_account(sender_address).nonce/balance and reference State.get_account,
state.accounts, and recipient_address to locate the lines to change.
In `@tests/test_transaction.py`:
- Around line 51-52: The unpacked variable other_signing_key is unused; instead
of assigning it to _ on a separate line, suppress it at the unpack site by
changing the destructuring from "other_signing_key, other_verify_key =
generate_key_pair()" to use "_" for the unused value (i.e., "_, other_verify_key
= generate_key_pair()"), so locate the call to generate_key_pair() in the test
and replace the unpacking to directly ignore other_signing_key.
Comment on lines
+15
to
+18
| - name: Setup Python | ||
| uses: actions/setup-python@v5 | ||
| with: | ||
| python-version: "3.11" |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add pip dependency caching to avoid re-downloading packages on every run.
actions/setup-python@v5 supports built-in pip caching via cache: 'pip'. Without it, every CI run re-downloads all dependencies from PyPI.
⚡ Proposed improvement
- name: Setup Python
uses: actions/setup-python@v5
with:
python-version: "3.11"
+ cache: "pip"📝 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
| - name: Setup Python | |
| uses: actions/setup-python@v5 | |
| with: | |
| python-version: "3.11" | |
| - name: Setup Python | |
| uses: actions/setup-python@v5 | |
| with: | |
| python-version: "3.11" | |
| cache: "pip" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci.yml around lines 15 - 18, The Setup Python step (name:
Setup Python, uses: actions/setup-python@v5) lacks pip caching; add the built-in
pip cache by adding cache: 'pip' under the with block for
actions/setup-python@v5 so dependencies are cached between CI runs and not
re-downloaded every time.
Comment on lines
+338
to
+340
|
|
||
|
|
||
| docs/ |
Contributor
There was a problem hiding this comment.
Broad docs/ ignore may silently suppress tracked documentation.
The bare docs/ pattern at Line 340 ignores the entire documentation tree. If any hand-authored docs (e.g., ADRs, API references) ever land in docs/, they will be invisible to git add by default, which is a common source of accidentally untracked documentation. Consider scoping it narrowly (e.g., docs/_build/ for Sphinx output) unless the intent is to never commit any file under docs/.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.gitignore around lines 338 - 340, The .gitignore currently contains a broad
"docs/" entry which can hide hand-authored documentation; replace this broad
pattern with a targeted ignore (for example change "docs/" to "docs/_build/" or
similar generated-output paths) or narrow it and add explicit negations for
source docs (e.g., keep "docs/*.md" or "!docs/**/README.md") so that generated
artifacts are ignored but authored docs in the docs tree remain trackable;
update the entry in the .gitignore where "docs/" appears.
| @@ -0,0 +1,21 @@ | |||
| PYTHON ?= python3 | |||
|
|
|||
| .PHONY: install dev-install test lint format start-node | |||
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding a clean target for cache/build artifact hygiene.
checkmake flags missing all (not critical here) and clean. A clean target for removing __pycache__, .pytest_cache, dist/, and *.egg-info/ is a useful convention.
♻️ Proposed addition
-.PHONY: install dev-install test lint format start-node
+.PHONY: install dev-install test lint format start-node clean
+clean:
+ find . -type d -name "__pycache__" -exec rm -rf {} +
+ find . -type d -name ".pytest_cache" -exec rm -rf {} +
+ find . -type d -name "*.egg-info" -exec rm -rf {} +
+ find . -type d -name "dist" -exec rm -rf {} +📝 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
| .PHONY: install dev-install test lint format start-node | |
| .PHONY: install dev-install test lint format start-node clean | |
| clean: | |
| find . -type d -name "__pycache__" -exec rm -rf {} + | |
| find . -type d -name ".pytest_cache" -exec rm -rf {} + | |
| find . -type d -name "*.egg-info" -exec rm -rf {} + | |
| find . -type d -name "dist" -exec rm -rf {} + |
🧰 Tools
🪛 checkmake (0.2.2)
[warning] 3-3: Missing required phony target "all"
(minphony)
[warning] 3-3: Missing required phony target "clean"
(minphony)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Makefile` at line 3, Add a standard clean target to the Makefile and include
it (and optionally all) in the .PHONY list: add a target named clean that
removes Python and build artifacts such as __pycache__, .pytest_cache, dist/,
build/, and any *.egg-info/ directories (use rm -rf on those patterns or a find
-name approach), and update the existing .PHONY line (which currently lists
install dev-install test lint format start-node) to include clean (and add all
if you want the checkmake suggestion honored); ensure the clean target is
runnable with make clean and does not depend on other targets.
Comment on lines
+38
to
+39
| [tool.ruff.lint] | ||
| select = ["E", "F", "I"] |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Ruff select excludes RUF and PT rules that are already flagging issues in this PR.
The current selection (E, F, I) won't catch:
RUF059— unused unpacked variable (signing_keyintests/test_crypto.py:41)PT017—asserton exception inexceptblock (tests/test_genesis.py:69,83)
Adding "RUF" and "PT" to select would surface these in CI.
♻️ Proposed fix
[tool.ruff.lint]
-select = ["E", "F", "I"]
+select = ["E", "F", "I", "RUF", "PT"]📝 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
| [tool.ruff.lint] | |
| select = ["E", "F", "I"] | |
| [tool.ruff.lint] | |
| select = ["E", "F", "I", "RUF", "PT"] |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pyproject.toml` around lines 38 - 39, The Ruff config currently restricts
selected rules to ["E","F","I"], which omits RUF and PT checks causing missed
issues like RUF059 (unused unpacked variable `signing_key` in
tests/test_crypto.py) and PT017 (assert in except in tests/test_genesis.py);
update the `select` array in the `[tool.ruff.lint]` section to include "RUF" and
"PT" so CI will surface those warnings (ensure the entry becomes e.g.
["E","F","I","RUF","PT"] and commit the pyproject.toml change).
Comment on lines
+53
to
+71
| ```text | ||
| .github/workflows/ci.yml | ||
| src/minichain/ | ||
| __init__.py | ||
| __main__.py | ||
| crypto.py | ||
| transaction.py | ||
| block.py | ||
| state.py | ||
| mempool.py | ||
| consensus.py | ||
| network.py | ||
| storage.py | ||
| node.py | ||
| tests/ | ||
| test_scaffold.py | ||
| issues.md | ||
| architectureProposal.md | ||
| ``` |
Contributor
There was a problem hiding this comment.
merkle.py and serialization.py are missing from the Repository Layout.
Both modules introduced in this PR (src/minichain/merkle.py, src/minichain/serialization.py) are absent from the layout listing.
📝 Proposed update
src/minichain/
__init__.py
__main__.py
crypto.py
transaction.py
block.py
+ merkle.py
+ serialization.py
state.py
mempool.py
consensus.py
network.py
storage.py
node.py📝 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
| ```text | |
| .github/workflows/ci.yml | |
| src/minichain/ | |
| __init__.py | |
| __main__.py | |
| crypto.py | |
| transaction.py | |
| block.py | |
| state.py | |
| mempool.py | |
| consensus.py | |
| network.py | |
| storage.py | |
| node.py | |
| tests/ | |
| test_scaffold.py | |
| issues.md | |
| architectureProposal.md | |
| ``` | |
| .github/workflows/ci.yml | |
| src/minichain/ | |
| __init__.py | |
| __main__.py | |
| crypto.py | |
| transaction.py | |
| block.py | |
| merkle.py | |
| serialization.py | |
| state.py | |
| mempool.py | |
| consensus.py | |
| network.py | |
| storage.py | |
| node.py | |
| tests/ | |
| test_scaffold.py | |
| issues.md | |
| architectureProposal.md |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 53 - 71, The README repository layout is missing the
two new modules; update the listed tree under the project root to include
src/minichain/merkle.py and src/minichain/serialization.py so the README
accurately reflects the codebase (look for the existing src/minichain/ listing
in README.md and add lines for merkle.py and serialization.py alongside
crypto.py, transaction.py, etc.).
Comment on lines
+60
to
+85
| def test_genesis_requires_empty_state() -> None: | ||
| config = GenesisConfig(initial_balances={"dd" * 20: 1}) | ||
| block = create_genesis_block(config) | ||
| state = State() | ||
| state.set_account("ff" * 20, Account(balance=1, nonce=0)) | ||
|
|
||
| try: | ||
| apply_genesis_block(state, block, config) | ||
| except ValueError as exc: | ||
| assert "empty state" in str(exc) | ||
| else: | ||
| raise AssertionError("Expected ValueError for non-empty state") | ||
|
|
||
|
|
||
| def test_genesis_block_rejects_wrong_previous_hash() -> None: | ||
| config = GenesisConfig(initial_balances={"ee" * 20: 10}) | ||
| block = create_genesis_block(config) | ||
| block.header = replace(block.header, previous_hash="11" * 32) | ||
| state = State() | ||
|
|
||
| try: | ||
| apply_genesis_block(state, block, config) | ||
| except ValueError as exc: | ||
| assert "previous_hash" in str(exc) | ||
| else: | ||
| raise AssertionError("Expected ValueError for invalid previous_hash") |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Replace try/except/assert with pytest.raises() in both error-path tests.
The try/except/assert/else pattern is fragile — an AssertionError raised inside the except block is indistinguishable from the assertion failing. pytest.raises is the idiomatic, safer equivalent (flagged as PT017 by ruff).
♻️ Proposed fix
+import pytest
def test_genesis_requires_empty_state() -> None:
config = GenesisConfig(initial_balances={"dd" * 20: 1})
block = create_genesis_block(config)
state = State()
state.set_account("ff" * 20, Account(balance=1, nonce=0))
- try:
- apply_genesis_block(state, block, config)
- except ValueError as exc:
- assert "empty state" in str(exc)
- else:
- raise AssertionError("Expected ValueError for non-empty state")
+ with pytest.raises(ValueError, match="empty state"):
+ apply_genesis_block(state, block, config)
def test_genesis_block_rejects_wrong_previous_hash() -> None:
config = GenesisConfig(initial_balances={"ee" * 20: 10})
block = create_genesis_block(config)
block.header = replace(block.header, previous_hash="11" * 32)
state = State()
- try:
- apply_genesis_block(state, block, config)
- except ValueError as exc:
- assert "previous_hash" in str(exc)
- else:
- raise AssertionError("Expected ValueError for invalid previous_hash")
+ with pytest.raises(ValueError, match="previous_hash"):
+ apply_genesis_block(state, block, config)🧰 Tools
🪛 Ruff (0.15.1)
[warning] 69-69: Found assertion on exception exc in except block, use pytest.raises() instead
(PT017)
[warning] 71-71: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 83-83: Found assertion on exception exc in except block, use pytest.raises() instead
(PT017)
[warning] 85-85: 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 `@tests/test_genesis.py` around lines 60 - 85, Replace the fragile
try/except/else blocks in test_genesis_requires_empty_state and
test_genesis_block_rejects_wrong_previous_hash with pytest.raises(ValueError)
context managers: wrap the apply_genesis_block(state, block, config) call in
"with pytest.raises(ValueError) as excinfo:" and then assert the expected
substring using "assert 'empty state' in str(excinfo.value)" and "assert
'previous_hash' in str(excinfo.value)" respectively; ensure pytest is imported
at top of the test file if not already.
Comment on lines
+25
to
+26
| imported = importlib.import_module(f"minichain.{module}") | ||
| assert imported is not None |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
assert imported is not None is redundant — importlib.import_module never returns None.
A failed import raises ModuleNotFoundError, so the assertion is always satisfied when execution reaches it. Removing it simplifies the loop without losing coverage.
♻️ Proposed fix
def test_component_modules_are_importable() -> None:
for module in COMPONENT_MODULES:
- imported = importlib.import_module(f"minichain.{module}")
- assert imported is not None
+ importlib.import_module(f"minichain.{module}")📝 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
| imported = importlib.import_module(f"minichain.{module}") | |
| assert imported is not None | |
| def test_component_modules_are_importable() -> None: | |
| for module in COMPONENT_MODULES: | |
| importlib.import_module(f"minichain.{module}") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_scaffold.py` around lines 25 - 26, Remove the redundant assertion
that checks "imported is not None" after calling importlib.import_module;
importlib.import_module will raise ModuleNotFoundError on failure and never
return None, so delete the line "assert imported is not None" (the import call
"importlib.import_module(f\"minichain.{module}\")" can remain to exercise the
import).
Comment on lines
+68
to
+69
| @pytest.mark.parametrize( | ||
| "payload,serializer,expected", |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use a tuple for the pytest.mark.parametrize argnames argument.
Ruff PT006 flags a string where a tuple is expected. Prefer the explicit tuple form for consistency:
♻️ Proposed fix
`@pytest.mark.parametrize`(
- "payload,serializer,expected",
+ ("payload", "serializer", "expected"),📝 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
| @pytest.mark.parametrize( | |
| "payload,serializer,expected", | |
| `@pytest.mark.parametrize`( | |
| ("payload", "serializer", "expected"), |
🧰 Tools
🪛 Ruff (0.15.1)
[warning] 69-69: Wrong type passed to first argument of pytest.mark.parametrize; expected tuple
Use a tuple for the first argument
(PT006)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_serialization.py` around lines 68 - 69, The parametrize call uses
a string for argnames which triggers Ruff PT006; update the
pytest.mark.parametrize invocation so the first argument is an explicit tuple of
names (e.g., ("payload", "serializer", "expected")) instead of the single string
"payload,serializer,expected" in the pytest.mark.parametrize decorator in
tests/test_serialization.py.
Comment on lines
+154
to
+157
| assert state.get_account(sender_address).balance == 100 | ||
| assert state.get_account(sender_address).nonce == 0 | ||
| assert state.get_account(recipient_address).balance == 0 | ||
| assert state.get_account(recipient_address).nonce == 0 |
Contributor
There was a problem hiding this comment.
Rollback assertions use get_account, which auto-creates accounts — making them vacuously true.
State.get_account inserts a default Account() when the address is absent. After a successful rollback, recipient_address is not in state.accounts, so calling state.get_account(recipient_address) returns 0 regardless of whether the rollback actually worked — and as a side effect, adds the recipient back to state.accounts. Use a direct containment check instead:
♻️ Proposed fix
- assert state.get_account(recipient_address).balance == 0
- assert state.get_account(recipient_address).nonce == 0
+ assert recipient_address not in state.accounts📝 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
| assert state.get_account(sender_address).balance == 100 | |
| assert state.get_account(sender_address).nonce == 0 | |
| assert state.get_account(recipient_address).balance == 0 | |
| assert state.get_account(recipient_address).nonce == 0 | |
| assert state.get_account(sender_address).balance == 100 | |
| assert state.get_account(sender_address).nonce == 0 | |
| assert recipient_address not in state.accounts |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_state.py` around lines 154 - 157, The test wrongly uses
State.get_account (which auto-creates accounts) to assert rollback removed
recipient; update the assertions to avoid get_account and instead assert the
recipient is absent from the internal store (e.g., assert recipient_address not
in state.accounts or state.accounts.get(recipient_address) is None) so the
rollback check is meaningful; keep existing checks for sender using
state.get_account(sender_address).nonce/balance and reference State.get_account,
state.accounts, and recipient_address to locate the lines to change.
Comment on lines
+51
to
+52
| other_signing_key, other_verify_key = generate_key_pair() | ||
| _ = other_signing_key |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Suppress the unused variable at the unpack site, not after.
Assigning _ = other_signing_key on a separate line is non-idiomatic and leaves a confusing statement. Destructure directly:
♻️ Proposed fix
- other_signing_key, other_verify_key = generate_key_pair()
- _ = other_signing_key
+ _, other_verify_key = generate_key_pair()📝 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
| other_signing_key, other_verify_key = generate_key_pair() | |
| _ = other_signing_key | |
| _, other_verify_key = generate_key_pair() |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_transaction.py` around lines 51 - 52, The unpacked variable
other_signing_key is unused; instead of assigning it to _ on a separate line,
suppress it at the unpack site by changing the destructuring from
"other_signing_key, other_verify_key = generate_key_pair()" to use "_" for the
unused value (i.e., "_, other_verify_key = generate_key_pair()"), so locate the
call to generate_key_pair() in the test and replace the unpacking to directly
ignore other_signing_key.
This was referenced Feb 23, 2026
Author
|
Superseded by #30. Closing this PR to keep review history in one place. |
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:
Part of #8
Depends on #25
Summary
Validation
Checklist
AI Usage Disclosure
Check one of the checkboxes below:
I have used the following AI models and tools: TODO
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.
Summary by CodeRabbit
Release Notes
New Features
Chores
Tests