-
Notifications
You must be signed in to change notification settings - Fork 17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Address price warning feedback #131
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe recent updates focus on enhancing user experience by adjusting the visibility of trade information and warnings. The USD difference estimate is now hidden when price feeds for both input and output tokens are unavailable, avoiding misleading zero percent differences. The warning screen has been renamed to "Bad Trade Warning" to better reflect its purpose, and the "Go Back" button has been made the primary action, encouraging users to reconsider potentially unfavorable trades. Changes
Assessment against linked issues
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? TipsChat with CodeRabbit Bot (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- src/components/AssetInput.tsx (1 hunks)
- src/components/PriceImpactWarning.tsx (3 hunks)
- src/components/SwapWidget/SwapWidget.tsx (1 hunks)
Additional comments: 5
src/components/PriceImpactWarning.tsx (3)
14-14: The condition added ensures that the component renders only when necessary, which is a good practice for performance and user experience.
36-36: The text change from "Price Impact Warning" to "Bad Trade Warning" aligns with the PR objectives to provide clearer messaging to the user.
52-57: The button order change and the
onGoBack
function call on the "Go Back" button click are consistent with the PR objectives to make "Go Back" the primary action and encourage users to reconsider their actions.src/components/AssetInput.tsx (1)
- 188-190: The new condition for rendering the percentage difference is a good addition to ensure that misleading information is not displayed to the user.
src/components/SwapWidget/SwapWidget.tsx (1)
- 271-271: The updated condition for showing the price impact warning seems correct, but ensure that the
PRICE_IMPACT_THRESHOLD
constant is now expected to be a decimal rather than a percentage, given the removal of the multiplication by 100.Verification successful
The search for
PRICE_IMPACT_THRESHOLD
has yielded results, showing that it is defined as0.1
in theuseSwapWidget.ts
file. This confirms that the threshold is indeed expected to be a decimal value, which aligns with the updated condition in theSwapWidget.tsx
file where the multiplication by 100 has been removed.
- The constant
PRICE_IMPACT_THRESHOLD
is defined as0.1
.- The updated condition
Math.abs(usdDiffPercent) > PRICE_IMPACT_THRESHOLD
inSwapWidget.tsx
is consistent with this definition.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that PRICE_IMPACT_THRESHOLD is defined as a decimal rg 'PRICE_IMPACT_THRESHOLD' --type jsLength of output: 37
Script:
#!/bin/bash # Search for PRICE_IMPACT_THRESHOLD without specifying file type to ensure all files are searched rg 'PRICE_IMPACT_THRESHOLD'Length of output: 658
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (4)
- src/components/PriceImpactWarning.tsx (2 hunks)
- src/components/SwapWidget/SwapWidget.tsx (3 hunks)
- src/components/SwapWidget/useSwapWidget.ts (4 hunks)
- src/components/TransactionDialog/index.tsx (3 hunks)
Files skipped from review as they are similar to previous changes (2)
- src/components/PriceImpactWarning.tsx
- src/components/SwapWidget/SwapWidget.tsx
Additional comments: 8
src/components/TransactionDialog/index.tsx (2)
17-18: The addition of
routeWarningMessage
androuteWarningTitle
to theProps
interface aligns with the PR objectives to provide more structured and clear warnings to the user.82-83: The
routeWarningMessage
androuteWarningTitle
properties are correctly passed to thePriceImpactWarning
component, ensuring that the warning messages are dynamic and can be set based on the context.src/components/SwapWidget/useSwapWidget.ts (6)
204-206: The initialization of the tuple
[routeWarningTitle, routeWarningMessage]
with[undefined, undefined]
whenrouteResponse
is falsy is correct and ensures that no warning is displayed when there is no route response.204-216: The logic within the
useMemo
callback correctly handles the scenario where the price impact cannot be calculated due to missing USD amounts, providing a clear warning title and message.234-237: The logic for setting a "Bad Trade Warning" when the USD percent difference exceeds the price impact threshold is correct and aligns with the PR objectives to warn users about potentially unfavorable trades.
248-251: The logic for setting a "Bad Trade Warning" when the swap price impact percent exceeds the threshold is correct and provides users with detailed information about the expected price impact of their trade.
254-254: The default return value of
[undefined, undefined]
for theuseMemo
callback when no conditions are met is appropriate and ensures that no incorrect warnings are displayed.278-279: The
routeWarningTitle
androuteWarningMessage
are correctly included in the return object of theuseSwapWidget
hook, making them available to the consuming components.
Closes FRE-474, FRE-473, and FRE-472
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Style