fix(defindex-adapter): apply real slippage floor on deposit/withdraw - #622
fix(defindex-adapter): apply real slippage floor on deposit/withdraw#622habnark wants to merge 2 commits into
Conversation
Fixes drydocs#558. DefindexAdapter::deposit/withdraw hardcoded 0 as the minimum acceptable amount on both legs of their calls into the underlying DeFindex vault, so the adapter accepted any execution price the vault happened to offer. drydocs#117 and drydocs#432 fixed the identical bug class off-chain, in packages/stellar-sdk-helpers/src/defindex.ts's transaction builder, but never touched this on-chain path -- the one MeridianVault::deposit/ withdraw actually invoke, carrying pooled depositor funds directly. - Added SLIPPAGE_BPS (50 bps / 0.5%, matching the issue's suggested tolerance) and a min_after_slippage() helper. - deposit() now passes amount floored by 0.5% as amounts_min, instead of 0. - withdraw() now quotes the expected payout via DeFindex's own get_asset_amounts_per_shares() immediately before withdrawing, and passes that floored by 0.5% as min_amounts_out, instead of 0. - The floor leaves headroom below the exact expected amount rather than matching it exactly, so ordinary rounding doesn't cause spurious reverts -- the same constraint drydocs#117 established off-chain. - Extended MockDefindexVault to record the amounts_min/min_amounts_out it's called with, and added two regression tests (deposit_passes_a_real_slippage_floor_not_zero, withdraw_passes_a_real_slippage_floor_not_zero) asserting both legs send a real floor rather than 0. No API/ABI changes: deposit()/withdraw() keep their existing signatures, so this is not a breaking change.
|
@habnark 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! 🚀 |
|
@habnark is attempting to deploy a commit to the Collins' projects Team on Vercel. A member of the Team first needs to authorize it. |
collinsezedike
left a comment
There was a problem hiding this comment.
deposit()'s fix is correct, min_after_slippage(amount) is a real floor on the exact requested amount, and it's well-tested. One gap in withdraw() needed before this can merge, noted inline.
| let expected = client | ||
| .get_asset_amounts_per_shares(&shares) | ||
| .get(0) | ||
| .unwrap_or(0); |
There was a problem hiding this comment.
This reuses the "safe and intentional" comment from a different site below, but it isn't safe here. A malformed or empty response collapses expected to 0, which feeds directly into min_after_slippage(0) == 0, so withdraw() calls DeFindex with min_amounts_out=[0], reintroducing exactly the "accept any price" bug this PR exists to close. The new regression test doesn't cover this, MockDefindexVault::get_asset_amounts_per_shares isn't overridden there and defaults to a normal 1:1 quote. Panic on a malformed response here instead (matching the pattern from #555), rather than silently defaulting to 0.
Fixes #558.
DefindexAdapter::deposit/withdraw hardcoded 0 as the minimum acceptable amount on both legs of their calls into the underlying DeFindex vault, so the adapter accepted any execution price the vault happened to offer. #117 and #432 fixed the identical bug class off-chain, in packages/stellar-sdk-helpers/src/defindex.ts's transaction builder, but never touched this on-chain path -- the one MeridianVault::deposit/ withdraw actually invoke, carrying pooled depositor funds directly.
No API/ABI changes: deposit()/withdraw() keep their existing signatures, so this is not a breaking change.
Summary
Test plan
pnpm lint && pnpm typecheck && pnpm testpass locallyCloses #