Repository navigation
fix: allocate/approve/reject stubs return 501 instead of faking success (#533) - #700
Merged
Merged
Conversation
…MergeFi#533) The stub resolved { status: 'budget_allocated' } without touching storage or the contract, so POST /milestones/:id/allocate reported an allocation that never happened.
…ergeFi#533) Same no-op stub problem for POST /bounties/:id/approve and /reject. Also drops a duplicate @nestjs/common import left by a bad merge and applies prettier to the file.
|
@jakespepe 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! 🚀 |
|
@jakespepe 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.
Problem
#253 was closed, but three guarded, Swagger-documented routes are still backed by stubs that read and write nothing and report success:
POST /milestones/:id/allocateMilestonesService.allocateBudget{ id, status: 'budget_allocated' }POST /bounties/:id/approveBountiesService.approve{ id, status: 'approved' }POST /bounties/:id/rejectBountiesService.reject{ id, status: 'rejected' }A maintainer (or the frontend) calling these gets a
201and a success-looking body, but no milestone budget is allocated on-chain, no bounty changes state and nothing is stored. Because the routes are also@Idempotent, that fake success is cached and replayed on retries. Lying about money-adjacent state transitions is worse than failing.What changed
I took the issue's second option ("return 501 until then"). Building the real flows is a larger change:
allocateBudgetneeds wiring to the milestones contract'sallocate, and approve/reject need state-machine transitions. That work should not hide behind routes that already claim to work.fix(milestones):allocateBudget(id)now rejects withNotImplementedException(HTTP 501, "Milestone budget allocation is not implemented yet"), with a doc comment explaining why.fix(bounties):approve(id)/reject(id)do the same. This commit also removes a duplicateimport { … } from '@nestjs/common'a bad merge left at the top ofbounties.service.ts, and applies Prettier to the file (formatting only).The routes, guards, idempotency keys and Swagger entries are unchanged. Clients now get an honest 501 instead of a fake success. Once the real implementations land, only the service bodies need to change.
Tests
milestones.service.spec.ts:allocateBudget (#533)expects aNotImplementedException.bounties.service.spec.ts:BountiesService approve / reject (#533)expects aNotImplementedExceptionfrom both. It is a standalonedescribebecause the file's sharedTestingModuledoesn't build onmain(IssueRepositoryis not provided), which makes every other test in that file fail before running.Verification
npx jest src/milestones src/bounties -t "#533": 2 / 2 passed.src/milestones+src/bountiesrun: 46 failed / 41 passed.maingives the same 46 failures (46 / 39), all from the broken bountiesTestingModuleand outdated milestonesfund/resolveIssueexpectations, none touched here.main→ 4 now. The remaining 4 (callerUserId/previousStatusunused, unsafeanyin milestones) were already there.Related: #253
Closes #533
Closes #530
Closes #532
Closes #528
🤖 Generated with Claude Code