feat: twinned swap input hook #4026
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
3 Skipped Deployments
|
| } | ||
|
|
||
| if (price.isNegative()) { | ||
| console.error("INPUT WARNING: Price is negative"); |
There was a problem hiding this comment.
Should this throw an error?
|
Warning Rate limit exceeded@crnbarr93 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 0 minutes and 57 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
WalkthroughThis pull request introduces significant modifications to the token and fiat amount management in the swap interface. The primary focus is on the new custom hook Changes
Possibly related PRs
Suggested reviewers
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: 3
🧹 Nitpick comments (7)
packages/web/hooks/use-twinned-swap-input.ts (3)
8-10: Consider simplifying or inliningisEmptyString.This helper function merely checks that the string is defined and non-whitespace. Inlining it or using a more concise check (e.g.,
!value?.trim()) might reduce indirection if it’s only used in a few places.
53-59: Improve error handling for reset logic.
resetsilently clears values without indicating the cause in the UI or logs. Consider providing feedback or logs when a reset happens, especially if it's triggered by invalid user input.
129-152: Avoid code duplication when setting quote type.
setAllFiatAmountsandsetAllTokenAmountsshare logic to updatequoteTypebased on market order scenarios. Extracting this logic into a shared helper function could improve maintainability and consistency.packages/web/utils/number.ts (1)
119-139: Streamline decimal transformations intransformAmount.While this function covers multiple edge cases (“.” prefix, trailing zeros), consider using a single, clearly commented flow to handle each scenario. This will make the function easier to maintain and extend for new edge cases.
packages/web/hooks/__tests__/use-twinned-swap-input.spec.ts (2)
58-179: Parametric test coverage is thorough.Testing “buy” vs. “sell” and “fiat” vs. “token” input scenarios ensures robust coverage of user interactions. Consider adding tests for boundary conditions, such as extremely large decimals or invalid inputs with partial decimal format (e.g., “0.”), if not already covered.
180-235: Excellent validation of price-change scenarios.The tests correctly assert that the focused input remains the same and the non-focused input updates predictably. Include additional checks for corner cases (e.g., price transitioning from zero to a positive number) to further minimize regressions.
packages/web/components/place-limit-tool/index.tsx (1)
279-313: Enhance clarity in theinputValuememo.The combination of
focused,tab, andtypelogic can be tricky to follow. Adding inline comments or simplifying the conditions would facilitate maintenance. Consider separating the logic for “market” vs. “limit” into separate helper functions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/web/components/place-limit-tool/index.tsx(3 hunks)packages/web/hooks/__tests__/use-twinned-swap-input.spec.ts(1 hunks)packages/web/hooks/use-twinned-swap-input.ts(1 hunks)packages/web/utils/number.ts(1 hunks)
🔇 Additional comments (5)
packages/web/hooks/use-twinned-swap-input.ts (1)
91-121: Check for negative or zero prices earlier.
Currently, the code prints a console error at lines 98 and 163 if the price is negative or zero, then returns. Consider surfacing these states in the UI or applying stricter input validation before the user enters the editing flow to reduce confusion.
packages/web/utils/number.ts (1)
88-94: Be mindful of floating-point limitations in roundUpToDecimal.
Using Math.ceil with floating multipliers can produce unexpected results for very large or small numbers. If extreme precision is required, consider using a decimal library to handle these operations reliably.
packages/web/hooks/__tests__/use-twinned-swap-input.spec.ts (1)
8-37: Good use of an internal test component.
Wrapping the hook in a small test component simplifies state management for each test scenario and keeps tests readable. This approach helps ensure that multiple states and transitions are thoroughly covered.
packages/web/components/place-limit-tool/index.tsx (2)
47-52: Imports for the new hook and revised number utilities look good.
Leveraging centralized logic from useTwinnedSwapInput and transformAmount keeps the component lean and promotes reusability.
215-228: Appropriate usage of useTwinnedSwapInput.
Injecting the hook’s handlers into the existing state flow simplifies swap logic. Just ensure that any new parameters added to useTwinnedSwapInput are properly documented and tested here.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…m/osmosis-labs/osmosis-frontend into connor/rerender-loop-limit-orders
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/web/utils/number.ts (4)
91-94: Add input validation for edge cases.The function should handle edge cases such as NaN, Infinity, or negative precision values to prevent unexpected behavior.
Consider this implementation:
export function roundUpToDecimal(value: number, precision: number) { + if (!Number.isFinite(value)) return value; + if (precision < 0) precision = 0; const multiplier = Math.pow(10, precision || 0); return Math.ceil(value * multiplier) / multiplier; }
100-114: Add input validation and handle negative decimal counts.The function should validate the input string format and handle negative decimal counts to prevent unexpected behavior.
Consider this implementation:
export function fixDecimalCount( value: string, decimalCount = 18, rounding = false ) { + if (!isNumeric(value)) return "0"; + if (decimalCount < 0) decimalCount = 0; if (rounding) { return roundUpToDecimal(parseFloat(value), decimalCount).toString(); } const split = value.split("."); const integerPart = split[0]; const fractionalPart = split[1] ? split[1].substring(0, decimalCount) : ""; const result = integerPart + (decimalCount > 0 ? "." + fractionalPart : ""); return result; }
120-140: Optimize period handling and add input validation.The function can be optimized by:
- Adding input validation
- Combining period handling conditions
- Removing redundant trim() as proper input handling should prevent whitespace
Consider this implementation:
export function transformAmount( value: string, decimalCount = 18, rounding = false ) { + if (!value || !isNumeric(value.replace(/\.$/, '0'))) return "0"; let updatedValue = value; - if (value.endsWith(".") && value.length === 1) { - updatedValue = value + "0"; - } - - if (value.startsWith(".")) { - updatedValue = "0" + value; - } + // Handle both leading and trailing periods + updatedValue = updatedValue.replace(/^\./, '0.').replace(/\.$/, '.0'); const decimals = countDecimals(updatedValue); return decimals > decimalCount ? fixDecimalCount(updatedValue, decimalCount, rounding) - : updatedValue - ).trim(); + : updatedValue; }
87-140: Well-structured number handling system.The three new functions form a well-designed hierarchy for handling numerical inputs in the context of the new
useTwinnedSwapInputhook:
roundUpToDecimalprovides the foundational rounding logicfixDecimalCountbuilds on it to ensure consistent decimal placestransformAmounthandles user input edge casesThis layered approach promotes code reuse and separation of concerns.
Consider extracting these number handling utilities into a separate package that can be shared across different parts of the application, as precise number handling is crucial for financial calculations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/web/utils/number.ts(1 hunks)
🔇 Additional comments (1)
packages/web/utils/number.ts (1)
120-140: Verify usage in PlaceLimitTool component.
The function is used in the PlaceLimitTool component. Let's verify its integration to ensure it's properly handling all input cases.
✅ Verification successful
transformAmount usage in PlaceLimitTool is properly integrated with appropriate error handling
The verification shows that:
transformAmountis used correctly in PlaceLimitTool with proper decimal handling (using 2 and 10 decimal places depending on the context)- The function is well-tested through its dependencies (
countDecimals,fixDecimalCount,trimPlaceholderZeros) - Error handling is in place for various scenarios including zero amounts, empty amounts, and insufficient funds
- The component properly handles input validation and displays appropriate error messages
Key integration points:
- Used for both sell and buy scenarios with market tab handling
- Integrated with
getTrimmedAmountfor proper input formatting - Connected to the swap state error handling system
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of transformAmount in PlaceLimitTool
# Find all usages of transformAmount in the codebase
echo "Searching for transformAmount usage..."
rg -A 5 "transformAmount" packages/web/components/place-limit-tool/
# Find test coverage for transformAmount
echo "Searching for transformAmount tests..."
rg -A 5 "transformAmount" "packages/web/**/*.spec.ts" "packages/web/**/*.test.ts"
Length of output: 2892
Script:
#!/bin/bash
# Let's verify the test coverage and implementation details
# Search for test files with different extensions
echo "Searching for test files..."
rg -A 5 "transformAmount" "packages/web/**/*test*" "packages/web/**/*spec*"
# Check the implementation details of getTrimmedAmount
echo "Checking getTrimmedAmount implementation..."
rg -A 5 "getTrimmedAmount" packages/web/components/place-limit-tool/
# Check for any error handling or validation related to transformAmount
echo "Checking error handling..."
rg -A 5 "catch|error.*transformAmount" packages/web/components/place-limit-tool/
# Look for any related utility functions
echo "Checking related utilities..."
rg -A 5 "countDecimals|fixDecimalCount" packages/web/utils/
Length of output: 4184
Script:
#!/bin/bash
# Let's check the test coverage for transformAmount and its dependencies
# Search for test files specifically for number utils
echo "Checking number utils tests..."
cat packages/web/utils/__tests__/number.spec.ts
# Check for any validation or error states in PlaceLimitTool
echo "Checking validation in PlaceLimitTool..."
rg "validation|error|invalid" packages/web/components/place-limit-tool/index.tsx
# Look for any other components using transformAmount
echo "Checking other usages of transformAmount..."
rg -l "transformAmount" packages/web/
Length of output: 8878
What is the purpose of the change:
These changes refactor the buy/sell tab inputs to use a new hook:
useTwinnedSwapInput. This hook takes both a fiat and token input along with a price and adjusts the values of each depending on the input of the other in relation to the price. For example at a token price of $0.50 if the user input $1 then the token amount would be 2. This change was to make the code more readable/maintainable and to also prevent infinite rerendering caused by the use ofuseEffect. This hook now primarily makes use of callbacks instead ofuseEffect, with the exception of the price updating as that can occur with no input.Linear Task
FE-1273
LIM-287
Brief Changelog
useTwinnedSwapInputhookPlaceLimitToolPlaceLimitToolTesting and Verifying
Manually tested and added unit tests for the new hook.