feat: account state and atomic ledger transitions#21
Open
arunabha003 wants to merge 15 commits intoStabilityNexus:mainfrom
Open
feat: account state and atomic ledger transitions#21arunabha003 wants to merge 15 commits intoStabilityNexus:mainfrom
arunabha003 wants to merge 15 commits intoStabilityNexus:mainfrom
Conversation
Contributor
WalkthroughEstablishes a Python blockchain project infrastructure with core primitives: CI/CD pipeline, project configuration, cryptographic key management (Ed25519), transaction and block data structures with Merkle tree validation, state machine for account transitions, and deterministic serialization. Includes 7 test modules validating functionality across all components. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/CLI
participant Crypto as Crypto Module
participant Transaction as Transaction
participant Serialization as Serialization
participant State as State Engine
participant Block as Block
User->>Crypto: generate_key_pair()
Crypto-->>User: (signing_key, verify_key)
User->>Transaction: create(sender, recipient, amount, nonce, fee, timestamp)
Transaction-->>User: unsigned_txn
User->>Transaction: sign(signing_key)
Transaction->>Crypto: derive_address(verify_key)
Crypto-->>Transaction: sender_address
Transaction->>Serialization: serialize signing_payload
Serialization-->>Transaction: canonical_bytes
Transaction->>Crypto: sign_message(canonical_bytes, signing_key)
Crypto-->>Transaction: signature
Transaction-->>User: signed_txn
User->>Block: create_block(transactions, header)
Block->>Serialization: serialize transaction payloads
Serialization-->>Block: serialized_txns
Block->>Block: compute_merkle_root(hashes)
Block-->>User: block_with_merkle
User->>State: apply_block(block)
State->>State: snapshot_state
loop for each transaction
State->>Transaction: verify()
Transaction->>Crypto: verify_signature(...)
Crypto-->>Transaction: valid?
Transaction-->>State: verified
State->>State: validate_nonce, balance
State->>State: update_accounts
end
State-->>User: state_updated or rolled_back
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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 |
![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: 22
🤖 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 20-23: Add a pip cache step before the "Install dependencies" step
in the CI workflow so pip wheels and C-extension builds are reused between runs:
insert an actions/cache@v3 step that caches the pip cache directory (e.g.,
~/.cache/pip) with a key that includes the OS and a hash of dependency files
(e.g., pyproject.toml, poetry.lock, requirements*.txt) and sensible restore-keys
fallback; keep this step immediately before the existing "Install dependencies"
run block that executes "python -m pip install --upgrade pip" and "python -m pip
install -e .[dev]".
In @.gitignore:
- Around line 261-270: The Python artifact ignore block containing
"__pycache__/", "*.py[cod]", ".pytest_cache/", ".ruff_cache/", and ".venv/" is
currently placed amidst LaTeX aux entries; please move that entire block so it
is grouped with the MiniChain-specific entries (e.g., near "issues.md" /
"architectureProposal.md") or appended to the end of the file for readability,
ensuring the block is preserved exactly but relocated together rather than split
between LaTeX sections.
- Line 340: The .gitignore currently contains a broad "docs/" entry which may
hide hand-authored docs; either remove this line if no generated docs are
planned, or narrow it to the actual generated output directory you intend to
ignore (e.g., change "docs/" to "docs/_build/" or "docs/build/") so that source
documentation remains tracked—locate and update the "docs/" entry in the
.gitignore file accordingly.
In `@Makefile`:
- Line 3: Add a new clean Makefile target and include it in the .PHONY list:
update the existing .PHONY line (which currently lists install dev-install test
lint format start-node) to also include clean, and add a target named clean that
removes common Python/build artifacts (e.g., __pycache__ directories,
*.egg-info, dist/, build/) using safe rm -rf/globbing commands so developers can
run make clean to purge build artifacts.
In `@pyproject.toml`:
- Around line 17-19: The dependency name in the extras list is incorrect:
replace the non-existent PyPI package string "py-libp2p>=0.2.0" with the correct
package name "libp2p>=0.2.0" so the network extra resolves properly; locate the
network extras entry (the list containing "py-libp2p>=0.2.0") and update that
string to "libp2p>=0.2.0".
In `@README.md`:
- Around line 67-70: The README's tests/ listing is out of date; update the
tests section (where test_scaffold.py is currently listed) to include the new
test modules by adding test_crypto.py, test_merkle.py, test_serialization.py,
test_transaction.py, and test_state.py alongside test_scaffold.py so the
repository layout reflects all current test files.
- Around line 3-15: Update the "Current Status" section in README.md to remove
the stale claim that only Issue `#1` (project scaffolding) is implemented and
instead list the actual implemented components (e.g., crypto, serialization,
Merkle trees, transactions, state transitions) and existing modules under
src/minichain (crypto, transaction, block, state, mempool, consensus, network,
storage, node), and adjust wording to accurately describe current capabilities
and next roadmap items so new contributors aren't misled by the old
scaffolding-only description.
In `@src/minichain/__main__.py`:
- Line 13: Add explicit validation for the CLI port so values are restricted to
1–65535: modify the argument handling around parser.add_argument("--port", ...)
to use a bounded-int validator (or validate args.port immediately after
parse_args) that raises argparse.ArgumentTypeError or prints a clear error and
exits when the value is outside 1..65535; ensure the validated port (args.port)
is passed into start_node unchanged so any socket errors are avoided by catching
invalid CLI input early.
In `@src/minichain/crypto.py`:
- Around line 21-24: The _require_nacl() function currently checks for "blake2b"
in globals() as a proxy for a successful PyNaCl import; replace this fragile
canary with a dedicated module-level boolean (e.g., _NACL_AVAILABLE) that you
set in the import try/except where _NACL_IMPORT_ERROR is defined, then have
_require_nacl() test that boolean and raise RuntimeError(msg) from
_NACL_IMPORT_ERROR if it's False; update references to the import sentinel so
all checks use _NACL_AVAILABLE (and keep _NACL_IMPORT_ERROR as the exception
source).
In `@src/minichain/mempool.py`:
- Line 1: This file is a placeholder with no implementation; create a tracked
TODO by opening an issue and also add a minimal skeleton in this module: define
a Mempool class with methods add(tx), remove(tx_id), and
select_transactions(limit) and include docstrings and raise NotImplementedError
(or explicit TODO exceptions) so callers see the contract; ensure exported
symbols (Mempool, add, remove, select_transactions) are present so future code
and tests can import them and the issue references the new skeleton and outlines
remaining tasks (concurrency, eviction, fee/prioritization).
In `@src/minichain/serialization.py`:
- Line 6: Replace the deprecated typing.Mapping import with the
collections.abc.Mapping import: update the import statement in serialization.py
so that Mapping is imported from collections.abc while keeping Any from typing
(e.g., change "from typing import Any, Mapping" to import Any from typing and
Mapping from collections.abc); ensure all type annotations that reference
Mapping continue to work unchanged.
- Around line 47-56: serialize_canonical currently passes sort_keys=True to
json.dumps which alphabetically reorders keys and nullifies the intent of the
field_order argument (coming from _to_field_map); remove sort_keys=True from
serialize_canonical so json uses the insertion order produced by _to_field_map
(and update the docstring to state canonical order is the provided field_order),
then verify _to_field_map preserves insertion order for the returned dict and
update any tests that assumed alphabetical ordering; alternatively, if you
prefer alphabetical canonicalization, rename the field-order tuples to something
like TRANSACTION_REQUIRED_FIELDS / BLOCK_HEADER_REQUIRED_FIELDS and add a
comment that canonical JSON uses alphabetical key ordering so the tuples are
only for validation.
In `@src/minichain/state.py`:
- Around line 65-67: The transaction handling currently deducts total_cost from
sender (sender.balance -= total_cost) and credits only transaction.amount to
recipient (recipient.balance += transaction.amount), effectively burning the fee
(total_cost - transaction.amount); add a concise docstring or comment in the
function that performs this logic (the function containing sender.balance,
sender.nonce, recipient.balance, and the total_cost/transaction.amount
computation) explicitly stating that fees are intentionally burned in this
minimal design and noting where to change behavior when adding fee destinations
(e.g., block-producer rewards) so future contributors understand the current
model and where to extend it.
- Around line 69-76: The current apply_block only rolls back on
StateTransitionError; change the except to catch all exceptions so the snapshot
is restored for any failure: in apply_block (use the existing snapshot variable
created at the top and the apply_transaction loop), replace the specific except
StateTransitionError as exc block with a broad except Exception as exc that sets
self.accounts = snapshot.accounts and then re-raises the original exception (use
plain raise to preserve the original exception type and traceback) so rollback
is unconditional for any error during apply_transaction.
- Around line 47-63: The long inline exception messages violate TRY003; refactor
by moving the diagnostic text into StateTransitionError (or by adding named
subclasses/factory helpers) and replace inline string raises in
apply_transaction (where you currently raise StateTransitionError(...) for
signature/identity, nonce mismatch, and insufficient balance) with calls that
construct the error without long inline messages (e.g.,
StateTransitionError.nonce_mismatch(sender=..., expected=..., got=...) or a
dedicated NonceMismatch(StateTransitionError)(...) factory). Ensure the new
StateTransitionError API stores/constructs the detailed message internally so
the raise sites only pass short identifiers/values (sender, expected, got,
total_cost, balance) rather than long formatted strings.
- Around line 49-63: apply_transaction currently calls
get_account(transaction.sender) and get_account(transaction.recipient) which
mutates self.accounts by inserting zeroed Account entries, causing ghost
accounts when validation (nonce/balance) fails; update apply_transaction to
perform validation using a non-mutating check (e.g., check membership in
self.accounts or add a new helper get_account_if_exists / has_account) before
calling get_account, so you only create or mutate accounts after all nonce and
balance checks pass; reference get_account, apply_transaction, self.accounts
(apply_block can remain unchanged since it rolls back).
In `@src/minichain/storage.py`:
- Line 1: The module is a placeholder; replace the docstring with a minimal
storage API and two skeleton implementations: define an abstract base class
StorageBackend (methods: save_state(state), load_state() -> state,
append_block(block), get_block(index) -> block, close()) and implement
InMemoryStorage (in-memory dict/list) and FileStorage (file-backed skeleton
using JSON/bytes with path init) to allow other modules to depend on
persistence; include clear TODOs about durability/transactions and raise
NotImplementedError for any unimplemented behavior in FileStorage, and add a
short unit-test scaffold that exercises InMemoryStorage so CI covers basic
behavior.
In `@src/minichain/transaction.py`:
- Around line 80-86: The sign method currently types signing_key as object which
is too broad; change the type hint to the concrete SigningKey (or to
"SigningKey" behind a TYPE_CHECKING guard) so attribute access to .verify_key is
type-safe—update the signature of sign(self, signing_key: SigningKey) -> None
(or use a string/TYPE_CHECKING import if PyNaCl is optional) and adjust any
imports/TYPE_CHECKING block so SigningKey is available for typing without
forcing a runtime import.
In `@tests/test_crypto.py`:
- Around line 40-47: The test test_invalid_signature_is_rejected unpacks
signing_key from generate_key_pair() but never uses it, causing a linter error;
fix by ignoring the unused value when calling generate_key_pair() (e.g., replace
"signing_key, verify_key = generate_key_pair()" with "_, verify_key =
generate_key_pair()" or similar) so only verify_key and other_signing_key are
kept for sign_message and verify_signature operations.
In `@tests/test_serialization.py`:
- Around line 68-69: Replace the string-style parameter names in the pytest
parametrize call with a tuple: update the pytest.mark.parametrize invocation
that currently uses "payload,serializer,expected" to use a tuple like
("payload", "serializer", "expected") so the linter rule Ruff PT006 is
satisfied; locate the parametrize call in tests/test_serialization.py and change
the first argument accordingly.
In `@tests/test_state.py`:
- Around line 73-76: Replace the verbose unused variable suppression in tests by
unpacking directly into _ when calling generate_key_pair; specifically in
test_insufficient_balance_is_rejected (and the other affected tests) change
assignments that do "recipient_key, recipient_verify = generate_key_pair(); _ =
recipient_key" to unpack the call as "_, recipient_verify = generate_key_pair()"
(or similarly use "_" for any unused first/second value) so the unused value is
ignored at assignment time.
In `@tests/test_transaction.py`:
- Around line 51-52: Replace the current two-step assignment and manual discard
with tuple unpacking so the unused signing key is discarded at assignment: call
generate_key_pair() and assign its first element to the throwaway variable (_)
and the second to other_verify_key (i.e., use "_" for the unused signing key and
"other_verify_key" for the verify key) instead of creating other_signing_key
then assigning `_ = other_signing_key`; this change should be applied where
generate_key_pair() is invoked in the test (variables other_signing_key and
other_verify_key).
Comment on lines
+20
to
+23
| - name: Install dependencies | ||
| run: | | ||
| python -m pip install --upgrade pip | ||
| python -m pip install -e .[dev] |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add pip dependency caching to speed up CI.
Without a cache step, every workflow run performs a full re-install of all packages, including C-extension builds (PyNaCl). This adds unnecessary latency on every push and PR.
⚡ Proposed addition
- name: Setup Python
uses: actions/setup-python@v5
with:
python-version: "3.11"
+ cache: "pip"
- name: Install dependencies
run: |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci.yml around lines 20 - 23, Add a pip cache step before
the "Install dependencies" step in the CI workflow so pip wheels and C-extension
builds are reused between runs: insert an actions/cache@v3 step that caches the
pip cache directory (e.g., ~/.cache/pip) with a key that includes the OS and a
hash of dependency files (e.g., pyproject.toml, poetry.lock, requirements*.txt)
and sensible restore-keys fallback; keep this step immediately before the
existing "Install dependencies" run block that executes "python -m pip install
--upgrade pip" and "python -m pip install -e .[dev]".
Comment on lines
+261
to
+270
| # MiniChain local planning docs (do not commit) | ||
| issues.md | ||
| architectureProposal.md | ||
|
|
||
| # Python caches and virtualenvs | ||
| __pycache__/ | ||
| *.py[cod] | ||
| .pytest_cache/ | ||
| .ruff_cache/ | ||
| .venv/ |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
LGTM — standard Python artifact ignores.
The *.py[cod] glob correctly covers .pyc, .pyo, and .pyd. All entries here (__pycache__/, .pytest_cache/, .ruff_cache/, .venv/) are standard for a Python project.
Minor nit: this block is inserted in the middle of the LaTeX auxiliary-file section (between easy-todo and xcolor). Grouping it with the MiniChain-specific entries below (or at the end of the file) would improve readability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.gitignore around lines 261 - 270, The Python artifact ignore block
containing "__pycache__/", "*.py[cod]", ".pytest_cache/", ".ruff_cache/", and
".venv/" is currently placed amidst LaTeX aux entries; please move that entire
block so it is grouped with the MiniChain-specific entries (e.g., near
"issues.md" / "architectureProposal.md") or appended to the end of the file for
readability, ensuring the block is preserved exactly but relocated together
rather than split between LaTeX sections.
| #*Notes.bib | ||
|
|
||
|
|
||
| docs/ |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
# Read the .gitignore file around line 340 to see context and placement
wc -l .gitignoreRepository: StabilityNexus/MiniChain
Length of output: 82
🏁 Script executed:
# Check context around the docs/ entry
sed -n '330,350p' .gitignoreRepository: StabilityNexus/MiniChain
Length of output: 339
🏁 Script executed:
# Also check the MiniChain-specific section mentioned at lines 261-263
sed -n '255,270p' .gitignoreRepository: StabilityNexus/MiniChain
Length of output: 272
🏁 Script executed:
# Check if docs/ directory exists and what it contains
fd docs --type d -x ls -la {} \;Repository: StabilityNexus/MiniChain
Length of output: 50
🏁 Script executed:
# Look for any documentation build configuration files
fd -e conf.py -e mkdocs.yml -e sphinx.cfg -e tox.ini | head -20Repository: StabilityNexus/MiniChain
Length of output: 50
Consider whether docs/ should be scoped more narrowly or removed entirely.
The docs/ entry is currently preemptive—the directory does not exist in the repository, and no documentation build system (Sphinx, MkDocs, etc.) is configured. If documentation generation is not planned, this entry can be removed. If auto-generated documentation is planned in the future (e.g., built to docs/_build/), consider narrowing the pattern to docs/_build/ or docs/build/ instead of the broad docs/ to avoid accidentally suppressing hand-authored files.
The entry's placement after LaTeX-specific ignores is reasonable; it is not misplaced relative to the MiniChain local planning docs section (lines 261–263), which serves a different purpose (local uncommitted files, not versioned documentation).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.gitignore at line 340, The .gitignore currently contains a broad "docs/"
entry which may hide hand-authored docs; either remove this line if no generated
docs are planned, or narrow it to the actual generated output directory you
intend to ignore (e.g., change "docs/" to "docs/_build/" or "docs/build/") so
that source documentation remains tracked—locate and update the "docs/" entry in
the .gitignore file accordingly.
| @@ -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
Add a clean target for build artifact removal.
The static analysis tool flags the missing clean target, and it's practically useful for removing __pycache__, *.egg-info, dist/, and build/ directories that accumulate during development.
🧹 Proposed addition
-.PHONY: install dev-install test lint format start-node
+.PHONY: install dev-install test lint format start-node clean start-node:
PYTHONPATH=src $(PYTHON) -m minichain --host 127.0.0.1 --port 7000
+
+clean:
+ find . -type d -name "__pycache__" -exec rm -rf {} +
+ find . -type d -name "*.egg-info" -exec rm -rf {} +
+ rm -rf dist/ build/ .ruff_cache/ .pytest_cache/📝 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 | |
| start-node: | |
| PYTHONPATH=src $(PYTHON) -m minichain --host 127.0.0.1 --port 7000 | |
| clean: | |
| find . -type d -name "__pycache__" -exec rm -rf {} + | |
| find . -type d -name "*.egg-info" -exec rm -rf {} + | |
| rm -rf dist/ build/ .ruff_cache/ .pytest_cache/ |
🧰 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 new clean Makefile target and include it in the
.PHONY list: update the existing .PHONY line (which currently lists install
dev-install test lint format start-node) to also include clean, and add a target
named clean that removes common Python/build artifacts (e.g., __pycache__
directories, *.egg-info, dist/, build/) using safe rm -rf/globbing commands so
developers can run make clean to purge build artifacts.
Comment on lines
+17
to
+19
| network = [ | ||
| "py-libp2p>=0.2.0", | ||
| ] |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the correct PyPI distribution names
echo "=== Checking libp2p ==="
curl -sf https://pypi.org/pypi/libp2p/json | jq '.info.name, .info.version'
echo -e "\n=== Checking py-libp2p ==="
curl -sf https://pypi.org/pypi/py-libp2p/json | jq '.info.name, .info.version' 2>/dev/null || echo "py-libp2p not found or error occurred"Repository: StabilityNexus/MiniChain
Length of output: 136
Incorrect PyPI package name: change py-libp2p to libp2p.
The Python libp2p implementation is distributed on PyPI as libp2p, not py-libp2p. The py-libp2p package does not exist on PyPI. Using py-libp2p>=0.2.0 will cause dependency resolution to fail when users run pip install -e .[network].
Fix
network = [
- "py-libp2p>=0.2.0",
+ "libp2p>=0.2.0",
]📝 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
| network = [ | |
| "py-libp2p>=0.2.0", | |
| ] | |
| network = [ | |
| "libp2p>=0.2.0", | |
| ] |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pyproject.toml` around lines 17 - 19, The dependency name in the extras list
is incorrect: replace the non-existent PyPI package string "py-libp2p>=0.2.0"
with the correct package name "libp2p>=0.2.0" so the network extra resolves
properly; locate the network extras entry (the list containing
"py-libp2p>=0.2.0") and update that string to "libp2p>=0.2.0".
Comment on lines
+80
to
+86
| def sign(self, signing_key: object) -> None: | ||
| """Sign this transaction in-place and populate auth fields.""" | ||
| if not self._validate_common_fields(): | ||
| raise ValueError("Invalid transaction fields") | ||
| verify_key = signing_key.verify_key | ||
| self.public_key = serialize_verify_key(verify_key) | ||
| self.signature = sign_message(self.signing_bytes(), signing_key).hex() |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider tightening the signing_key type hint.
signing_key: object is very broad — the method accesses .verify_key on line 84 without type safety. This will produce an opaque AttributeError if called with the wrong type.
I understand this might be intentional to avoid importing SigningKey (which requires PyNaCl at import time), but since SigningKey is already imported at module level (line 7-14), you could use it in the type hint via a TYPE_CHECKING guard or directly:
♻️ Proposed fix
- def sign(self, signing_key: object) -> None:
+ def sign(self, signing_key: SigningKey) -> None:Note: SigningKey is already imported at the top of the file. When PyNaCl is missing, it falls back to Any, so this won't break the optional-dependency pattern.
📝 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
| def sign(self, signing_key: object) -> None: | |
| """Sign this transaction in-place and populate auth fields.""" | |
| if not self._validate_common_fields(): | |
| raise ValueError("Invalid transaction fields") | |
| verify_key = signing_key.verify_key | |
| self.public_key = serialize_verify_key(verify_key) | |
| self.signature = sign_message(self.signing_bytes(), signing_key).hex() | |
| def sign(self, signing_key: SigningKey) -> None: | |
| """Sign this transaction in-place and populate auth fields.""" | |
| if not self._validate_common_fields(): | |
| raise ValueError("Invalid transaction fields") | |
| verify_key = signing_key.verify_key | |
| self.public_key = serialize_verify_key(verify_key) | |
| self.signature = sign_message(self.signing_bytes(), signing_key).hex() |
🧰 Tools
🪛 Ruff (0.15.1)
[warning] 83-83: 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 `@src/minichain/transaction.py` around lines 80 - 86, The sign method currently
types signing_key as object which is too broad; change the type hint to the
concrete SigningKey (or to "SigningKey" behind a TYPE_CHECKING guard) so
attribute access to .verify_key is type-safe—update the signature of sign(self,
signing_key: SigningKey) -> None (or use a string/TYPE_CHECKING import if PyNaCl
is optional) and adjust any imports/TYPE_CHECKING block so SigningKey is
available for typing without forcing a runtime import.
Comment on lines
+40
to
+47
| def test_invalid_signature_is_rejected() -> None: | ||
| signing_key, verify_key = generate_key_pair() | ||
| other_signing_key, _ = generate_key_pair() | ||
| message = b"minichain-message" | ||
|
|
||
| wrong_signature = sign_message(message, other_signing_key) | ||
|
|
||
| assert not verify_signature(message, wrong_signature, verify_key) |
Contributor
There was a problem hiding this comment.
Unused variable signing_key (Ruff RUF059).
signing_key is unpacked but never referenced in the test body. This will be flagged by the CI linter.
🔧 Proposed fix
def test_invalid_signature_is_rejected() -> None:
- signing_key, verify_key = generate_key_pair()
+ _, verify_key = generate_key_pair()
other_signing_key, _ = generate_key_pair()🧰 Tools
🪛 Ruff (0.15.1)
[warning] 41-41: Unpacked variable signing_key is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_crypto.py` around lines 40 - 47, The test
test_invalid_signature_is_rejected unpacks signing_key from generate_key_pair()
but never uses it, causing a linter error; fix by ignoring the unused value when
calling generate_key_pair() (e.g., replace "signing_key, verify_key =
generate_key_pair()" with "_, verify_key = generate_key_pair()" or similar) so
only verify_key and other_signing_key are kept for sign_message and
verify_signature operations.
Comment on lines
+68
to
+69
| @pytest.mark.parametrize( | ||
| "payload,serializer,expected", |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Ruff PT006: prefer a tuple for parametrize argument names.
The string form works but the CI linter prefers the tuple form.
♻️ 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, Replace the string-style
parameter names in the pytest parametrize call with a tuple: update the
pytest.mark.parametrize invocation that currently uses
"payload,serializer,expected" to use a tuple like ("payload", "serializer",
"expected") so the linter rule Ruff PT006 is satisfied; locate the parametrize
call in tests/test_serialization.py and change the first argument accordingly.
Comment on lines
+73
to
+76
| def test_insufficient_balance_is_rejected() -> None: | ||
| sender_key, sender_verify = generate_key_pair() | ||
| recipient_key, recipient_verify = generate_key_pair() | ||
| _ = recipient_key |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Simplify unused key suppression.
The _ = recipient_key pattern to silence linters is verbose. This occurs in multiple tests (lines 54, 76, 95, 114, 135). Prefer unpacking directly with _:
♻️ Proposed fix (apply to all similar occurrences)
- recipient_key, recipient_verify = generate_key_pair()
- _ = recipient_key
+ _, recipient_verify = 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
| def test_insufficient_balance_is_rejected() -> None: | |
| sender_key, sender_verify = generate_key_pair() | |
| recipient_key, recipient_verify = generate_key_pair() | |
| _ = recipient_key | |
| def test_insufficient_balance_is_rejected() -> None: | |
| sender_key, sender_verify = generate_key_pair() | |
| _, recipient_verify = generate_key_pair() |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_state.py` around lines 73 - 76, Replace the verbose unused
variable suppression in tests by unpacking directly into _ when calling
generate_key_pair; specifically in test_insufficient_balance_is_rejected (and
the other affected tests) change assignments that do "recipient_key,
recipient_verify = generate_key_pair(); _ = recipient_key" to unpack the call as
"_, recipient_verify = generate_key_pair()" (or similarly use "_" for any unused
first/second value) so the unused value is ignored at assignment time.
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
Use tuple unpacking to discard the unused signing key instead of _ = other_signing_key.
The _ = other_signing_key pattern on line 52 is a code smell; discard at the unpack site.
♻️ 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, Replace the current two-step
assignment and manual discard with tuple unpacking so the unused signing key is
discarded at assignment: call generate_key_pair() and assign its first element
to the throwaway variable (_) and the second to other_verify_key (i.e., use "_"
for the unused signing key and "other_verify_key" for the verify key) instead of
creating other_signing_key then assigning `_ = other_signing_key`; this change
should be applied where generate_key_pair() is invoked in the test (variables
other_signing_key and other_verify_key).
1 task
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 #20
Summary
balance,nonce)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
Tests
Chores