fix: send input ui breaking with multiple periods #567
Conversation
WalkthroughThe pull request introduces enhancements to the input validation logic across multiple send transaction components for Bitcoin, Ethereum, and Solana wallet applications. Each component now employs a more robust validation process using the Changes
Possibly related PRs
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 (5)
packages/extension/src/providers/common/ui/send-transaction/send-input-amount.vue (1)
Line range hint
51-60: Add error handling for BigNumber calculationsThe
fiatEquivalentcomputed property should handle invalid numerical inputs gracefully to prevent potential errors.Consider adding error handling:
const fiatEquivalent = computed(() => { - return new BigNumber(props.fiatValue).times(props.amount).toString(); + try { + if (!props.amount || !props.fiatValue) return '0'; + const value = new BigNumber(props.fiatValue).times(props.amount); + return value.isNaN() ? '0' : value.toString(); + } catch (error) { + console.error('Error calculating fiat equivalent:', error); + return '0'; + } });packages/extension/src/providers/bitcoin/ui/send-transaction/index.vue (2)
308-311: Consider enhancing user feedback for invalid inputsWhile the validation logic is solid, consider providing more specific feedback to users when their input is invalid. This could help users understand why the send button is disabled.
Consider adding computed properties for specific error states:
+const inputErrors = computed(() => { + const errors = []; + const sendAmountBigNumber = new BigNumber(sendAmount.value); + + if (sendAmountBigNumber.isNaN()) { + errors.push('Invalid number format'); + } else if (sendAmountBigNumber.gt(assetMaxValue.value)) { + errors.push('Amount exceeds available balance'); + } + return errors; +});Then use these errors to display feedback in the template.
308-311: Consider synchronizing validation logic with inputAmount methodThe
inputAmountmethod (around line 400) also uses BigNumber for validation but handles it differently. Consider unifying the validation approach for consistency.Extract the validation logic into a reusable function:
+const isValidAmount = (amount: string): boolean => { + const amountBN = new BigNumber(amount); + return !amountBN.isNaN() && !amountBN.gt(assetMaxValue.value); +}; const inputAmount = (inputAmount: string) => { if (inputAmount === '') { inputAmount = '0'; } - const inputAmountBn = new BigNumber(inputAmount); - isMaxSelected.value = false; - amount.value = inputAmountBn.lt(0) ? '0' : inputAmount; + isMaxSelected.value = false; + amount.value = isValidAmount(inputAmount) ? inputAmount : '0'; };packages/extension/src/providers/solana/ui/send-transaction/index.vue (1)
402-404: LGTM! The BigNumber validation fixes the multiple periods issue.The new validation logic properly handles invalid numeric inputs, including cases with multiple periods, by using BigNumber's isNaN check.
Consider enhancing the validation with explicit checks and user feedback:
const sendAmountBigNumber = new BigNumber(sendAmount.value); if (sendAmountBigNumber.isNaN()) return false; +if (sendAmountBigNumber.isNegative()) return false; if (sendAmountBigNumber.gt(assetMaxValue.value)) return false;Additionally, consider emitting validation errors to show helpful messages to users when validation fails:
const getValidationError = computed(() => { const amount = new BigNumber(sendAmount.value); if (amount.isNaN()) return 'Please enter a valid number'; if (amount.isNegative()) return 'Amount cannot be negative'; if (amount.gt(assetMaxValue.value)) return 'Insufficient balance'; return null; });packages/extension/src/providers/ethereum/ui/send-transaction/index.vue (1)
489-491: Consider adding user feedback for invalid inputsThe validation silently fails when encountering invalid numbers. Consider providing feedback to help users understand why their input was rejected.
+ const isValidAmount = computed(() => { + if ((sendAmount.value.match(/\./g) || []).length > 1) { + return { valid: false, error: 'Multiple decimal points are not allowed' }; + } + const sendAmountBigNumber = new BigNumber(sendAmount.value); + if (sendAmountBigNumber.isNaN()) { + return { valid: false, error: 'Invalid number format' }; + } + if (sendAmountBigNumber.gt(assetMaxValue.value)) { + return { valid: false, error: 'Amount exceeds maximum value' }; + } + return { valid: true, error: null }; + }); const isInputsValid = computed<boolean>(() => { if (!props.network.isAddress(addressTo.value)) return false; if ( isSendToken.value && !isValidDecimals(sendAmount.value, selectedAsset.value.decimals!) ) { return false; } if (!isSendToken.value && !selectedNft.value.id) { return false; } - const sendAmountBigNumber = new BigNumber(sendAmount.value) - if (sendAmountBigNumber.isNaN()) return false - if (sendAmountBigNumber.gt(assetMaxValue.value)) return false; + if (!isValidAmount.value.valid) return false; if (gasCostValues.value.REGULAR.nativeValue === '0') return false; return true; });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
packages/extension/src/providers/bitcoin/ui/send-transaction/index.vue(1 hunks)packages/extension/src/providers/common/ui/send-transaction/send-input-amount.vue(1 hunks)packages/extension/src/providers/ethereum/ui/send-transaction/index.vue(1 hunks)packages/extension/src/providers/solana/ui/send-transaction/index.vue(1 hunks)
🔇 Additional comments (3)
packages/extension/src/providers/common/ui/send-transaction/send-input-amount.vue (1)
76-85: Verify the fix for multiple periods across different scenarios
While the implementation prevents multiple periods, we should verify it handles all edge cases correctly.
✅ Verification successful
Implementation correctly handles decimal input validation
The code implements proper validation for numeric input with decimals:
- Allows single decimal point entry
- Prevents multiple decimal points
- Handles edge case where decimal is first character by converting it to "0."
- Consistent with standard numeric input patterns
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar input validation patterns across the codebase
# to ensure consistent handling of decimal inputs
# Look for other Vue components with number input validation
rg -l "keypress.*onlyNumber" --type vue
# Check for other BigNumber usage patterns
rg "new BigNumber.*toString" --type vue -A 2
# Look for related test files
fd -e spec.ts -e test.ts send-input
Length of output: 182
Script:
#!/bin/bash
# Search for similar input validation patterns across the codebase
# Look for keypress event handlers and number validation
rg -t js -t ts -t vue "keypress|keyCode.*preventDefault" -A 3
# Check for BigNumber usage patterns
rg -t js -t ts -t vue "BigNumber.*toString" -A 2
# Look for test files related to send-input
find . -type f -name "*send-input*.spec.ts" -o -name "*send-input*.test.ts"
# Look for similar numeric input validation in other files
rg -t js -t ts -t vue "indexOf\('\.'\)|indexOf\(\"\.\"\)" -A 2
Length of output: 345
Script:
#!/bin/bash
# Search without specifying file types to catch all relevant files
rg "keypress.*preventDefault" -A 3
# Look for decimal point validation patterns
rg "indexOf\\(\\.\\)" -A 2
# Find all Vue files first to understand the structure
fd -e vue
# Look for numeric input validation patterns
rg "keyCode.*[0-9].*preventDefault" -A 3
Length of output: 25627
Script:
#!/bin/bash
# Look at the specific file content to understand the validation logic
cat "packages/extension/src/providers/common/ui/send-transaction/send-input-amount.vue"
# Search for similar numeric input validation patterns in the codebase
rg "keyCode.*[0-9].*preventDefault|indexOf\\(\"\\.\"\\)|indexOf\\('\\.'\\)" -A 3 -B 1
Length of output: 4822
packages/extension/src/providers/bitcoin/ui/send-transaction/index.vue (1)
308-311: Approve the enhanced input validation logic
The new validation using BigNumber effectively addresses the issue with multiple periods in the input by:
- Properly handling invalid numeric inputs through
isNaN()check - Correctly comparing the amount against maximum value using
gt()
Let's verify the fix handles the multiple periods case correctly:
✅ Verification successful
Based on the search results, I can now generate the final response since I have enough information to verify the review comment. The search shows that similar BigNumber validation is consistently implemented across different wallet providers (Bitcoin, Ethereum, Solana, etc.) for handling send amounts, and specifically confirms that the Bitcoin implementation follows the same pattern as other providers.
Approve the BigNumber validation implementation
The validation using BigNumber is correctly implemented and consistent with the codebase patterns. The code:
- Properly validates numeric input through
isNaN()check - Correctly compares against maximum value using
gt() - Follows the same validation pattern used in other providers (Ethereum, Solana) for handling send amounts
- Is part of a broader input validation strategy across the wallet
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if similar validation is consistently applied across the codebase
# Look for other instances where we might need similar BigNumber validation
# Search for numeric input validation patterns
rg -A 3 'new BigNumber\(' --type vue
# Search for potential places handling decimal input
rg -A 3 'inputAmount|sendAmount|amount\.value' --type vue
Length of output: 154
Script:
#!/bin/bash
# Let's try without file type filtering since .vue might not be recognized
# Search for BigNumber validation patterns
rg -A 3 'new BigNumber\('
# Search for amount validation patterns
rg -A 3 'inputAmount|sendAmount|amount\.value'
# Also check for similar files in the bitcoin provider directory
fd -e vue . packages/extension/src/providers/bitcoin
Length of output: 79855
packages/extension/src/providers/ethereum/ui/send-transaction/index.vue (1)
489-491: Verify the fix addresses multiple periods across components
The PR title suggests this is a UI breaking issue with multiple periods, but the changes only handle NaN values. Let's verify if similar fixes are needed in other components.
packages/extension/src/providers/common/ui/send-transaction/send-input-amount.vue
Show resolved
Hide resolved
Summary by CodeRabbit
New Features
Bug Fixes
Documentation