Skip to content

Comments

feat: canonical serialization layer for tx and block headers#11

Closed
arunabha003 wants to merge 6 commits intoStabilityNexus:mainfrom
arunabha003:issue-8-serialization-layer
Closed

feat: canonical serialization layer for tx and block headers#11
arunabha003 wants to merge 6 commits intoStabilityNexus:mainfrom
arunabha003:issue-8-serialization-layer

Conversation

@arunabha003
Copy link

@arunabha003 arunabha003 commented Feb 17, 2026

Addressed Issues:

Part of #8
Depends on #10

Summary

  • Add canonical deterministic JSON serialization for transactions
  • Add canonical deterministic JSON serialization for block headers
  • Enforce strict required/extra field validation
  • Add serialization test coverage for determinism and mutation sensitivity
  • Include lint-only formatting cleanup via chore: commit

Validation

  • python -m ruff check src tests
  • python -m pytest

Checklist

  • [x ] My PR addresses a single issue, fixes a single bug or makes a single improvement.
  • [x ] My code follows the project's code style and conventions.
  • [x ] If applicable, I have made corresponding changes or additions to the documentation.
  • [x ] If applicable, I have made corresponding changes or additions to tests.
  • [x ] My changes generate no new warnings or errors.
  • [x ] I have joined the Stability Nexus's Discord server and I will share a link to this PR with the project maintainers there.
  • [x ] I have read the Contribution Guidelines.
  • [x ] Once I submit my PR, CodeRabbit AI will automatically review it and I will address CodeRabbit's comments.

⚠️ AI Notice - Important!

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

    • Cryptographic key generation, signing, and verification capabilities
    • Node CLI entrypoint with configurable host and port options
    • Canonical JSON serialization for consensus-critical data
    • Python project configuration with automated CI/CD workflow
  • Documentation

    • Restructured README with simplified setup instructions and common development commands
  • Tests

    • Added comprehensive test coverage for cryptographic operations and serialization logic

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 17, 2026

Walkthrough

This pull request establishes initial project infrastructure for MiniChain, a minimal Python blockchain. It introduces CI/CD automation, project configuration (pyproject.toml, Makefile), cryptographic utilities using PyNaCl, deterministic serialization helpers for consensus-critical data, comprehensive test coverage, and placeholder modules for future blockchain components.

Changes

Cohort / File(s) Summary
CI & Build Configuration
.github/workflows/ci.yml, Makefile, pyproject.toml
Adds GitHub Actions CI workflow that runs lint and tests on push/PR; defines Makefile targets for install, test, lint, format, and node startup; configures Python project with hatchling build system, PyNaCl and optional dev dependencies, and minichain-node console entry point.
Project Documentation
.gitignore, README.md
Extends .gitignore with Python caches, test artifacts, and docs directory; replaces verbose README with concise setup guide, Python-based installation instructions, Common Commands section featuring Makefile targets, and Repository Layout overview.
Package Initialization
src/minichain/__init__.py, src/minichain/__main__.py
Establishes MiniChain package with version 0.1.0 export; adds CLI entry point with argument parser supporting --host and --port options that invoke start_node().
Cryptography & Serialization
src/minichain/crypto.py, src/minichain/serialization.py
Implements Ed25519 key generation, address derivation via blake2b, hex serialization/deserialization, message signing/verification using PyNaCl; provides canonical JSON serialization with strict field validation for transactions and block headers ensuring deterministic consensus-critical serialization.
Module Placeholders
src/minichain/{block,consensus,mempool,network,state,storage,transaction}.py, src/minichain/node.py
Adds module stub files with docstrings indicating future implementation areas; node.py includes start_node() placeholder function.
Test Suite
tests/test_crypto.py, tests/test_scaffold.py, tests/test_serialization.py
Introduces comprehensive tests covering key pair generation, deterministic address derivation, signature verification, key serialization round-trips, module importability, CLI parser defaults, serialization determinism, and field validation for transactions and block headers.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Suggested labels

Python Lang, Make Lang, Documentation, Linter

Poem

🐰 Hop along, a blockchain's born,
With crypto keys and serialized form,
From test to CLI, the scaffold stands tall,
A minimal chain to build it all!
Make test, make lint—let's start the race,
MiniChain blooms in this GitHub space! 🌱

🚥 Pre-merge checks | ✅ 2
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'feat: canonical serialization layer for tx and block headers' directly and concisely describes the main change: adding canonical serialization functionality for transactions and block headers, which is the primary focus of this PR.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 13

🤖 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: Add a pip cache step before installing dependencies to
avoid re-downloading packages each run: insert an actions/cache@v4 step (e.g.,
named "Cache pip") that caches the pip cache directory (path ~/.cache/pip) and
uses a key based on runner OS, python-version and a hash of dependency files
(e.g., requirements.txt or pyproject.lock) so the "Setup Python" step and
subsequent install steps reuse cached packages.

In @.gitignore:
- Around line 338-340: The .gitignore entry "docs/" is too broad and may
silently exclude a future tracked documentation directory; narrow the pattern or
remove it and instead ignore only generated artifacts (e.g., docs/_build,
docs/site, or specific file types) and add a clarifying comment above the rule
explaining intent so maintainers know why those files are excluded; update the
"docs/" pattern to a more specific pattern (or delete it) and add a comment in
the .gitignore near the pattern.

In `@Makefile`:
- Around line 1-21: Add a default all target and a clean target to the Makefile:
create an "all" target that depends on dev-install so running make runs
dev-install by default, and add a "clean" target that removes build artifacts
(e.g., .eggs, any *.egg-info directories, __pycache__ folders, and
.pytest_cache) using rm -rf so contributors can quickly clean the repo; ensure
both targets are listed as .PHONY along with existing targets (install,
dev-install, test, lint, format, start-node).

In `@README.md`:
- Around line 67-70: The README.md tests/ listing is out of sync with this PR:
add the two new test files to the repository layout or remove the stale section;
specifically update the tests/ block in README.md to include test_crypto.py and
test_serialization.py (or delete the tests/ listing entirely) so the README
accurately reflects the repository contents and won't become stale.

In `@src/minichain/crypto.py`:
- Around line 7-16: The variable _NACL_IMPORT_ERROR is only set inside the
except block so it may be undefined when PyNaCl is present; initialize
_NACL_IMPORT_ERROR = None before the try, typing it as Optional[Exception] if
using typing, and keep the existing except assignment to override it on import
failure so functions like _require_nacl and any other code can safely reference
_NACL_IMPORT_ERROR regardless of import success.
- Around line 21-24: The _require_nacl guard currently checks for "blake2b" in
globals(), which is brittle; instead detect the actual import failure by
checking the import-error sentinel used during module import (e.g.,
_NACL_IMPORT_ERROR). Modify _require_nacl to raise RuntimeError when
_NACL_IMPORT_ERROR is truthy (or not None) and include that error when raising
(raise RuntimeError(msg) from _NACL_IMPORT_ERROR), rather than relying on the
presence of "blake2b" in globals(); update references in _require_nacl to use
_NACL_IMPORT_ERROR and keep the existing message text.
- Around line 34-38: In derive_address, avoid computing the default 32-byte
Blake2b digest then slicing; instead call nacl.hash.blake2b with
digest_size=ADDRESS_LENGTH_BYTES to produce a 20-byte digest directly (use the
existing ADDRESS_LENGTH_BYTES constant and the blake2b symbol) and return its
hex form; update the blake2b invocation in derive_address to pass
digest_size=ADDRESS_LENGTH_BYTES while keeping encoder=RawEncoder.

In `@src/minichain/serialization.py`:
- Around line 28-44: The function _to_field_map currently treats Mapping and
object inputs differently by only extracting listed attributes from objects and
silently ignoring any extra attributes; update _to_field_map to build a
consistent source for object inputs (e.g., use vars(value) or value.__dict__
filtered for non-private keys, or inspect attributes) so you can compute missing
and extras the same way as for Mapping, then raise a ValueError on unexpected
extras (or alternatively add a comment documenting an intentional silent-drop
behavior); ensure you still produce the final ordered dict of fields and
reference _to_field_map, source, missing, and extras when making the change.
- Around line 47-56: serialize_canonical currently passes sort_keys=True which
forces alphabetical key order and negates the field_order tuple's intended
ordering; change the implementation to rely on the provided field_order for
canonical ordering by removing sort_keys=True and ensuring the mapping returned
by _to_field_map (and/or the variable canonical_map) preserves insertion order
(e.g., construct an OrderedDict or regular dict in insertion order using the
field_order sequence) so the tuple truly defines the canonical serialization
order; alternatively, if you prefer to keep alphabetical sorting, rename the
field_order symbols to something like TRANSACTION_REQUIRED_FIELDS /
BLOCK_HEADER_REQUIRED_FIELDS to avoid misleading callers.
- Line 6: The import in serialization.py currently uses Mapping from typing
which is deprecated; update the import to pull Mapping from collections.abc
instead (keep Any from typing as-is) by replacing the current "from typing
import Any, Mapping" with an import that references collections.abc.Mapping so
all usages of Mapping in this module continue to work on Python 3.11+.

In `@tests/test_crypto.py`:
- Around line 40-47: The test unpacks signing_key from generate_key_pair() but
never uses it, triggering a linter RUF059; change the local variable name in
test_invalid_signature_is_rejected to _signing_key (leave verify_key as-is) so
the intent is clear and the linter is satisfied while keeping logic in
sign_message, verify_signature, generate_key_pair unchanged.

In `@tests/test_serialization.py`:
- Around line 52-65: Add a deterministic serialization test for block headers:
create two dicts with identical key/value pairs but inserted in different orders
(similar to the transaction determinism test), call serialize_block_header on
both, and assert the returned bytes are equal; add it as a new test (e.g.,
test_block_header_serialization_is_deterministic) alongside the existing
test_changing_block_header_field_changes_serialization and reference the
serialize_block_header function to ensure canonical field ordering is enforced.
- Around line 68-69: The pytest parametrize call uses a comma-separated string
for argument names which Ruff flags; update the pytest.mark.parametrize
invocation to use a tuple of strings instead — replace the current
"payload,serializer,expected" argument with ("payload", "serializer",
"expected") in the pytest.mark.parametrize call so the test function parameters
payload, serializer, and expected are declared idiomatically.

Comment on lines +15 to +18
- name: Setup Python
uses: actions/setup-python@v5
with:
python-version: "3.11"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Consider caching pip dependencies to speed up CI runs.

Adding pip caching avoids re-downloading packages on every run.

♻️ Proposed change
       - 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, Add a pip cache step before
installing dependencies to avoid re-downloading packages each run: insert an
actions/cache@v4 step (e.g., named "Cache pip") that caches the pip cache
directory (path ~/.cache/pip) and uses a key based on runner OS, python-version
and a hash of dependency files (e.g., requirements.txt or pyproject.lock) so the
"Setup Python" step and subsequent install steps reuse cached packages.

Comment on lines +338 to +340


docs/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Ignoring docs/ may silently exclude future committed documentation.

If the project later adds a docs/ directory for user-facing documentation (Sphinx, MkDocs, etc.), this blanket ignore will prevent it from being tracked. Consider narrowing this to the specific generated files you want to exclude, or add a comment clarifying intent.

🤖 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 entry "docs/" is too broad
and may silently exclude a future tracked documentation directory; narrow the
pattern or remove it and instead ignore only generated artifacts (e.g.,
docs/_build, docs/site, or specific file types) and add a clarifying comment
above the rule explaining intent so maintainers know why those files are
excluded; update the "docs/" pattern to a more specific pattern (or delete it)
and add a comment in the .gitignore near the pattern.

Comment on lines +1 to +21
PYTHON ?= python3

.PHONY: install dev-install test lint format start-node

install:
$(PYTHON) -m pip install .

dev-install:
$(PYTHON) -m pip install -e .[dev]

test:
$(PYTHON) -m pytest

lint:
$(PYTHON) -m ruff check src tests

format:
$(PYTHON) -m ruff format src tests

start-node:
PYTHONPATH=src $(PYTHON) -m minichain --host 127.0.0.1 --port 7000
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Consider adding all and clean targets.

Standard Makefile convention includes an all default target and a clean target. For this project, all could alias dev-install and clean could remove build artifacts (.eggs, *.egg-info, __pycache__, .pytest_cache).

♻️ Suggested additions
 PYTHON ?= python3

-.PHONY: install dev-install test lint format start-node
+.PHONY: all install dev-install test lint format start-node clean
+
+all: dev-install

 install:
 	$(PYTHON) -m pip install .
 ...
+
+clean:
+	rm -rf build dist *.egg-info .pytest_cache
+	find . -type d -name __pycache__ -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` around lines 1 - 21, Add a default all target and a clean target to
the Makefile: create an "all" target that depends on dev-install so running make
runs dev-install by default, and add a "clean" target that removes build
artifacts (e.g., .eggs, any *.egg-info directories, __pycache__ folders, and
.pytest_cache) using rm -rf so contributors can quickly clean the repo; ensure
both targets are listed as .PHONY along with existing targets (install,
dev-install, test, lint, format, start-node).

Comment on lines +67 to +70
tests/
test_scaffold.py
issues.md
architectureProposal.md
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Repository layout is incomplete — missing test files added in this PR.

test_crypto.py and test_serialization.py are part of this PR but not listed. Consider keeping this section in sync or removing it to avoid staleness.

📝 Suggested addition
 tests/
   test_scaffold.py
+  test_crypto.py
+  test_serialization.py
 issues.md
📝 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
tests/
test_scaffold.py
issues.md
architectureProposal.md
tests/
test_scaffold.py
test_crypto.py
test_serialization.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 67 - 70, The README.md tests/ listing is out of sync
with this PR: add the two new test files to the repository layout or remove the
stale section; specifically update the tests/ block in README.md to include
test_crypto.py and test_serialization.py (or delete the tests/ listing entirely)
so the README accurately reflects the repository contents and won't become
stale.

Comment on lines +7 to +16
try:
from nacl.encoding import HexEncoder, RawEncoder
from nacl.exceptions import BadSignatureError
from nacl.hash import blake2b
from nacl.signing import SigningKey, VerifyKey
except ModuleNotFoundError as exc: # pragma: no cover - exercised in dependency-light envs
_NACL_IMPORT_ERROR = exc
HexEncoder = RawEncoder = None # type: ignore[assignment]
BadSignatureError = Exception # type: ignore[assignment]
SigningKey = VerifyKey = Any # type: ignore[assignment]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Fallback assignments work but note _NACL_IMPORT_ERROR is only defined in the except branch.

If PyNaCl is installed (the happy path), _NACL_IMPORT_ERROR is never assigned. If any future code path references it outside _require_nacl, you'll get a NameError. Consider initializing it to None before the try block.

🛡️ Proposed fix
+_NACL_IMPORT_ERROR: ModuleNotFoundError | None = None
+
 try:
     from nacl.encoding import HexEncoder, RawEncoder
     ...
📝 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
try:
from nacl.encoding import HexEncoder, RawEncoder
from nacl.exceptions import BadSignatureError
from nacl.hash import blake2b
from nacl.signing import SigningKey, VerifyKey
except ModuleNotFoundError as exc: # pragma: no cover - exercised in dependency-light envs
_NACL_IMPORT_ERROR = exc
HexEncoder = RawEncoder = None # type: ignore[assignment]
BadSignatureError = Exception # type: ignore[assignment]
SigningKey = VerifyKey = Any # type: ignore[assignment]
_NACL_IMPORT_ERROR: ModuleNotFoundError | None = None
try:
from nacl.encoding import HexEncoder, RawEncoder
from nacl.exceptions import BadSignatureError
from nacl.hash import blake2b
from nacl.signing import SigningKey, VerifyKey
except ModuleNotFoundError as exc: # pragma: no cover - exercised in dependency-light envs
_NACL_IMPORT_ERROR = exc
HexEncoder = RawEncoder = None # type: ignore[assignment]
BadSignatureError = Exception # type: ignore[assignment]
SigningKey = VerifyKey = Any # type: ignore[assignment]
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/minichain/crypto.py` around lines 7 - 16, The variable _NACL_IMPORT_ERROR
is only set inside the except block so it may be undefined when PyNaCl is
present; initialize _NACL_IMPORT_ERROR = None before the try, typing it as
Optional[Exception] if using typing, and keep the existing except assignment to
override it on import failure so functions like _require_nacl and any other code
can safely reference _NACL_IMPORT_ERROR regardless of import success.

Comment on lines +28 to +44
def _to_field_map(
value: Mapping[str, Any] | object, field_order: tuple[str, ...]
) -> dict[str, Any]:
if isinstance(value, Mapping):
source = dict(value)
else:
source = {field: getattr(value, field) for field in field_order if hasattr(value, field)}

missing = [field for field in field_order if field not in source]
if missing:
raise ValueError(f"Missing required fields: {', '.join(missing)}")

extras = sorted(set(source) - set(field_order))
if extras:
raise ValueError(f"Unexpected fields: {', '.join(extras)}")

return {field: source[field] for field in field_order}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Object-path silently drops extra attributes — verify this is intentional.

When value is an object (not a Mapping), Line 34 only extracts attributes that match field_order. Any extra attributes on the object are silently ignored — unlike the Mapping path which raises on extras. This asymmetry means an object with a typo'd extra field won't be caught.

If this is intentional (objects naturally have many internal attributes), consider adding a brief comment to document the design choice.

🧰 Tools
🪛 Ruff (0.15.0)

[warning] 38-38: Avoid specifying long messages outside the exception class

(TRY003)


[warning] 42-42: 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/serialization.py` around lines 28 - 44, The function
_to_field_map currently treats Mapping and object inputs differently by only
extracting listed attributes from objects and silently ignoring any extra
attributes; update _to_field_map to build a consistent source for object inputs
(e.g., use vars(value) or value.__dict__ filtered for non-private keys, or
inspect attributes) so you can compute missing and extras the same way as for
Mapping, then raise a ValueError on unexpected extras (or alternatively add a
comment documenting an intentional silent-drop behavior); ensure you still
produce the final ordered dict of fields and reference _to_field_map, source,
missing, and extras when making the change.

Comment on lines +47 to +56
def serialize_canonical(value: Mapping[str, Any] | object, field_order: tuple[str, ...]) -> bytes:
"""Serialize a structure to canonical UTF-8 JSON bytes."""
canonical_map = _to_field_map(value, field_order)
text = json.dumps(
canonical_map,
ensure_ascii=False,
sort_keys=True,
separators=(",", ":"),
)
return text.encode("utf-8")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

sort_keys=True makes the tuple ordering redundant — the tuples are effectively field whitelists.

json.dumps(..., sort_keys=True) always produces alphabetically sorted keys, so the insertion order from field_order on Line 44 has no effect on the output. The tuples serve only as required-field validation sets. The naming *_FIELD_ORDER could mislead future contributors into thinking the tuple order controls serialization order.

Consider either:

  1. Renaming to TRANSACTION_REQUIRED_FIELDS / BLOCK_HEADER_REQUIRED_FIELDS, or
  2. Dropping sort_keys=True and relying on the tuple-defined insertion order (since json.dumps preserves dict insertion order in Python 3.7+), which would give you explicit control over canonical field ordering.

Option 2 is arguably better for a consensus-critical serialization layer — it lets you define the canonical order explicitly rather than depending on alphabetical sort.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/minichain/serialization.py` around lines 47 - 56, serialize_canonical
currently passes sort_keys=True which forces alphabetical key order and negates
the field_order tuple's intended ordering; change the implementation to rely on
the provided field_order for canonical ordering by removing sort_keys=True and
ensuring the mapping returned by _to_field_map (and/or the variable
canonical_map) preserves insertion order (e.g., construct an OrderedDict or
regular dict in insertion order using the field_order sequence) so the tuple
truly defines the canonical serialization order; alternatively, if you prefer to
keep alphabetical sorting, rename the field_order symbols to something like
TRANSACTION_REQUIRED_FIELDS / BLOCK_HEADER_REQUIRED_FIELDS to avoid misleading
callers.

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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Unused variable signing_key flagged by linter.

The unpacked signing_key is never used. Prefix with underscore to signal intent and silence the Ruff RUF059 warning.

♻️ Proposed 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.

Suggested change
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.0)

[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 unpacks signing_key from
generate_key_pair() but never uses it, triggering a linter RUF059; change the
local variable name in test_invalid_signature_is_rejected to _signing_key (leave
verify_key as-is) so the intent is clear and the linter is satisfied while
keeping logic in sign_message, verify_signature, generate_key_pair unchanged.

Comment on lines +52 to +65
def test_changing_block_header_field_changes_serialization() -> None:
base = {
"version": 0,
"previous_hash": "00" * 32,
"merkle_root": "11" * 32,
"timestamp": 123_456_789,
"difficulty_target": 1_000_000,
"nonce": 7,
"block_height": 3,
}
mutated = dict(base)
mutated["nonce"] = 8

assert serialize_block_header(base) != serialize_block_header(mutated)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Consider adding a deterministic serialization test for block headers.

Transaction serialization has an explicit determinism test (identical content, different key order → same bytes). Block headers only have a mutation-sensitivity test but no equivalent determinism check. Adding one would ensure canonical field ordering is verified for both types.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/test_serialization.py` around lines 52 - 65, Add a deterministic
serialization test for block headers: create two dicts with identical key/value
pairs but inserted in different orders (similar to the transaction determinism
test), call serialize_block_header on both, and assert the returned bytes are
equal; add it as a new test (e.g.,
test_block_header_serialization_is_deterministic) alongside the existing
test_changing_block_header_field_changes_serialization and reference the
serialize_block_header function to ensure canonical field ordering is enforced.

Comment on lines +68 to +69
@pytest.mark.parametrize(
"payload,serializer,expected",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Use a tuple for the parametrize argument names (PT006).

Ruff flags the comma-separated string. The idiomatic pytest style is a tuple of strings.

♻️ Proposed fix
 `@pytest.mark.parametrize`(
-    "payload,serializer,expected",
+    ("payload", "serializer", "expected"),
     [
🧰 Tools
🪛 Ruff (0.15.0)

[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 pytest parametrize
call uses a comma-separated string for argument names which Ruff flags; update
the pytest.mark.parametrize invocation to use a tuple of strings instead —
replace the current "payload,serializer,expected" argument with ("payload",
"serializer", "expected") in the pytest.mark.parametrize call so the test
function parameters payload, serializer, and expected are declared
idiomatically.

@arunabha003
Copy link
Author

Superseded by #12 . Closing this PR to keep review history in one place.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant