Reject non-integer ledger entry amounts#249
Conversation
aunysillyme
left a comment
There was a problem hiding this comment.
No blockers from my review.
Evidence checked:
- Inspected the PR diff in
app/ledger/service.pyandtests/test_ledger.py. - Confirmed the new
add_ledger_entry()guard rejects booleans before Python can treatTrueas integer1, and rejects non-integer amounts before the existing negative-amount comparison can raise a raw type error or let SQLite accept loose values. - Confirmed the change stays at the shared ledger-entry boundary, so valid integer ledger writes keep flowing through the existing reserve, release, payout, transfer, and genesis paths.
- Confirmed regression coverage exercises
True,1.5, and"1"direct service inputs and asserts the publicLedgerErrormessage. - Checked the PR metadata: mergeable, CI green on
Quality, readiness, docs, and image checks, and no files outside the focused ledger/test surface.
One small non-blocking note: the test uses type: ignore[arg-type] intentionally to hit the runtime guard, which is appropriate here because the bug is about malformed direct service inputs crossing the Python type boundary.
No secrets, wallet material, private deployment values, private vulnerability details, PayPal details, or MRWK price claims were used.
|
No blockers from my review. Evidence checked:
The change is narrow, preserves the existing negative-amount error path for valid integer inputs, and adds coverage for the non-integer cases introduced by the fix. |
MolhamHamwi
left a comment
There was a problem hiding this comment.
No blockers from my review.
Evidence checked:
- Inspected
app/ledger/service.py::add_ledger_entry()and confirmed the new guard runs before the negative-amount comparison, sobool,float, and string amounts fail with a controlledLedgerErrorinstead of Python coercion/type errors. - Inspected
tests/test_ledger.py::test_add_ledger_entry_rejects_non_integer_amounts()and confirmed it coversTrue,1.5, and"1"against the shared ledger-entry boundary after genesis initialization. - Ran
pytest tests/test_ledger.py -qlocally: 19 passed. - Ran
ruff check app/ledger/service.py tests/test_ledger.pyandruff format --check app/ledger/service.py tests/test_ledger.pylocally: both passed.
This is a small, focused ledger hardening change and I did not see supply-conservation, wallet-signing, deployment, payout-detail, or secret-handling impact in the touched scope.
Refs #228
Summary
amount_microunitsvalues at the sharedadd_ledger_entry()boundary.True,1.5, and string1direct service inputs.Evidence
Before this change, a direct service caller could pass
amount_microunits=Trueand Python would treat it as integer1; non-integer values could either persist loosely under SQLite or fail with a raw comparison/type error before MergeWork returned a clearLedgerError.This is distinct from the open nonce and bounty-id validation PRs: it protects the ledger-entry amount field used by genesis, bounty reserve/release/payment, GitHub claims, and wallet-transfer ledger writes.
Verification
uv run --python 3.12 --extra dev python -m pytest tests/test_ledger.py::test_add_ledger_entry_rejects_non_integer_amounts tests/test_ledger.py::test_bounty_reserve_and_payout_conserve_supply -q->4 passeduv run --python 3.12 --extra dev python -m pytest tests/test_ledger.py -q->19 passeduv run --python 3.12 --extra dev python -m pytest -q->208 passed, 2 warningsuv run --python 3.12 --extra dev ruff check app/ledger/service.py tests/test_ledger.py-> passeduv run --python 3.12 --extra dev ruff format --check app/ledger/service.py tests/test_ledger.py-> passedgit diff --check-> cleanNo secrets, wallet material, private vulnerability details, deployment values, payout details, PayPal details, or MRWK price claims are included.