fix: evm solana send low balance error messages#571
fix: evm solana send low balance error messages#571NickKelly1 wants to merge 6 commits intodevelopfrom
Conversation
WalkthroughThe pull request introduces changes to the Changes
Possibly related PRs
Suggested reviewers
Warning Rate limit exceeded@NickKelly1 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 22 minutes and 5 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. 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
Documentation and Community
|
![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
🧹 Outside diff range and nitpick comments (3)
packages/extension/src/providers/solana/ui/send-transaction/index.vue (2)
Line range hint
228-234: Fix indentation in early return blockWhile the logic is correct, there's an indentation issue in the early return block that should be fixed for consistency.
const hasEnoughBalance = computed((): boolean => { if (!hasValidDecimals.value) { - return false; + return false; } return toBN(selectedAsset.value.balance ?? '0').gte( toBN(toBase(sendAmount.value ?? '0', selectedAsset.value.decimals!)), ); });
Line range hint
97-234: Consider adding unit tests for the new validation logicWhile the implementation looks good, it would be beneficial to add unit tests specifically for these validation scenarios:
- Invalid decimals with sufficient balance
- Valid decimals with insufficient balance
- Invalid decimals with insufficient balance
Would you like me to help create these unit tests?
packages/extension/src/providers/ethereum/ui/send-transaction/index.vue (1)
223-229: Consider adding a comment explaining the validation flow.The updated
hasEnoughBalanceproperty now correctly depends onhasValidDecimals. Consider adding a comment to explain this dependency for better maintainability.const hasEnoughBalance = computed(() => { + // Ensure decimals are valid before checking balance if (!hasValidDecimals.value) { return false; } return toBN(selectedAsset.value.balance ?? '0').gte( toBN(toBase(sendAmount.value ?? '0', selectedAsset.value.decimals!)), ); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
packages/extension/src/providers/ethereum/ui/send-transaction/index.vue(2 hunks)packages/extension/src/providers/solana/ui/send-transaction/index.vue(2 hunks)packages/extension/src/ui/action/views/swap/views/swap-best-offer/components/swap-best-offer-block/components/best-offer-warning.vue(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/extension/src/ui/action/views/swap/views/swap-best-offer/components/swap-best-offer-block/components/best-offer-warning.vue
🔇 Additional comments (6)
packages/extension/src/providers/solana/ui/send-transaction/index.vue (3)
97-100: Improved error handling for insufficient balance
The updated condition for displaying the alert now properly validates both decimal precision and balance requirements before showing the error message. This aligns with the PR objective of improving error messaging for low balance scenarios.
225-227: LGTM! Clean implementation of decimal validation
The new hasValidDecimals computed property provides a clear separation of concerns by isolating decimal validation logic. This makes the code more maintainable and reusable.
Line range hint 225-234: Verify usage of computed properties
Let's verify that other components correctly handle these computed properties, especially in error scenarios.
✅ Verification successful
No issues found with the computed properties integration
The verification shows that the computed properties hasEnoughBalance and hasValidDecimals are consistently implemented across different blockchain providers (Solana, Ethereum, Bitcoin, Polkadot, Kadena) in their respective send-transaction components. The properties are used correctly for:
- Input validation in the UI
- Balance checks before transaction submission
- Proper decimal handling with the shared utility functions
- Consistent error handling for insufficient balance scenarios
The implementation in the file under review aligns with the established patterns across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for components using hasEnoughBalance or hasValidDecimals
echo "Searching for components using these computed properties..."
rg "hasEnoughBalance|hasValidDecimals" --type vue
# Search for similar validation patterns in other transaction components
echo "Checking for similar validation patterns in other transaction components..."
fd -e vue -e ts "send-transaction" | xargs rg "balance.*validation|decimal.*validation"
Length of output: 457
Script:
#!/bin/bash
# Search for components using these computed properties with proper file type
echo "Searching for components using these computed properties..."
rg "hasEnoughBalance|hasValidDecimals"
# Search for similar validation patterns in transaction components
echo "Checking for similar validation patterns..."
rg -A 3 "isValidDecimals|toBN.*balance|toBase.*decimals"
# Find all Vue files in send-transaction directory for context
echo "Finding send-transaction related Vue files..."
fd -e vue "send-transaction"
Length of output: 31650
packages/extension/src/providers/ethereum/ui/send-transaction/index.vue (3)
107-110: LGTM! Enhanced validation for send-alert visibility.
The updated condition now properly validates decimals before showing the balance error message, which aligns with the PR's objective of improving error handling for low balance scenarios.
220-222: LGTM! Well-implemented decimal validation.
The new hasValidDecimals computed property correctly utilizes the isValidDecimals utility to validate the send amount against the asset's decimals.
220-229: Verify consistent decimal validation across providers.
Let's ensure this pattern is consistently implemented across other blockchain providers.
✅ Verification successful
Based on the search results, I can now generate the final response as I have sufficient information about the decimal validation implementation across different blockchain providers:
Decimal validation is consistently implemented across providers
The isValidDecimals check from @enkryptcom/utils is uniformly implemented across all providers:
- Ethereum: Lines 220-229 in
packages/extension/src/providers/ethereum/ui/send-transaction/index.vue - Solana: Similar implementation in
packages/extension/src/providers/solana/ui/send-transaction/index.vue - Bitcoin: Matching pattern in
packages/extension/src/providers/bitcoin/ui/send-transaction/index.vue - Polkadot: Consistent usage in
packages/extension/src/providers/polkadot/ui/send-transaction/index.vue - Kadena: Same validation in
packages/extension/src/providers/kadena/ui/send-transaction/index.vue
All implementations follow the same pattern:
- Import
isValidDecimalsfrom@enkryptcom/utils - Check decimals before balance validation
- Use the validation result to control UI alerts and transaction flow
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar decimal validation patterns in other providers
echo "Searching for decimal validation in other providers..."
rg -A 5 "hasValidDecimals|isValidDecimals" "packages/extension/src/providers/"
# Search for send-alert component usage
echo "Checking send-alert usage in other providers..."
rg -A 5 "send-alert" "packages/extension/src/providers/"
Length of output: 28898
NickKelly1
changed the title
|
💼 Build Files |
NickKelly1
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (10)
packages/extension/src/providers/ethereum/ui/send-transaction/components/send-alert.vue (3)
2-5: Enhance accessibility for screen readersWhile the current implementation includes good accessibility features, consider adding
aria-hidden="true"to the alert icon since it's decorative and the message is already conveyed through the text.<div class="send-alert" role="alert" aria-live="polite"> - <alert-icon /> + <alert-icon aria-hidden="true" /> <p aria-label="Error message">{{ errorMsg }}</p> </div>
11-15: Add prop validation for errorMsgConsider adding validation to ensure the error message is not empty and meets any length requirements.
interface IProps { - errorMsg: string; + errorMsg: string & { length: 1 }; // Ensures non-empty string } -defineProps<IProps>(); +const props = withDefaults(defineProps<IProps>(), { + errorMsg: '' // Provide a default value +});
21-28: Consider using relative units for better responsivenessThe fixed pixel margins might not scale well on different screen sizes. Consider using relative units like
remorem..send-alert { - margin: 0 32px 8px 32px; + margin: 0 2rem 0.5rem 2rem; - padding: 12px 16px 12px 57px; + padding: 0.75rem 1rem 0.75rem 3.5rem; border-radius: 10px; position: relative; box-sizing: border-box; }packages/extension/src/providers/solana/ui/send-transaction/index.vue (4)
Line range hint
222-231: Consider early returns for validation checks.The
hasEnoughBalancecomputed property could be simplified using early returns for better readability.const hasEnoughBalance = computed((): boolean => { - if (!hasValidDecimals.value) { - return false; - } - if (!hasPositiveSendAmount.value) { - return false; - } - return toBN(selectedAsset.value.balance ?? '0').gte( - toBN(toBase(sendAmount.value ?? '0', selectedAsset.value.decimals!)), - ); + if (!hasValidDecimals.value || !hasPositiveSendAmount.value) return false; + + return toBN(selectedAsset.value.balance ?? '0').gte( + toBN(toBase(sendAmount.value ?? '0', selectedAsset.value.decimals!)), + ); });
362-403: Consider using a constant for error messages.The error messages in the
errorMsgcomputed property should be extracted into constants to maintain consistency and enable easier updates.+const ERROR_MESSAGES = { + INVALID_DECIMALS: 'Too many decimals.', + INVALID_AMOUNT: 'Invalid amount.', + INSUFFICIENT_FUNDS: (amount: string, currency: string, usd: string) => + `Not enough funds. You are ~${amount} ${currency} ($ ${usd}) short.`, + INVALID_ADDRESS: 'Invalid to address.', + INVALID_TOKEN_DECIMALS: (symbol: string) => `Invalid decimals for ${symbol}.`, + INVALID_NFT: 'Invalid NFT selected.', + EXCEEDS_MAX: 'Amount exceeds maximum value.', +} as const; const errorMsg = computed(() => { - if (!hasValidDecimals.value) { - return `Too many decimals.`; - } + if (!hasValidDecimals.value) return ERROR_MESSAGES.INVALID_DECIMALS; // ... rest of the function using ERROR_MESSAGES });
460-464: Consider combining validation checks.The
isValidSendcomputed property could be simplified by combining the validation checks.const isValidSend = computed<boolean>(() => { - if (!isInputsValid.value) return false; - if (nativeBalanceAfterTransactionInBaseUnits.value.isNeg()) return false; - if (!isEstimateValid.value) return false; - if (gasCostValues.value.ECONOMY.nativeValue === '0') return false; - return true; + return ( + isInputsValid.value && + !nativeBalanceAfterTransactionInBaseUnits.value.isNeg() && + isEstimateValid.value && + gasCostValues.value.ECONOMY.nativeValue !== '0' + ); });
Update required: References to old property found in Bitcoin provider
The old property
nativeBalanceAfterTransactionis still being used inpackages/extension/src/providers/bitcoin/ui/send-transaction/index.vue. This needs to be updated to maintain consistency with the new properties:
packages/extension/src/providers/bitcoin/ui/send-transaction/index.vue: Update the property name and its usages in template and script sections to match the new naming convention (nativeBalanceAfterTransactionInBaseUnitsandnativeBalanceAfterTransactionInCoreUnits).🔗 Analysis chain
Line range hint
290-346: Excellent documentation and clear naming.The balance calculation properties are well-documented and clearly named. The separation into base units and core units improves clarity and maintainability.
However, verify that all callers of the old
nativeBalanceAfterTransactionproperty have been updated to use the new properties.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to the old property name rg "nativeBalanceAfterTransaction[^I]" --type vueLength of output: 79
Script:
#!/bin/bash # Search for any remaining references to the old property name, using ripgrep without file type filter rg "nativeBalanceAfterTransaction[^I]"Length of output: 690
packages/extension/src/providers/ethereum/ui/send-transaction/index.vue (3)
Line range hint
216-231: Consider removing redundant validation.The check
if (!isNumericPositive(sendAmount.value))is redundant since we already havehasPositiveSendAmountvalidation.const hasEnoughBalance = computed(() => { if (!hasValidDecimals.value) { return false; } if (!hasPositiveSendAmount.value) { return false; } - // check if valid sendAmount.value - if (!isNumericPositive(sendAmount.value)) { - return false; - } return toBN(selectedAsset.value.balance ?? '0').gte( toBN(toBase(sendAmount.value ?? '0', selectedAsset.value.decimals!)), ); });
419-459: Enhance error message clarity for low balance.The error message for insufficient funds could be more user-friendly. Consider:
- Using template literals for better readability
- Rounding the shortage amount to a reasonable number of decimals
- Adding parentheses around the USD amount
- return `Not enough funds. You are - ~${formatFloatingPointValue(nativeBalanceAfterTransactionInCoreUnits.value).value} - ${props.network.currencyName} ($ ${ - formatFiatValue(priceDifferenceUsd.value).value - }) short.`; + return `Not enough funds. You are short ${ + formatFloatingPointValue(nativeBalanceAfterTransactionInCoreUnits.value.abs()).value + } ${props.network.currencyName} ($${ + formatFiatValue(priceDifferenceUsd.value).value + })`;
549-552: Consider a more descriptive variable name.The variable name
isValidSendcould be more specific, such ascanExecuteTransactionorisTransactionValid, to better reflect its purpose of validating the entire transaction, not just the send operation.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
packages/extension/src/providers/ethereum/ui/send-transaction/components/send-alert.vue(1 hunks)packages/extension/src/providers/ethereum/ui/send-transaction/index.vue(7 hunks)packages/extension/src/providers/solana/ui/send-transaction/index.vue(6 hunks)
🔇 Additional comments (4)
packages/extension/src/providers/ethereum/ui/send-transaction/components/send-alert.vue (2)
1-53: Well-structured and accessible error alert component
The implementation provides a good foundation for displaying transaction error messages with proper accessibility considerations. The component is reusable and aligns well with the PR's objective of improving error messaging for low balance scenarios.
23-23: Verify color contrast for accessibility
Please ensure that the error text color (@error) has sufficient contrast against the background color (@error01) to meet WCAG accessibility standards.
Also applies to: 41-41
packages/extension/src/providers/solana/ui/send-transaction/index.vue (1)
216-221: LGTM! Well-structured validation properties.
The new computed properties hasValidDecimals and hasPositiveSendAmount effectively separate concerns and improve code maintainability.
packages/extension/src/providers/ethereum/ui/send-transaction/index.vue (1)
210-215: LGTM! Clean validation implementation.
The new computed properties provide clear, single-responsibility validations that improve code readability and maintainability.
There was a problem hiding this comment.
Remove .abs() call in balance computation.
Using .abs() on the balance after transaction could hide negative balances and lead to incorrect error messages. The absolute value is only needed when displaying the shortage amount in the error message.
const nativeBalanceAfterTransactionInCoreUnits = computed(() => {
return fromBase(
- nativeBalanceAfterTransactionInBaseUnits.value.abs().toString(),
+ nativeBalanceAfterTransactionInBaseUnits.value.toString(),
props.network.decimals,
);
});📝 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.
| const nativeBalanceAfterTransactionInCoreUnits = computed(() => { | |
| return fromBase( | |
| nativeBalanceAfterTransactionInBaseUnits.value.abs().toString(), | |
| props.network.decimals, | |
| ); | |
| }); | |
| const nativeBalanceAfterTransactionInCoreUnits = computed(() => { | |
| return fromBase( | |
| nativeBalanceAfterTransactionInBaseUnits.value.toString(), | |
| props.network.decimals, | |
| ); | |
| }); |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
packages/extension/src/providers/ethereum/ui/send-transaction/index.vue (3)
Line range hint
216-231: Consider adding early returns for better readability.The
hasEnoughBalancecomputed property could be more readable with early returns.const hasEnoughBalance = computed(() => { if (!hasValidDecimals.value) { return false; } if (!hasPositiveSendAmount.value) { return false; } - // check if valid sendAmount.value - if (!isNumericPositive(sendAmount.value)) { - return false; - } return toBN(selectedAsset.value.balance ?? '0').gte( toBN(toBase(sendAmount.value ?? '0', selectedAsset.value.decimals!)), ); });
406-416: Consider handling undefined price more gracefully.The
nativeCurrencyUsdPricefalls back to '0' when price is undefined, which could lead to misleading USD calculations.const nativeCurrencyUsdPrice = computed(() => { - return accountAssets.value[0]?.price || '0'; + const price = accountAssets.value[0]?.price; + return price !== undefined ? price : null; }); const balanceAfterInUsd = computed(() => { + if (nativeCurrencyUsdPrice.value === null) { + return null; + } return new BigNumber( nativeBalanceAfterTransactionInHumanUnits.value.toString(), ) - .times(nativeCurrencyUsdPrice.value ?? '0') + .times(nativeCurrencyUsdPrice.value) .toFixed(); });
548-551: Consider adding a comment explaining the validation logic.The
isValidSendcomputed property includes critical checks, but the reasoning behind the order of checks isn't immediately clear.const isValidSend = computed<boolean>(() => { + // Check basic input validation before checking balance and gas estimation if (!isInputsValid.value) return false; if (nativeBalanceAfterTransactionInBaseUnits.value.isNeg()) return false; if (!isEstimateValid.value) return false; return true; });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
packages/extension/src/providers/ethereum/ui/send-transaction/index.vue(7 hunks)packages/extension/src/providers/solana/ui/send-transaction/index.vue(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/extension/src/providers/solana/ui/send-transaction/index.vue
🔇 Additional comments (4)
packages/extension/src/providers/ethereum/ui/send-transaction/index.vue (4)
106-106: LGTM: Error alert integration looks good.
The integration of the send-alert component with the new errorMsg computed property provides clear user feedback.
210-215: LGTM: Well-structured validation properties.
The new computed properties hasValidDecimals and hasPositiveSendAmount improve code readability by separating concerns.
Line range hint 342-404: Verify the impact of using .abs() on balance calculations.
Using .abs() on nativeBalanceAfterTransactionInBaseUnits in the human units calculation could hide negative balances and lead to incorrect error messages.
418-458: LGTM: Comprehensive error message handling.
The errorMsg computed property provides clear and specific error messages for different validation scenarios, aligning well with the PR objective of improving error messaging.
|
merged to #573 |
3 participants
On EVM and Solana networks, when a users balance is too low to send a transaction an error is displayed with the amount that they are short by. This copies the behavior from Bitcoin send.
Summary by CodeRabbit
send-alertcomponent for displaying error messages related to transaction sending.best-offer-warningcomponent.