Skip to content

fix: accept_token_update must reject with partial unclaimed refunds (#470) - #613

Open
baedboibidex-cmyk wants to merge 2 commits into
Iris-IV:mainfrom
baedboibidex-cmyk:fix/accept_token_update_strand_refund_balances
Open

fix: accept_token_update must reject with partial unclaimed refunds (#470)#613
baedboibidex-cmyk wants to merge 2 commits into
Iris-IV:mainfrom
baedboibidex-cmyk:fix/accept_token_update_strand_refund_balances

Conversation

@baedboibidex-cmyk

@baedboibidex-cmyk baedboibidex-cmyk commented Jul 29, 2026

Copy link
Copy Markdown

closes #470
Adds integration test verifying that the total_raised_global != 0 gate correctly blocks accept_token_update when a cancelled campaign has partial unclaimed refunds.

Scenario tested:

  1. Cancel campaign with two contributors
  2. One contributor claims refund (partial refund)
  3. accept_token_update must reject (total_raised_global = 500 != 0)
  4. Remaining contributor claims refund
  5. accept_token_update then succeeds (total_raised_global = 0)

Co-authored-by: Promise Olubudo 254755326+baedboibidex-cmyk@users.noreply.github.com

@drips-wave

drips-wave Bot commented Jul 29, 2026

Copy link
Copy Markdown

@baedboibidex-cmyk Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits.

You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀

Learn more about application limits

@davidmaronio davidmaronio left a comment

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.

the actual #470 test here is really well done, partial refund blocking the swap and then succeeding after the last claim, with the get_token assertion at the end, is exactly the coverage the issue asked for.

but this pr ships a lot that has nothing to do with #470 and i can't merge it as-is:

  1. src/lib.rs:473, src/contributions.rs:326, src/errors.rs:98 - a full remove_personal_cap feature for issue #503 (new entrypoint, new error code, event, docs, ~145 lines of tests). that's a separate feature that needs its own pr and its own review. please split it out.
  2. src/campaigns/transfer.rs:51 - reordering require_not_paused ahead of pending.require_auth() in accept_campaign_transfer changes behavior when paused. unrelated to #470 and unreviewed here, please revert or move to its own pr with a test.
  3. frontend/src/components/MilestoneProgressBar.tsx and frontend/src/types/campaign.ts deletions, the docs/CAMPAIGN_LIFECYCLE.md restructure, the "reverted two accidental merges" changelog entry, and the comment-only test_campaign_update.rs edits are all out of scope. drop them from this branch.

if you trim this down to just the src/tests/test_admin.rs test (and optionally a one-line changelog entry for #470), i'll approve quickly, the test itself is the best version of this coverage i've seen, better scoped than the competing #695 which bundles in an unrelated trust model doc.

@davidmaronio

Copy link
Copy Markdown
Contributor

the merge from main didn't change the scope, the branch still ships the whole remove_personal_cap feature, the transfer.rs pause/auth reorder, the frontend file deletions and the docs restructure alongside the #470 test. the ask stands: trim this down to just the src/tests/test_admin.rs test (plus an optional one-line changelog entry) and i'll approve quickly. if the remove_personal_cap work depends on #722, wait for that pr to land and open it separately.

@baedboibidex-cmyk
baedboibidex-cmyk force-pushed the fix/accept_token_update_strand_refund_balances branch from e1baec6 to 0572828 Compare August 4, 2026 06:51
@baedboibidex-cmyk

Copy link
Copy Markdown
Author

Thanks for the detailed review — the branch is rebuilt from a clean main and trimmed to exactly the scope you asked for. The entire diff is now one file:

src/tests/test_admin.rs (+52) — only the test_token_swap_blocked_after_partial_refund test. (No changelog entry, since it was optional.)

Everything else from your review is out of the branch:

Verification:

  • cargo test --features testutils → 413 passed, 0 failed
  • cargo clippy --all-targets --features testutils -- -D warnings → clean
  • cargo fmt --all -- --check → clean
  • cargo build --target wasm32-unknown-unknown --release → succeeds

CI is green on the new head (test / fmt / clippy all passing).

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.

[Security] accept_token_update can strand refund balances in old token during migration

2 participants