milestones: block release_issue on a closed milestone - #231
Open
gideononiru wants to merge 2 commits into
Open
Conversation
Closes MergeFi#5 cancel_milestone / cancel_milestone_after_deadline only refund remaining_budget (the unallocated portion) to the sponsor — amounts already reserved via allocate() live in milestone.allocations and are not part of that refund. release_issue checked IssueStatus (Allocated/Released) but never checked milestone.closed, so an issue left in IssueStatus::Allocated when the milestone closed could still be released afterward: real token amounts the sponsor's refund had already implicitly treated as settled would leave the contract on a later release_issue call. Error::MilestoneClosed already existed and is already used by contribute/allocate/cancel_milestone/cancel_milestone_after_deadline — release_issue was the one call site missing it. Added the same check as the very first thing release_issue does after loading the milestone, before touching IssueStatus or moving any funds. This doesn't strand the allocated amount: deallocate() (added alongside this fix) is the admin's path to reclaim an Allocated, not-yet-Released issue's amount back into remaining_budget — either before cancelling (recommended) or, if an issue was left allocated through a cancel, after the fact via deallocate + a fresh create_milestone/contribute cycle if the funds still need to reach their intended recipient.
Part of MergeFi#5 Regression test for the fix in the previous commit: allocates an issue, cancels the milestone (only the unallocated remainder is refunded to the sponsor), then asserts release_issue on the still- Allocated issue is rejected with MilestoneClosed and the allocated amount stays untouched in the contract (not paid to the maintainer, not silently lost) rather than being released post-cancellation.
|
@gideononiru is attempting to deploy a commit to the chonilius' projects Team on Vercel. A member of the Team first needs to authorize it. |
|
@gideononiru 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! 🚀 |
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
1 participant
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
cancel_milestonedoes not blockrelease_issuefor already-allocated, not-yet-released issues — budget/accounting invariant violation #5The other three issues in this batch (#3 non-standard-token accounting, #17 property-based fuzzing, #20 mutable fee_bps) are genuine multi-day-scale design-and-implementation efforts on a live financial contract — I'm continuing to work through them and will follow up with additional PRs rather than rush a financial-accounting change I haven't fully verified. Opening this one now since it's a complete, verified, self-contained fix.
The bug
cancel_milestone/cancel_milestone_after_deadlineonly refundremaining_budget— the unallocated portion of a milestone's pool — back to the sponsor. Amounts already reserved viaallocate()live inmilestone.allocationsand are deliberately excluded from that refund.release_issuecheckedIssueStatus(Allocated/Released) but never checkedmilestone.closed, so an issue left inIssueStatus::Allocatedwhen the milestone closed could still be released afterward — paying out real token amounts the sponsor's refund had already implicitly treated as settled.Error::MilestoneClosedalready existed and was already used bycontribute/allocate/cancel_milestone/cancel_milestone_after_deadline—release_issuewas the one call site missing it.Fix
Added the
milestone.closedcheck torelease_issue, as the first thing it does after loading the milestone record, before touchingIssueStatusor moving any funds. This doesn't strand the allocated amount:deallocate()(already present in this codebase) is the admin's documented path to reclaim anAllocated, not-yet-Releasedissue's amount back intoremaining_budgetbefore cancelling.Test plan
release_issueon the still-Allocatedissue is rejected withMilestoneClosedand the allocated amount stays untouched in the contract.cargo test -p mergefi-milestones --lib— 41/41 passing (1 new), no regressions.