-
Notifications
You must be signed in to change notification settings - Fork 18
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
fix format number pool v3 #974
Conversation
WalkthroughThe pull request introduces significant changes to the formatting and handling of monetary values within the application. A new Changes
Possibly related PRs
Suggested reviewers
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 (
|
Quality Gate failedFailed conditions See analysis details on SonarCloud Catch issues before they fail your Quality Gate with our IDE extension SonarLint |
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.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (10)
src/pages/Pool-V3/components/CreatePositionForm/index.module.scss (4)
243-250
: LGTM! Consider adding a transition for smooth interactions.The new
.price
class improves the layout of price information within.currentPrice
. The use of flexbox for alignment and the right-justification is appropriate for numerical values.Consider adding a transition property for smooth hover effects:
.price { display: flex; align-items: center; justify-content: flex-end; + transition: all 0.3s ease; svg { margin-right: 4px; } }
Line range hint
352-396
: Good improvements to the min/max price display. Consider unifying border-radius.The updates to
.minMaxPrice
enhance the visual presentation and layout of the min/max price information. The added padding and background color improve readability and visual distinction.For consistency with other elements, consider using the same border-radius:
.minMaxPrice { - border-radius: 8px; + border-radius: 16px; display: flex; gap: 12px; align-items: center; justify-content: space-between; @include theme { background-color: theme-get('neutral-6'); } width: 100%; padding: 12px; // ... rest of the styles }
Line range hint
496-501
: Good addition of a disabled state. Consider improving accessibility.The new
.disabled
state effectively communicates when the input is inactive. The use ofopacity
andpointer-events
is a standard approach for disabling elements.Consider improving accessibility by adding
aria-disabled="true"
to the element when it's in a disabled state. This can be done in the component's JSX. Also, you might want to reconsider the blur effect as it could affect readability:&.disabled { opacity: 0.5; - filter: blur(2px); pointer-events: none; }
Line range hint
449-516
: Nice addition of animation to the button. Consider user preferences.The new animation for the button span adds a nice visual cue to draw attention. The use of
@keyframes
for creating a custom animation is appropriate.Consider respecting user preferences for reduced motion:
@keyframes greenLighter { 0% { transform: scale(1); opacity: 0.7; } 50% { transform: scale(1.1); opacity: 1; } 100% { transform: scale(1); opacity: 0.7; } } span { // ... existing styles ... + @media (prefers-reduced-motion: no-preference) { animation: greenLighter 2s infinite; + } }This ensures that users who have set their system preferences to reduce motion won't see the animation.
src/pages/Pool-V3/components/CreatePositionForm/index.tsx (1)
Line range hint
1-1132
: Consider refactoring the component for improved maintainability.The
CreatePositionForm
component is quite large and complex, handling multiple responsibilities. Consider breaking it down into smaller, more focused components to improve maintainability and readability. This could include separating the zap-in functionality, price range selection, and token input sections into their own components.Additionally, review the dependencies of your
useEffect
hooks to ensure they are optimized and not causing unnecessary re-renders. Some effects have multiple dependencies, which could potentially be simplified or memoized.src/pages/Pool-V3/helpers/helper.ts (5)
170-170
: Clarify or remove ambiguous commentThe comment
// 8e-6 usdt = 1 pepe
is unclear in the context of the code. It might confuse readers who are not familiar with its relevance. Please consider removing it or providing additional context to explain its purpose.
Line range hint
186-195
: Inconsistent BigInt conversion ingetTick
functionIn the
getTick
function, you convert several properties toBigInt
, such asfee_growth_outside_y
,liquidity_change
,liquidity_gross
, andsqrt_price
. However,fee_growth_outside_x
is not converted. For consistency and to prevent potential type mismatches, consider convertingfee_growth_outside_x
toBigInt
as well.Apply this diff to ensure consistent
BigInt
conversions:export const getTick = (tickData) => { return { - fee_growth_outside_x: tickData.fee_growth_outside_x, + fee_growth_outside_x: BigInt(tickData.fee_growth_outside_x), fee_growth_outside_y: BigInt(tickData.fee_growth_outside_y), index: tickData.index, liquidity_change: BigInt(tickData.liquidity_change), sign: tickData.sign, liquidity_gross: BigInt(tickData.liquidity_gross), seconds_outside: tickData.seconds_outside, sqrt_price: BigInt(tickData.sqrt_price) }; };
Line range hint
197-206
: Ensure consistentBigInt
conversion ingetConvertedPool
functionIn the
getConvertedPool
function, properties likeliquidity
,sqrt_price
,fee_growth_global_x
, andfee_growth_global_y
are converted toBigInt
, butcurrent_tick_index
,start_timestamp
, andlast_timestamp
are not. For consistency and to avoid potential type mismatches, consider converting these properties toBigInt
if they represent large numerical values.Apply this diff if appropriate:
export const getConvertedPool = (positions) => { return { liquidity: BigInt(positions.poolData.pool.liquidity), sqrt_price: BigInt(positions.poolData.pool.sqrt_price), - current_tick_index: positions.poolData.pool.current_tick_index, + current_tick_index: BigInt(positions.poolData.pool.current_tick_index), fee_growth_global_x: BigInt(positions.poolData.pool.fee_growth_global_x), fee_growth_global_y: BigInt(positions.poolData.pool.fee_growth_global_y), fee_protocol_token_x: BigInt(positions.poolData.pool.fee_protocol_token_x), fee_protocol_token_y: BigInt(positions.poolData.pool.fee_protocol_token_y), - start_timestamp: positions.poolData.pool.start_timestamp, + start_timestamp: BigInt(positions.poolData.pool.start_timestamp), - last_timestamp: positions.poolData.pool.last_timestamp, + last_timestamp: BigInt(positions.poolData.pool.last_timestamp), fee_receiver: positions.poolData.pool.fee_receiver }; };
Line range hint
238-249
: Refactor recursivesqrt
function to iterative to prevent stack overflowThe
sqrt
function uses recursion vianewtonIteration
, which might lead to stack overflow errors for large input values ofvalue
. Consider refactoring thenewtonIteration
function into an iterative version to improve performance and reliability.Here's an iterative version of the
sqrt
function:const sqrt = (value: bigint): bigint => { if (value < 0n) { throw Error('square root of negative numbers is not supported'); } if (value < 2n) { return value; } let x0 = value; let x1 = (x0 + value / x0) >> 1n; while (x1 < x0) { x0 = x1; x1 = (x0 + value / x0) >> 1n; } return x0; };
Line range hint
252-261
: Verify correctness of slippage calculation incalculateSqrtPriceAfterSlippage
In the
calculateSqrtPriceAfterSlippage
function, the calculation ofpriceWithSlippage
might not correctly apply the slippage percentage. Multiplying bygetPercentageDenominator()
may not be appropriate, as applying a percentage typically involves division.Please review the calculation and consider adjusting it as follows:
const multiplier = getPercentageDenominator() + BigInt(up ? slippage : -slippage); const price = sqrtPriceToPrice(sqrtPrice); - const priceWithSlippage = BigInt(price) * multiplier * getPercentageDenominator(); + const priceWithSlippage = (BigInt(price) * multiplier) / getPercentageDenominator(); const sqrtPriceWithSlippage = priceToSqrtPrice(priceWithSlippage) / getPercentageDenominator();This change ensures the slippage percentage is accurately applied to the price.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- src/helper/index.tsx (1 hunks)
- src/pages/Pool-V3/components/CreatePositionForm/index.module.scss (1 hunks)
- src/pages/Pool-V3/components/CreatePositionForm/index.tsx (2 hunks)
- src/pages/Pool-V3/components/PositionItem/index.tsx (0 hunks)
- src/pages/Pool-V3/helpers/helper.ts (1 hunks)
💤 Files with no reviewable changes (1)
- src/pages/Pool-V3/components/PositionItem/index.tsx
🔇 Additional comments (5)
src/pages/Pool-V3/components/CreatePositionForm/index.module.scss (2)
Line range hint
252-288
: Great improvements to the current price display!The updates to
.currentPriceWrapper
enhance the layout and readability of the current price information. The use of flexbox for alignment and the distinct styling for title and value create a clear visual hierarchy.
Line range hint
1-1000
: Overall, great improvements to the CreatePositionForm styles!The changes in this file significantly enhance the layout and visual presentation of the CreatePositionForm component. The updates improve readability, provide better visual feedback, and create a more polished user interface. The suggestions provided aim to further improve accessibility and respect user preferences.
src/helper/index.tsx (2)
Line range hint
1-618
: Overall assessment: Good improvements with some suggestions for refinement.The changes in this file, particularly the introduction of the
formatMoney
function and its use inminimize
, represent a positive step towards centralizing and improving number formatting logic. However, there are opportunities for further refinement in theformatMoney
function as detailed in the previous comments.Remember to update any other parts of the codebase that might be affected by these changes, especially if the behavior of
minimize
has been altered.
600-600
: LGTM. Verify behavior consistency.The change to use
formatMoney
simplifies theminimize
function and centralizes the formatting logic. This is a good improvement.Please ensure that this change maintains the same behavior and precision as the previous implementation. Run the following script to compare the outputs of the old and new implementations:
This script will help identify any discrepancies between the old and new implementations.
src/pages/Pool-V3/components/CreatePositionForm/index.tsx (1)
Line range hint
1-1132
: Summary of review for CreatePositionForm component.Overall, the
CreatePositionForm
component is well-implemented and provides comprehensive functionality for creating positions in a liquidity pool. Here's a summary of the main points:
- The component structure follows React best practices, using functional components and hooks.
- State management is comprehensive, though complex, which could benefit from further modularization.
- The UI rendering and user interactions are handled effectively, with appropriate conditional rendering.
- There are opportunities for performance optimization, particularly in memoizing certain values and reviewing effect dependencies.
- Some minor improvements in type safety and code organization could enhance the overall quality of the component.
Consider implementing the suggested refactors to improve maintainability, performance, and type safety of the component.
Summary by CodeRabbit
New Features
formatMoney
function for enhanced monetary formatting.Improvements
CreatePositionForm
component, improving layout and visual feedback.CreatePositionForm
for better user experience.Bug Fixes
Refactor