feat: transaction structure and validation#12
feat: transaction structure and validation#12arunabha003 wants to merge 8 commits intoStabilityNexus:mainfrom
Conversation
WalkthroughThis PR introduces the initial scaffolding for the MiniChain Python project, including project configuration, GitHub Actions CI/CD, cryptographic utilities with Ed25519 signing, transaction data structures with validation, deterministic serialization for consensus-critical data, a CLI entrypoint, node orchestration, placeholder modules for future features, and comprehensive unit tests. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Transaction
participant Crypto
participant Serialization
User->>Transaction: create(sender, recipient, amount, ...)
User->>Transaction: sign(signing_key)
Transaction->>Transaction: signing_payload()
Transaction->>Serialization: serialize_transaction(payload)
Serialization-->>Transaction: canonical bytes
Transaction->>Crypto: sign_message(bytes, signing_key)
Crypto-->>Transaction: signature (hex)
Transaction->>Crypto: serialize_verify_key(verify_key)
Crypto-->>Transaction: public_key (hex)
User->>Transaction: verify()
Transaction->>Transaction: validate_fields()
Transaction->>Crypto: deserialize_verify_key(public_key_hex)
Crypto-->>Transaction: verify_key
Transaction->>Crypto: derive_address(verify_key)
Crypto-->>Transaction: address
Transaction->>Transaction: check sender == derived_address
Transaction->>Serialization: serialize_transaction(payload)
Serialization-->>Transaction: canonical bytes
Transaction->>Crypto: verify_signature(bytes, signature_hex, verify_key)
Crypto-->>Transaction: valid (bool)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
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}
There was a problem hiding this comment.
Actionable comments posted: 18
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 15-18: The Setup Python step using actions/setup-python@v5 is
missing pip dependency caching; add the cache key (cache: "pip") to the step
that invokes actions/setup-python@v5 (the block containing python-version:
"3.11") so GitHub Actions will cache and restore pip wheels between runs and
speed up CI.
- Around line 8-10: The CI job named "test" currently runs only on a single
runner; add a strategy matrix to the "test" job to run across supported Python
versions (e.g., matrix.python-version: ["3.11","3.12"]) and update the test
steps to use actions/setup-python with python-version: ${{ matrix.python-version
}} so the "test" job executes for each Python version declared in the matrix.
In @.gitignore:
- Around line 265-270: The .gitignore currently ignores ".venv/" but not other
common virtualenv directories; update the .gitignore to also ignore "venv/" and
"env/" so developers who create virtual environments with python -m venv venv or
python -m venv env don't accidentally commit them—add entries for venv/ and env/
alongside the existing .venv/ rule to cover all three common virtualenv
directories.
- Around line 338-340: The current ignore pattern "docs/" will silently prevent
committing any documentation; update the pattern to be more targeted (for
example ignore specific generated output like "docs/_build/" or "/docs/_build/"
or add negation rules such as "!docs/*.md" or "!/docs/**" to allow authored
docs) so authored docs can be tracked; locate the "docs/" entry in the
.gitignore and replace or refine it with the targeted pattern(s) that match only
generated artefacts or add negation rules to permit committed documentation.
In `@README.md`:
- Around line 5-15: Update the "Current Status" section to reflect the new
features added by this PR: explicitly mention Issue `#8` and list the implemented
components — cryptographic utilities, deterministic serialization, and
transaction validation — and note any new or updated modules (e.g., the crypto
utilities and transaction validation code under src/minichain). Keep the same
terse bullet style as existing items and ensure the text clearly indicates these
additions are implemented so new contributors aren't misled.
- Around line 53-70: The README layout omits the real module
src/minichain/serialization.py which tests reference; add a new module file
named serialization.py under src/minichain implementing the serialization
functionality expected by tests (ensure it exports the functions/classes
referenced by COMPONENT_MODULES and tests/test_serialization.py), and update the
README listing to include "serialization.py" in the src/minichain/ file list so
the repository layout matches the actual modules used by tests.
In `@src/minichain/__init__.py`:
- Around line 3-4: Replace the hardcoded __version__ string with a runtime
lookup from package metadata: import importlib.metadata (or importlib_metadata
for older Python) and set __version__ = importlib.metadata.version("minichain")
inside a try/except that catches PackageNotFoundError (or Exception) and falls
back to a sensible default like "0.0.0" or "unknown"; ensure __all__ still
exports "__version__" and keep the module-level name __version__ as the single
source of truth so the value comes from pyproject metadata rather than being
hardcoded.
In `@src/minichain/__main__.py`:
- Line 13: The --port argument accepts any int; add a validator function (e.g.,
validate_port) and use it as the type for parser.add_argument to restrict values
to the TCP port range; implement validate_port(value) to parse int, raise
argparse.ArgumentTypeError for non-ints or values outside 1..65535, then change
the parser.add_argument("--port", default=7000, type=int, ...) call to use
type=validate_port and update the help text to indicate the valid range.
In `@src/minichain/serialization.py`:
- Around line 28-44: The function _to_field_map treats Mapping inputs and object
instances differently—Mappings are validated for unexpected keys while object
attributes outside field_order are silently ignored—so add a concise docstring
to _to_field_map describing this intentional asymmetry: explain that when value
is a Mapping unexpected keys will raise a ValueError, whereas when value is an
object only attributes in field_order are considered (extra attributes are
ignored), and document why (e.g., to exclude fields like signature/public_key
from signing payloads) and how callers should pass data if they want strict
validation; reference the function name _to_field_map and the parameters value
and field_order in the docstring so future maintainers understand the design
choice.
- Line 6: Replace the deprecated typing.Mapping import with the canonical
collections.abc.Mapping: update the import statement in serialization.py so that
Mapping is imported from collections.abc while retaining Any from typing (i.e.,
keep any usages of Mapping in functions/types unchanged, just change the import
source).
In `@src/minichain/transaction.py`:
- Around line 87-90: In the verify() method replace the broad "except Exception"
around deserialize_verify_key(self.public_key) with specific exception types
(e.g., ValueError and nacl.exceptions.CryptoError or the exact exceptions thrown
by deserialize_verify_key) so it only swallows expected deserialization errors;
update the except clause to catch those specific exceptions and return False,
and add any needed imports for nacl.exceptions so the change compiles.
- Around line 55-68: The _validate_common_fields method currently uses
isinstance(..., int) which accepts bools; update the checks for amount, nonce,
fee, and timestamp in _validate_common_fields to exclude booleans (e.g., require
type(x) is int or use isinstance(x, int) and not isinstance(x, bool)) so
True/False no longer pass validation; keep the existing non-negative checks
intact and target the variables amount, nonce, fee, timestamp inside
_validate_common_fields.
- Around line 70-76: The sign method in class Transaction (sign) currently types
signing_key as object which prevents static checking and leads to runtime
AttributeError; import and use the SigningKey type from the crypto module and
update the sign method signature to accept signing_key: SigningKey so tools can
catch wrong types—modify the top-of-file imports to include SigningKey and
change the sign(self, signing_key: object) -> None declaration to use SigningKey
instead while keeping the existing body that accesses signing_key.verify_key.
In `@tests/test_crypto.py`:
- Around line 40-47: Test unpacks signing_key but doesn't use it; change the
unused variable in test_invalid_signature_is_rejected by prefixing it with an
underscore (e.g., _signing_key) when calling generate_key_pair() so Ruff RUF059
is satisfied; keep verify_key and other_signing_key as-is and ensure
sign_message and verify_signature calls remain unchanged.
In `@tests/test_scaffold.py`:
- Line 1: Update the module docstring at the top of tests/test_scaffold.py to
reference the correct issue number: change "Issue `#1`" to "Issue `#8`" so the
docstring accurately reflects the current PR and the added import of
serialization.
- Around line 21-24: The assertion assert imported is not None in
test_component_modules_are_importable is vacuous because importlib.import_module
either returns a module or raises; remove that assertion and instead rely on
importlib.import_module(f"minichain.{module}") to raise on failure (or
optionally wrap in pytest.importorskip or assert isinstance(imported,
types.ModuleType) if you want an explicit check). Update the
test_component_modules_are_importable to iterate COMPONENT_MODULES and import
each module with importlib.import_module without the redundant None-check.
In `@tests/test_serialization.py`:
- Around line 68-96: The pytest.mark.parametrize call is using a comma-separated
string for the parameter names; change it to an explicit tuple of strings so
lint rule Ruff PT006 is satisfied. Edit the decorator in
tests/test_serialization.py (the parametrize that passes payload,
serialize_transaction/serialize_block_header, expected) to use ("payload",
"serializer", "expected") instead of "payload,serializer,expected" so the test
still parametrizes the same values but uses the idiomatic tuple form.
In `@tests/test_transaction.py`:
- Around line 49-55: The test defines an unused other_signing_key and assigns it
to `_` as a lint workaround; instead, unpack generate_key_pair() directly into
(_, other_verify_key) in test_mismatched_public_key_and_sender_is_rejected to
remove the redundant `_ = other_signing_key` line and keep other_verify_key for
serialize_verify_key usage.
| jobs: | ||
| test: | ||
| runs-on: ubuntu-latest |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
# Find and display the CI workflow file
fd -e yml -g 'ci.yml' --exec cat -n {}Repository: StabilityNexus/MiniChain
Length of output: 50
🏁 Script executed:
# Find and display pyproject.toml if it exists
fd -e toml -g 'pyproject.toml' --exec cat -n {}Repository: StabilityNexus/MiniChain
Length of output: 1044
🏁 Script executed:
# List files in .github/workflows directory
ls -la .github/workflows/ 2>/dev/null || echo "Directory not found"Repository: StabilityNexus/MiniChain
Length of output: 231
🏁 Script executed:
# Find all yml/yaml files in .github directory
fd -e yml -o -e yaml . .github/ 2>/dev/null | head -20Repository: StabilityNexus/MiniChain
Length of output: 50
🏁 Script executed:
# Search for CI configuration files more broadly
find .github -type f \( -name "*.yml" -o -name "*.yaml" \) 2>/dev/nullRepository: StabilityNexus/MiniChain
Length of output: 92
🏁 Script executed:
cat -n .github/workflows/ci.ymlRepository: StabilityNexus/MiniChain
Length of output: 782
Add Python version matrix to test against supported versions.
The CI currently tests only against Python 3.11, but pyproject.toml declares requires-python = ">=3.11". Testing against multiple supported versions catches compatibility regressions early:
♻️ Proposed refactor
jobs:
test:
+ strategy:
+ matrix:
+ python-version: ["3.11", "3.12"]
runs-on: ubuntu-latest
steps:
...
- name: Setup Python
uses: actions/setup-python@v5
with:
- python-version: "3.11"
+ python-version: ${{ matrix.python-version }}
+ 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 8 - 10, The CI job named "test"
currently runs only on a single runner; add a strategy matrix to the "test" job
to run across supported Python versions (e.g., matrix.python-version:
["3.11","3.12"]) and update the test steps to use actions/setup-python with
python-version: ${{ matrix.python-version }} so the "test" job executes for each
Python version declared in the matrix.
| - name: Setup Python | ||
| uses: actions/setup-python@v5 | ||
| with: | ||
| python-version: "3.11" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add pip dependency caching to speed up CI.
setup-python@v5 natively supports pip caching via a single cache key; without it every run re-downloads and reinstalls all packages from scratch.
⚡ Proposed fix
- 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.
| - name: Setup Python | |
| uses: actions/setup-python@v5 | |
| with: | |
| python-version: "3.11" | |
| - name: Setup Python | |
| uses: actions/setup-python@v5 | |
| with: | |
| python-version: "3.11" | |
| cache: "pip" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci.yml around lines 15 - 18, The Setup Python step using
actions/setup-python@v5 is missing pip dependency caching; add the cache key
(cache: "pip") to the step that invokes actions/setup-python@v5 (the block
containing python-version: "3.11") so GitHub Actions will cache and restore pip
wheels between runs and speed up CI.
| # Python caches and virtualenvs | ||
| __pycache__/ | ||
| *.py[cod] | ||
| .pytest_cache/ | ||
| .ruff_cache/ | ||
| .venv/ |
There was a problem hiding this comment.
venv/ and env/ virtualenv directories are not covered.
Line 270 covers .venv/ only. Developers using python -m venv venv or python -m venv env (both extremely common naming conventions) will find those directories unignored. Consider adding:
🔧 Proposed fix
# Python caches and virtualenvs
__pycache__/
*.py[cod]
.pytest_cache/
.ruff_cache/
.venv/
+venv/
+env/
+dist/
+*.egg-info/
+.coverage
+htmlcov/📝 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.
| # Python caches and virtualenvs | |
| __pycache__/ | |
| *.py[cod] | |
| .pytest_cache/ | |
| .ruff_cache/ | |
| .venv/ | |
| # Python caches and virtualenvs | |
| __pycache__/ | |
| *.py[cod] | |
| .pytest_cache/ | |
| .ruff_cache/ | |
| .venv/ | |
| venv/ | |
| env/ | |
| dist/ | |
| *.egg-info/ | |
| .coverage | |
| htmlcov/ |
🤖 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 ignores ".venv/"
but not other common virtualenv directories; update the .gitignore to also
ignore "venv/" and "env/" so developers who create virtual environments with
python -m venv venv or python -m venv env don't accidentally commit them—add
entries for venv/ and env/ alongside the existing .venv/ rule to cover all three
common virtualenv directories.
|
|
||
|
|
||
| docs/ |
There was a problem hiding this comment.
docs/ exclusion silently prevents committing any future documentation.
This blanket pattern will block generated or authored docs from ever being tracked without an explicit git add -f or negation rule. If generated API docs (e.g., Sphinx _build/) are the actual target, prefer a more targeted pattern:
🔧 More targeted alternative
-docs/
+docs/_build/📝 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.
| docs/ | |
| docs/_build/ |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.gitignore around lines 338 - 340, The current ignore pattern "docs/" will
silently prevent committing any documentation; update the pattern to be more
targeted (for example ignore specific generated output like "docs/_build/" or
"/docs/_build/" or add negation rules such as "!docs/*.md" or "!/docs/**" to
allow authored docs) so authored docs can be tracked; locate the "docs/" entry
in the .gitignore and replace or refine it with the targeted pattern(s) that
match only generated artefacts or add negation rules to permit committed
documentation.
| ## 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 | ||
| - `Makefile` for common developer tasks | ||
| - Basic CI workflow (`.github/workflows/ci.yml`) | ||
| - Baseline tests under `tests/` |
There was a problem hiding this comment.
"Current Status" section only reflects Issue #1 scaffolding.
This PR adds cryptographic utilities, deterministic serialization, and transaction validation (Issue #8), but the status section doesn't mention these additions. The section will mislead new contributors about what is actually implemented.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~14-~14: The official name of this software platform is spelled with a capital “H”.
Context: ...on developer tasks - Basic CI workflow (.github/workflows/ci.yml) - Baseline tests und...
(GITHUB)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 5 - 15, Update the "Current Status" section to
reflect the new features added by this PR: explicitly mention Issue `#8` and list
the implemented components — cryptographic utilities, deterministic
serialization, and transaction validation — and note any new or updated modules
(e.g., the crypto utilities and transaction validation code under
src/minichain). Keep the same terse bullet style as existing items and ensure
the text clearly indicates these additions are implemented so new contributors
aren't misled.
| 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.
Prefix unused signing_key with _ (Ruff RUF059).
signing_key is unpacked but never referenced in the test body; only verify_key is used.
🐛 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()📝 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: | |
| _, 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.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, Test unpacks signing_key but
doesn't use it; change the unused variable in test_invalid_signature_is_rejected
by prefixing it with an underscore (e.g., _signing_key) when calling
generate_key_pair() so Ruff RUF059 is satisfied; keep verify_key and
other_signing_key as-is and ensure sign_message and verify_signature calls
remain unchanged.
| @@ -0,0 +1,32 @@ | |||
| """Scaffolding checks for Issue #1.""" | |||
There was a problem hiding this comment.
Stale issue reference in module docstring.
The docstring says Issue #1`` but this PR (and this test file, which now imports serialization) pertains to Issue `#8`.
📝 Proposed fix
-"""Scaffolding checks for Issue `#1`."""
+"""Scaffolding checks for Issues `#1` and `#8`."""📝 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.
| """Scaffolding checks for Issue #1.""" | |
| """Scaffolding checks for Issues `#1` and `#8`.""" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_scaffold.py` at line 1, Update the module docstring at the top of
tests/test_scaffold.py to reference the correct issue number: change "Issue `#1`"
to "Issue `#8`" so the docstring accurately reflects the current PR and the added
import of serialization.
| def test_component_modules_are_importable() -> None: | ||
| for module in COMPONENT_MODULES: | ||
| imported = importlib.import_module(f"minichain.{module}") | ||
| assert imported is not None |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
assert imported is not None is a vacuous assertion.
importlib.import_module either returns a module object or raises ImportError/ModuleNotFoundError; it never returns None. The real intent — catching import failures — is already served by the implicit exception propagation. The assertion always evaluates to True and adds no meaningful test value.
♻️ Proposed fix
def test_component_modules_are_importable() -> None:
for module in COMPONENT_MODULES:
- imported = importlib.import_module(f"minichain.{module}")
- assert imported is not None
+ importlib.import_module(f"minichain.{module}")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_component_modules_are_importable() -> None: | |
| for module in COMPONENT_MODULES: | |
| imported = importlib.import_module(f"minichain.{module}") | |
| assert imported is not None | |
| def test_component_modules_are_importable() -> None: | |
| for module in COMPONENT_MODULES: | |
| importlib.import_module(f"minichain.{module}") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_scaffold.py` around lines 21 - 24, The assertion assert imported
is not None in test_component_modules_are_importable is vacuous because
importlib.import_module either returns a module or raises; remove that assertion
and instead rely on importlib.import_module(f"minichain.{module}") to raise on
failure (or optionally wrap in pytest.importorskip or assert
isinstance(imported, types.ModuleType) if you want an explicit check). Update
the test_component_modules_are_importable to iterate COMPONENT_MODULES and
import each module with importlib.import_module without the redundant
None-check.
| @pytest.mark.parametrize( | ||
| "payload,serializer,expected", | ||
| [ | ||
| ( | ||
| { | ||
| "sender": "aa" * 20, | ||
| "recipient": "bb" * 20, | ||
| "amount": 1, | ||
| "nonce": 1, | ||
| "timestamp": 1, | ||
| }, | ||
| serialize_transaction, | ||
| "Missing required fields: fee", | ||
| ), | ||
| ( | ||
| { | ||
| "version": 0, | ||
| "previous_hash": "00" * 32, | ||
| "merkle_root": "11" * 32, | ||
| "timestamp": 1, | ||
| "difficulty_target": 1, | ||
| "nonce": 1, | ||
| "block_height": 1, | ||
| "extra": "x", | ||
| }, | ||
| serialize_block_header, | ||
| "Unexpected fields: extra", | ||
| ), | ||
| ], |
There was a problem hiding this comment.
Use a tuple for the pytest.mark.parametrize names argument (Ruff PT006).
A comma-separated string is accepted by pytest but a tuple is the idiomatic, lint-clean form.
🐛 Proposed fix
`@pytest.mark.parametrize`(
- "payload,serializer,expected",
+ ("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 - 96, The
pytest.mark.parametrize call is using a comma-separated string for the parameter
names; change it to an explicit tuple of strings so lint rule Ruff PT006 is
satisfied. Edit the decorator in tests/test_serialization.py (the parametrize
that passes payload, serialize_transaction/serialize_block_header, expected) to
use ("payload", "serializer", "expected") instead of
"payload,serializer,expected" so the test still parametrizes the same values but
uses the idiomatic tuple form.
| def test_mismatched_public_key_and_sender_is_rejected() -> None: | ||
| tx, _ = _build_signed_transaction() | ||
| other_signing_key, other_verify_key = generate_key_pair() | ||
| _ = other_signing_key | ||
| tampered = replace(tx, public_key=serialize_verify_key(other_verify_key)) | ||
|
|
||
| assert not tampered.verify() |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Simplify unused variable handling.
_ = other_signing_key on line 52 is a workaround for lint. Use _ directly in the unpacking instead.
♻️ Proposed fix
def test_mismatched_public_key_and_sender_is_rejected() -> None:
tx, _ = _build_signed_transaction()
- other_signing_key, other_verify_key = generate_key_pair()
- _ = other_signing_key
+ _, other_verify_key = generate_key_pair()
tampered = replace(tx, public_key=serialize_verify_key(other_verify_key))
assert not tampered.verify()📝 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_mismatched_public_key_and_sender_is_rejected() -> None: | |
| tx, _ = _build_signed_transaction() | |
| other_signing_key, other_verify_key = generate_key_pair() | |
| _ = other_signing_key | |
| tampered = replace(tx, public_key=serialize_verify_key(other_verify_key)) | |
| assert not tampered.verify() | |
| def test_mismatched_public_key_and_sender_is_rejected() -> None: | |
| tx, _ = _build_signed_transaction() | |
| _, other_verify_key = generate_key_pair() | |
| tampered = replace(tx, public_key=serialize_verify_key(other_verify_key)) | |
| assert not tampered.verify() |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_transaction.py` around lines 49 - 55, The test defines an unused
other_signing_key and assigns it to `_` as a lint workaround; instead, unpack
generate_key_pair() directly into (_, other_verify_key) in
test_mismatched_public_key_and_sender_is_rejected to remove the redundant `_ =
other_signing_key` line and keep other_verify_key for serialize_verify_key
usage.
|
Superseded by #18 . Closing this PR to keep review history in one place. |
1 participant
Addressed Issues:
Part of #8
Depends on #11
Summary
Validation
python -m ruff check src testspython -m pytestChecklist
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
New Features
Chores