fix: sending funds using native tokens fails with error, even wen enough balance #679
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe changes enhance the transaction flow in the native token workflow by estimating gas fees with a 10% buffer and incorporating these fees into balance checks. In the 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 (
|
![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
🧹 Nitpick comments (2)
src/components/Create/Link/Input.view.tsx (1)
95-127: Consider consolidating repeated loading state calls and making the gas buffer configurable.Within the native token flow, you are setting the loading state multiple times in quick succession (lines 89, 98, 128, etc.). Consider consolidating or batching these state updates for a smoother UX. Also, the 10% buffer for gas price fluctuations (line 125) might be better extracted into a shared constant or configuration option to make it easier to adjust or parameterize in the future.
src/components/Create/useCreateLink.tsx (1)
45-63: Gas fee consideration in balance checks is robust.Adding gasAmount to the user’s effective token value (lines 58–63) correctly covers scenarios where native currency is being transferred, ensuring users always have enough to cover transaction fees. Consider validating gasAmount to avoid unexpected edge cases (e.g., negative values), though practically it should never be negative.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/Create/Link/Input.view.tsx(1 hunks)src/components/Create/useCreateLink.tsx(3 hunks)src/utils/sdkErrorHandler.utils.tsx(1 hunks)
🔇 Additional comments (3)
src/components/Create/Link/Input.view.tsx (1)
128-131: Flow for non-native tokens is well-structured.By bypassing additional gas estimation for non-native tokens, the logic remains simpler and consistent with the new approach. This separation of native and non-native token handling appears logically correct and maintains user clarity.
src/utils/sdkErrorHandler.utils.tsx (1)
85-85: Clarified error message is helpful.Emphasizing the need for enough balance to cover both tokens and gas fees reduces user confusion and aligns with the updated workflow.
src/components/Create/useCreateLink.tsx (1)
17-17: Optional gasAmount parameter is a good addition.Allowing the parameter to be optional accommodates non-native tokens while still enabling gas fee checks for native tokens. This design is flexible and future-proof.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/components/Create/Link/Input.view.tsx (2)
141-282: Reduce code duplication and improve organization.The transaction preparation logic has several areas of code duplication and could benefit from better organization:
- Gas estimation logic is duplicated between direct and link flows.
- Points estimation logic is similar in both flows.
- Network switching happens too late in the process.
Consider extracting common functionality into helper functions:
+ const estimateGasAndCost = async (preparedTx: any) => { + try { + const { feeOptions, transactionCostUSD } = await estimateGasFee({ + chainId: selectedChainID, + preparedTx, + }) + setFeeOptions(feeOptions) + setTransactionCostUSD(transactionCostUSD) + return feeOptions + } catch (error) { + console.error(error) + setFeeOptions(undefined) + setTransactionCostUSD(undefined) + return undefined + } + } + const estimatePointsForAction = async ( + preparedTx: any, + actionType: 'CREATE' | 'TRANSFER' + ) => { + const points = await estimatePoints({ + chainId: selectedChainID, + address: address ?? '', + amountUSD: parseFloat(usdValue ?? '0'), + preparedTx, + actionType, + }) + setEstimatedPoints(points || 0) + }🧰 Tools
🪛 Biome (1.9.4)
[error] 237-238: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
283-294: Enhance error handling for native token operations.The error handling could be more specific for native token operations to help users better understand and resolve issues.
Consider enhancing the error handling:
} catch (error) { + const isNativeToken = isNativeCurrency(selectedTokenAddress) const errorString = ErrorHandler(error) + const enhancedError = isNativeToken + ? `${errorString}. For native token transactions, ensure you have enough balance for both the transfer amount and gas fees.` + : errorString setErrorState({ showError: true, - errorMessage: errorString, + errorMessage: enhancedError, }) } finally { setLoadingState('Idle') }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/Create/Link/Input.view.tsx(1 hunks)src/components/Create/useCreateLink.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/Create/useCreateLink.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Deploy-Preview
🔇 Additional comments (1)
src/components/Create/Link/Input.view.tsx (1)
93-294: Implementation successfully addresses native token payment issues.The changes effectively solve the native token payment failures by:
- Properly estimating gas fees before transactions
- Including gas costs in balance checks
- Adding a 10% buffer for gas price fluctuations
- Providing clearer error messages
🧰 Tools
🪛 Biome (1.9.4)
[error] 237-238: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
|
|
||
| // for native/non-stable tokens, estimate gas first before checking balance | ||
| if (isNativeCurrency(selectedTokenAddress)) { | ||
| setLoadingState('Loading') | ||
| const linkDetails = generateLinkDetails({ | ||
| tokenValue: tokenValue, | ||
| envInfo: environmentInfo, | ||
| walletType: walletType, | ||
| }) | ||
|
|
||
| const password = await generatePassword() | ||
| const prepareDepositTxsResponse = await prepareDepositTxs({ | ||
| _linkDetails: linkDetails, | ||
| _password: password, | ||
| }) | ||
|
|
||
| if (prepareDepositTxsResponse) { | ||
| const { feeOptions } = await estimateGasFee({ | ||
| chainId: selectedChainID, | ||
| preparedTx: prepareDepositTxsResponse.unsignedTxs[0], | ||
| }) | ||
|
|
||
| // calculate gas amount in ETH | ||
| const gasLimit = BigInt(feeOptions.gasLimit) | ||
| const gasPrice = BigInt(feeOptions.maxFeePerGas || feeOptions.gasPrice) | ||
| const maxGasAmount = Number(formatEther(gasLimit * gasPrice)) | ||
|
|
||
| setLoadingState('Loading') | ||
| await checkUserHasEnoughBalance({ | ||
| tokenValue: tokenValue, | ||
| gasAmount: maxGasAmount * 1.1, // Add 10% buffer for gas price fluctuations | ||
| }) | ||
| } | ||
| } else { | ||
| setLoadingState('Loading') | ||
| await checkUserHasEnoughBalance({ tokenValue: tokenValue }) | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve gas estimation flow and error handling.
The gas estimation logic for native tokens has several issues that need to be addressed:
- Gas estimation is performed before switching networks, which could lead to incorrect estimates.
- Redundant loading state at line 106.
- Missing error handling for gas estimation failures.
Apply this diff to improve the implementation:
if (isNativeCurrency(selectedTokenAddress)) {
- setLoadingState('Loading')
const linkDetails = generateLinkDetails({
tokenValue: tokenValue,
envInfo: environmentInfo,
walletType: walletType,
})
const password = await generatePassword()
+ await switchNetwork(selectedChainID) // Switch network before gas estimation
const prepareDepositTxsResponse = await prepareDepositTxs({
_linkDetails: linkDetails,
_password: password,
})
if (prepareDepositTxsResponse) {
+ try {
const { feeOptions } = await estimateGasFee({
chainId: selectedChainID,
preparedTx: prepareDepositTxsResponse.unsignedTxs[0],
})
// calculate gas amount in ETH
const gasLimit = BigInt(feeOptions.gasLimit)
const gasPrice = BigInt(feeOptions.maxFeePerGas || feeOptions.gasPrice)
const maxGasAmount = Number(formatEther(gasLimit * gasPrice))
- setLoadingState('Loading')
await checkUserHasEnoughBalance({
tokenValue: tokenValue,
gasAmount: maxGasAmount * 1.1, // Add 10% buffer for gas price fluctuations
})
+ } catch (error) {
+ throw new Error('Failed to estimate gas. Please try again.')
+ }
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // for native/non-stable tokens, estimate gas first before checking balance | |
| if (isNativeCurrency(selectedTokenAddress)) { | |
| setLoadingState('Loading') | |
| const linkDetails = generateLinkDetails({ | |
| tokenValue: tokenValue, | |
| envInfo: environmentInfo, | |
| walletType: walletType, | |
| }) | |
| const password = await generatePassword() | |
| const prepareDepositTxsResponse = await prepareDepositTxs({ | |
| _linkDetails: linkDetails, | |
| _password: password, | |
| }) | |
| if (prepareDepositTxsResponse) { | |
| const { feeOptions } = await estimateGasFee({ | |
| chainId: selectedChainID, | |
| preparedTx: prepareDepositTxsResponse.unsignedTxs[0], | |
| }) | |
| // calculate gas amount in ETH | |
| const gasLimit = BigInt(feeOptions.gasLimit) | |
| const gasPrice = BigInt(feeOptions.maxFeePerGas || feeOptions.gasPrice) | |
| const maxGasAmount = Number(formatEther(gasLimit * gasPrice)) | |
| setLoadingState('Loading') | |
| await checkUserHasEnoughBalance({ | |
| tokenValue: tokenValue, | |
| gasAmount: maxGasAmount * 1.1, // Add 10% buffer for gas price fluctuations | |
| }) | |
| } | |
| } else { | |
| setLoadingState('Loading') | |
| await checkUserHasEnoughBalance({ tokenValue: tokenValue }) | |
| } | |
| // for native/non-stable tokens, estimate gas first before checking balance | |
| if (isNativeCurrency(selectedTokenAddress)) { | |
| const linkDetails = generateLinkDetails({ | |
| tokenValue: tokenValue, | |
| envInfo: environmentInfo, | |
| walletType: walletType, | |
| }) | |
| const password = await generatePassword() | |
| await switchNetwork(selectedChainID) // Switch network before gas estimation | |
| const prepareDepositTxsResponse = await prepareDepositTxs({ | |
| _linkDetails: linkDetails, | |
| _password: password, | |
| }) | |
| if (prepareDepositTxsResponse) { | |
| try { | |
| const { feeOptions } = await estimateGasFee({ | |
| chainId: selectedChainID, | |
| preparedTx: prepareDepositTxsResponse.unsignedTxs[0], | |
| }) | |
| // calculate gas amount in ETH | |
| const gasLimit = BigInt(feeOptions.gasLimit) | |
| const gasPrice = BigInt(feeOptions.maxFeePerGas || feeOptions.gasPrice) | |
| const maxGasAmount = Number(formatEther(gasLimit * gasPrice)) | |
| await checkUserHasEnoughBalance({ | |
| tokenValue: tokenValue, | |
| gasAmount: maxGasAmount * 1.1, // Add 10% buffer for gas price fluctuations | |
| }) | |
| } catch (error) { | |
| throw new Error('Failed to estimate gas. Please try again.') | |
| } | |
| } | |
| } else { | |
| setLoadingState('Loading') | |
| await checkUserHasEnoughBalance({ tokenValue: tokenValue }) | |
| } |
kushagrasarathe
changed the title
kushagrasarathe
changed the title
| ? Number(tokenValue) + gasAmount | ||
| : Number(tokenValue) | ||
|
|
||
| if (!balanceAmount || (balanceAmount && balanceAmount < effectiveAmount)) { | ||
| throw new Error( | ||
| 'Please ensure that you have sufficient balance of the token you are trying to send' |
There was a problem hiding this comment.
Q: is this the correct error message if no balanceAmount from the balanceByToken call? what if the balance provider just doesn't find that token
There was a problem hiding this comment.
i think yes, the balanceAmount is being calculated based on accounts balance, and selected token, if balance provider doesn't find that token in the account, then its highly likely the case that user doesnt have balance for that token, in which case this error is fine imo
| @@ -82,7 +82,7 @@ export const ErrorHandler = (error: any) => { | |||
| .toString() | |||
| .includes('Please ensure that you have sufficient balance of the token you are trying to send') | |||
| ) { | |||
| return 'Please ensure that you have sufficient balance of requested token.' | |||
There was a problem hiding this comment.
i have very mixed feelings about this file
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/components/Create/useCreateLink.tsx (1)
64-68: Update error message to mention gas fees.The error message should clarify that the balance check includes gas fees for native tokens.
Apply this diff to improve the error message:
- throw new Error( - 'Please ensure that you have sufficient balance of the token you are trying to send' - ) + throw new Error( + isNativeCurrency(selectedTokenAddress) && gasAmount + ? 'Please ensure that you have sufficient balance to cover both the token amount and gas fees' + : 'Please ensure that you have sufficient balance of the token you are trying to send' + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/Create/Link/Input.view.tsx(1 hunks)src/components/Create/useCreateLink.tsx(3 hunks)src/utils/sdkErrorHandler.utils.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/utils/sdkErrorHandler.utils.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Deploy-Preview
🔇 Additional comments (2)
src/components/Create/Link/Input.view.tsx (1)
104-139: Improve gas estimation flow and error handling.The gas estimation logic for native tokens has several issues that need to be addressed:
- Network switching should be done before gas estimation to ensure accurate estimates.
- Missing error handling for gas estimation failures.
- Redundant loading state at line 106.
- No buffer for gas price fluctuations.
Apply this diff to improve the implementation:
if (isNativeCurrency(selectedTokenAddress)) { - setLoadingState('Loading') const linkDetails = generateLinkDetails({ tokenValue: tokenValue, envInfo: environmentInfo, walletType: walletType, }) const password = await generatePassword() + await switchNetwork(selectedChainID) // Switch network before gas estimation const prepareDepositTxsResponse = await prepareDepositTxs({ _linkDetails: linkDetails, _password: password, }) if (prepareDepositTxsResponse) { + try { const { feeOptions } = await estimateGasFee({ chainId: selectedChainID, preparedTx: prepareDepositTxsResponse.unsignedTxs[0], }) // calculate gas amount in ETH const gasLimit = BigInt(feeOptions.gasLimit) const gasPrice = BigInt(feeOptions.maxFeePerGas || feeOptions.gasPrice) const maxGasAmount = Number(formatEther(gasLimit * gasPrice)) - setLoadingState('Loading') await checkUserHasEnoughBalance({ tokenValue: tokenValue, - gasAmount: maxGasAmount, + gasAmount: maxGasAmount * 1.1, // Add 10% buffer for gas price fluctuations }) + } catch (error) { + throw new Error('Failed to estimate gas. Please try again.') + } } }src/components/Create/useCreateLink.tsx (1)
58-62: Improve variable name for better clarity.The variable name
totalNativeTokenAmountcould be more specific since we have multiple amounts here.Apply this diff to improve the variable name:
- const totalNativeTokenAmount = + const effectiveNativeTokenAmount = isNativeCurrency(selectedTokenAddress) && gasAmount ? Number(tokenValue) + gasAmount : Number(tokenValue)
fixes TASK-8767
Summary by CodeRabbit