feat: cli commands for key management chain queries and transaction flow#37
feat: cli commands for key management chain queries and transaction flow#37arunabha003 wants to merge 35 commits intoStabilityNexus:mainfrom
Conversation
WalkthroughThis PR introduces the complete MiniChain blockchain system, establishing core consensus, state management, persistent storage, and transaction handling infrastructure alongside comprehensive testing, CI/CD automation, and a command-line interface. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/CLI
participant Node as MiniChainNode
participant Mempool as Mempool
participant Mining as Mining
participant Chain as ChainManager
participant Storage as SQLiteStorage
User->>Node: submit_transaction(tx)
Node->>Mempool: add_transaction(tx)
Mempool-->>Node: tx_id
User->>Node: mine_one_block(miner_address)
Node->>Mining: build_candidate_block(chain, mempool, miner)
Mining->>Mempool: get_transactions_for_mining(limit)
Mempool-->>Mining: selected_txs
Mining->>Mining: mine_candidate_block(candidate)
Mining-->>Node: mined_block
Node->>Chain: add_block(mined_block)
Chain->>Chain: _replay_state_for_tip(block_hash)
Chain-->>Node: "extended"
Node->>Storage: persist_block_state_and_metadata(block, state)
Storage-->>Node: persisted
Node-->>User: mined_block
sequenceDiagram
participant Storage as SQLiteStorage
participant Chain as ChainManager
participant State as State
participant Block as Block
Storage->>Storage: load_chain_metadata()
Storage-->>Chain: (height, head_hash)
loop for each persisted block
Storage->>Storage: get_block_by_hash(block_hash)
Storage-->>Chain: block
Chain->>Chain: _replay_state_for_tip(block_hash)
Chain->>State: apply_block(block)
State-->>Chain: updated_state
end
Chain-->>Storage: chain_initialized
User->>Chain: add_block(new_block)
Chain->>Chain: _validate_consensus(block, parent_headers)
Chain->>Chain: _replay_state_for_tip(new_block.hash)
Chain-->>Storage: chain_updated
Estimated Code Review Effort🎯 5 (Critical) | ⏱️ ~120 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 |
There was a problem hiding this comment.
Actionable comments posted: 28
♻️ Duplicate comments (1)
tests/test_cli.py (1)
52-54:⚠️ Potential issue | 🟡 MinorIncorrect type annotation for
tmp_path(same issue astest_node.py).
tmp_pathprovides apathlib.Path, notpytest.TempPathFactory. Same applies on line 88.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_cli.py` around lines 52 - 54, The test function test_mine_chain_info_and_balance_commands uses the wrong type for tmp_path; change the parameter annotation from pytest.TempPathFactory to pathlib.Path (i.e., annotate tmp_path: pathlib.Path) and do the same fix for the other test that uses tmp_path (the second occurrence referenced in the review); update imports if necessary to ensure pathlib.Path is available in the test module.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.gitignore:
- Around line 265-270: The .gitignore currently lists .venv/ but misses common
virtualenv names and places docs/ far from the Python block; update the ignore
entries near the Python block (the existing __pycache__/, *.py[cod],
.pytest_cache/, .ruff_cache/, .venv/ lines) to either add venv/, env/, .env/ to
cover common virtualenv names or consolidate into a single pattern (e.g.,
{venv,env,.venv,.env}/); also review the docs/ entry: if docs/ contains
source-controlled hand-written docs, remove it from .gitignore or move/replace
it with only generated subpaths (e.g., docs/_build/), otherwise move the docs/
ignore into this Python/project-specific section so intent is clear.
In `@README.md`:
- Around line 3-12: Update the README wording to stop calling core modules
“placeholder” or “scaffolding” and instead describe them as implemented modules;
specifically change the bullet that lists `crypto`, `transaction`, `block`,
`state`, `mempool`, `consensus`, `network`, `storage`, `node` to state they
contain working implementations (or “implemented core modules”) and adjust the
introductory sentence under "Current Status" to reflect that Issue `#1` delivered
the package layout and implemented module code rather than only scaffolding;
keep the rest of the status text intact and ensure module names (`crypto`,
`transaction`, `block`, `state`, `mempool`, `consensus`, `network`, `storage`,
`node`) are preserved for clarity.
In `@src/minichain/__main__.py`:
- Around line 101-105: The startup message incorrectly claims a network listener
is running; update the handling for command == "start" in
src/minichain/__main__.py so it does not state "started on
{args.host}:{args.port}" unless a socket listener was actually created. Change
the print to indicate the node was initialized locally (e.g., "MiniChain node
initialized (local only)") or add a conditional that prints the host:port only
after a real listener is bound; locate the logic in the try block that checks
command == "start" and adjust the message around node.height/node.tip_hash
accordingly.
- Around line 193-238: The CLI currently lets user validation raise ValueError
(e.g., from _run_submit_transaction) which prints full tracebacks; update the
top-level CLI entry (main) to catch ValueError around command handler
invocation(s) and print a concise error message to stderr (e.g., "Error:
<message>") and exit with non-zero status (sys.exit(1)), while allowing
unexpected exceptions to still surface or be logged separately; ensure you
reference and wrap calls that invoke _run_submit_transaction (and similar
command functions) rather than changing those validators so behavior stays
consistent.
- Around line 44-46: The CLI currently exposes the private key via the submit_tx
parser (--private-key) which leaks secrets; update the submit_tx argument setup
to stop accepting raw private key on the command line and instead add an
optional --private-key-file (path) and/or allow omission to prompt securely, and
then modify the _run_submit_transaction function to read the key from the file
(Path.read_text().strip()) or prompt using getpass.getpass("Private key (hex):
") if neither file nor env var is provided; also update the help strings for
submit_tx.add_argument entries to reflect the new --private-key-file and remove
encouragement to pass raw keys on the CLI.
- Around line 261-264: Remove the duplicated helper _is_lower_hex and centralize
it: create a single implementation in a new module minichain.utils (exporting
_is_lower_hex) and replace the local copies in __main__.py, transaction.py,
storage.py, node.py, mining.py, and genesis.py with an import from
minichain.utils; ensure the function signature and behavior remain identical and
update any tests/usages to import the symbol from minichain.utils instead of
defining it locally.
In `@src/minichain/block.py`:
- Around line 29-34: Block.hash() and hash_hex() currently recompute
serialize_block_header(self) -> blake2b_digest(...) on each call; change them to
compute once and cache the result (e.g. use functools.cached_property on
Block.hash or implement a lazy slot like self._hash set on first call) and have
hash_hex() return the cached hex string (or cached cached_property) to avoid
repeated serialize_block_header and blake2b_digest work; reference the existing
methods Block.hash, Block.hash_hex and the helpers serialize_block_header and
blake2b_digest and ensure the cached value is used on subsequent calls.
In `@src/minichain/consensus.py`:
- Line 7: The import currently uses the deprecated typing.Sequence; update the
import to use collections.abc.Sequence instead by replacing the statement that
imports Sequence from typing (the "from typing import Sequence" line) with an
import from collections.abc so all uses of the Sequence type annotation in this
module continue to work but avoid the deprecated typing alias.
- Around line 92-99: The loop currently calls replace(header_template,
nonce=nonce) each iteration which allocates a new BlockHeader per nonce;
instead, make a single mutable BlockHeader copy once before the for-loop (so you
don't mutate the original header_template), then update its nonce field in-place
inside the loop and call candidate.hash() each iteration; keep the stop_event
check and the existing hash_to_int(candidate.hash()) <=
candidate.difficulty_target test, and remove the per-iteration call to
replace(header_template, nonce=nonce).
In `@src/minichain/genesis.py`:
- Around line 20-27: GenesisConfig's frozen=True doesn't prevent in-place
mutation of the initial_balances dict; wrap and freeze that mapping in
__post_init__ so downstream code can't mutate it. In GenesisConfig.__post_init__
convert initial_balances to an immutable mapping (e.g.,
types.MappingProxyType(dict(self.initial_balances))) and assign it back using
object.__setattr__(self, "initial_balances", <mapping>) so the frozen dataclass
accepts the change; reference the GenesisConfig class, the initial_balances
field, the __post_init__ method, types.MappingProxyType, and object.__setattr__
when making the change.
In `@src/minichain/mempool.py`:
- Around line 146-166: The code currently recomputes sender pools twice in
remove_confirmed_transactions: first for touched_senders, then again for every
sender via iterating self._sender_pools; remove the redundant full sweep. Keep
the loop that calls self._recompute_sender_pool(sender, state) for each sender
in touched_senders and delete the subsequent loop "for sender in
list(self._sender_pools): self._recompute_sender_pool(sender, state)" so only
touched senders are recomputed.
In `@src/minichain/mining.py`:
- Around line 68-84: The docstring for mine_candidate_block does not document
that it propagates MiningInterrupted (when stop_event triggers) and RuntimeError
(when nonce range is exhausted via mine_block_header); update the docstring of
mine_candidate_block to explicitly list these exceptions (raise
MiningInterrupted: if mining was interrupted via stop_event; raise RuntimeError:
if no valid nonce found within the provided range) and reference that these come
from mine_block_header so callers know to handle them.
- Around line 87-90: The helper _is_lower_hex is duplicated across
transaction.py, storage.py, node.py, mining.py, genesis.py, and __main__.py;
extract a single implementation into a shared utilities module (e.g., create a
utilities function named _is_lower_hex in a new utils module) and replace the
local copies by importing that function in each of the six modules
(transaction.py, storage.py, node.py, mining.py, genesis.py, __main__.py);
ensure the new utility has the same signature (value: str, expected_length: int)
and behavior, remove the duplicate definitions, update the import lines, and run
tests/lint to confirm no name collisions or unresolved imports.
In `@src/minichain/node.py`:
- Around line 92-112: The start method opens SQLiteStorage(db_path) and then
calls _initialize_chain_manager; if _initialize_chain_manager raises the SQLite
connection (storage) remains open — ensure storage is closed on failure by
wrapping the chain manager initialization in a try/except/finally (or try with
immediate close on exception) so that if _initialize_chain_manager(...) raises
you call storage.close() (or storage.shutdown()) before re-raising; update the
start() flow to only assign self._storage/self._chain_manager after successful
initialization to avoid leaving partially-initialized state.
In `@src/minichain/serialization.py`:
- Around line 47-56: serialize_canonical currently calls json.dumps(...,
sort_keys=True) which ignores the ordering produced by _to_field_map and
therefore makes the provided field_order tuples misleading; choose one of two
fixes: (A) if you intend to preserve the supplied field_order in the output,
remove sort_keys=True from json.dumps so the mapping returned by _to_field_map
is serialized in that order, and ensure _to_field_map returns an OrderedDict
preserving the tuple order; or (B) if alphabetical keys are intended, rename the
field-order constants (e.g. FIELD_ORDER) to names that do not imply
serialization order (e.g. TRANSACTION_FIELDS or BLOCK_HEADER_FIELDS) and add a
clarifying comment that the tuple is only used for validation/allowed fields;
update references to serialize_canonical, _to_field_map and the FIELD_ORDER
symbols accordingly.
- Line 6: Replace the deprecated typing.Mapping import: change the import
statement that currently reads "from typing import Any, Mapping" so that Mapping
is imported from collections.abc instead (e.g., "from typing import Any" and
"from collections.abc import Mapping") to address the UP035 deprecation; keep
the symbol name Mapping unchanged so existing function/type annotations continue
to work.
In `@src/minichain/state.py`:
- Around line 40-43: get_account currently mutates state by inserting Account()
into self.accounts on reads, which breaks the genesis emptiness guard used by
apply_genesis_block; change the API so reads are non-mutating: implement a
read-only accessor (e.g., has_account(address) or peek_account(address) that
returns None or Account without creating it) and rename the mutating behavior to
a clear get_or_create_account(address) that creates and returns an Account, or
alter get_account to only return existing accounts and raise/return None if
missing; update call sites in genesis.py (the if state.accounts check) and any
tests to use the appropriate read-only accessor or the explicit
get_or_create_account so reads no longer have side effects.
- Around line 95-101: Remove the unused module-level wrapper functions
apply_transaction and apply_block from this module: they simply delegate to
State.apply_transaction and State.apply_block and are not referenced elsewhere;
delete the two top-level functions (apply_transaction(state: State, transaction:
Transaction) and apply_block(state: State, block: Block, *, block_reward: int =
0)) so callers use the instance methods directly and the module no longer
exports these redundant symbols.
In `@src/minichain/storage.py`:
- Around line 254-280: The _row_to_block function currently parses
transactions_json and constructs Transaction objects directly, which can raise
generic exceptions for malformed JSON or transaction payloads; wrap the JSON
parsing and Transaction construction (the transaction_dicts =
json.loads(str(transactions_json)) and transactions = [Transaction(**tx) for tx
in transaction_dicts] steps) in a try/except that catches ValueError, TypeError,
json.JSONDecodeError and any other construction errors and re-raise them as
StorageError with a clear message including the underlying exception; keep the
existing merkle_root validation and raise StorageError for parsing/construct
failures so callers of _row_to_block / Block / StorageError can uniformly handle
storage corruption.
- Around line 221-232: In load_chain_metadata, validate and wrap parsing errors
so malformed data raises StorageError: check that kv contains "height" and
"head_hash", ensure kv["height"] can be converted to int inside a try/except and
raise StorageError on ValueError, and validate kv["head_hash"] is a non-empty
string (or matches expected format) raising StorageError for invalid values;
catch any unexpected exceptions during parsing and rethrow them as StorageError
with a clear message so callers get consistent error types (reference
load_chain_metadata and StorageError).
In `@src/minichain/transaction.py`:
- Around line 23-26: The helper _is_lower_hex can raise on non-string inputs;
update _is_lower_hex to first check that value is an instance of str and return
False immediately if not (so verify() won't throw), then keep the existing
length check and hex-character check (using the existing "0123456789abcdef"
membership test on the string); reference the _is_lower_hex function and the
verify() call site so the change prevents verify() from raising when
sender/recipient are None or other non-strings.
In `@tests/test_block.py`:
- Around line 75-77: The current test test_block_hash_is_deterministic only
calls block.hash() twice on the same instance which is trivial; instead create
two independently constructed blocks with identical content (use _make_block to
build both but ensure deterministic key/tx content or seed the transactions so
they match, avoiding generate_key_pair randomness) and assert block1.hash() ==
block2.hash(); alternatively serialize the block and assert
hash(serialize(block)) == hash(serialize(deserialized_block)) or compute h1 =
block.hash(); h2 = Block.from_serialized(block.serialize()).hash() (use the
project’s Block serialization/deserialization helpers) to exercise the
serialization→hash pipeline.
In `@tests/test_consensus.py`:
- Around line 76-96: Replace the manual try/except/else patterns in
test_mining_honors_stop_event and test_mining_raises_when_nonce_range_exhausted
with pytest.raises context managers: import pytest at the top, then in
test_mining_honors_stop_event wrap the mine_block_header(...) call with
pytest.raises(MiningInterrupted) and assert the exception message contains
"interrupted"; in test_mining_raises_when_nonce_range_exhausted wrap
mine_block_header(header, start_nonce=0, max_nonce=10) with
pytest.raises(RuntimeError) and assert "No valid nonce found" is in the
exception string; keep references to the existing helper _header_template and
the mine_block_header call sites.
In `@tests/test_crypto.py`:
- Around line 40-47: The test function test_invalid_signature_is_rejected
contains an unused binding signing_key which triggers a lint warning; rename it
to _signing_key (or _) when calling generate_key_pair() so the variable is
explicitly marked unused, e.g. change "signing_key, verify_key =
generate_key_pair()" to use "_signing_key, verify_key = generate_key_pair()" and
keep other_signing_key, message, wrong_signature, sign_message, and
verify_signature unchanged.
In `@tests/test_genesis.py`:
- Around line 60-85: Replace the try/except/else patterns in the tests
test_genesis_requires_empty_state and
test_genesis_block_rejects_wrong_previous_hash with pytest.raises(...)
assertions: call apply_genesis_block(...) inside a with
pytest.raises(ValueError, match="...") context (use "empty state" for the first
test and "previous_hash" for the second) so the tests assert both the exception
type and message; locate these tests around the use of create_genesis_block,
GenesisConfig and apply_genesis_block to update them to the pytest.raises form.
In `@tests/test_mining.py`:
- Around line 18-60: The duplicated test helpers _build_manager and
_signed_transaction appear across multiple test files; extract them into a
shared test module (e.g., tests/conftest.py or tests/helpers.py) and update
imports/fixtures to use the shared functions so all tests reference a single
implementation. Move the _build_manager and _signed_transaction definitions into
the new shared file, export or register them (as fixtures if you prefer pytest
fixtures) and replace the local copies in test_state.py, test_node.py,
test_mining.py, and test_cli.py with imports from the shared module (referencing
the same function names _build_manager and _signed_transaction). Ensure any
test-specific defaults (like genesis_target or ChainConfig values) remain
configurable via parameters or wrapper fixtures so existing tests keep their
behavior.
In `@tests/test_node.py`:
- Around line 40-42: The three test functions
test_node_start_initializes_and_persists_genesis,
test_node_mine_block_then_reload_from_disk, and
test_accept_block_persists_chain_head use the wrong type for the pytest tmp_path
fixture; change the parameter annotation from pytest.TempPathFactory to
pathlib.Path (i.e., import pathlib.Path if not already imported) so each
function signature uses tmp_path: pathlib.Path to match the actual fixture type.
In `@tests/test_storage.py`:
- Line 59: Change the tmp_path parameter type annotation from
pytest.TempPathFactory to pathlib.Path in the test functions that use the pytest
tmp_path fixture: update the signature of
test_store_and_load_block_round_trip(tmp_path: ...) to use tmp_path:
pathlib.Path, and make the same change for the two other test functions in this
file that currently annotate tmp_path as pytest.TempPathFactory (the other two
functions that take the tmp_path fixture). Ensure you import pathlib.Path at the
top if not already imported and update all three function signatures
consistently.
---
Duplicate comments:
In `@tests/test_cli.py`:
- Around line 52-54: The test function test_mine_chain_info_and_balance_commands
uses the wrong type for tmp_path; change the parameter annotation from
pytest.TempPathFactory to pathlib.Path (i.e., annotate tmp_path: pathlib.Path)
and do the same fix for the other test that uses tmp_path (the second occurrence
referenced in the review); update imports if necessary to ensure pathlib.Path is
available in the test module.
ℹ️ Review info
Configuration used: Path: .coderabbit.yml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (36)
.github/workflows/ci.yml.gitignoreMakefileREADME.mdpyproject.tomlsrc/minichain/__init__.pysrc/minichain/__main__.pysrc/minichain/block.pysrc/minichain/chain.pysrc/minichain/consensus.pysrc/minichain/crypto.pysrc/minichain/genesis.pysrc/minichain/mempool.pysrc/minichain/merkle.pysrc/minichain/mining.pysrc/minichain/network.pysrc/minichain/node.pysrc/minichain/serialization.pysrc/minichain/state.pysrc/minichain/storage.pysrc/minichain/transaction.pytests/test_block.pytests/test_chain.pytests/test_cli.pytests/test_consensus.pytests/test_crypto.pytests/test_genesis.pytests/test_mempool.pytests/test_merkle.pytests/test_mining.pytests/test_node.pytests/test_scaffold.pytests/test_serialization.pytests/test_state.pytests/test_storage.pytests/test_transaction.py
| # Python caches and virtualenvs | ||
| __pycache__/ | ||
| *.py[cod] | ||
| .pytest_cache/ | ||
| .ruff_cache/ | ||
| .venv/ |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
docs/ placement and coverage gaps in venv patterns
Two independent nits:
-
docs/placement (lines 338-340): Thedocs/entry is appended after ~70 lines of unrelated LaTeX content rather than grouped with the project-specific block here. More importantly, the PR summary notes that hand-written documentation lives underdocs/. Ignoring the entire directory will silently prevent those files from ever being staged or committed — confirm this is intentional (e.g., docs are fully generated) or movedocs/to a committed location and only ignore the generated sub-path. -
Incomplete venv coverage:
.venv/covers only one naming convention.venv/,env/, and.env/are equally common and will not be excluded.
♻️ Suggested additions near the Python block
# Python caches and virtualenvs
__pycache__/
*.py[cod]
.pytest_cache/
.ruff_cache/
.venv/
+venv/
+env/
+.env/Move docs/ here too if the intent is to exclude generated documentation, or remove it entirely if docs/ contains source-controlled content.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.gitignore around lines 265 - 270, The .gitignore currently lists .venv/ but
misses common virtualenv names and places docs/ far from the Python block;
update the ignore entries near the Python block (the existing __pycache__/,
*.py[cod], .pytest_cache/, .ruff_cache/, .venv/ lines) to either add venv/,
env/, .env/ to cover common virtualenv names or consolidate into a single
pattern (e.g., {venv,env,.venv,.env}/); also review the docs/ entry: if docs/
contains source-controlled hand-written docs, remove it from .gitignore or
move/replace it with only generated subpaths (e.g., docs/_build/), otherwise
move the docs/ ignore into this Python/project-specific section so intent is
clear.
| MiniChain is a minimal, research-oriented blockchain implementation in Python. This repository currently contains the project scaffolding and development environment for the v0 core chain roadmap. | ||
|
|
||
| | ||
| ## Current Status | ||
|
|
||
| <!-- Organization Name --> | ||
| <div align="center"> | ||
| Issue #1 (project scaffolding) is implemented with: | ||
|
|
||
| [](https://TODO.stability.nexus/) | ||
| - Python package layout under `src/minichain` | ||
| - Placeholder component modules for: | ||
| - `crypto`, `transaction`, `block`, `state`, `mempool`, `consensus`, `network`, `storage`, `node` | ||
| - `pyproject.toml` project configuration |
There was a problem hiding this comment.
Update README wording to reflect implemented modules.
The text still frames core modules as “placeholder”/“scaffolding,” which is misleading now that these modules contain real implementations.
Suggested wording update
-MiniChain is a minimal, research-oriented blockchain implementation in Python. This repository currently contains the project scaffolding and development environment for the v0 core chain roadmap.
+MiniChain is a minimal, research-oriented blockchain implementation in Python. This repository contains the initial core chain implementation and development environment for the v0 roadmap.
@@
-- Placeholder component modules for:
+- Core component modules for:📝 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.
| MiniChain is a minimal, research-oriented blockchain implementation in Python. This repository currently contains the project scaffolding and development environment for the v0 core chain roadmap. | |
| | |
| ## Current Status | |
| <!-- Organization Name --> | |
| <div align="center"> | |
| Issue #1 (project scaffolding) is implemented with: | |
| [](https://TODO.stability.nexus/) | |
| - Python package layout under `src/minichain` | |
| - Placeholder component modules for: | |
| - `crypto`, `transaction`, `block`, `state`, `mempool`, `consensus`, `network`, `storage`, `node` | |
| - `pyproject.toml` project configuration | |
| MiniChain is a minimal, research-oriented blockchain implementation in Python. This repository contains the initial core chain implementation and development environment for the v0 roadmap. | |
| ## Current Status | |
| Issue `#1` (project scaffolding) is implemented with: | |
| - Python package layout under `src/minichain` | |
| - Core component modules for: | |
| - `crypto`, `transaction`, `block`, `state`, `mempool`, `consensus`, `network`, `storage`, `node` | |
| - `pyproject.toml` project configuration |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 3 - 12, Update the README wording to stop calling
core modules “placeholder” or “scaffolding” and instead describe them as
implemented modules; specifically change the bullet that lists `crypto`,
`transaction`, `block`, `state`, `mempool`, `consensus`, `network`, `storage`,
`node` to state they contain working implementations (or “implemented core
modules”) and adjust the introductory sentence under "Current Status" to reflect
that Issue `#1` delivered the package layout and implemented module code rather
than only scaffolding; keep the rest of the status text intact and ensure module
names (`crypto`, `transaction`, `block`, `state`, `mempool`, `consensus`,
`network`, `storage`, `node`) are preserved for clarity.
| submit_tx = subparsers.add_parser("submit-tx", help="Submit a signed transfer transaction") | ||
| submit_tx.add_argument("--private-key", required=True, help="hex-encoded Ed25519 signing key") | ||
| submit_tx.add_argument("--recipient", required=True, help="20-byte lowercase hex address") |
There was a problem hiding this comment.
Security: private key passed as a CLI argument is exposed in process listings and shell history.
--private-key will appear in ps aux, /proc/*/cmdline, and shell history, leaking the signing key. Consider reading it from stdin, an environment variable, or a file path instead.
🔒 Suggested approach
- submit_tx.add_argument("--private-key", required=True, help="hex-encoded Ed25519 signing key")
+ submit_tx.add_argument(
+ "--private-key",
+ default=None,
+ help="hex-encoded Ed25519 signing key (prefer --private-key-file or stdin)",
+ )
+ submit_tx.add_argument(
+ "--private-key-file",
+ default=None,
+ help="path to file containing hex-encoded Ed25519 signing key",
+ )Then in _run_submit_transaction, read from file or prompt via getpass:
import getpass
if private_key_hex is None and private_key_file:
private_key_hex = Path(private_key_file).read_text().strip()
elif private_key_hex is None:
private_key_hex = getpass.getpass("Private key (hex): ")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/minichain/__main__.py` around lines 44 - 46, The CLI currently exposes
the private key via the submit_tx parser (--private-key) which leaks secrets;
update the submit_tx argument setup to stop accepting raw private key on the
command line and instead add an optional --private-key-file (path) and/or allow
omission to prompt securely, and then modify the _run_submit_transaction
function to read the key from the file (Path.read_text().strip()) or prompt
using getpass.getpass("Private key (hex): ") if neither file nor env var is
provided; also update the help strings for submit_tx.add_argument entries to
reflect the new --private-key-file and remove encouragement to pass raw keys on
the CLI.
| try: | ||
| if command == "start": | ||
| print(f"MiniChain node started on {args.host}:{args.port}") | ||
| print(f"chain_height={node.height} tip={node.tip_hash}") | ||
| return |
There was a problem hiding this comment.
Misleading "started on host:port" message — no network listener is implemented.
The start command prints "MiniChain node started on {args.host}:{args.port}" but the node only initializes local chain state; no socket is bound. This will confuse users into thinking a server is running.
🔧 Proposed fix
if command == "start":
- print(f"MiniChain node started on {args.host}:{args.port}")
+ print("MiniChain node initialized")
print(f"chain_height={node.height} tip={node.tip_hash}")
return📝 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.
| try: | |
| if command == "start": | |
| print(f"MiniChain node started on {args.host}:{args.port}") | |
| print(f"chain_height={node.height} tip={node.tip_hash}") | |
| return | |
| try: | |
| if command == "start": | |
| print("MiniChain node initialized") | |
| print(f"chain_height={node.height} tip={node.tip_hash}") | |
| return |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/minichain/__main__.py` around lines 101 - 105, The startup message
incorrectly claims a network listener is running; update the handling for
command == "start" in src/minichain/__main__.py so it does not state "started on
{args.host}:{args.port}" unless a socket listener was actually created. Change
the print to indicate the node was initialized locally (e.g., "MiniChain node
initialized (local only)") or add a conditional that prints the host:port only
after a real listener is bound; locate the logic in the try block that checks
command == "start" and adjust the message around node.height/node.tip_hash
accordingly.
| def _run_submit_transaction( | ||
| *, | ||
| node: MiniChainNode, | ||
| private_key_hex: str, | ||
| recipient: str, | ||
| amount: int, | ||
| fee: int, | ||
| nonce: int | None, | ||
| mine_now: bool, | ||
| ) -> None: | ||
| if amount < 0: | ||
| raise ValueError("amount must be non-negative") | ||
| if fee < 0: | ||
| raise ValueError("fee must be non-negative") | ||
| if not _is_lower_hex(recipient, ADDRESS_HEX_LENGTH): | ||
| raise ValueError("recipient must be a 20-byte lowercase hex string") | ||
|
|
||
| signing_key = deserialize_signing_key(private_key_hex) | ||
| sender_address = derive_address(signing_key.verify_key) | ||
| sender_account = node.chain_manager.state.get_account(sender_address) | ||
| resolved_nonce = sender_account.nonce if nonce is None else nonce | ||
| if resolved_nonce < 0: | ||
| raise ValueError("nonce must be non-negative") | ||
|
|
||
| transaction = Transaction( | ||
| sender=sender_address, | ||
| recipient=recipient, | ||
| amount=amount, | ||
| nonce=resolved_nonce, | ||
| fee=fee, | ||
| timestamp=int(time.time()), | ||
| ) | ||
| transaction.sign(signing_key) | ||
|
|
||
| transaction_id = node.submit_transaction(transaction) | ||
| print(f"submitted_tx_id={transaction_id}") | ||
| print(f"sender={sender_address}") | ||
| print(f"recipient={recipient}") | ||
|
|
||
| if not mine_now: | ||
| print("queued_in_mempool=true") | ||
| return | ||
|
|
||
| mined_block = node.mine_one_block() | ||
| print(f"mined_block_height={mined_block.header.block_height}") | ||
| print(f"mined_block_hash={mined_block.hash().hex()}") |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
User-facing validation errors raise ValueError with raw tracebacks.
CLI input errors (invalid address, negative amount, etc.) raise ValueError, producing a full Python traceback for the user. Consider catching these at the main() level and printing a clean error message with a non-zero exit code instead.
♻️ Suggested pattern
+import sys
+
def main(argv: list[str] | None = None) -> None:
+ try:
+ _main_inner(argv)
+ except (ValueError, Exception) as exc:
+ print(f"error: {exc}", file=sys.stderr)
+ sys.exit(1)
+
+def _main_inner(argv: list[str] | None = None) -> None:
args = build_parser().parse_args(argv)
...Or use a more targeted catch around specific command handlers.
🧰 Tools
🪛 Ruff (0.15.2)
[warning] 204-204: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 206-206: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 208-208: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 215-215: 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/__main__.py` around lines 193 - 238, The CLI currently lets
user validation raise ValueError (e.g., from _run_submit_transaction) which
prints full tracebacks; update the top-level CLI entry (main) to catch
ValueError around command handler invocation(s) and print a concise error
message to stderr (e.g., "Error: <message>") and exit with non-zero status
(sys.exit(1)), while allowing unexpected exceptions to still surface or be
logged separately; ensure you reference and wrap calls that invoke
_run_submit_transaction (and similar command functions) rather than changing
those validators so behavior stays consistent.
| 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) |
There was a problem hiding this comment.
Avoid unused signing_key binding in test.
This triggers the reported lint warning; rename to _signing_key or _.
🔧 Suggested fix
- signing_key, verify_key = generate_key_pair()
+ _signing_key, 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.
| 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) | |
| 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) |
🧰 Tools
🪛 Ruff (0.15.2)
[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 function
test_invalid_signature_is_rejected contains an unused binding signing_key which
triggers a lint warning; rename it to _signing_key (or _) when calling
generate_key_pair() so the variable is explicitly marked unused, e.g. change
"signing_key, verify_key = generate_key_pair()" to use "_signing_key, verify_key
= generate_key_pair()" and keep other_signing_key, message, wrong_signature,
sign_message, and verify_signature unchanged.
| 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") |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Replace try/except/else with pytest.raises — both error-case tests use the fragile PT017 pattern.
The try/except/else pattern is fragile: if a ValueError is raised for an unrelated reason, the assert … in str(exc) check may still pass and the raise AssertionError branch is never reached. pytest.raises with match= enforces both the exception type and the message in a single expression.
♻️ Proposed refactor
+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.2)
[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 try/except/else
patterns in the tests test_genesis_requires_empty_state and
test_genesis_block_rejects_wrong_previous_hash with pytest.raises(...)
assertions: call apply_genesis_block(...) inside a with
pytest.raises(ValueError, match="...") context (use "empty state" for the first
test and "previous_hash" for the second) so the tests assert both the exception
type and message; locate these tests around the use of create_genesis_block,
GenesisConfig and apply_genesis_block to update them to the pytest.raises form.
| def _build_manager( | ||
| *, | ||
| initial_balances: dict[str, int], | ||
| genesis_target: int = MAX_TARGET, | ||
| ) -> ChainManager: | ||
| genesis_block, genesis_state = create_genesis_state( | ||
| GenesisConfig( | ||
| initial_balances=initial_balances, | ||
| timestamp=1_739_000_000, | ||
| difficulty_target=genesis_target, | ||
| ) | ||
| ) | ||
| return ChainManager( | ||
| genesis_block=genesis_block, | ||
| genesis_state=genesis_state, | ||
| config=ChainConfig( | ||
| block_reward=50, | ||
| difficulty_adjustment_interval=10, | ||
| target_block_time_seconds=30, | ||
| ), | ||
| ) | ||
|
|
||
|
|
||
| def _signed_transaction( | ||
| *, | ||
| sender_key: object, | ||
| sender_address: str, | ||
| recipient: str, | ||
| amount: int, | ||
| nonce: int, | ||
| fee: int, | ||
| timestamp: int, | ||
| ) -> Transaction: | ||
| tx = Transaction( | ||
| sender=sender_address, | ||
| recipient=recipient, | ||
| amount=amount, | ||
| nonce=nonce, | ||
| fee=fee, | ||
| timestamp=timestamp, | ||
| ) | ||
| tx.sign(sender_key) | ||
| return tx |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider extracting shared test helpers.
_signed_transaction and _build_manager patterns are duplicated (with slight variations) across test_state.py, test_node.py, test_mining.py, and test_cli.py. Consider extracting them into a shared tests/conftest.py or tests/helpers.py module to reduce drift across copies.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_mining.py` around lines 18 - 60, The duplicated test helpers
_build_manager and _signed_transaction appear across multiple test files;
extract them into a shared test module (e.g., tests/conftest.py or
tests/helpers.py) and update imports/fixtures to use the shared functions so all
tests reference a single implementation. Move the _build_manager and
_signed_transaction definitions into the new shared file, export or register
them (as fixtures if you prefer pytest fixtures) and replace the local copies in
test_state.py, test_node.py, test_mining.py, and test_cli.py with imports from
the shared module (referencing the same function names _build_manager and
_signed_transaction). Ensure any test-specific defaults (like genesis_target or
ChainConfig values) remain configurable via parameters or wrapper fixtures so
existing tests keep their behavior.
| def test_node_start_initializes_and_persists_genesis( | ||
| tmp_path: pytest.TempPathFactory, | ||
| ) -> None: |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find and display the exact lines mentioned in the review
sed -n '40,42p' tests/test_node.py
sed -n '73,75p' tests/test_node.py
sed -n '128,130p' tests/test_node.pyRepository: StabilityNexus/MiniChain
Length of output: 342
🏁 Script executed:
# Check imports in the test file
head -20 tests/test_node.pyRepository: StabilityNexus/MiniChain
Length of output: 639
🏁 Script executed:
# Search for all tmp_path type annotations in the test file
rg -n 'tmp_path:' tests/test_node.pyRepository: StabilityNexus/MiniChain
Length of output: 191
🏁 Script executed:
# Check if pathlib.Path is already imported
rg -n 'from pathlib|import pathlib' tests/test_node.pyRepository: StabilityNexus/MiniChain
Length of output: 50
🏁 Script executed:
# Check the full imports section of the file to see what's available
head -30 tests/test_node.pyRepository: StabilityNexus/MiniChain
Length of output: 851
🏁 Script executed:
# Let's verify pytest fixture types using web search strategy or by checking Python type stubs
python3 << 'EOF'
import inspect
import pytest
# Check the type hints for tmp_path fixture
print("Checking pytest.TempPathFactory type...")
print(f"TempPathFactory: {pytest.TempPathFactory}")
# Let's also check what the actual tmp_path fixture returns
# by looking at pytest's type stubs or documentation
EOFRepository: StabilityNexus/MiniChain
Length of output: 165
🏁 Script executed:
# Check if there's any use of tmp_path in the tests to understand what it's actually doing
rg -A 5 'def test_node' tests/test_node.py | head -40Repository: StabilityNexus/MiniChain
Length of output: 506
🏁 Script executed:
# Check pytest type stubs or actual fixture definitions
python3 << 'EOF'
import pytest
import inspect
from pathlib import Path
# Let's check what the tmp_path fixture actually returns
# by looking at pytest's fixture registry
print("Checking tmp_path fixture type from pytest...")
# Try to get the fixture info
try:
import _pytest.tmpdir
# Check the TempdirFactory and tmp_path fixtures
print(f"TempPathFactory class exists: {_pytest.tmpdir.TempPathFactory}")
print(f"TempPathFactory signature: {inspect.signature(_pytest.tmpdir.TempPathFactory)}")
except Exception as e:
print(f"Error: {e}")
# Let's check online documentation or use the pytest module directly
# The tmp_path fixture is documented to return pathlib.Path
print("\nAccording to pytest documentation:")
print("- tmp_path: Returns pathlib.Path")
print("- tmp_path_factory: Returns TempPathFactory")
EOFRepository: StabilityNexus/MiniChain
Length of output: 494
🏁 Script executed:
# Let's look at actual usage of tmp_path in the file to see what operations are performed
rg -B 2 -A 8 'tmp_path / ' tests/test_node.py | head -30Repository: StabilityNexus/MiniChain
Length of output: 985
🏁 Script executed:
# Let's verify the full context of how tmp_path is used in the first test
sed -n '40,60p' tests/test_node.pyRepository: StabilityNexus/MiniChain
Length of output: 798
🏁 Script executed:
# Let's double-check by looking at what pathlib.Path supports
python3 << 'EOF'
from pathlib import Path
# Verify that Path supports the / operator
p = Path("/tmp")
result = p / "subdir"
print(f"Path / str operation result type: {type(result)}")
print(f"Result: {result}")
# Verify TempPathFactory does NOT support this operation
import pytest
import _pytest.tmpdir
try:
factory = _pytest.tmpdir.TempPathFactory(None, 3, "none", None)
# This should fail
result = factory / "subdir"
except TypeError as e:
print(f"\nTempPathFactory / operation error: {e}")
EOFRepository: StabilityNexus/MiniChain
Length of output: 348
Incorrect type annotation for tmp_path fixture.
The tmp_path pytest fixture provides a pathlib.Path, not a pytest.TempPathFactory. The latter is the type for the tmp_path_factory fixture. This same mistake is repeated on lines 73 and 128.
🔧 Proposed fix
+from pathlib import Path
-def test_node_start_initializes_and_persists_genesis(
- tmp_path: pytest.TempPathFactory,
-) -> None:
+def test_node_start_initializes_and_persists_genesis(
+ tmp_path: Path,
+) -> None:Apply the same change to test_node_mine_block_then_reload_from_disk (line 73) and test_accept_block_persists_chain_head (line 128).
📝 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.
| def test_node_start_initializes_and_persists_genesis( | |
| tmp_path: pytest.TempPathFactory, | |
| ) -> None: | |
| from pathlib import Path | |
| def test_node_start_initializes_and_persists_genesis( | |
| tmp_path: Path, | |
| ) -> None: |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_node.py` around lines 40 - 42, The three test functions
test_node_start_initializes_and_persists_genesis,
test_node_mine_block_then_reload_from_disk, and
test_accept_block_persists_chain_head use the wrong type for the pytest tmp_path
fixture; change the parameter annotation from pytest.TempPathFactory to
pathlib.Path (i.e., import pathlib.Path if not already imported) so each
function signature uses tmp_path: pathlib.Path to match the actual fixture type.
| return block | ||
|
|
||
|
|
||
| def test_store_and_load_block_round_trip(tmp_path: pytest.TempPathFactory) -> None: |
There was a problem hiding this comment.
tmp_path parameter is annotated as pytest.TempPathFactory — the correct type is pathlib.Path.
tmp_path is the path-fixture that yields a pathlib.Path per test. pytest.TempPathFactory is the type of the separate tmp_path_factory fixture. The wrong annotation will be flagged by mypy/pyright and can confuse readers.
🔧 Proposed fix (all three affected signatures)
+import pathlib
-def test_store_and_load_block_round_trip(tmp_path: pytest.TempPathFactory) -> None:
+def test_store_and_load_block_round_trip(tmp_path: pathlib.Path) -> None:
-def test_state_and_metadata_persist_across_restart(
- tmp_path: pytest.TempPathFactory,
-) -> None:
+def test_state_and_metadata_persist_across_restart(
+ tmp_path: pathlib.Path,
+) -> None:
-def test_atomic_persist_rolls_back_on_metadata_failure(
- tmp_path: pytest.TempPathFactory,
-) -> None:
+def test_atomic_persist_rolls_back_on_metadata_failure(
+ tmp_path: pathlib.Path,
+) -> None:Also applies to: 107-109, 132-134
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_storage.py` at line 59, Change the tmp_path parameter type
annotation from pytest.TempPathFactory to pathlib.Path in the test functions
that use the pytest tmp_path fixture: update the signature of
test_store_and_load_block_round_trip(tmp_path: ...) to use tmp_path:
pathlib.Path, and make the same change for the two other test functions in this
file that currently annotate tmp_path as pytest.TempPathFactory (the other two
functions that take the tmp_path fixture). Ensure you import pathlib.Path at the
top if not already imported and update all three function signatures
consistently.
|
Superseded by #38. Closing this PR to keep review history in one place. |
Addressed Issues:
Part of #8
Depends on #36
Summary
mine-now]
support
immediately
commands
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
Documentation
Chores