Skip to content

Feat/fee handling#91

Open
SIDDHANTCOOKIE wants to merge 6 commits into
StabilityNexus:mainfrom
SIDDHANTCOOKIE:feat/fee-handling
Open

Feat/fee handling#91
SIDDHANTCOOKIE wants to merge 6 commits into
StabilityNexus:mainfrom
SIDDHANTCOOKIE:feat/fee-handling

Conversation

@SIDDHANTCOOKIE
Copy link
Copy Markdown
Member

@SIDDHANTCOOKIE SIDDHANTCOOKIE commented Jun 5, 2026

Addressed Issues:

This PR implements the transaction fee system. The system now fully supports senders attaching optional fees to their transactions, and miners collecting those fees as a reward for network participation!
Added a dedicated test case (test_transaction_fee) in test_core.py to ensure exact mathematical balances across sender, receiver, and miner.

Screenshots/Recordings:

TODO: If applicable, add screenshots or recordings that demonstrate the interface before and after the changes.

Additional Notes:

AI Usage Disclosure:

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. AI slop is strongly discouraged and may lead to banning and blocking. Do not spam our repos with AI slop.

Check one of the checkboxes below:

  • This PR does not contain AI-generated code at all.
  • This PR contains AI-generated code. I have read the AI Usage Policy and this PR complies with this policy. I have tested the code locally and I am responsible for it.

I have used the following AI models and tools: TODO

Checklist

  • My PR addresses a single issue, fixes a single bug or makes a single improvement.
  • My code follows the project's code style and conventions
  • If applicable, I have made corresponding changes or additions to the documentation
  • If applicable, I have made corresponding changes or additions to tests
  • My changes generate no new warnings or errors
  • I have joined the Discord server and I will share a link to this PR with the project maintainers there
  • I have read the Contribution Guidelines
  • Once I submit my PR, CodeRabbit AI will automatically review it and I will address CodeRabbit's comments.
  • I have filled this PR template completely and carefully, and I understand that my PR may be closed without review otherwise.

Summary by CodeRabbit

  • New Features

    • Transactions now support configurable fees
    • Receipts track transaction execution status and details
    • Blocks include receipt verification and root validation
  • Improvements

    • Mining rewards now incorporate collected transaction fees
    • Mempool prioritizes transactions by fee
    • Enhanced error reporting for transaction validation failures

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Jun 5, 2026

Review Change Stack

Walkthrough

This PR introduces a transaction fee system and comprehensive receipt tracking for the MiniChain blockchain. Transactions now carry a fee field that is deducted from the sender's balance. State application returns structured Receipt objects instead of booleans, capturing transaction outcomes including status codes and error details. Blocks include receipt roots and receipts lists for consensus validation. Mining rewards are augmented by collected transaction fees.

Changes

Transaction Fees and Receipt Tracking System

Layer / File(s) Summary
Transaction Fee Field
minichain/transaction.py
Transaction now includes a fee parameter (default 0) in the constructor, serialization, and deserialization. Fee is part of _TX_FIELDS, so it affects transaction IDs and signatures.
Receipt Data Type
minichain/receipt.py
New Receipt class models transaction outcomes with tx_hash, status, optional gas_used/error_message/logs/contract_address. Provides serialization and deserialization with sensible defaults.
State Validation with Fee-Aware Costs and Receipt Returns
minichain/state.py
validate_and_apply() returns None for invalid transactions; apply_transaction() returns Receipt instead of boolean. Balance checks use amount + fee. Contract failures return failure Receipt with error messages. Nonce is retained but balance is refunded on failure.
Block Receipt Integration and Merkle Root Computation
minichain/block.py
New calculate_receipt_root() computes receipt Merkle root. Block now accepts receipt_root and receipts parameters, freezes receipts into tuples, derives receipt root when not provided, includes receipt root in headers and receipts list in bodies, and validates receipt root matches on deserialization.
Chain Genesis and Block Consensus Validation
minichain/chain.py
Genesis block explicitly initializes receipt_root=None and receipts=[]. Block validation accumulates receipts, rejects on failed transactions, verifies receipt_root and receipts payloads match, and computes total_fees for mining reward augmentation.
Mining and Fee-Based Transaction Prioritization
main.py, minichain/mempool.py
Mining captures receipts and accumulates only successful transactions. Miner is credited with base reward plus sum of transaction fees. Mempool prioritizes transactions by descending fee, then timestamp, sender, and nonce.
P2P Protocol Message Validation
minichain/p2p.py
Transaction validation requires fee integer field and allows zero amounts. Block validation requires receipt_root and receipts fields with per-receipt schema enforcing tx_hash and status, validating optional typed fields.
Persistence Block Serialization
minichain/persistence.py
Block persistence now uses Block.from_dict() directly, removing the internal _deserialize_block() helper.
MPT Implementation Refactored to HexaryTrie
minichain/mpt.py, requirements.txt
Custom MPT implementation replaced with thin HexaryTrie wrapper. Dependency updated: libp2p==0.5.0 removed, trie>=3.1.0 added.
Test Suite Updates for Receipts and Fees
tests/test_contract.py, tests/test_core.py, tests/test_persistence.py, tests/test_persistence_runtime.py, tests/test_protocol_hardening.py, tests/test_transaction_signing.py
Contract tests validate receipt outcomes via status and error messages. New comprehensive fee test validates deduction and mining reward augmentation. Persistence tests verify receipt field preservation. Tampering tests use object.__setattr__ for immutable fields. Protocol tests construct full Block/Receipt objects.
Logging and Package Configuration
minichain/contract.py, setup.py, minichain/state.py
Contract logging converted to parameterized format. setup.py removed. Import ordering in state.py adjusted.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

Suggested labels

Python Lang

Suggested reviewers

  • Zahnentferner

Poem

🐰 A rabbit hops through the fee-filled chain,
Receipts in pockets, rewards to gain,
Transactions now carry their weight in weights,
While blocks tally up all the contract fates,
Merkle roots dance as the miners sing praise! 🎵

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Feat/fee handling' directly corresponds to the PR's primary objective of implementing a transaction fee system. It accurately reflects the main change across the codebase.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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
Copy Markdown
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: 7

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
tests/test_core.py (2)

59-60: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick win

Use explicit None check for failed transactions.

assertFalse(result) works because None is falsy, but assertIsNone(result) makes the expected behavior explicit and aligns with the receipt API contract.

✅ Proposed improvement
         result = self.state.apply_transaction(tx)
-        self.assertFalse(result)
+        self.assertIsNone(result)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_core.py` around lines 59 - 60, The test currently uses a falsy
check for a failed transaction result; replace the generic
self.assertFalse(result) with an explicit self.assertIsNone(result) to make the
contract that apply_transaction(tx) returns None on failure explicit — locate
the call to self.state.apply_transaction(tx) and the assertion on the local
variable result and update the assertion accordingly.

45-46: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick win

Update assertion to validate receipt status explicitly.

With the receipt-based API, assertTrue(result) is semantically unclear—it relies on receipt truthiness rather than validating success status.

✅ Proposed improvement
         result = self.state.apply_transaction(tx)
-        self.assertTrue(result)
+        self.assertIsNotNone(result)
+        self.assertEqual(result.status, 1)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_core.py` around lines 45 - 46, The test currently uses a
truthiness check (self.assertTrue(result)) after calling
self.state.apply_transaction(tx); replace this with an explicit assertion on the
transaction receipt status returned by apply_transaction: capture the receipt
(e.g., receipt = self.state.apply_transaction(tx)) and assert its success status
explicitly (for example self.assertEqual(receipt.status, 'SUCCESS') or
self.assertEqual(receipt['status'], 'SUCCESS') or
self.assertEqual(receipt.status, ReceiptStatus.SUCCESS) depending on whether
receipt is an object, dict, or uses an enum) so the test verifies the actual
success indicator rather than truthiness.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@minichain/block.py`:
- Line 56: In the Block class constructor remove the duplicate assignment of
self.miner so the field is only set once; locate the __init__ (or constructor)
where self.miner is assigned twice and delete one of the assignments (keep the
one that is in the correct logical place for initialization), ensuring no other
initialization logic depends on the removed line and that the remaining
assignment is the authoritative value for the miner attribute.

In `@minichain/mpt.py`:
- Around line 17-20: The get() method currently treats any non-None bytes
(including b'') as a present value, so missing keys that return b'' become empty
strings instead of None; update the logic in get (which calls self._trie.get) to
treat empty bytes as missing by returning None when val is None or len(val) ==
0, otherwise return val.decode(); change the condition from "if val is not None"
to an explicit emptiness check against val from self._trie.get.

In `@minichain/p2p.py`:
- Around line 143-144: The transaction validation currently checks
payload["amount"] but misses validating the fee; update the P2P-side validation
(the block that checks payload["amount"] in minichain/p2p.py, e.g., inside the
transaction validation/handler function) to also enforce payload["fee"] >= 0 and
return False when the fee is negative. Locate the same validation scope that
contains the line `if payload["amount"] < 0: return False` and add a similar
guard for `payload["fee"]`, ensuring the check uses the same semantics (reject
negative fees) and mirrors error handling/return behavior.

In `@requirements.txt`:
- Line 2: The requirements entry for the package "trie" currently uses an
open-ended range "trie>=3.1.0"; change it to a bounded or compatible specifier
to avoid accidental major upgrades — e.g., replace with "trie>=3.1.0,<4.0.0" or
"trie~=3.1.0" in requirements.txt so installations remain reproducible and safe
from breaking changes.

In `@tests/test_core.py`:
- Around line 65-114: Add a new unit test named
test_insufficient_balance_with_fee in tests/test_core.py that asserts a
transaction where amount alone is <= balance but amount + fee > balance is
rejected; ensure the test uses Transaction(..., fee=...) and signs it, then
calls the state's validation/apply method (validate_and_apply or
apply_transaction depending on existing API) and expects None for the failing
case and a non-None receipt for the borderline succeeding case. If tests fail,
fix the state validation in State.validate_and_apply / State.apply_transaction
to check total_cost = amount + fee (and nonce) against sender balance (use
get_account or accounts[...] to read balances) and return None / raise/abort
when balance < total_cost so fees are correctly considered in balance
validation.
- Around line 105-107: Don't mutate chain.state.accounts directly; instead use
the State/Chain public API to set balances and nonces so tests don't rely on
internal dict layout. Replace the three direct assignments to
chain.state.accounts[self.alice_pk]['balance'] / ['nonce'] and
chain.state.accounts[self.bob_pk]['balance'] with the appropriate API calls
(e.g. state.set_balance(self.alice_pk, 100), state.set_nonce(self.alice_pk, 0),
state.set_balance(self.bob_pk, 0) or
Chain.set_account_balance/Chain.set_account_nonce if those exist), or add small
test helpers on State/Chain to perform these updates atomically, referencing
chain.state and self.alice_pk/self.bob_pk from the diff.
- Around line 84-94: Remove the unused manual Block construction: the local
variable block created via Block(...) is never used because
mine_and_process_block later builds and processes its own block; delete the
entire Block(...) instantiation (the variable named block) from the test to
eliminate dead code and potential confusion, leaving the call to
mine_and_process_block intact.

---

Outside diff comments:
In `@tests/test_core.py`:
- Around line 59-60: The test currently uses a falsy check for a failed
transaction result; replace the generic self.assertFalse(result) with an
explicit self.assertIsNone(result) to make the contract that
apply_transaction(tx) returns None on failure explicit — locate the call to
self.state.apply_transaction(tx) and the assertion on the local variable result
and update the assertion accordingly.
- Around line 45-46: The test currently uses a truthiness check
(self.assertTrue(result)) after calling self.state.apply_transaction(tx);
replace this with an explicit assertion on the transaction receipt status
returned by apply_transaction: capture the receipt (e.g., receipt =
self.state.apply_transaction(tx)) and assert its success status explicitly (for
example self.assertEqual(receipt.status, 'SUCCESS') or
self.assertEqual(receipt['status'], 'SUCCESS') or
self.assertEqual(receipt.status, ReceiptStatus.SUCCESS) depending on whether
receipt is an object, dict, or uses an enum) so the test verifies the actual
success indicator rather than truthiness.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 828ad7d3-7b61-4f80-bb46-76d12b69e09c

📥 Commits

Reviewing files that changed from the base of the PR and between 329e7cb and e556935.

📒 Files selected for processing (19)
  • main.py
  • minichain/block.py
  • minichain/chain.py
  • minichain/contract.py
  • minichain/mempool.py
  • minichain/mpt.py
  • minichain/p2p.py
  • minichain/persistence.py
  • minichain/receipt.py
  • minichain/state.py
  • minichain/transaction.py
  • requirements.txt
  • setup.py
  • tests/test_contract.py
  • tests/test_core.py
  • tests/test_persistence.py
  • tests/test_persistence_runtime.py
  • tests/test_protocol_hardening.py
  • tests/test_transaction_signing.py
💤 Files with no reviewable changes (1)
  • setup.py

Comment thread minichain/block.py
# Freeze transactions into an immutable tuple to prevent header/body mismatch
self.transactions = tuple(transactions) if transactions else ()
self.receipts = tuple(receipts) if receipts else ()
self.miner = miner
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick | 🔵 Trivial | 💤 Low value

Duplicate assignment of self.miner.

self.miner is assigned twice: once at line 56 and again at line 68. Remove one of them.

🧹 Proposed fix
         self.transactions = tuple(transactions) if transactions else ()
         self.receipts = tuple(receipts) if receipts else ()
-        self.miner = miner
         # Deterministic timestamp (ms)
         self.timestamp: int = (
             round(time.time() * 1000)

Also applies to: 68-68

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@minichain/block.py` at line 56, In the Block class constructor remove the
duplicate assignment of self.miner so the field is only set once; locate the
__init__ (or constructor) where self.miner is assigned twice and delete one of
the assignments (keep the one that is in the correct logical place for
initialization), ensuring no other initialization logic depends on the removed
line and that the remaining assignment is the authoritative value for the miner
attribute.

Comment thread minichain/mpt.py
Comment on lines 17 to +20
def get(self, key_hex: str) -> Optional[str]:
if not self.root:
return None
return self._get(self.root, to_nibbles(key_hex))

def _get(self, node: Optional[Node], path: List[int]) -> Optional[str]:
if not node:
return None

if isinstance(node, LeafNode):
if node.path == path:
return node.value
return None

elif isinstance(node, ExtensionNode):
if path[:len(node.path)] == node.path:
return self._get(node.child, path[len(node.path):])
return None

elif isinstance(node, BranchNode):
if not path:
return node.value
nibble = path[0]
return self._get(node.branches[nibble], path[1:])

return None
key = bytes.fromhex(key_hex)
val = self._trie.get(key)
return val.decode() if val is not None else None
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

get() may return "" instead of None for missing keys.

HexaryTrie.get() typically returns b'' (empty bytes) for non-existent keys rather than None. The check if val is not None will be True for b'', causing the method to return "" instead of None for missing keys. This breaks the Optional[str] contract.

🐛 Proposed fix
     def get(self, key_hex: str) -> Optional[str]:
         key = bytes.fromhex(key_hex)
         val = self._trie.get(key)
-        return val.decode() if val is not None else None
+        return val.decode() if val else None
py-trie HexaryTrie get method return value for missing keys
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@minichain/mpt.py` around lines 17 - 20, The get() method currently treats any
non-None bytes (including b'') as a present value, so missing keys that return
b'' become empty strings instead of None; update the logic in get (which calls
self._trie.get) to treat empty bytes as missing by returning None when val is
None or len(val) == 0, otherwise return val.decode(); change the condition from
"if val is not None" to an explicit emptiness check against val from
self._trie.get.

Comment thread minichain/p2p.py
Comment on lines +143 to 144
if payload["amount"] < 0:
return False
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Enforce non-negative fee in transaction schema validation.

Line 143 validates amount but does not constrain fee. A negative fee is an economic-consensus exploit vector (can undercharge sender and distort miner rewards). Validate payload["fee"] >= 0 at the P2P boundary.

Proposed fix
-        if payload["amount"] < 0:
+        if payload["amount"] < 0:
+            return False
+        if payload["fee"] < 0:
             return False
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@minichain/p2p.py` around lines 143 - 144, The transaction validation
currently checks payload["amount"] but misses validating the fee; update the
P2P-side validation (the block that checks payload["amount"] in
minichain/p2p.py, e.g., inside the transaction validation/handler function) to
also enforce payload["fee"] >= 0 and return False when the fee is negative.
Locate the same validation scope that contains the line `if payload["amount"] <
0: return False` and add a similar guard for `payload["fee"]`, ensuring the
check uses the same semantics (reject negative fees) and mirrors error
handling/return behavior.

Comment thread requirements.txt
@@ -1,2 +1,2 @@
pynacl==1.6.2
libp2p==0.5.0
trie>=3.1.0
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick | 🔵 Trivial | 💤 Low value

Consider pinning or bounding the trie dependency version.

Using >=3.1.0 without an upper bound allows future major versions to be installed, which could introduce breaking changes. Consider using a bounded range like trie>=3.1.0,<4.0.0 or the compatible release specifier trie~=3.1.0 for reproducible builds.

♻️ Proposed fix
 pynacl==1.6.2
-trie>=3.1.0
+trie>=3.1.0,<4.0.0
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
trie>=3.1.0
trie>=3.1.0,<4.0.0
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@requirements.txt` at line 2, The requirements entry for the package "trie"
currently uses an open-ended range "trie>=3.1.0"; change it to a bounded or
compatible specifier to avoid accidental major upgrades — e.g., replace with
"trie>=3.1.0,<4.0.0" or "trie~=3.1.0" in requirements.txt so installations
remain reproducible and safe from breaking changes.

Comment thread tests/test_core.py
Comment on lines +65 to +114
def test_transaction_fee(self):
"""Test that transaction fees are properly deducted and credited."""
self.state.credit_mining_reward(self.alice_pk, 100)

tx = Transaction(self.alice_pk, self.bob_pk, 40, 0, fee=5)
tx.sign(self.alice_sk)

receipt = self.state.validate_and_apply(tx)
self.assertIsNotNone(receipt)

# Check sender balance (100 - 40 - 5 = 55)
self.assertEqual(self.state.get_account(self.alice_pk)['balance'], 55)
# Check receiver balance (40)
self.assertEqual(self.state.get_account(self.bob_pk)['balance'], 40)

# Test miner reward with fee
from minichain.block import Block, calculate_receipt_root
import main

# Manually create a block to simulate chain processing
block = Block(
index=1,
previous_hash="0",
transactions=[tx],
difficulty=1,
state_root=self.state.state_root(),
receipt_root=calculate_receipt_root([receipt]),
receipts=[receipt],
miner=self.bob_pk
)

# We need to simulate the chain.add_block logic for the miner credit
# Actually, let's just use Blockchain.add_block logic directly by mining
self.chain.state = self.state.copy()

from minichain.mempool import Mempool
mempool = Mempool()
mempool.add_transaction(tx)

# Revert state to before tx since mine_and_process_block will re-apply it
self.chain.state.accounts[self.alice_pk]['balance'] = 100
self.chain.state.accounts[self.alice_pk]['nonce'] = 0
self.chain.state.accounts[self.bob_pk]['balance'] = 0

mined_block = main.mine_and_process_block(self.chain, mempool, self.bob_pk)
self.assertIsNotNone(mined_block)

# Bob was the miner. Bob gets amount(40) + mining_reward(50) + fee(5) = 95
self.assertEqual(self.chain.state.get_account(self.bob_pk)['balance'], 95)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Add test case for insufficient balance due to fees.

The test suite validates insufficient amount (test_insufficient_funds) and sufficient balance with fees (test_transaction_fee), but misses the edge case where amount is valid but amount + fee exceeds balance. This gap could mask bugs in the balance validation logic that checks amount + fee.

🧪 Suggested test case
def test_insufficient_balance_with_fee(self):
    """Test that transactions fail when amount+fee exceeds balance."""
    self.state.credit_mining_reward(self.alice_pk, 50)
    
    # Amount is 45 (valid), but amount + fee = 50, which equals balance
    # Should succeed
    tx1 = Transaction(self.alice_pk, self.bob_pk, 45, 0, fee=5)
    tx1.sign(self.alice_sk)
    result1 = self.state.apply_transaction(tx1)
    self.assertIsNotNone(result1)
    
    # Reset for next test
    self.state.accounts[self.alice_pk]['balance'] = 50
    self.state.accounts[self.alice_pk]['nonce'] = 0
    
    # Amount is 46 (valid alone), but amount + fee = 52 > 50
    # Should fail
    tx2 = Transaction(self.alice_pk, self.bob_pk, 46, 0, fee=6)
    tx2.sign(self.alice_sk)
    result2 = self.state.apply_transaction(tx2)
    self.assertIsNone(result2)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_core.py` around lines 65 - 114, Add a new unit test named
test_insufficient_balance_with_fee in tests/test_core.py that asserts a
transaction where amount alone is <= balance but amount + fee > balance is
rejected; ensure the test uses Transaction(..., fee=...) and signs it, then
calls the state's validation/apply method (validate_and_apply or
apply_transaction depending on existing API) and expects None for the failing
case and a non-None receipt for the borderline succeeding case. If tests fail,
fix the state validation in State.validate_and_apply / State.apply_transaction
to check total_cost = amount + fee (and nonce) against sender balance (use
get_account or accounts[...] to read balances) and return None / raise/abort
when balance < total_cost so fees are correctly considered in balance
validation.

Comment thread tests/test_core.py
Comment on lines +84 to +94
# Manually create a block to simulate chain processing
block = Block(
index=1,
previous_hash="0",
transactions=[tx],
difficulty=1,
state_root=self.state.state_root(),
receipt_root=calculate_receipt_root([receipt]),
receipts=[receipt],
miner=self.bob_pk
)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick | 🔵 Trivial | ⚡ Quick win

Remove unused block construction.

This block is created but never used—mine_and_process_block on line 109 creates its own block. The dead code adds confusion.

🧹 Proposed cleanup
-        # Test miner reward with fee
-        from minichain.block import Block, calculate_receipt_root
-        import main
-        
-        # Manually create a block to simulate chain processing
-        block = Block(
-            index=1,
-            previous_hash="0",
-            transactions=[tx],
-            difficulty=1,
-            state_root=self.state.state_root(),
-            receipt_root=calculate_receipt_root([receipt]),
-            receipts=[receipt],
-            miner=self.bob_pk
-        )
-        
-        # We need to simulate the chain.add_block logic for the miner credit
-        # Actually, let's just use Blockchain.add_block logic directly by mining
+        # Test miner reward with fee by simulating full mining flow
+        import main
         self.chain.state = self.state.copy()
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_core.py` around lines 84 - 94, Remove the unused manual Block
construction: the local variable block created via Block(...) is never used
because mine_and_process_block later builds and processes its own block; delete
the entire Block(...) instantiation (the variable named block) from the test to
eliminate dead code and potential confusion, leaving the call to
mine_and_process_block intact.

Comment thread tests/test_core.py
Comment on lines +105 to +107
self.chain.state.accounts[self.alice_pk]['balance'] = 100
self.chain.state.accounts[self.alice_pk]['nonce'] = 0
self.chain.state.accounts[self.bob_pk]['balance'] = 0
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick | 🔵 Trivial | ⚡ Quick win

Avoid direct state dictionary mutation.

Bypassing state management by directly mutating chain.state.accounts is brittle and couples the test to State's internal representation. If State changes its storage structure, this test will break.

♻️ Proposed refactor using state management API
-        # Revert state to before tx since mine_and_process_block will re-apply it
-        self.chain.state.accounts[self.alice_pk]['balance'] = 100
-        self.chain.state.accounts[self.alice_pk]['nonce'] = 0
-        self.chain.state.accounts[self.bob_pk]['balance'] = 0
+        # Create fresh state with initial balances for mining test
+        self.chain.state = State()
+        self.chain.state.credit_mining_reward(self.alice_pk, 100)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_core.py` around lines 105 - 107, Don't mutate chain.state.accounts
directly; instead use the State/Chain public API to set balances and nonces so
tests don't rely on internal dict layout. Replace the three direct assignments
to chain.state.accounts[self.alice_pk]['balance'] / ['nonce'] and
chain.state.accounts[self.bob_pk]['balance'] with the appropriate API calls
(e.g. state.set_balance(self.alice_pk, 100), state.set_nonce(self.alice_pk, 0),
state.set_balance(self.bob_pk, 0) or
Chain.set_account_balance/Chain.set_account_nonce if those exist), or add small
test helpers on State/Chain to perform these updates atomically, referencing
chain.state and self.alice_pk/self.bob_pk from the diff.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant