Routes error on request pay #413
Conversation
From the request pay init, we need to now the token address, chain and decimals in order to calculate the squid router fee. Previously we depended on those three data points separately. This caused that when the user seleted a token, we tried to calculate the route for that token but with the decimals from the previous one (the effect tasked with cleaning this also depends on the change of selected token, so they both executed at the same time). In reality, from the pay we depend on the whole token data and only care when it updates completely. This refactor reflects that and avoid calling squid with bad data. Refs: #102
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Warning Rate limit exceeded@jjramirezn has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 2 minutes and 1 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. 📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces significant changes to the Changes
Possibly related PRs
Suggested labels
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: 5
🧹 Outside diff range and nitpick comments (3)
src/context/tokenSelector.context.tsx (1)
Line range hint
61-126: Update effect dependency array toselectedTokenDatato prevent race conditionsAs per the PR objectives, to resolve the race condition between the token selector and the effect that calls squid, the
useEffecthook should depend onselectedTokenDatainstead ofselectedTokenAddressandselectedChainID. This ensures the effect runs only afterselectedTokenDatais fully updated, preventing calls with incomplete or incorrect parameters.Apply this diff to update the dependency array:
useEffect(() => { let isCurrent = true async function fetchAndSetTokenPrice(tokenAddress: string, chainId: string) { // Function implementation } - }, [selectedTokenAddress, selectedChainID, isConnected]) + }, [selectedTokenData, isConnected])src/components/Request/Pay/Views/Initial.view.tsx (2)
34-34: Consider consolidating 'setSelectedChainID' and 'setSelectedTokenAddress' into 'setSelectedTokenData'.Since
selectedTokenDatanow encapsulates both the chain ID and token address, you might simplify state management by using a singlesetSelectedTokenDatafunction instead of separatesetSelectedChainIDandsetSelectedTokenAddressfunctions. This change would enhance consistency and reduce potential state mismatches.
Line range hint
146-185: Refactor 'handleOnNext' to reduce code duplication.In the
handleOnNextfunction, the code within theifandelsebranches (lines 146-185) is largely similar, with only a few differences in variables and function calls. This duplication can make the code harder to maintain and increase the risk of bugs. Consider abstracting the common logic into a separate function or adjusting your code structure to eliminate duplication.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- src/components/Request/Pay/Views/Initial.view.tsx (7 hunks)
- src/context/tokenSelector.context.tsx (6 hunks)
🧰 Additional context used
📓 Learnings (1)
src/context/tokenSelector.context.tsx (1)
Learnt from: jjramirezn PR: peanutprotocol/peanut-ui#404 File: src/context/tokenSelector.context.tsx:121-121 Timestamp: 2024-10-03T09:57:43.885Z Learning: In `TokenContextProvider` within `tokenSelector.context.tsx`, when token data is loaded from preferences, it's acceptable to set `isTokenPriceFetchingComplete` to `true` because the token data is already available.
🔇 Additional comments (7)
src/context/tokenSelector.context.tsx (7)
26-26: Addition ofselectedTokenDatato contextIncluding
selectedTokenDatain the context aligns with the PR objectives to prevent race conditions and is implemented correctly.
29-34: Definition ofITokenDatatypeThe
ITokenDatatype is properly defined with all the necessary fields:tokenAddress,chainId,decimals, andprice.
48-48: Initialization ofselectedTokenDatastate variableThe state variable
selectedTokenDatais correctly initialized usinguseStatewith an initial value ofundefined.
76-79: Resetting token data for unsupported chainsProperly resetting
selectedTokenDataand related variables when the chain is unsupported ensures consistency in the application state.
89-94: SettingselectedTokenDataafter successful token price fetch
selectedTokenDatais correctly populated with the fetched token information, which helps in preventing race conditions by ensuring all necessary token data is available together.
101-104: Resetting token data on token price fetch failureAppropriate resetting of
selectedTokenDataand related variables when the token price fetch fails maintains the integrity of the application state.
113-116: Resetting token data when user disconnectsProperly resets
selectedTokenDataand other related state variables when the user is not connected, ensuring that stale data is not used.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/components/Request/Pay/Views/Initial.view.tsx (2)
49-64: Improved error handling and consistency increateXChainUnsignedTxThe addition of the
selectedTokenDatacheck improves error handling, and the use ofselectedTokenDataproperties is consistent with the earlier state management changes. However, there's a minor improvement opportunity:Consider removing the non-null assertion (
!) on line 56 and subsequent lines, as it's redundant given the check on line 51:- fromToken: selectedTokenData!.tokenAddress, + fromToken: selectedTokenData.tokenAddress,Apply this change to all occurrences of
selectedTokenData!in this function.
Line range hint
73-116: Improved cross-chain handling inuseEffectThe changes in the
useEffecthook enhance the handling of cross-chain scenarios and align well with the new state management approach. The dependency update on line 113 is correct and consistent.Consider removing the redundant condition on line 85:
- if (Number(feeEstimation) > 0) { - setErrorState({ showError: false, errorMessage: '' }) setIsFeeEstimationError(false) setTxFee(Number(feeEstimation).toFixed(2)) setLinkState(RequestStatus.CLAIM) - } else { - setErrorState({ showError: true, errorMessage: ERR_NO_ROUTE }) - setIsFeeEstimationError(true) - setTxFee('0') - setLinkState(RequestStatus.NOT_FOUND) - }This condition is always true at this point because of the check on line 84, so the
elseblock is never reached.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- src/components/Request/Pay/Views/Initial.view.tsx (7 hunks)
🧰 Additional context used
📓 Learnings (2)
📓 Common learnings
Learnt from: jjramirezn PR: peanutprotocol/peanut-ui#413 File: src/context/tokenSelector.context.tsx:118-123 Timestamp: 2024-10-04T13:40:16.067Z Learning: In the `TokenContextProvider` component within `src/context/tokenSelector.context.tsx`, in the TypeScript React application, when data changes and before calling `fetchAndSetTokenPrice`, it is necessary to reset `selectedTokenData`, `selectedTokenPrice`, `selectedTokenDecimals`, and `inputDenomination` to discard stale data.src/components/Request/Pay/Views/Initial.view.tsx (1)
Learnt from: Hugo0 PR: peanutprotocol/peanut-ui#413 File: src/components/Request/Pay/Views/Initial.view.tsx:71-72 Timestamp: 2024-10-04T13:10:49.199Z Learning: In `src/components/Request/Pay/Views/Initial.view.tsx`, it's acceptable to use the `!` operator in TypeScript to assert that `selectedTokenData` is not `null` or `undefined`, and potential runtime errors from accessing its properties without checks can be disregarded.
🔇 Additional comments (2)
src/components/Request/Pay/Views/Initial.view.tsx (2)
34-37: Great simplification of state management!The use of
selectedTokenDatainstead of separateselectedChainIDandselectedTokenAddressvariables is a good improvement. This consolidation reduces the chances of state inconsistency and simplifies the component's logic.
145-148: Improved scenario handling inhandleOnNextand consistent UI updatesThe changes in the
handleOnNextfunction enhance the handling of different scenarios (cross-chain and non-cross-chain) and align well with the new state management approach. The updated network switching logic is correct and consistent.The JSX changes for displaying network costs accurately reflect the updated logic, providing a better user experience by showing the appropriate cost based on whether it's a cross-chain transaction or not.
These improvements contribute to a more robust and user-friendly component.
Also applies to: 181-182, 326-329
This PR addresses two issues