Fix #451: creator absorbs remaining pool as final revenue claimant - #723
Open
meem08 wants to merge 6 commits into
Open
Fix #451: creator absorbs remaining pool as final revenue claimant#723meem08 wants to merge 6 commits into
meem08 wants to merge 6 commits into
Conversation
…mant claim_creator_revenue computes the creator share with truncating integer division, so across deposit rounds cumulative rounding can drain the pool before the creator (the final claimant) receives their full share, leaving dust stuck forever. Once every contributor has claimed at least once, give the creator whatever remains of the pool instead of their individually-truncated share, so the full allocation is paid out exactly. Adds a regression test.
|
@meem08 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! 🚀 |
Upstream main (900bb53, campaign tag filtering Iris-IV#618) accidentally added a broken src/campaigns.rs file containing an uncompilable category-cap code snippet, alongside the existing src/campaigns/ directory module. Rust rejects both (E0761: file for module campaigns found at both paths), breaking fmt, clippy, and test CI. Remove the stale artifact; the campaigns module lives in src/campaigns/.
Upstream commit 900bb53 (Iris-IV#618, campaign tag filtering) was merged into this branch and is itself broken: it added a duplicate src/campaigns.rs alongside the src/campaigns/ directory (E0761), duplicate #[contractimpl] blocks in admin.rs and lib.rs (E0252/E0428), and non-Rust garbage files (src/clients.rs with TypeScript, src/events.ts, src/types.ts, src/test.rs). Upstream main is currently red on all CI checks because of it. Reverting keeps the legitimate Iris-IV#699/Iris-IV#616 upstream improvements while removing the broken Iris-IV#618 changes so this branch compiles again.
The two campaign_update tests still parsed the event payload as a 2-tuple, but update.rs (Iris-IV#510/Iris-IV#602) publishes (old_title, old_description, new_title, new_description). Update assertions to the 4-tuple so the test job goes green.
assert_eq!(data, ()) always succeeds on a unit value and trips clippy::unit_cmp with -D warnings, failing the clippy job. The typed conversion already verifies the payload shape, so bind the unit directly instead.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #451 ([Security]) —
claim_creator_revenuecan under-pay the creator (the final revenue claimant) due to cumulative integer-division precision loss.The creator's share is computed with truncating integer division (
floor(total_pool * creator_share_bps / BPS)), the same pattern the issue flags for contributors. Across deposit rounds, cumulative rounding can drain the pool before the creator receives their full allocation, leaving revenue dust permanently stuck in the contract — the creator's last claim silently returns0/less than their entitlement.Root cause
Each distribution truncates fractional stroops. The sum of every contributor's individually-truncated share plus the creator's individually-truncated share can fall short of the full pool. Once the pool stops growing, nobody can claim the leftover stroops.
The fix
As proposed in the issue — track the distributed amount as a running sum and give the final claimant
pool_remaininginstead of their calculated share:contributors_claimed >= total_contributors), the creator — the last party that can still claim — receives whatever remains of the pool (total_pool - contributor_distributed - creator_already_claimed) instead of their individually-truncated share, so the full allocation is paid out exactly and no dust is left stuck.contributor_revenue_distributed,contributor_revenue_claimants) — no new storage keys.checked_add/checked_sub→Error::Overflowon pathological pools, never a panic ([P1][Bug] Unchecked arithmetic incontributeanomaly detection can panic #408 pattern).Caveat (documented in code)
The guard assumes contributors claim before the creator after each deposit round. If the creator claims creator-revenue before contributors have claimed their share of a fresh deposit,
pool_remainingincludes their still-unclaimed share. Contributors should claim promptly after each deposit to keep this invariant.Changes
src/revenue.rsclaim_creator_revenue: final-claimant dust absorption (running-sumpool_remaining), with ordering caveat commentsrc/tests/test_regressions.rstest_creator_final_claimant_absorbs_rounding_dustCI repair commits (this branch)
The branch was updated against upstream
mainand three follow-up commits were required to get all CI checks green — upstreammainitself is currently red (all checks fail) because of commit900bb53(#618, campaign tag filtering), which accidentally added a duplicatesrc/campaigns.rsalongside thesrc/campaigns/directory (rustc E0761) plus duplicate#[contractimpl]blocks inadmin.rs/lib.rs(E0252/E0428) and non-Rust garbage files (src/clients.rscontaining TypeScript,src/events.ts,src/types.ts,src/test.rs).27af3a0Remove stalesrc/campaigns.rsduplicate modulesrc/campaigns/module → E0761 broke fmt/clippy/test5512fc9Revert broken #618 commit900bb53#[contractimpl]blocks and deletes the non-Rust garbage files; keeps the legit #699/#616 upstream changes that were also in the mergef1ff24aFixcampaign_metadata_updatedevent shape in stale teststest_campaign_updatetests still parsed the event payload as a 2-tuple; the code (#510/#602) publishes(old_title, old_description, new_title, new_description)016a437Fix clippyunit_cmplintassert_eq!(data, ())intest_withdrawals.rstripsclippy::unit_cmpunder-D warningsIf maintainers prefer, the
5512fc9revert of #618 could be split into its own PR, but keeping it here is what makes this branch mergeable (a PR against brokenmaincannot go green otherwise).Test plan
cargo fmt --all -- --check✅cargo clippy --all-targets --features testutils -- -D warnings✅cargo test --features testutils— 401 passed, 0 failed ✅cargo build --target wasm32-unknown-unknown --release✅ (the test job's second step)test_creator_final_claimant_absorbs_rounding_dustpasses and covers: contributor claims full share across two deposit rounds, then creator absorbs the remaining pool (7 stroops) instead of their truncated share, leaving the contract fully drainedCloses #451