Skip to content

Closes #48 Implemented: replace silent underflow clamp with typed panic in release_milestone_multi_asset asset_raised decrement#55

Open
Promise278 wants to merge 3 commits into
OrbitChainLabs:mainfrom
Promise278:milestone_multi_asset
Open

Closes #48 Implemented: replace silent underflow clamp with typed panic in release_milestone_multi_asset asset_raised decrement#55
Promise278 wants to merge 3 commits into
OrbitChainLabs:mainfrom
Promise278:milestone_multi_asset

Conversation

@Promise278

Copy link
Copy Markdown
Contributor

Fixes an inconsistent underflow handling pattern in
release_milestone_multi_asset where a silent .unwrap_or(0).max(0)
clamp on the asset_raised decrement could mask a real ledger
invariant violation — while the surrounding code in the same
function correctly panics with a typed error on underflow.

Problem

In multi_asset_release.rs lines 188-189, the per-asset raised
counter is decremented using a silent clamp:

let new_asset_raised = asset_raised
.checked_sub(clamped_release)
.unwrap_or(0) // silent on underflow
.max(0); // second clamp, same intent

This is inconsistent with the typed-error contract established
by the release_amount path just above it in the same function:

let milestone_release = milestone.target_amount
.checked_sub(milestone.released_amount)
.unwrap_or_else(|| panic_with_error!(env, Error::MilestoneReleasedExceedsTarget));

One underflow path panics loudly with a typed error.
The other collapses silently to 0.

What Changed

campaign/src/multi_asset_release.rs

  • Replaced .unwrap_or(0).max(0) with a typed panic
    that mirrors the rest of the function:

    let new_asset_raised = asset_raised
    .checked_sub(clamped_release)
    .unwrap_or_else(|| panic_with_error!(env, Error::LedgerUnderflow));

  • Reconciled storage_set_total_raised in the surrounding
    loop with the same typed-panic approach for consistency

campaign/src/types.rs

  • Added new Error::LedgerUnderflow variant (= 81,
    following the existing numbered scheme) so the cause
    is unmistakable in logs and distinguishable from
    the generic Error::Overflow attribution

campaign/src/test/release_milestone_tests.rs

  • Added test that forces the underflow condition by
    injecting funds above the recorded asset_raised ledger
  • Asserts that the typed LedgerUnderflow panic fires
    instead of silently collapsing to 0

Invariant Being Enforced

asset_raised >= clamped_release must always hold
by construction before the decrement runs.

If it does not hold, the contract has encountered
a state it cannot safely reason about. Surfacing it
loudly as a typed panic is correct — silent correction
via clamp is not.

How to Verify

cargo test

Alqku commented Jun 21, 2026

Copy link
Copy Markdown
Contributor

Typed panic on underflow is the right move for #48, but two things: (1) the two new files campaign/src/test/scratch_test.rs and campaign/src/test_token_scratch.rs look like leftover debugging scratchpads — please drop those before we merge; (2) the new Error::LedgerUnderflow = 81 variant is unnecessary scope creep — Error::Overflow already covers arithmetic failure modes and keeps the error enum lean. Once those are cleaned up this is good to go.

Alqku commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

Hey @Promise278 👋 — the typed Error::LedgerUnderflow change on #48 looks great. However no CI checks have run on this PR. Could you rebase against the latest main (or push a no-op commit) so the workflow reruns? Once fmt/clippy/test/wasm are all green I'll merge ✅

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.

2 participants