fix: buidl#680 deposit preview and simulation error handling#58
fix: buidl#680 deposit preview and simulation error handling#58temptemp3 wants to merge 1 commit into
Conversation
- PoolAdd: Provider_deposit expects (uint256,uint256); remove two stray Math.round values when swapAForB is false so pool-side amounts match ABI. - PoolAdd: throw when deposit group simulation fails instead of returning an Error (execution was continuing into sign/send). - PoolRemove: throw on failed balance, withdraw preview, and custom group simulation instead of returning Error. Co-authored-by: Cursor <cursoragent@cursor.com>
📝 WalkthroughWalkthroughThis PR improves error handling and contract interaction in two pool operation components. PoolAdd refines the ChangesPool Operations Error Handling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/components/PoolAdd/index.tsx (2)
1073-1092:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClear the preview when
Provider_depositfails.
expectedOutcomeis only updated on success here, so a failed preview leaves the previous LP/share estimate on screen for the new inputs. Reset it in the failure path (and on rejection) to avoid showing stale numbers.Suggested fix
- ci.Provider_deposit( + ci.Provider_deposit( 1, swapAForB ? [ fromAmountBI, toAmountBI, ] : [toAmountBI, fromAmountBI], 0 ).then((Provider_depositR: any) => { if (Provider_depositR.success) { setExpectedOutcome(Provider_depositR.returnValue); + } else { + setExpectedOutcome(undefined); } - }); + }).catch(() => { + setExpectedOutcome(undefined); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/PoolAdd/index.tsx` around lines 1073 - 1092, The preview isn't cleared on failed Provider_deposit calls, so update the Promise chain for ci.Provider_deposit (the call using swapAForB with fromAmountBI/toAmountBI) to clear the displayed preview by calling setExpectedOutcome(undefined) (or null) in both the failure branch (when Provider_depositR.success is false) and in the .catch rejection handler; ensure setExpectedOutcome is referenced exactly as used now and that the success branch still sets the returnValue as before.
1055-1086:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNormalize token IDs before deciding the deposit tuple order.
This branch still keys off
token.tokenId === pool.tokA. For VOI/wVOI, the selected token can havetokenId === 0while the pool side is stored as390001, soswapAForBflips tofalseand the preview tuple is still reversed for those pools. That leaves the deposit preview incorrect on VOI pairs even after removing the extra tuple values.Suggested fix
- const swapAForB = token.tokenId === pool.tokA; + const swapAForB = tokenId(token) === pool.tokA;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/PoolAdd/index.tsx` around lines 1055 - 1086, The comparison that sets swapAForB currently uses raw token.tokenId vs pool.tokA which fails for VOI/wVOI (tokenId 0 vs pool side 390001); add a small normalization step (e.g., normalizeTokenId) that maps the zero/placeholder ID to the pool's canonical VOI ID (390001) and use it when computing swapAForB (replace token.tokenId === pool.tokA with normalizeTokenId(token.tokenId) === normalizeTokenId(pool.tokA)); update any related comparisons that determine tuple order (the swapAForB calculation used before Provider_deposit) so the preview tuple order is correct for VOI pairs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/components/PoolAdd/index.tsx`:
- Around line 1073-1092: The preview isn't cleared on failed Provider_deposit
calls, so update the Promise chain for ci.Provider_deposit (the call using
swapAForB with fromAmountBI/toAmountBI) to clear the displayed preview by
calling setExpectedOutcome(undefined) (or null) in both the failure branch (when
Provider_depositR.success is false) and in the .catch rejection handler; ensure
setExpectedOutcome is referenced exactly as used now and that the success branch
still sets the returnValue as before.
- Around line 1055-1086: The comparison that sets swapAForB currently uses raw
token.tokenId vs pool.tokA which fails for VOI/wVOI (tokenId 0 vs pool side
390001); add a small normalization step (e.g., normalizeTokenId) that maps the
zero/placeholder ID to the pool's canonical VOI ID (390001) and use it when
computing swapAForB (replace token.tokenId === pool.tokA with
normalizeTokenId(token.tokenId) === normalizeTokenId(pool.tokA)); update any
related comparisons that determine tuple order (the swapAForB calculation used
before Provider_deposit) so the preview tuple order is correct for VOI pairs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 93d62579-619d-44bc-b2d1-7d2986d94eba
📒 Files selected for processing (2)
src/components/PoolAdd/index.tsxsrc/components/PoolRemove/index.tsx
1 participant
Summary
Addresses [buidl#680] Deposit issues in
humble-interface(branch frombeta).Root cause
Provider_depositpreview — The Reach/ABI shape isProvider_deposit(byte,(uint256,uint256),uint256). When the user’s first token is not pool token A (swapAForB === false), the code passed four numeric values in the tuple slot instead of two (two strayMath.roundamounts). That corrupts the contract call used for expected LP / share preview whenever token order is B-first.!swapR.success, the handler usedreturn new Error(...)instead ofthrow, so execution continued into signing/sending despite a failed simulation (misleading failures or unsafe follow-on).return new Erroranti-pattern existed on remove-liquidity paths; replaced withthrowso failures surface in the catch/toast and do not fall through.Testing
npx tsc -bpasses locally.Notes
gh issue view; buidl reference kept in title/body per task tracking.Made with Cursor
Summary by CodeRabbit
Release Notes