Convert variants: check for large difference in amount #3958
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
4 Skipped Deployments
|
WalkthroughThe pull request introduces modifications to the Changes
Possibly related PRs
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
packages/web/hooks/__tests__/use-convert-variant.test.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the config "next/core-web-vitals" to extend from. Please check that the name of the config is correct. The config "next/core-web-vitals" was referenced from the config file in "/packages/web/.eslintrc.json". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team. 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/web/hooks/use-convert-variant.ts (1)
153-162: Consider improving code documentation and maintainabilityThe implementation correctly checks for a significant difference between input and output amounts, with proper null checks and decimal handling. However, a few improvements could make the code more maintainable:
- Extract the threshold as a named constant to improve maintainability
- Enhance the comments to explain the rationale behind the 95% threshold
Consider this improvement:
+// Threshold for maximum acceptable difference between input and output amounts +// Set to 95% to protect users from significant value loss during conversion +const MINIMUM_OUTPUT_RATIO = new Dec("0.95"); + // Check for large difference in amount of in v out -// Include decimals, but we know the amounts should be priced 1:1 +// For non-alloy variants (e.g., USDC), we expect the conversion to maintain +// approximately equal value. If output is less than 95% of input, we flag it +// as a potentially unfavorable conversion to protect users. const isLargeAmountDiff = useMemo(() => { // toDec should include decimals const inAmount = variant?.amount.toDec(); const outAmount = quote?.amount.toDec(); if (!inAmount || !outAmount || inAmount.isZero()) return false; // Out amount should be 95% or more of the input amount - return outAmount.quo(inAmount).lt(new Dec("0.95")); + return outAmount.quo(inAmount).lt(MINIMUM_OUTPUT_RATIO); }, [variant, quote?.amount]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/web/components/table/portfolio-asset-balances.tsx(2 hunks)packages/web/hooks/use-convert-variant.ts(2 hunks)
🔇 Additional comments (5)
packages/web/hooks/use-convert-variant.ts (3)
1-1: LGTM: Appropriate import for decimal calculations
The addition of Dec from @keplr-wallet/unit is well-placed and necessary for the precise decimal calculations in the new amount difference check.
170-170: LGTM: Error flag properly integrates the new check
The modification correctly incorporates the new amount difference check into the error conditions, effectively preventing conversions with significant value discrepancies.
153-170: Verify interaction with existing slippage protection
The new amount difference check provides user protection at the UI level. However, we should verify its interaction with the existing slippage check in getConvertVariantMessages.
Let's analyze the relationship between these checks:
✅ Verification successful
Let me search for the existing slippage check in getConvertVariantMessages to verify the interaction.
Let me search for the implementation details of the quote handling in variant conversion to understand how the new check interacts with existing protections.
Based on the code analysis, I can now provide a final response about the interaction between the new amount difference check and existing protections.
New UI check complements existing quote protection
The new 5% amount difference check (isLargeAmountDiff) operates at the UI level and is independent of the quote handling in getConvertVariantMessages. It serves as an early warning system before the transaction is even constructed, while any quote-related protections would apply during the actual swap execution.
- The check uses
isErrorflag to prevent proceeding when output is less than 95% of input - This is complementary to existing protections as it:
- Catches potential issues before transaction construction
- Provides clear feedback about significant amount differences
- Does not interfere with quote handling or execution
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential conflicts between the new amount difference check
# and existing slippage protection
# Search for slippage-related code and amount checks
rg -A 5 "slippage|amount.*check"
# Look for other conversion protection mechanisms
ast-grep --pattern 'maxSlippage = $_'
Length of output: 99978
Script:
#!/bin/bash
# Search for getConvertVariantMessages implementation and any slippage-related checks
rg -A 10 "getConvertVariantMessages"
# Also search for any other amount difference checks in the codebase
ast-grep --pattern 'amount.$_?.toDec().quo($_).lt($_)'
Length of output: 3462
Script:
#!/bin/bash
# Search for quote-related code in variant conversion
rg -A 15 "quote.*amount" packages/web/hooks/use-convert-variant.ts
# Look for any existing protection mechanisms in the conversion flow
ast-grep --pattern 'function getConvertVariantMessages($$$)'
Length of output: 910
packages/web/components/table/portfolio-asset-balances.tsx (2)
575-575: LGTM! Button styling is consistent with design system.
The styling changes for the activate button maintain consistency with the overall UI design.
590-590: LGTM! Convert button styling aligns with activate button.
The styling changes maintain visual consistency between the convert and activate buttons. However, since this PR introduces checks for large differences in amount during conversion, let's verify the integration with the conversion modal.
✅ Verification successful
Based on the shell script results, I can now generate the final response since I have found evidence of the amount difference check implementation in the codebase.
Amount difference check is properly integrated with conversion logic
The conversion logic includes a safety check that verifies the output amount is at least 95% of the input amount via isLargeAmountDiff in use-convert-variant.ts. This check is properly integrated into the error state handling of the conversion flow, ensuring users are protected from unfavorable conversions.
- The check is implemented in
packages/web/hooks/use-convert-variant.tsand is used to set the error state:isError: isLargeAmountDiff || isQuoteError || isPortfolioAssetsError - The 95% threshold is consistent with the e2e tests in
swap-router.spec.tswhich verify similar protections for token swaps
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the integration between the convert button and the amount difference check
# Expected: Find the implementation of the amount difference check in the conversion logic
# Look for the conversion-related hooks and their usage
ast-grep --pattern 'useConvertVariant'
# Look for amount difference check implementation
rg -A 10 'isLargeAmountDiff|amountDiff|difference.*amount'
Length of output: 6379
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/web/hooks/__tests__/use-convert-variant.test.ts (2)
5-8: Consider additional edge cases and more descriptive test names.While testing zero input is important, consider:
- Adding tests for very small (near-zero) input amounts
- Renaming the test to clarify why zero input is a special case (e.g., "skips difference check when input amount is zero")
- Adding test cases for negative amounts if they should be handled
10-20: Tests align well with requirements, consider additional scenarios.The test cases effectively verify the 95% threshold requirement and include good boundary testing. However, consider adding:
- Tests with different decimal places (e.g., 1.5, 0.05)
- Tests with very large numbers to verify no overflow issues
- Tests with maximum decimal precision supported by
DecExample additions:
test("handles decimal amounts correctly", () => { expect(checkLargeAmountDiff(new Dec("1.5"), new Dec("1.425"))).toBe(false); // 95% expect(checkLargeAmountDiff(new Dec("1.5"), new Dec("1.424"))).toBe(true); // 94.9% }); test("handles large numbers correctly", () => { expect(checkLargeAmountDiff(new Dec("1000000"), new Dec("950000"))).toBe(false); expect(checkLargeAmountDiff(new Dec("1000000"), new Dec("949999"))).toBe(true); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/web/hooks/__tests__/use-convert-variant.test.ts(1 hunks)packages/web/hooks/use-convert-variant.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/web/hooks/use-convert-variant.ts
🔇 Additional comments (2)
packages/web/hooks/__tests__/use-convert-variant.test.ts (2)
1-3: LGTM! Appropriate imports for decimal-precise calculations.
The use of Dec from @keplr-wallet/unit ensures precise decimal calculations, which is crucial for financial operations.
1-21: Verify test coverage with the main implementation.
The tests effectively verify the 5% difference threshold mentioned in the PR objectives. Let's ensure the implementation matches these tests.
✅ Verification successful
Let me check the actual implementation to verify it matches the test cases.
Tests align perfectly with implementation
The implementation in use-convert-variant.ts matches the test cases exactly. The checkLargeAmountDiff function uses the same 95% threshold (0.95) and follows the same logic:
- Returns false for zero input amounts
- Returns false when output is 95% or more of input amount
- Returns true when output is less than 95% of input amount
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find and verify the main implementation
ast-grep --pattern 'export function checkLargeAmountDiff($_) {
$$$
}'
# Check for any other uses of the 0.95 threshold
rg -l "0\.95|95" --type ts
Length of output: 3407
Script:
#!/bin/bash
# Get the implementation of checkLargeAmountDiff
cat packages/web/hooks/use-convert-variant.ts
Length of output: 7775
What is the purpose of the change:
Add more user protection imposing a limit on price impact when converting non-alloy variants, such as USDC.
If the amounts in vs out differ by more than 5% in the quote, block the conversion tx from being possible. Since we're converting like kinds, we can assume the amounts (with respective decimals in place) should be about the same. This is instead of comparing USD values, which may not be accurate if there's issues getting prices.
Linear Task
Hot fix.
Brief Changelog
Testing and Verifying
Tested small change locally