fix: error message for send when address has no balance for rent#561
fix: error message for send when address has no balance for rent#561gamalielhere merged 14 commits intodevelopfrom
Conversation
WalkthroughThe changes introduce a new Vue component, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SendTransaction
participant SendAlert
User->>SendTransaction: Initiate transaction
SendTransaction->>SendTransaction: Check balance, address, NFT validity
SendTransaction->>SendTransaction: Compute errorMsg
alt Error conditions met
SendTransaction->>SendAlert: Render <send-alert> with errorMsg
else No errors
SendTransaction->>SendTransaction: Proceed with transaction
end
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
|
|
💼 Build Files |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (6)
packages/extension/src/providers/solana/ui/send-transaction/components/send-alert.vue (3)
8-16: Add prop validation and component documentationConsider adding prop validation and JSDoc documentation to improve component maintainability.
Apply these changes:
<script setup lang="ts"> import AlertIcon from '@action/icons/send/alert-icon.vue'; +/** + * Alert component for displaying error messages in the Solana UI + * @component + * @example + * <send-alert error-msg="Insufficient balance for transaction" /> + */ + interface IProps { - errorMsg: string; + /** + * Error message to display in the alert + * @required + */ + errorMsg: string & { readonly __brand: unique symbol }; } -defineProps<IProps>(); +const props = withDefaults(defineProps<IProps>(), { + errorMsg: '' as IProps['errorMsg'] +}); + +// Validate empty error message +if (props.errorMsg.trim() === '') { + console.warn('SendAlert: Empty error message provided'); +} </script>
18-53: Improve styling maintainability and responsivenessThe styling could benefit from better maintainability and mobile responsiveness.
Consider these improvements:
<style lang="less"> @import '@action/styles/theme.less'; +// Variables for consistent spacing and sizing +@alert-padding: 16px; +@alert-icon-size: 24px; +@alert-border-radius: 10px; + .send-alert { - margin: 0 32px 8px 32px; + margin: 0 @alert-padding*2 8px @alert-padding*2; background: @error01; - border-radius: 10px; - padding: 12px 16px 12px 57px; + border-radius: @alert-border-radius; + padding: 12px @alert-padding 12px (@alert-padding * 3.5); position: relative; box-sizing: border-box; + // Mobile responsiveness + @media screen and (max-width: 768px) { + margin: 0 @alert-padding 8px @alert-padding; + } + svg { position: absolute; - left: 16px; + left: @alert-padding; top: 50%; - margin-top: -12px; + transform: translateY(-50%); // Better centering + width: @alert-icon-size; + height: @alert-icon-size; } p { font-weight: 400; font-size: 14px; line-height: 20px; letter-spacing: 0.25px; color: @error; margin: 0; + word-break: break-word; // Handle long error messages a { color: @error; + transition: opacity 0.2s ease; // Smooth hover effect &:hover { text-decoration: none; + opacity: 0.8; } } } } </style>
1-53: Consider component reusability across the applicationThe alert component seems specific to send transaction errors, but its design could be generalized for broader use across the application.
Consider:
- Renaming to a more generic name like
StatusAlertorMessageAlert- Adding a
typeprop to support different alert types (error, warning, success)- Making it part of a shared components library for reuse in other parts of the application
packages/extension/src/providers/solana/ui/send-transaction/index.vue (3)
96-96: Usev-ifInstead ofv-showfor Conditional RenderingThe
send-alertcomponent is conditionally rendered usingv-show="errorMsg". SinceerrorMsgcan be an empty string, which is falsy, usingv-ifmight be more appropriate to prevent rendering the component when there's no error message.Apply this diff to use
v-if:-<send-alert v-show="errorMsg" :error-msg="errorMsg" /> +<send-alert v-if="errorMsg" :error-msg="errorMsg" />
371-406: Simplify Logic incheckIfToIsRentExemptFunctionThe conditional statements within
checkIfToIsRentExemptare complex and can be simplified for better readability and maintainability.Consider refactoring the function as follows:
const checkIfToIsRentExempt = async () => { const toPubKey = new PublicKey(getAddress(addressTo.value)); const balance = await solConnection.value!.web3.getBalance(toPubKey); const rentExempt = await solConnection.value!.web3.getMinimumBalanceForRentExemption( ACCOUNT_SIZE, ); // Rent is added to ATA, so no need to check for non-native tokens if ( isSendToken.value && selectedAsset.value.contract !== NATIVE_TOKEN_ADDRESS ) { hasMinimumForRent.value = true; return; } - /** balance is lower than rent but to amount is higher than rent - else if balance is higher than rent - else if balance is lower than rent and to amount is lower than rent **/ - if ( - toBN(balance).lt(toBN(rentExempt)) && - toBN(sendAmount.value).gt(toBN(rentExempt)) - ) { + // Check if recipient's balance or the amount being sent covers the rent exemption + if (toBN(balance).gte(toBN(rentExempt)) || toBN(sendAmount.value).gte(toBN(rentExempt))) { hasMinimumForRent.value = true; } else { hasMinimumForRent.value = false; } };
350-351: Clarify the Error Message for Insufficient Recipient BalanceThe current error message may be unclear to the user. Consider rephrasing it for better understanding.
Apply this diff to improve the message:
-return `Recipient doesn't have enough ${props.network.currencyName} to receive.`; +return `Recipient doesn't have enough ${props.network.currencyName} to receive the transaction.`;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
packages/extension/src/providers/solana/ui/send-transaction/components/send-alert.vue(1 hunks)packages/extension/src/providers/solana/ui/send-transaction/index.vue(7 hunks)
🔇 Additional comments (2)
packages/extension/src/providers/solana/ui/send-transaction/index.vue (2)
462-462: Ensure hasMinimumForRent Is Evaluated Correctly
In the isValidSend computed property, the condition if (!hasMinimumForRent.value) return false; might prevent valid transactions if hasMinimumForRent hasn't been updated yet. Ensure that checkIfToIsRentExempt is called and hasMinimumForRent is updated before this validation.
Consider verifying that checkIfToIsRentExempt is triggered appropriately before isValidSend is evaluated.
603-603: Confirm checkIfToIsRentExempt Is Watching the Correct Dependencies
The checkIfToIsRentExempt function is called within a watcher. Ensure that all relevant dependencies are included so that the function is triggered at the correct times.
Verify that the watcher includes all necessary dependencies. If additional dependencies are needed, update the watcher accordingly.
packages/extension/src/providers/solana/ui/send-transaction/components/send-alert.vue
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
packages/extension/src/providers/solana/ui/send-transaction/index.vue (3)
96-96: Consider adding ARIA attributes for better accessibility.The error alert component should have appropriate ARIA attributes to ensure screen readers can properly announce errors to users.
- <send-alert v-show="errorMsg" :error-msg="errorMsg" /> + <send-alert + v-show="errorMsg" + :error-msg="errorMsg" + role="alert" + aria-live="polite" + />
460-478: Improve error handling and extract magic numbers.The rent exemption check implementation could be improved:
- Add error handling for web3 calls
- Extract ACCOUNT_SIZE to a constant
+ try { const toPubKey = new PublicKey(getAddress(addressTo.value)); const balance = await solConnection.value!.web3.getBalance(toPubKey); const rentExempt = await solConnection.value!.web3.getMinimumBalanceForRentExemption( ACCOUNT_SIZE, ); if (toBN(balance).lt(toBN(rentExempt))) { storageFee.value = await solConnection.value!.web3.getMinimumBalanceForRentExemption( ACCOUNT_SIZE, ); } + } catch (error) { + console.error('Failed to check rent exemption:', error); + storageFee.value = 0; + }
460-478: Consider caching rent exemption values.The code makes multiple calls to
getMinimumBalanceForRentExemptionwith the same parameter. Consider caching this value to reduce RPC calls.+const rentExemptCache = new Map<number, number>(); + +const getRentExempt = async (size: number) => { + if (!rentExemptCache.has(size)) { + rentExemptCache.set( + size, + await solConnection.value!.web3.getMinimumBalanceForRentExemption(size) + ); + } + return rentExemptCache.get(size)!; +};
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
packages/extension/src/providers/solana/ui/send-transaction/components/send-alert.vue(1 hunks)packages/extension/src/providers/solana/ui/send-transaction/index.vue(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/extension/src/providers/solana/ui/send-transaction/components/send-alert.vue
🔇 Additional comments (1)
packages/extension/src/providers/solana/ui/send-transaction/index.vue (1)
342-346: Remove unintended tilde character in error message.
This issue was previously identified. The tilde character in the error message should be removed as it's a leftover from line annotations.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/extension/src/ui/action/views/swap/index.vue (2)
541-550: Approve the edge case handling with documentation improvements.The logic for handling Solana's special case is correct. However, the documentation could be more professional and detailed.
Consider improving the comment to be more descriptive:
- /** - * weird edgecase for solana where rango doesn't return - * minumum amount for the swap and returns changelly - * but rango's actual minimum is a lot lower than changelly's - */ + /** + * Special case for Solana network: + * When Rango returns only Changelly as a provider, we display a different message + * because Rango's actual minimum swap amount can be lower than Changelly's reported minimum. + * This prevents users from being blocked by an artificially high minimum amount. + */
541-555: Consider refactoring for better maintainability.While the changes are well integrated, the
pickBestQuotefunction is handling multiple responsibilities. Consider breaking it down into smaller, focused functions for better maintainability.Example refactor:
function getErrorMessageForMinimumAmount( fromT: SwapToken, lowestMinimum: BN, quotes: ProviderQuoteResponse[] ): string { if (quotes.length === 1) { return `Minimum amount may be lower than: ~${fromT.toReadable(lowestMinimum)}`; } return `Minimum amount: ${fromT.toReadable(lowestMinimum)}`; } function validateQuoteAmounts( fromAmountBN: BN, quotes: ProviderQuoteResponse[], fromT: SwapToken ): string | null { const { lowestMinimum, highestMaximum } = getQuoteBounds(quotes); if (fromAmountBN.lt(lowestMinimum)) { return getErrorMessageForMinimumAmount(fromT, lowestMinimum, quotes); } if (fromAmountBN.gt(highestMaximum)) { return `Maximum amount: ${fromT.toReadable(highestMaximum)}`; } return null; }
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
packages/extension/src/libs/cache-fetch/index.ts (3)
Line range hint
45-52: Improve error handling robustnessThe current error check
!json.errorhas several potential issues:
- No null/undefined check for
json- Doesn't handle different error formats (e.g.,
errorsarray,status: "error", etc.)- Missing type checking for the error property
Consider this safer implementation:
- if (!json.error) { + if (json && typeof json === 'object' && !json.error && !json.errors) { const store: StoredData = { timestamp: new Date().getTime(), data: jsonstring, }; return storage.set(hash, store).then(() => jsondata); } return jsondata;
Line range hint
1-54: Consider implementing security measuresThe cache implementation could benefit from additional security measures:
- URL validation to prevent cache poisoning
- Response data validation before caching
- Size limits for cached data
- CORS and content-type checks
Consider implementing:
- URL allowlist or pattern validation
- JSON schema validation for responses
- Maximum size limits for cached data
- Response headers validation
Would you like me to provide a detailed implementation for these security measures?
Error handling patterns are inconsistent across the codebase
The verification reveals inconsistent error handling patterns:
packages/swap/src/common/estimateGasList.ts- Returns structured error object withisErroranderrorMessagefromjson.error.messagepackages/swap/src/providers/zerox/index.ts- Usesresponse.codeandresponse.reasonpackages/swap/src/providers/paraswap/index.ts- Only logsresponse.errorto consolepackages/extension/src/providers/ethereum/libs/tx-broadcaster.ts- Rejects withjRes.errordirectlypackages/extension/src/providers/ethereum/libs/assets-handlers/- Rejects with stringified error messagepackages/extension/src/providers/bitcoin/libs/api.ts- Multiple patterns: returns null, '0', [], or handles specific error codesThe current implementation in cache-fetch silently ignores errors by returning
jsondatawhenjson.errorexists, which differs from most other implementations that either reject the promise or return a structured error response.Consider standardizing error handling by either:
- Rejecting the promise with a structured error object
- Returning a consistent error response format across all API handlers
🔗 Analysis chain
Line range hint
45-52: Verify error handling across the codebaseLet's verify how errors are structured in other API responses to ensure consistent handling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for error handling patterns in fetch responses rg -A 5 "\.then\(.*json.*\)" | rg -B 2 "error" # Search for error property access patterns ast-grep --pattern 'json.error'Length of output: 6837
packages/extension/src/providers/solana/libs/api.ts (1)
67-67: Consider using a specific commit hash or tag instead of master.While using Solflare's token list is a good choice, referencing the master branch directly could lead to unexpected breaking changes. Consider using a specific commit hash or tag for better stability.
Example URL format:
-'https://raw.githubusercontent.com/solflare-wallet/token-list/refs/heads/master/solana-tokenlist.json' +'https://raw.githubusercontent.com/solflare-wallet/token-list/<commit-hash>/solana-tokenlist.json'packages/extension/src/providers/solana/ui/send-transaction/index.vue (1)
460-481: Add documentation for rent exemption logicThe rent exemption check and fee calculation are correctly implemented. However, consider adding a comment explaining why this check is necessary for Solana accounts.
Add this comment before the rent exemption check:
const toBalance = await solConnection.value!.web3.getBalance(to); + // Check if the recipient account needs rent exemption deposit + // Solana accounts must maintain a minimum balance to stay rent-exempt const rentExempt = await solConnection.value!.web3.getMinimumBalanceForRentExemption( ACCOUNT_SIZE, );
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
packages/extension/src/libs/cache-fetch/index.ts(1 hunks)packages/extension/src/providers/solana/libs/api.ts(2 hunks)packages/extension/src/providers/solana/ui/send-transaction/index.vue(5 hunks)
🔇 Additional comments (4)
packages/extension/src/providers/solana/libs/api.ts (1)
Line range hint 67-77: Verify token list URL and content structure.
The changes look good. Let's verify the token list URL availability and content structure to ensure reliability.
✅ Verification successful
Token list URL and structure verification successful
The token list endpoint is accessible and contains the expected data structure with all required fields (address, decimals, name, and symbol) in the tokens array, ensuring reliable token information retrieval.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify token list URL and content structure
# Check if the URL is accessible and returns valid JSON with expected structure
curl -s 'https://raw.githubusercontent.com/solflare-wallet/token-list/refs/heads/master/solana-tokenlist.json' | \
jq '
# Verify the response has tokens array
if has("tokens") then
# Check first token has required fields
.tokens[0] |
if has("address") and has("decimals") and has("name") and has("symbol") then
"✓ Token list structure is valid"
else
"✗ Token list is missing required fields"
end
else
"✗ Invalid token list format"
end
'
Length of output: 513
packages/extension/src/providers/solana/ui/send-transaction/index.vue (3)
96-96: LGTM: Alert component integration
The conditional rendering of the alert component is properly implemented.
330-332: Past review comment is still applicable
The null check for empty accountAssets array is still missing.
340-365: LGTM: Comprehensive error handling
The error message computation covers all essential validation scenarios:
- Insufficient funds with detailed balance information
- Invalid address validation
- Token-specific decimal validation
- NFT selection validation
- Maximum value validation
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
packages/swap/src/providers/changelly/index.ts (2)
102-102: Consider using TypeScript's empty function type.For better type safety, consider using the TypeScript empty function type.
- debug = () => { }; + debug = (() => {}) as typeof debug;
289-290: Use template literals for better readability.Consider using template literals (backticks) instead of string concatenation for better readability and maintainability of error messages.
Example for the first error message:
- `Error validating address with via Changelly` + - ` code=${String(response.error.code)}` + - ` message=${String(response.error.message)}`, + `Error validating address with via Changelly + code=${String(response.error.code)} + message=${String(response.error.message)}`Also applies to: 359-363, 516-521, 729-733
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
packages/swap/src/providers/changelly/index.ts(28 hunks)
🔇 Additional comments (1)
packages/swap/src/providers/changelly/index.ts (1)
1012-1015: 🛠️ Refactor suggestion
Improve decimal handling robustness.
The decimal handling logic could be improved with better type safety and error handling. Consider adding input validation and using BigInt consistently.
Also applies to: 1030-1035
3 participants
Summary by CodeRabbit
New Features
send-transactioncomponent to provide users with clear feedback on transaction eligibility, including checks for balance sufficiency and address validity.Bug Fixes
Improvements