Skip to content

Reject non-integer ledger entry amounts#249

Merged
ramimbo merged 1 commit into
ramimbo:mainfrom
prettyboyvic:mrwk-228-ledger-amount-guard
May 25, 2026
Merged

Reject non-integer ledger entry amounts#249
ramimbo merged 1 commit into
ramimbo:mainfrom
prettyboyvic:mrwk-228-ledger-amount-guard

Conversation

@prettyboyvic
Copy link
Copy Markdown
Contributor

Refs #228

Summary

  • Reject boolean and non-integer amount_microunits values at the shared add_ledger_entry() boundary.
  • Add regression coverage for True, 1.5, and string 1 direct service inputs.

Evidence

Before this change, a direct service caller could pass amount_microunits=True and Python would treat it as integer 1; non-integer values could either persist loosely under SQLite or fail with a raw comparison/type error before MergeWork returned a clear LedgerError.

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 passed
  • uv run --python 3.12 --extra dev python -m pytest tests/test_ledger.py -q -> 19 passed
  • uv run --python 3.12 --extra dev python -m pytest -q -> 208 passed, 2 warnings
  • uv run --python 3.12 --extra dev ruff check app/ledger/service.py tests/test_ledger.py -> passed
  • uv run --python 3.12 --extra dev ruff format --check app/ledger/service.py tests/test_ledger.py -> passed
  • git diff --check -> clean

No secrets, wallet material, private vulnerability details, deployment values, payout details, PayPal details, or MRWK price claims are included.

Copy link
Copy Markdown

@aunysillyme aunysillyme left a comment

Choose a reason for hiding this comment

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

No blockers from my review.

Evidence checked:

  • Inspected the PR diff in app/ledger/service.py and tests/test_ledger.py.
  • Confirmed the new add_ledger_entry() guard rejects booleans before Python can treat True as integer 1, 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 public LedgerError message.
  • 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.

@kbaajjg-max
Copy link
Copy Markdown

No blockers from my review.

Evidence checked:

  • Inspected the diff in app/ledger/service.py: the new guard rejects bool explicitly before the generic int check, which is important because bool is an int subclass in Python.
  • Inspected the regression coverage in tests/test_ledger.py: it exercises True, 1.5, and "1", and verifies the new LedgerError path before ledger mutation.
  • Ran focused ledger tests: python -m pytest tests/test_ledger.py -q -> 19 passed.
  • Ran formatting/lint checks on touched files: ruff check app/ledger/service.py tests/test_ledger.py and ruff format --check app/ledger/service.py tests/test_ledger.py both passed.
  • Ran the full suite on the PR branch: python -m pytest -q -> 208 passed, 2 warnings.

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.

Copy link
Copy Markdown
Contributor

@MolhamHamwi MolhamHamwi left a comment

Choose a reason for hiding this comment

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

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, so bool, float, and string amounts fail with a controlled LedgerError instead of Python coercion/type errors.
  • Inspected tests/test_ledger.py::test_add_ledger_entry_rejects_non_integer_amounts() and confirmed it covers True, 1.5, and "1" against the shared ledger-entry boundary after genesis initialization.
  • Ran pytest tests/test_ledger.py -q locally: 19 passed.
  • Ran ruff check app/ledger/service.py tests/test_ledger.py and ruff format --check app/ledger/service.py tests/test_ledger.py locally: 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.

@ramimbo ramimbo merged commit e1f54bb into ramimbo:main May 25, 2026
1 check passed
@ramimbo ramimbo added mrwk:accepted Maintainer accepted for payout mrwk:paid Ledger payment recorded labels May 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

mrwk:accepted Maintainer accepted for payout mrwk:paid Ledger payment recorded

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants