Conversation
|
|
||
| // Step 11: Destroy InternalPosition and unlock | ||
| destroy self.positions.remove(key: pid)! | ||
| self._unlockPosition(pid) |
There was a problem hiding this comment.
Follow-up fix for stale async queue entry after close:
Context: when closePosition destroys the internal position, its pid can still be present in positionsNeedingUpdates. A later asyncUpdate() callback can then hit a stale pid and panic. PR #198 adds queue-stale handling + regression test coverage.
…n-stale-async-queue
…sync-queue Fix stale async queue pid after closePosition + add regression test
| var processed: UInt64 = 0 | ||
| while self.positionsNeedingUpdates.length > 0 && processed < self.positionsProcessedPerCallback { | ||
| let pid = self.positionsNeedingUpdates.removeFirst() | ||
| if self.positions[pid] == nil { |
There was a problem hiding this comment.
Do we need both this and _removePositionFromUpdateQueue, or would one or the other be sufficient? (Mainly for my understanding, I'm fine to keep both and be defensive.)
There was a problem hiding this comment.
technically only one should suffice, but this check is a fallback to make sure that the processing position doesn't panic
| access(self) fun _removePositionFromUpdateQueue(pid: UInt64) { | ||
| // Keep this operation linear-time: | ||
| // find first matching pid, then remove once while preserving queue order. | ||
| var i = 0 |
There was a problem hiding this comment.
for-in loop doesn't work because we're mutating the container during iteration
| // User should have ~9800 FLOW (10000 - 50 - 150) | ||
| let expectedAfterDeposit = 10_000.0 - 50.0 - 150.0 | ||
| equalWithinVariance(flowBalanceAfterDeposit, expectedAfterDeposit) | ||
| // Test.assert(flowBalanceAfterDeposit >= expectedAfterDeposit - 1.0, message: "Should have withdrawn full deposit amount") |
| // Final: 10000 | ||
| let expectedFinal = 10_000.0 // All deposits returned | ||
| equalWithinVariance(flowBalanceAfter, expectedFinal) | ||
| // Test.assert(flowBalanceAfter >= expectedFinal - 10.0, message: "Should return all deposits (processed + queued)") |
| ) | ||
| Test.expect(closeRes, Test.beSucceeded()) | ||
|
|
||
| // Post-conditions: zero debt, collateral redeemed, HF == ceiling |
There was a problem hiding this comment.
What's the reasoning for removing these assertions?
There was a problem hiding this comment.
the position is destroyed, and there is no way to get the metrics after closure
| ) | ||
| Test.expect(asyncRes, Test.beSucceeded()) | ||
|
|
||
| // Step 5: run one more callback to prove queue state remains clean. |
There was a problem hiding this comment.
Why is this step necessary for the test?
There was a problem hiding this comment.
removed this step
| [UInt64(1)], | ||
| user | ||
| ) | ||
| Test.expect(closeRes, Test.beSucceeded()) |
There was a problem hiding this comment.
Can we verify in the test that the debt was actually repaid and collateral returned to the user's account?
| // Helper Functions | ||
| // ============================================================================= | ||
|
|
||
| access(all) fun logBalances(_ balances: [FlowALPv0.PositionBalance]) { |
There was a problem hiding this comment.
maybe move this to test_helpers.cdc?
| } | ||
|
|
||
| access(all) | ||
| fun test_closePosition_clearsQueuedAsyncUpdateEntry() { |
There was a problem hiding this comment.
I would move this test case to close_position_with_queued_deposits_test.cdc and remove this file
| log("✅ Successfully closed position with debt: \(moetBalance) MOET") | ||
| } | ||
|
|
||
| // ============================================================================= |
There was a problem hiding this comment.
What about test cases with multiple debt/collateral balances?
- I have N>1 debt balances but only provide M<N sources
- I have multiple collateral positions
There was a problem hiding this comment.
multi collateral/debt test case will be invalidated by #184
I'll implement it when we figure out how to treat multi collateral/debt position
…n.cdc Co-authored-by: Jordan Schalm <jordan.schalm@gmail.com>
5 participants
duplicate of #163 to a
pre-refactorbranchDescription
This PR introduces a full closePosition implementation with multi-token support, improved rounding safety, and an extensive precision test suite.
Implemented pool-level closePosition
Added PositionClosed event with detailed repayment + withdrawal breakdown
Hardened rounding logic
Added getTotalDebt() returning grouped debt per token type
Added precision & volatility tests (8 scenarios)
For contributor use:
masterbranchFiles changedin the Github PR explorer