fix(maintenance-pool): restore balance on poolWithdraw failure - #475
Open
Fabulouz34 wants to merge 1 commit into
Open
Fabulouz34 wants to merge 1 commit into
Fabulouz34 wants to merge 1 commit into
Conversation
Move cheap preconditions (assertLocked, assertValidAmount, assertRecipientsMatchUsers) before the atomic balance decrement in assignReward() so bad-input errors never touch the pool balance. Wrap the poolWithdraw() call in try/catch: on any throw, restore the decremented amount via poolRepo.increment() before rethrowing. The escrow stays LOCKED in all failure paths (no USDC moved), so the increment returns the pool to its true on-chain state. Also fix a pre-existing syntax error in EscrowService.invokeRelease() (duplicate return statement left from an earlier partial edit). Adds 6 regression tests in 'balance restoration on poolWithdraw failure' asserting increment is called on failure, not called on success, the exact amount is restored, the original error is rethrown, and that a pre-decrement validation failure neither decrements nor restores. Closes MergeFi#274. Related: MergeFi#51, MergeFi#163.
|
@Fabulouz34 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! 🚀 |
|
@Fabulouz34 is attempting to deploy a commit to the chonilius' projects Team on Vercel. A member of the Team first needs to authorize it. |
This branch has not been deployed
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
MaintenancePoolService.assignReward() atomically decremented pool.balance before calling
escrowService.poolWithdraw(). Any throw inside poolWithdraw — escrow not LOCKED, invalid
amount, recipient/user mismatch, Soroban simulate/send/poll failure, or a paymentRepo.save
error — left the balance permanently reduced even though no USDC ever left the contract. Over
time this caused the DB balance to drift below the real on-chain balance, eventually blocking
legitimate rewards with "exceeds pool balance" with no admin path to reconcile.
Changes
EscrowService (escrow.service.ts)
inside poolWithdraw (assertLocked, assertValidAmount, assertRecipientsMatchUsers) so callers
can invoke them before touching any state.
earlier partial edit that broke compilation).
MaintenancePoolService.assignReward() (maintenance-pool.service.ts)
:amount query. Bad input (malformed amount, escrow not LOCKED, recipient/user mismatch) is now
rejected without ever touching the DB balance.
'balance', Number(amount)) atomically restores the reservation before rethrowing the original
error. The escrow stays LOCKED in all failure paths — no USDC moved — so the increment
faithfully returns the pool to its real on-chain state.
Tests (maintenance-pool.service.spec.ts)
Added a describe('balance restoration on poolWithdraw failure') block with 6 regression tests:
All 22 unit tests pass.
Testing
npx jest src/maintenance-pool/maintenance-pool.service.spec.ts
22 passed, 0 failed
Closes #459