Add partial liquidity withdraw #2510
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe pull request introduces a new feature for partial liquidity withdrawal in the Changes
Sequence DiagramsequenceDiagram
participant User
participant LiquidityResourceRow
participant WithdrawalSlider
participant ConfirmationPopup
User->>WithdrawalSlider: Select withdrawal percentage
WithdrawalSlider->>LiquidityResourceRow: Update withdrawalPercentage
LiquidityResourceRow->>LiquidityResourceRow: calculateWithdrawAmounts()
LiquidityResourceRow->>ConfirmationPopup: Display withdrawal details
User->>ConfirmationPopup: Confirm withdrawal
Assessment against linked issues
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
You are out of MentatBot reviews. Your usage will refresh December 16 at 08:00 AM. |
|
Failed to generate code suggestions for PR |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
client/src/ui/components/bank/LiquidityResourceRow.tsx (3)
35-35: Consider initializingwithdrawalPercentageto a safer default valueSetting the default withdrawal percentage to 100% may cause users to withdraw all their liquidity unintentionally. Consider initializing it to a lower value, such as 0%, to encourage deliberate action from the user.
Apply this diff to change the default value:
-const [withdrawalPercentage, setWithdrawalPercentage] = useState(100); +const [withdrawalPercentage, setWithdrawalPercentage] = useState(0);
109-110: Clarify logic for determiningmaxSharesThe condition for setting
maxSharescurrently uses a ternary operator to select the lesser ofsharesUnscaledandtotalLiquidityUnscaled. For better readability and clarity, consider using a comparison that directly conveys this intent.Apply this diff to improve readability:
-const maxShares = sharesUnscaled > totalLiquidityUnscaled ? totalLiquidityUnscaled : sharesUnscaled; +const maxShares = sharesUnscaled < totalLiquidityUnscaled ? sharesUnscaled : totalLiquidityUnscaled;
147-154: Enhance accessibility of the range input sliderTo improve accessibility for users relying on assistive technologies, consider adding
aria-labelor other relevant ARIA attributes to the range input slider.Apply this diff to add an ARIA label:
<input type="range" min="0" max="100" value={withdrawalPercentage} onChange={(e) => setWithdrawalPercentage(Number(e.target.value))} + aria-label="Withdrawal Percentage" className="w-full appearance-none bg-gold/20 h-2 rounded-lg focus:outline-none [&::-webkit-slider-thumb]:appearance-none [&::-webkit-slider-thumb]:w-4 [&::-webkit-slider-thumb]:h-4 [&::-webkit-slider-thumb]:bg-gold [&::-webkit-slider-thumb]:rounded-full [&::-webkit-slider-thumb]:cursor-pointer [&::-moz-range-thumb]:w-4 [&::-moz-range-thumb]:h-4 [&::-moz-range-thumb]:bg-gold [&::-moz-range-thumb]:border-0 [&::-moz-range-thumb]:rounded-full [&::-moz-range-thumb]:cursor-pointer" />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
client/src/ui/components/bank/LiquidityResourceRow.tsx(4 hunks)client/src/ui/components/trading/MarketModal.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- client/src/ui/components/trading/MarketModal.tsx
🔇 Additional comments (1)
client/src/ui/components/bank/LiquidityResourceRow.tsx (1)
105-122: Verify BigInt arithmetic and potential precision loss in calculateWithdrawAmounts
When using BigInt for arithmetic operations, integer division truncates any fractional parts. Ensure that this truncation does not result in significant precision loss in the calculation of withdrawShares. If higher precision is required, consider alternative approaches to handle fractional shares accurately.
| const onWithdraw = useCallback( | ||
| (percentage: number) => { | ||
| setIsLoading(true); | ||
| const { withdrawShares } = calculateWithdrawAmounts(percentage); | ||
|
|
||
| dojoContext.setup.systemCalls | ||
| .remove_liquidity({ | ||
| bank_entity_id: bankEntityId, | ||
| entity_id: entityId, | ||
| resource_type: BigInt(resourceId), | ||
| shares: withdrawShares, | ||
| signer: dojoContext.account.account, | ||
| }) | ||
| .finally(() => { | ||
| setIsLoading(false); | ||
| setOpenConfirmation(false); | ||
| }); | ||
| }, | ||
| [dojoContext, bankEntityId, entityId, resourceId, marketManager], | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling to onWithdraw function
The onWithdraw function currently lacks error handling for the remove_liquidity system call. Implement error handling to manage potential failures, such as network errors or transaction rejections, to improve reliability and user experience.
Apply this diff to add error handling:
dojoContext.setup.systemCalls
.remove_liquidity({
bank_entity_id: bankEntityId,
entity_id: entityId,
resource_type: BigInt(resourceId),
shares: withdrawShares,
signer: dojoContext.account.account,
})
+ .catch((error) => {
+ console.error("Failed to remove liquidity:", error);
+ // Optionally, display an error message to the user
+ })
.finally(() => {
setIsLoading(false);
setOpenConfirmation(false);
});Committable suggestion skipped: line range outside the PR's diff.
Closes #2508
Summary by CodeRabbit