fix(milestones): enforce issue_id uniqueness across milestones - #192
Open
phalap1 wants to merge 1 commit into
Open
fix(milestones): enforce issue_id uniqueness across milestones#192phalap1 wants to merge 1 commit into
phalap1 wants to merge 1 commit into
Conversation
`allocate`'s only duplicate guard was `milestone.allocations.contains_key`, scoped to the single `Milestone` record it had loaded, and `IssueStatus` is keyed `(milestone_id, issue_id)`. Neither can see another milestone's state, so `allocate(1, 555, x)` and `allocate(2, 555, y)` both succeeded and both could be released — the same merged work paid for twice, with nothing on-chain objecting. Add `DataKey::GlobalIssueClaim(issue_id) -> milestone_id`, a contract-instance-wide registry keyed by `issue_id` alone, in the spirit of `escrow::DataKey::Escrow(issue_id)`. `allocate` checks it after the per-milestone guard, so a repeat allocation in the same milestone still reports `IssueAlreadyAllocated` while a cross-milestone collision reports the new `IssueClaimedByOtherMilestone`, and writes the claim once every other check has passed. The claim is `deallocate`'s to release when that lands, so a removed allocation frees the issue for legitimate reallocation instead of leaving a permanent false "already claimed".
|
@phalap1 is attempting to deploy a commit to the chonilius' projects Team on Vercel. A member of the Team first needs to authorize it. |
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.
Closes #51
Problem
allocate's only duplicate-allocation guard ismilestone.allocations.contains_key(issue_id), scoped to the singleMilestonerecord the function has loaded, andIssueStatusis keyed(milestone_id, issue_id). Neither has any visibility into anothermilestone's state, so
allocate(1, 555, x)andallocate(2, 555, y)bothsucceed unconditionally, and both allocations can then be independently
released via
release_issue(1, 555, ...)andrelease_issue(2, 555, ...)—each paying out its own full amount for what is, off-chain, one piece of
merged work.
Neither release is wrong in isolation; each pays exactly what its own
milestone reserved. The defect is that the second commitment was allowed to
exist at all, so the fix belongs in
allocate, notrelease_issue.Fix
Add
DataKey::GlobalIssueClaim(issue_id) -> milestone_id, acontract-instance-wide registry keyed by
issue_idalone — the shapeescrow::DataKey::Escrow(issue_id)already has, and the one thing thiscontract's keyspace was missing.
allocatereads it after the existing per-milestone guard and rejects whenthe issue is claimed by a different
milestone_id, then writes the claimonce every other check has passed. Ordering the global check second is
deliberate: a repeat allocation within the same milestone still reports
IssueAlreadyAllocated, so the two collisions stay distinguishable byerror code — "you already allocated this here" (likely a duplicate request)
versus "another milestone owns this issue" (your data is wrong, a human
should look).
The comparison is
claimed_by != milestone_idrather than bare keypresence, so that once
deallocatelands and can clearallocationswhilea claim is still being reconciled, a milestone re-claiming its own issue
still succeeds.
Both the new
DataKeyvariant and the newErrorvariant are appended,so no existing storage key encoding or error discriminant changes.
This is defense-in-depth within the trust model, not a change to it. The
oracle still decides whether work was done; what this removes is the case
where an honest backend mistake — one GitHub issue mapped onto two release
milestones by a bad import or a double-written row — silently becomes a
double payout with nothing on-chain objecting.
Acceptance criteria
allocateErrorvariant distinguishing "already allocated in thismilestone" (
IssueAlreadyAllocated) from "already allocated in adifferent milestone" (
IssueClaimedByOtherMilestone)test_allocate_rejects_issue_already_claimed_by_different_milestonedeallocate— N/A,deallocatehas notlanded (
grep -rn "fn deallocate" contracts/is empty), and thecriterion is conditional on it. The decision it depends on is
documented on the
GlobalIssueClaimvariant:deallocatereleasesthe claim, so the registry never becomes a source of permanent false
"already claimed" rejections. Happy to fold the test into whichever
PR lands
deallocate.cargo test --workspacepassesA note on
cancel_milestoneWorth flagging for whoever implements
deallocate:cancel_milestonemust not release claims. It refunds only
remaining_budget— theunallocated portion — so an already-allocated issue keeps its funds in the
contract and keeps a working
release_issuepath (release_issuehas noclosedcheck). Freeing its claim on cancel would allow the same issue tobe allocated in a second milestone while the first can still pay it out,
reopening this exact bug through a side door. Releasing a claim is
deallocate's job, and only for allocations it actually removes.Verification
All four CI steps pass locally on this branch:
mergefi-milestonesgoes from 20 to 21 tests; escrow (36) andmaintenance-pool (7) are unchanged.
I ran the new test against a deliberately disabled guard: with the
rejection branch commented out it fails, with it restored it passes — so it
exercises the fix rather than asserting a tautology.