Fix inverted contribute/refund time gates in token-fundraiser (anchor)#614
Fix inverted contribute/refund time gates in token-fundraiser (anchor)#614boymak wants to merge 2 commits into
Conversation
The contribute and refund instructions gate on the campaign window with inverted comparisons: contributions were only accepted *after* the window had closed, and refunds were only allowed *while* the campaign was still open. The bug is masked by the tests using duration = 0, where both inverted conditions happen to evaluate correctly. - contribute now rejects once `elapsed_days >= duration` (campaign ended), so contributions are accepted while the campaign is open - refund now rejects while `elapsed_days < duration` (campaign still open), so refunds are allowed only after the campaign ends The tests now use a non-zero (open) campaign so contributions are valid, and assert that a refund is rejected while the campaign is still open. The same bug was fixed in the Pinocchio port of this example (PR solana-foundation#613), where the corrected behaviour was verified end-to-end on a local solana-test-validator. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Greptile SummaryThis PR fixes inverted time-gate comparisons in the
Confidence Score: 5/5The change is a targeted two-line inversion fix with no new dependencies or state mutations; the test suite now exercises both the open-campaign rejection and the post-campaign refund happy path. Both operator flips are correct and symmetric, the parenthesisation of the No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[User calls contribute] --> B{elapsed_days < duration?}
B -- Yes: campaign open --> C[Accept contribution\ntransfer tokens to vault]
B -- No: campaign ended --> D[Error: FundraiserEnded]
E[User calls refund] --> F{elapsed_days >= duration?}
F -- Yes: campaign ended --> G{vault.amount < amount_to_raise?}
F -- No: campaign still open --> H[Error: FundraiserNotEnded]
G -- Yes: goal not met --> I[Refund contributor\nclose contributor account]
G -- No: goal was met --> J[Error: TargetMet]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[User calls contribute] --> B{elapsed_days < duration?}
B -- Yes: campaign open --> C[Accept contribution\ntransfer tokens to vault]
B -- No: campaign ended --> D[Error: FundraiserEnded]
E[User calls refund] --> F{elapsed_days >= duration?}
F -- Yes: campaign ended --> G{vault.amount < amount_to_raise?}
F -- No: campaign still open --> H[Error: FundraiserNotEnded]
G -- Yes: goal not met --> I[Refund contributor\nclose contributor account]
G -- No: goal was met --> J[Error: TargetMet]
Reviews (2): Last reviewed commit: "Address review: narrow refund-rejection ..." | Re-trigger Greptile |
| } catch { | ||
| rejected = true; | ||
| } |
There was a problem hiding this comment.
Catch block too broad — masks unrelated failures
The bare catch {} sets rejected = true for any thrown error, not just the expected FundraiserNotEnded. If the contributor account was never created (e.g. an earlier contribute test failed), the RPC will throw an account-not-found error and rejected will still be true, making the assertion pass while the business-logic gate was never actually exercised. Narrowing the catch to check for the specific error code keeps the test meaningful even when the fixture is in a bad state.
| it("Refund is rejected while the campaign is still open", async () => { | ||
| // The campaign was created with a 5-day duration and has only just started, | ||
| // so a refund must be rejected until the campaign has ended. (A successful | ||
| // refund requires advancing the validator clock past `duration`, e.g. with | ||
| // bankrun's `setClock`.) | ||
| const vault = getAssociatedTokenAddressSync(mint, fundraiser, true); | ||
|
|
||
| const contributorAccount = await program.account.contributor.fetch(contributor); | ||
| console.log("\nContributor balance", contributorAccount.amount.toString()); | ||
|
|
||
| const tx = await program.methods | ||
| .refund() | ||
| .accountsPartial({ | ||
| contributor: provider.publicKey, | ||
| maker: maker.publicKey, | ||
| mintToRaise: mint, | ||
| fundraiser, | ||
| contributorAccount: contributor, | ||
| contributorAta: contributorATA, | ||
| vault, | ||
| tokenProgram: TOKEN_PROGRAM_ID, | ||
| systemProgram: anchor.web3.SystemProgram.programId, | ||
| }) | ||
| .rpc() | ||
| .then(confirm); | ||
| let rejected = false; | ||
| try { | ||
| await program.methods | ||
| .refund() | ||
| .accountsPartial({ | ||
| contributor: provider.publicKey, | ||
| maker: maker.publicKey, | ||
| mintToRaise: mint, | ||
| fundraiser, | ||
| contributorAccount: contributor, | ||
| contributorAta: contributorATA, | ||
| vault, | ||
| tokenProgram: TOKEN_PROGRAM_ID, | ||
| systemProgram: anchor.web3.SystemProgram.programId, | ||
| }) | ||
| .rpc() | ||
| .then(confirm); | ||
| } catch { | ||
| rejected = true; | ||
| } | ||
|
|
||
| console.log("\nRefunded contributions", tx); | ||
| console.log("Your transaction signature", tx); | ||
| console.log("Vault balance", (await provider.connection.getTokenAccountBalance(vault)).value.amount); | ||
| assert.ok(rejected, "refund should be rejected while the campaign is still open"); | ||
| }); |
There was a problem hiding this comment.
No positive-path test for a post-campaign refund
The test suite now only verifies that a refund is rejected while the campaign is open; there is no test that confirms a refund succeeds after the campaign ends. The PR description notes this requires advancing bankrun's clock via setClock, but without that test the happy path of the fixed refund gate is never exercised, leaving the most critical behavior change unverified.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
…gn refund test - The refund-while-open test now asserts the caught error is `FundraiserNotEnded` instead of accepting any exception. - Adds a positive-path test that advances the bankrun clock past the campaign `duration` (via `banksClient.getClock()` + `context.setClock`) and asserts a successful refund: the contribution is returned, the vault is emptied, and the contributor account is closed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Thanks — addressed both test-quality points in 063ec63:
That covers both sides of the corrected |
Summary
The
token-fundraiser(Anchor) example gatescontributeandrefundon the campaign window with inverted comparisons:contributeonly accepted contributions after the campaign window had closed (require!(duration <= elapsed_days, FundraiserEnded)).refundonly allowed refunds while the campaign was still open (require!(duration >= elapsed_days, FundraiserNotEnded)).So for any real (non-zero duration) fundraiser, contributions are rejected for the entire campaign and only allowed once it's over, while refunds are allowed during the campaign and blocked afterwards — the opposite of the intended behaviour.
The bug is invisible today because the test uses
duration = 0, where both inverted conditions happen to evaluate correctly (0 <= 0and0 >= 0).Fix
contributenow rejects onceelapsed_days >= duration(campaign ended) → contributions accepted while the campaign is open.refundnow rejects whileelapsed_days < duration(campaign still open) → refunds allowed only after it ends.(The cast is parenthesised —
(… as u16) < duration— to avoid Rust parsingas u16 <as a generic.)Tests
The tests now create a non-zero (open) campaign so contributions are valid, and assert that a refund is rejected while the campaign is still open. A successful refund requires advancing the validator clock past
duration(e.g. via bankrun'ssetClock).Verification
The Anchor program compiles (
cargo build-sbf). The identical semantic fix was applied to the Pinocchio port of this example in #613, where the corrected behaviour was verified end-to-end on a localsolana-test-validator(contributions accepted while open, rejected after the campaign ends; refunds rejected while open).