Refactor fees components #17250
Refactor fees components #17250
Conversation
|
🚀 Expo preview is ready!
|
enjojoy
force-pushed
the
feat/refactor-fees-component
branch
2 times, most recently
from
3de2b0a to
c85c7e6
Compare
|
@trezor/qa Right now priority fee feature is turned off, this PR is essentially just a refactor of the code, so please make sure that nothing in the fees components changed or broke. |
|
Note Reviews pausedUse the following commands to manage reviews:
WalkthroughThe update introduces extensive changes focused on fee calculation and display across the application. A new 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: 2
🧹 Nitpick comments (8)
packages/suite/src/components/wallet/Fees/StandardFee/EthereumFeeCards.tsx (1)
33-68: Remove unnecessary Fragment.The Fragment at lines 35-67 contains only one child (the array mapping) and is not needed.
- return ( - showFee && ( - <> - {feeOptions?.map((fee, index) => ( - <FeeCard - key={index} - value={fee.value} - isSelected={selectedLevel.label === fee.value} - changeFeeLevel={changeFeeLevel} - topLeftChild={ - <span data-testid={`@fee-card/${fee.value}`}>{fee.label}</span> - } - topRightChild={getTimeEstimate(fee)} - bottomLeftChild={ - <FiatValue - disableHiddenPlaceholder - amount={fee.networkAmount || ''} - symbol={symbol} - showApproximationIndicator - /> - } - bottomRightChild={ - <FeeRate - feeRate={ - isEip1559 - ? fromWei(fee.effectiveGasPrice || '0', 'gwei') - : fee?.feePerUnit - } - networkType={networkType} - symbol={symbol} - /> - } - /> - ))} - </> - ) - ); + return ( + showFee && ( + feeOptions?.map((fee, index) => ( + <FeeCard + key={index} + value={fee.value} + isSelected={selectedLevel.label === fee.value} + changeFeeLevel={changeFeeLevel} + topLeftChild={ + <span data-testid={`@fee-card/${fee.value}`}>{fee.label}</span> + } + topRightChild={getTimeEstimate(fee)} + bottomLeftChild={ + <FiatValue + disableHiddenPlaceholder + amount={fee.networkAmount || ''} + symbol={symbol} + showApproximationIndicator + /> + } + bottomRightChild={ + <FeeRate + feeRate={ + isEip1559 + ? fromWei(fee.effectiveGasPrice || '0', 'gwei') + : fee?.feePerUnit + } + networkType={networkType} + symbol={symbol} + /> + } + /> + )) + ) + );🧰 Tools
🪛 Biome (1.9.4)
[error] 35-67: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
packages/suite/src/components/wallet/Fees/StandardFee/StandardFee.tsx (1)
47-62: ResizeObserver implementation should store the observer instance.The current implementation creates a new observer in both the observe and disconnect calls, which is inefficient.
useEffect(() => { if (!ref.current) return; - resizeObserver(feeOptions, setColumns).observe(ref.current); + const observer = resizeObserver(feeOptions, setColumns); + observer.observe(ref.current); - return () => resizeObserver(feeOptions, setColumns).disconnect(); + return () => observer.disconnect(); }, [feeOptions, setColumns]);packages/suite/src/components/wallet/Fees/StandardFee/BitcoinFeeCards.tsx (2)
26-65: Remove the unnecessary fragment.Per the static analysis hint, returning the map expression directly can simplify the JSX and eliminate the extra fragment.
- return ( - showFee && ( - <> - {feeOptions?.map((fee, index) => ( - <FeeCard - key={index} - ... - /> - ))} - </> - ) - ); + return ( + showFee && + feeOptions?.map((fee, index) => ( + <FeeCard + key={index} + ... + /> + )) + );🧰 Tools
🪛 Biome (1.9.4)
[error] 26-65: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
38-38: Eliminate the stray tilde character.There's a lone tilde (“~”) that seems accidental and might confuse future contributors.
-<> - ~ - {formatDurationStrict(feeInfo.blockTime * (fee?.blocks ?? 0) * 60, locale)} -</> +<> + {formatDurationStrict(feeInfo.blockTime * (fee?.blocks ?? 0) * 60, locale)} +</>packages/suite/src/components/wallet/Fees/CustomFee/CustomFeeEthereum.tsx (4)
42-43: Remove or implement placeholder errors.Currently,
customMaxBaseFeePerGasErrorandcustomMaxPriorityFeePerGasErrorare set toundefined. If you plan to use these for future validations, consider implementing them or removing the placeholders.
114-125: Clean up large commented-out block.This code block for base fee validation is fully commented out. If it's not needed, please remove it. Otherwise, leave a clarifying comment explaining why it remains.
164-170: Remove or explain the commented-out error handling.Your EIP-1559 validation references for
customMaxBaseFeePerGasErrorandcustomMaxPriorityFeePerGasErrorare commented out here. Clarify if they are postponed features or remove them to keep the codebase tidy.
18-219: Add more comprehensive unit test coverage.This component has detailed validation logic for gas limits and fee ranges. Please consider adding unit tests to ensure correctness of all scenarios, especially for edge cases.
Would you like me to propose a testing strategy or open a follow-up issue to track adding unit tests?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
packages/product-components/src/components/FeeRate/FeeRate.tsx(1 hunks)packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewSummary.tsx(4 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/BasicTxDetails.tsx(2 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/CancelTransaction/CancelTransaction.tsx(1 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/ChangeFee/ChangeFee.tsx(1 hunks)packages/suite/src/components/wallet/Fees/CustomFee.tsx(0 hunks)packages/suite/src/components/wallet/Fees/CustomFee/CurrentFee.tsx(1 hunks)packages/suite/src/components/wallet/Fees/CustomFee/CustomFee.tsx(1 hunks)packages/suite/src/components/wallet/Fees/CustomFee/CustomFeeEthereum.tsx(1 hunks)packages/suite/src/components/wallet/Fees/CustomFee/CustomFeeMisc.tsx(1 hunks)packages/suite/src/components/wallet/Fees/CustomFee/CustomFeeWrapper.tsx(1 hunks)packages/suite/src/components/wallet/Fees/CustomFee/index.tsx(1 hunks)packages/suite/src/components/wallet/Fees/FeeDetails.tsx(0 hunks)packages/suite/src/components/wallet/Fees/Fees.tsx(8 hunks)packages/suite/src/components/wallet/Fees/StandardFee/BitcoinFeeCards.tsx(1 hunks)packages/suite/src/components/wallet/Fees/StandardFee/EthereumFeeCards.tsx(1 hunks)packages/suite/src/components/wallet/Fees/StandardFee/FeeCard.tsx(1 hunks)packages/suite/src/components/wallet/Fees/StandardFee/MiscFeeCards.tsx(1 hunks)packages/suite/src/components/wallet/Fees/StandardFee/StandardFee.tsx(1 hunks)packages/suite/src/components/wallet/Fees/StandardFee/index.tsx(1 hunks)packages/suite/src/support/messages.ts(4 hunks)suite-common/wallet-config/src/utils.ts(1 hunks)
💤 Files with no reviewable changes (2)
- packages/suite/src/components/wallet/Fees/CustomFee.tsx
- packages/suite/src/components/wallet/Fees/FeeDetails.tsx
✅ Files skipped from review due to trivial changes (2)
- packages/suite/src/components/wallet/Fees/CustomFee/index.tsx
- packages/suite/src/components/wallet/Fees/StandardFee/index.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
packages/suite/src/components/wallet/Fees/StandardFee/EthereumFeeCards.tsx
[error] 35-67: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
packages/suite/src/components/wallet/Fees/StandardFee/BitcoinFeeCards.tsx
[error] 26-65: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
🔇 Additional comments (37)
packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/BasicTxDetails.tsx (2)
151-155: Addition of network symbol to enhance FeeRate componentThe symbol property has been added to the FeeRate component for Bitcoin transactions, providing network context that will be necessary for proper fee calculation and display.
175-179: Consistent symbol property addition for Ethereum transactionsSimilar to the Bitcoin implementation, the symbol property has been added to the FeeRate component for Ethereum transactions. This ensures consistent handling of fee rates across different network types.
packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/ChangeFee/ChangeFee.tsx (1)
32-34: Added symbol property to FeeRate componentThe symbol property has been appropriately added to the FeeRate component in the ChangeFee component, maintaining consistency with the fee component refactoring pattern applied across the codebase.
suite-common/wallet-config/src/utils.ts (1)
79-88: New utility function for settlement layer detectionWell-structured implementation of the
hasNetworkSettlementLayerutility function that will be useful for handling different fee calculation logic, especially for Ethereum L2 networks. The function is properly documented with JSDoc comments.packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/CancelTransaction/CancelTransaction.tsx (1)
71-75: Consistent addition of symbol property in CancelTransaction componentThe symbol property has been added to the FeeRate component in the CancelTransaction component, maintaining the consistent pattern across all fee-related components. This change supports the fee component refactoring without affecting existing functionality.
packages/suite/src/components/wallet/Fees/CustomFee/CurrentFee.tsx (1)
1-35: Well-structured component for displaying current fee information.This component effectively displays the current fee information with conditional rendering based on
isEip1559, showing either the current base fee or the custom fee. The implementation is clean and follows good React practices with appropriate prop typing.Note that the comment on line 15 indicates this component is designed for priority fees, which aligns with the PR objective to prepare for EIP-1559 priority fees while keeping the feature disabled for now.
packages/suite/src/components/wallet/Fees/CustomFee/CustomFeeWrapper.tsx (1)
1-29: Clean implementation of a reusable fee wrapper component.This component provides a consistent layout for custom fee interfaces, including an appropriate warning banner with a "Learn More" link to documentation. The component follows good composition patterns by accepting and rendering children elements.
packages/suite/src/components/wallet/Fees/CustomFee/CustomFeeMisc.tsx (3)
18-57: Feedback mechanism for fee rate differences is well implemented.The component correctly detects when the composed fee rate differs from the user input and generates appropriate warnings for Bitcoin networks. The conditional logic and translation handling look good.
59-78: Thorough validation rules for custom fee inputs.The validation logic properly handles range checking and Bitcoin-specific decimal validation. The error messages are clear and include appropriate values.
80-99: Clean UI implementation with appropriate error handling.The component renders a clean interface with proper error state handling and conditional warnings. The hidden input for fee limit maintains compatibility with the form structure while keeping the UI focused.
packages/suite/src/components/wallet/Fees/StandardFee/FeeCard.tsx (1)
1-45: Well-designed, reusable fee card component.This component provides a consistent card UI for fee selection with a clean, responsive layout. The component accepts customizable content for all four corners, making it flexible for different fee display requirements across network types. The use of
RadioCardprovides intuitive selection behavior.packages/suite/src/components/wallet/Fees/StandardFee/MiscFeeCards.tsx (1)
1-48: Implementation looks good for single fee option networks.The component correctly handles networks like Solana, Ripple, and Cardano that have only one fee option. The different fee calculation logic for Solana vs other networks is appropriate.
Regarding the comment on line 19 about Solana needing its own details component in the future - do you plan to create an issue to track this future enhancement?
packages/suite/src/components/wallet/Fees/StandardFee/EthereumFeeCards.tsx (1)
23-31: The EIP-1559 detection and time estimate implementation looks good.The check for EIP-1559 support and the conditional time formatting are well implemented.
packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewSummary.tsx (3)
53-57: Feature flag correctly implemented for EIP-1559.You've properly prepared for EIP-1559 priority fees while keeping the feature disabled with the hardcoded
falsevalue, which aligns with the PR objectives.
65-70: Good refactoring of EVMFeeInfo into a separate JSX element.This extraction improves readability by isolating the Ethereum-specific fee display logic.
51-52:❓ Verification inconclusive
Network derivation changed from account.symbol to symbol.
This change improves consistency but should be carefully tested to ensure it behaves identically across all scenarios.
🏁 Script executed:
#!/bin/bash # Check for any other direct usages of account.symbol in related files rg "account\.symbol" --type=typescript --glob="**/wallet/Fees/**" --glob="**/modals/ReduxModal/**"Length of output: 135
Below is an updated review comment. Because the initial search using the
--type=typescriptflag produced an error, please run the following revised shell script to double-check for any lingering references toaccount.symbolin the relevant directories:Attention: Verify Network Derivation Change
- The network is now derived from
symbolinstead ofaccount.symbol(i.e. using:). This improves consistency.const network = networks[symbol];- Action Needed: Please confirm that no unexpected references to
account.symbolremain in thewallet/Feesandmodals/ReduxModaldirectories and that the change behaves identically across all scenarios.
🏁 Script executed:
[run_scripts] #!/bin/bash # Search for direct usages of 'account.symbol' in the wallet/Fees and modals/ReduxModal directories without file type filtering. rg "account\.symbol" wallet/Fees modals/ReduxModalLength of output: 284
Attention: Verify Network Derivation Consistency
The change from using
account.symbolto directly usingsymbol(i.e., updating to:const network = networks[symbol];) does improve consistency. However, please ensure that this alteration does not introduce any side-effects in areas where the network derivation relies on the previous usage of
account.symbol.
- Action Item: Manually verify that no critical functionality depends on
account.symbol—for example, by running a comprehensive repository search (e.g.,rg "account\.symbol" --glob "!node_modules/**") to confirm there are no lingering references.- Testing: Please perform full regression testing on scenarios involving network handling to ensure the new derivation behaves identically.
packages/suite/src/components/wallet/Fees/StandardFee/StandardFee.tsx (2)
78-84: Well-structured network type to component mapping.Using a mapping object with default fallback to MiscFeeCards provides clean extensibility for future network additions.
21-32: StandardFeeProps definition is comprehensive.The interface includes all necessary properties and has helpful comments about eslint false-positives for future maintainers.
packages/suite/src/components/wallet/Fees/StandardFee/BitcoinFeeCards.tsx (1)
31-31: VerifyselectedLevelavailability.If
selectedLevelmight be undefined in certain states, accessingselectedLevel.labelcould cause runtime errors. Ensure it's reliably set or handle the undefined case.packages/suite/src/components/wallet/Fees/CustomFee/CustomFee.tsx (1)
80-82: ValidatenormalLevelexistence.This code assumes a 'normal' level is always present. If
feeInfo.levelslacks a 'normal' label,normalLevelwill be undefined, causing potential errors down the line.packages/suite/src/support/messages.ts (7)
3421-3424: Message added for displaying network base fee.The addition of the
TR_CURRENT_BASE_FEEmessage will allow the interface to correctly display the current network base fee to users. This aligns with the PR objective of refactoring fees components to prepare for EIP-1559 priority fees.
3425-3432: Message added for EIP-1559 max base fee.This message will be used to represent the maximum base fee per gas parameter in EIP-1559 transactions. The message is properly formatted and consistent with existing style.
3433-3436: Added gas limit error message without TR_ prefix.This message doesn't follow the common
TR_prefix pattern used for most message IDs in this file. However, there are other message IDs in the file without this prefix, so this isn't necessarily an inconsistency.
5578-5581: Added "Why fees?" message.Added a new message that will help users understand the purpose of transaction fees, improving user education within the interface.
5607-5610: Added medium fee level option.This message adds a "Medium" fee level option, expanding the existing fee levels (which currently include High, Normal, and Low). This provides users with more granular control over transaction fees.
5611-5618: Added EIP-1559 priority fee messages.These messages are required for implementing EIP-1559 fee structure, which uses both a base fee and a priority fee. These additions are in line with the PR's goal of preparing for EIP-1559 priority fees implementation.
5632-5635: Added validation message for custom base fee.This message will alert users when they attempt to set a custom base fee that's below the current network base fee, which would cause transaction issues. This is an important validation message that helps prevent transaction failures.
packages/suite/src/components/wallet/Fees/Fees.tsx (10)
30-30: Component replacement: FeeDetails → StandardFeeThe import has been changed from FeeDetails to StandardFee, which suggests a component rename or replacement as part of the fee components refactoring.
46-48: Improved type documentation with network-specific commentsAdding network-specific comments to the FeeOption type properties enhances code clarity and documentation, making it easier to understand which properties are used for specific networks.
76-76: Enhanced type safety with explicit return typeAdding the explicit return type
FeeOption[]to the buildFeeOptions function improves type safety and code clarity.
115-123: Ethereum fee handling refactored for EIP-1559 supportThe Ethereum case in buildFeeOptions has been completely refactored to include maxWaitTime and effectiveGasPrice properties, which are essential for EIP-1559 support. This change prepares the codebase for the future implementation of priority fees while maintaining backward compatibility.
157-159: Improved robustness with fallback fee levelAdding a fallback to the 'normal' fee level when the selected option isn't found improves the component's robustness and prevents potential undefined errors.
164-164: Added EIP-1559 detectionThe new boolean variable
isEip1559detects whether any fee option has an effectiveGasPrice property, which is used later to conditionally display appropriate fee labels.
213-216: Improved readability of SelectBar optionsThe SelectBar options array has been restructured for better readability while maintaining the same functionality.
227-227: Component usage updated to StandardFeeThe component usage has been updated from FeeDetails to StandardFee, matching the import change on line 30.
256-256: Conditional fee label based on fee structureThe Translation component now conditionally displays different labels based on whether EIP-1559 is detected, showing "MAX_FEE" for EIP-1559 transactions and "FEE" for others. This improves user understanding of fee structures.
1-290:✅ Verification successful
Overall refactoring approach aligns with PR objectives
The changes successfully refactor the fees component to prepare for EIP-1559 priority fees while keeping the feature disabled. The modifications improve type safety, enhance code clarity, and add detection for EIP-1559 capabilities without breaking existing functionality.
Run this script to verify that the StandardFee component accepts all the props passed to it:
🏁 Script executed:
#!/bin/bash # Check if StandardFee component has all the necessary props ast-grep --pattern $'function StandardFee({ networkType, feeInfo, selectedLevel, transactionInfo, showFee, feeOptions, symbol, changeFeeLevel }: StandardFeeProps)' # Check imports related to fee components rg -A 3 "import.*Fee" --glob "*.tsx" --glob "*.ts" packages/suite/src/components/wallet/Fees/Length of output: 9938
Verification confirmed: The fees component refactor is implemented correctly
The refactor effectively supports EIP-1559 enhancements without breaking existing functionality. The StandardFee component now correctly accepts and processes the expected props (networkType, feeInfo, selectedLevel, transactionInfo, showFee, feeOptions, symbol, changeFeeLevel), and the related fee component imports are in order.
| case 'ethereum': | ||
| fee = (Math.ceil(Number(feeRate) || 0 * multiplier) / multiplier).toFixed( | ||
| numOfDecimalPlaces, | ||
| ); | ||
| break; |
There was a problem hiding this comment.
Correct operator precedence for fee calculation.
The current expression may resolve incorrectly if Number(feeRate) is zero or falsy. Enclose (Number(feeRate) || 0) before multiplying to avoid unintended behavior.
-case 'ethereum':
- fee = (Math.ceil(Number(feeRate) || 0 * multiplier) / multiplier).toFixed(
- numOfDecimalPlaces,
- );
+case 'ethereum':
+ fee = (
+ Math.ceil((Number(feeRate) || 0) * multiplier) / multiplier
+ ).toFixed(numOfDecimalPlaces);📝 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.
| case 'ethereum': | |
| fee = (Math.ceil(Number(feeRate) || 0 * multiplier) / multiplier).toFixed( | |
| numOfDecimalPlaces, | |
| ); | |
| break; | |
| case 'ethereum': | |
| fee = ( | |
| Math.ceil((Number(feeRate) || 0) * multiplier) / multiplier | |
| ).toFixed(numOfDecimalPlaces); | |
| break; |
enjojoy
force-pushed
the
feat/refactor-fees-component
branch
from
c85c7e6 to
cf8aaaf
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/suite/src/components/wallet/Fees/CustomFee/CustomFeeEthereum.tsx (1)
42-45: Consider removing or finalizing commented-out error references.
Leaving these lines commented-out for too long may cause confusion. Either fully remove them or implement the error handling if this is part of the future EIP-1559 support.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/product-components/src/components/FeeRate/FeeRate.tsx(1 hunks)packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewSummary.tsx(3 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/BasicTxDetails.tsx(2 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/CancelTransaction/CancelTransaction.tsx(1 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/ChangeFee/ChangeFee.tsx(1 hunks)packages/suite/src/components/wallet/Fees/CustomFee/CustomFeeEthereum.tsx(1 hunks)packages/suite/src/support/messages.ts(4 hunks)suite-common/wallet-config/src/utils.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/CancelTransaction/CancelTransaction.tsx
- packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/BasicTxDetails.tsx
- packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/ChangeFee/ChangeFee.tsx
- suite-common/wallet-config/src/utils.ts
- packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewSummary.tsx
- packages/suite/src/support/messages.ts
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: run-desktop-tests (@group=passphrase, trezor-user-env-unix)
- GitHub Check: EAS Update
- GitHub Check: build-web
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: prepare_android_test_app
- GitHub Check: build-web
🔇 Additional comments (7)
packages/suite/src/components/wallet/Fees/CustomFee/CustomFeeEthereum.tsx (5)
1-3: Imports look organized and straightforward.
No issues found with the import statements for external dependencies and local modules.
37-38: ValidatefeeInfoto avoid undefined properties.
Make surefeeInfois always passed and valid to prevent potential runtime errors when destructuringmaxFeeandminFee.
71-76: Double-check the custom fee range logic.
Currently, the code returns an error message if the fee is outside[minFee, maxFee]. Confirm that users can intentionally override the recommended range or handle that scenario gracefully.
81-94: Good approach to block base fees below current base fee.
The logic ensures the base fee cannot be lower than the network base fee. This helps prevent invalid EIP-1559 transactions.
212-218: Overall structure provides clear distinction between EIP-1559 and Legacy fees.
The final return neatly routes the user to EIP-1559 or Legacy EVMTx fields. Good separation of concerns.packages/product-components/src/components/FeeRate/FeeRate.tsx (2)
16-17: Nice enhancement for networks with settlement layers.
Dynamically adjusting decimal places withhasNetworkSettlementLayer(symbol)improves fee presentation for different networks.
19-23: Fix operator precedence when multiplying bymultiplier.
This reproduces a previously noted issue. Wrap(Number(feeRate) || 0)in parentheses to ensure the multiplication is handled correctly.Apply this diff for clarity:
- fee = (Math.ceil(Number(feeRate) || 0 * multiplier) / multiplier).toFixed( - numOfDecimalPlaces, - ); + fee = ( + Math.ceil((Number(feeRate) || 0) * multiplier) / multiplier + ).toFixed(numOfDecimalPlaces);
enjojoy
force-pushed
the
feat/refactor-fees-component
branch
2 times, most recently
from
991dbbe to
ddbc5e4
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
packages/suite/src/components/wallet/Fees/CustomFee/CustomFee.tsx (1)
68-73:⚠️ Potential issueFix the integer-only check to allow decimals for Ethereum.
The comment at line 68 mentions allowing decimals in ETH, but the code checks for integer-only when
networkTypeis'ethereum'. This is contradictory and may block valid GWEI inputs.- if (['bitcoin', 'ethereum'].includes(networkType) && !isInteger(value)) { + // Only require an integer for Bitcoin; allow decimals for Ethereum + if (networkType === 'bitcoin' && !isInteger(value)) { return translationString('CUSTOM_FEE_IS_NOT_INTEGER'); }
🧹 Nitpick comments (3)
packages/suite/src/components/wallet/Fees/StandardFee/EthereumFeeCards.tsx (1)
33-68: Simplify fragment usage in the component.The outer fragment is unnecessary since it only contains a single child (the conditional expression). This can be simplified for better readability.
- return ( - showFee && ( - <> - {feeOptions?.map((fee, index) => ( - <FeeCard - key={index} - value={fee.value} - isSelected={selectedLevel.label === fee.value} - changeFeeLevel={changeFeeLevel} - topLeftChild={ - <span data-testid={`@fee-card/${fee.value}`}>{fee.label}</span> - } - topRightChild={getTimeEstimate(fee)} - bottomLeftChild={ - <FiatValue - disableHiddenPlaceholder - amount={fee.networkAmount || ''} - symbol={symbol} - showApproximationIndicator - /> - } - bottomRightChild={ - <FeeRate - feeRate={ - isEip1559 - ? fromWei(fee.effectiveGasPrice || '0', 'gwei') - : fee?.feePerUnit - } - networkType={networkType} - symbol={symbol} - /> - } - /> - ))} - </> - ) - ); + return showFee ? ( + feeOptions?.map((fee, index) => ( + <FeeCard + key={index} + value={fee.value} + isSelected={selectedLevel.label === fee.value} + changeFeeLevel={changeFeeLevel} + topLeftChild={ + <span data-testid={`@fee-card/${fee.value}`}>{fee.label}</span> + } + topRightChild={getTimeEstimate(fee)} + bottomLeftChild={ + <FiatValue + disableHiddenPlaceholder + amount={fee.networkAmount || ''} + symbol={symbol} + showApproximationIndicator + /> + } + bottomRightChild={ + <FeeRate + feeRate={ + isEip1559 + ? fromWei(fee.effectiveGasPrice || '0', 'gwei') + : fee?.feePerUnit + } + networkType={networkType} + symbol={symbol} + /> + } + /> + )) + ) : null;🧰 Tools
🪛 Biome (1.9.4)
[error] 35-67: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
packages/suite/src/components/wallet/Fees/StandardFee/BitcoinFeeCards.tsx (1)
24-66: Simplify fragment usage in the component.The outer fragment is unnecessary since it only contains a single child. This can be simplified for better readability.
- return ( - showFee && ( - <> - {feeOptions?.map((fee, index) => ( - <FeeCard - key={index} - value={fee.value} - isSelected={selectedLevel.label === fee.value} - changeFeeLevel={changeFeeLevel} - topLeftChild={ - <span data-testid={`@fee-card/${fee.value}`}>{fee.label}</span> - } - topRightChild={ - <> - ~ - {formatDurationStrict( - feeInfo.blockTime * (fee?.blocks ?? 0) * 60, - locale, - )} - </> - } - bottomLeftChild={ - <FiatValue - disableHiddenPlaceholder - amount={fee?.networkAmount ?? ''} - symbol={symbol} - showApproximationIndicator - /> - } - bottomRightChild={ - <> - <FeeRate - feeRate={fee.feePerUnit} - networkType={networkType} - symbol={symbol} - /> - {hasInfo ? ` (${transactionInfo.bytes} B)` : ''} - </> - } - /> - ))} - </> - ) - ); + return showFee ? ( + feeOptions?.map((fee, index) => ( + <FeeCard + key={index} + value={fee.value} + isSelected={selectedLevel.label === fee.value} + changeFeeLevel={changeFeeLevel} + topLeftChild={ + <span data-testid={`@fee-card/${fee.value}`}>{fee.label}</span> + } + topRightChild={ + formatDurationStrict( + feeInfo.blockTime * (fee?.blocks ?? 0) * 60, + locale, + ) + } + bottomLeftChild={ + <FiatValue + disableHiddenPlaceholder + amount={fee?.networkAmount ?? ''} + symbol={symbol} + showApproximationIndicator + /> + } + bottomRightChild={ + <> + <FeeRate + feeRate={fee.feePerUnit} + networkType={networkType} + symbol={symbol} + /> + {hasInfo ? ` (${transactionInfo.bytes} B)` : ''} + </> + } + /> + )) + ) : null;🧰 Tools
🪛 Biome (1.9.4)
[error] 26-65: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
packages/suite/src/components/wallet/Fees/CustomFee/CustomFee.tsx (1)
9-10: Consider switching from deprecated web3-utils to ethers.js.The
web3-utilslibrary is considered older tech, and many projects have migrated to using ethers.js for better type safety and more maintained codebase.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
packages/product-components/src/components/FeeRate/FeeRate.tsx(1 hunks)packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewSummary.tsx(3 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/BasicTxDetails.tsx(2 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/CancelTransaction/CancelTransaction.tsx(1 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/ChangeFee/ChangeFee.tsx(1 hunks)packages/suite/src/components/wallet/Fees/CustomFee.tsx(0 hunks)packages/suite/src/components/wallet/Fees/CustomFee/CurrentFee.tsx(1 hunks)packages/suite/src/components/wallet/Fees/CustomFee/CustomFee.tsx(1 hunks)packages/suite/src/components/wallet/Fees/CustomFee/CustomFeeEthereum.tsx(1 hunks)packages/suite/src/components/wallet/Fees/CustomFee/CustomFeeMisc.tsx(1 hunks)packages/suite/src/components/wallet/Fees/CustomFee/CustomFeeWrapper.tsx(1 hunks)packages/suite/src/components/wallet/Fees/CustomFee/index.tsx(1 hunks)packages/suite/src/components/wallet/Fees/FeeDetails.tsx(0 hunks)packages/suite/src/components/wallet/Fees/Fees.tsx(8 hunks)packages/suite/src/components/wallet/Fees/StandardFee/BitcoinFeeCards.tsx(1 hunks)packages/suite/src/components/wallet/Fees/StandardFee/EthereumFeeCards.tsx(1 hunks)packages/suite/src/components/wallet/Fees/StandardFee/FeeCard.tsx(1 hunks)packages/suite/src/components/wallet/Fees/StandardFee/MiscFeeCards.tsx(1 hunks)packages/suite/src/components/wallet/Fees/StandardFee/StandardFee.tsx(1 hunks)packages/suite/src/components/wallet/Fees/StandardFee/index.tsx(1 hunks)packages/suite/src/support/messages.ts(4 hunks)suite-common/wallet-config/src/utils.ts(1 hunks)
💤 Files with no reviewable changes (2)
- packages/suite/src/components/wallet/Fees/CustomFee.tsx
- packages/suite/src/components/wallet/Fees/FeeDetails.tsx
🚧 Files skipped from review as they are similar to previous changes (15)
- packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/BasicTxDetails.tsx
- packages/suite/src/components/wallet/Fees/StandardFee/index.tsx
- packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/CancelTransaction/CancelTransaction.tsx
- packages/suite/src/components/wallet/Fees/CustomFee/index.tsx
- suite-common/wallet-config/src/utils.ts
- packages/suite/src/components/wallet/Fees/CustomFee/CustomFeeWrapper.tsx
- packages/suite/src/components/wallet/Fees/CustomFee/CurrentFee.tsx
- packages/suite/src/components/wallet/Fees/StandardFee/FeeCard.tsx
- packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/ChangeFee/ChangeFee.tsx
- packages/suite/src/components/wallet/Fees/StandardFee/StandardFee.tsx
- packages/suite/src/components/wallet/Fees/CustomFee/CustomFeeMisc.tsx
- packages/suite/src/components/wallet/Fees/StandardFee/MiscFeeCards.tsx
- packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewSummary.tsx
- packages/suite/src/components/wallet/Fees/Fees.tsx
- packages/suite/src/support/messages.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/suite/src/components/wallet/Fees/StandardFee/EthereumFeeCards.tsx
[error] 35-67: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
packages/suite/src/components/wallet/Fees/StandardFee/BitcoinFeeCards.tsx
[error] 26-65: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: prepare_android_test_app
- GitHub Check: run-desktop-tests (@group=passphrase, trezor-user-env-unix)
- GitHub Check: EAS Update
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: build-web
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: build-web
🔇 Additional comments (11)
packages/suite/src/components/wallet/Fees/CustomFee/CustomFeeEthereum.tsx (5)
42-45: Commented code indicates future feature for priority fees.The commented error variables and later commented UI elements suggest that priority fee functionality is prepared but not yet fully implemented. This aligns with the PR objective to prepare for EIP-1559 priority fees without enabling the feature yet.
114-125: Commented-out validation props for future feature.This code block contains commented-out validation properties for the max base fee, which will be needed when priority fee functionality is fully implemented. This is consistent with the PR objective to prepare the code structure for EIP-1559 without enabling it yet.
164-170: Commented-out error handling for future feature.This section contains commented-out error handling for the max base fee input, which will be needed when priority fee functionality is fully implemented. This aligns with the PR objective.
187-188: Commented-out error handling for priority fee input.Similar to other commented sections, this is preparing for the full implementation of priority fees while keeping the feature disabled for now, which aligns with the PR objective.
212-217: Clean conditional rendering based on EIP-1559 support.The component provides a clean structure for rendering different input fields based on whether EIP-1559 is supported, allowing for smooth transition when the feature is fully implemented.
packages/product-components/src/components/FeeRate/FeeRate.tsx (4)
8-8: Newsymbolproperty added to FeeRateProps.The addition of the
symbolproperty toFeeRatePropsis a necessary change to support the refactoring of fee components for EIP-1559 priority fees.
16-17: Improved fee decimal precision based on network settlement layer.The new implementation determines the appropriate decimal precision based on whether the network has a settlement layer, which provides more accurate fee displays across different networks.
19-32: Fixed operator precedence in fee calculation.The Ethereum fee calculation now correctly applies parentheses around
(Number(feeRate) || 0)before multiplying, which fixes the previous issue with operator precedence. The implementation properly handles different network types with appropriate decimal formatting.
36-36: Simplified fee display in the component.The rendering has been simplified to directly use the calculated fee variable instead of conditionally formatting it based on the network type, which makes the code more maintainable.
packages/suite/src/components/wallet/Fees/CustomFee/CustomFee.tsx (2)
24-26: Good use of constants and component structure.The code properly uses constants for form field names and has a clean component structure with appropriate separation of concerns between
CurrentFee,CustomFeeEthereum, andCustomFeeMisc. The architecture facilitates the future addition of EIP-1559 priority fees.Also applies to: 99-105
106-134: Well-structured conditional rendering.The conditional rendering between Ethereum and other network types is well implemented, with proper prop passing to child components. The component architecture is clean and maintainable.
| } | ||
| topRightChild={ | ||
| <> | ||
| ~ |
There was a problem hiding this comment.
Remove stray tilde character.
There's an unexpected tilde character that should be removed.
- <>
- ~
+ <>Committable suggestion skipped: line range outside the PR's diff.
| const getCurrentFee = () => { | ||
| if (isEip1559) { | ||
| return `${currentBaseFee}`; | ||
| } | ||
|
|
||
| const { levels } = feeInfo; | ||
| const middleIndex = Math.floor((levels.length - 1) / 2); | ||
|
|
||
| return levels[middleIndex].feePerUnit; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add fallback for empty fee levels.
The getCurrentFee function doesn't handle the case where feeInfo.levels might be empty. This could cause index out of bounds errors.
const getCurrentFee = () => {
if (isEip1559) {
return `${currentBaseFee}`;
}
const { levels } = feeInfo;
+ if (!levels.length) {
+ return '0';
+ }
const middleIndex = Math.floor((levels.length - 1) / 2);
return levels[middleIndex].feePerUnit;
};📝 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 getCurrentFee = () => { | |
| if (isEip1559) { | |
| return `${currentBaseFee}`; | |
| } | |
| const { levels } = feeInfo; | |
| const middleIndex = Math.floor((levels.length - 1) / 2); | |
| return levels[middleIndex].feePerUnit; | |
| }; | |
| const getCurrentFee = () => { | |
| if (isEip1559) { | |
| return `${currentBaseFee}`; | |
| } | |
| const { levels } = feeInfo; | |
| if (!levels.length) { | |
| return '0'; | |
| } | |
| const middleIndex = Math.floor((levels.length - 1) / 2); | |
| return levels[middleIndex].feePerUnit; | |
| }; |
| const normalLevel = feeInfo.levels.filter(level => level.label === 'normal')[0]; | ||
|
|
||
| const isEip1559 = normalLevel.maxPriorityFeePerGas !== undefined; | ||
|
|
||
| const currentBaseFee = fromWei(Number(normalLevel.baseFeePerGas), 'Gwei'); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for missing normal fee level.
The code assumes that a 'normal' fee level always exists in feeInfo.levels. If this level is missing, normalLevel would be undefined, leading to runtime errors when accessing normalLevel.maxPriorityFeePerGas and normalLevel.baseFeePerGas.
- const normalLevel = feeInfo.levels.filter(level => level.label === 'normal')[0];
+ const normalLevel = feeInfo.levels.find(level => level.label === 'normal');
+
+ if (!normalLevel) {
+ console.error('Normal fee level not found');
+ // Provide fallback or handle the error case
+ }
- const isEip1559 = normalLevel.maxPriorityFeePerGas !== undefined;
+ const isEip1559 = normalLevel?.maxPriorityFeePerGas !== undefined;
- const currentBaseFee = fromWei(Number(normalLevel.baseFeePerGas), 'Gwei');
+ const currentBaseFee = normalLevel?.baseFeePerGas
+ ? fromWei(Number(normalLevel.baseFeePerGas), 'Gwei')
+ : '0';📝 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 normalLevel = feeInfo.levels.filter(level => level.label === 'normal')[0]; | |
| const isEip1559 = normalLevel.maxPriorityFeePerGas !== undefined; | |
| const currentBaseFee = fromWei(Number(normalLevel.baseFeePerGas), 'Gwei'); | |
| const normalLevel = feeInfo.levels.find(level => level.label === 'normal'); | |
| if (!normalLevel) { | |
| console.error('Normal fee level not found'); | |
| // Provide fallback or handle the error case | |
| } | |
| const isEip1559 = normalLevel?.maxPriorityFeePerGas !== undefined; | |
| const currentBaseFee = normalLevel?.baseFeePerGas | |
| ? fromWei(Number(normalLevel.baseFeePerGas), 'Gwei') | |
| : '0'; |
| isEip1559?: boolean; | ||
| }; | ||
|
|
||
| // For priority fees it should show current base fee |
| <Text variant="default" typographyStyle="hint"> | ||
| <Row alignItems="center" gap={spacings.xxs}> | ||
| <Text> | ||
| {currentFee} {getFeeUnits(networkType)} |
packages/suite/src/components/wallet/Fees/CustomFee/CustomFeeMisc.tsx
Outdated
Show resolved
Hide resolved
| const baseFee = fees[symbol].levels[0].baseFeePerGas; | ||
| const hasEip1559Feature = getNetworkFeatures(symbol).includes('eip1559'); | ||
| const shouldUsePriorityFees = !!tx.fee && hasEip1559Feature && !!baseFee; | ||
| console.log('shouldUsePriorityFees', shouldUsePriorityFees, tx, hasEip1559Feature, baseFee); |
There was a problem hiding this comment.
I swear I thought I deleted it, anyway now deleted for sure
| const numOfDecimalPlaces = hasNetworkSettlementLayer(symbol) ? 4 : 2; | ||
| const multiplier = Math.pow(10, numOfDecimalPlaces); |
There was a problem hiding this comment.
this can be defined in the ethereum case
tomasklim
added
the
blocked
|
Adding |
Ok, there's a fix for a little bug that's on develop now, I'll create a separate small PR for it tomorrow so it can be included into the freeze. |
I think this makes sense, I'll remove it. Thought I'll leave it there but turn it off, but it's better to separate it indeed and test separately |
enjojoy
force-pushed
the
feat/refactor-fees-component
branch
from
ddbc5e4 to
89a12cd
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
packages/suite/src/components/wallet/Fees/CustomFee/CustomFee.tsx (3)
70-72:⚠️ Potential issueFix the integer-only check to allow decimals for Ethereum.
The comment at line 68 mentions allowing decimals in ETH, but the code checks for integer-only when
networkTypeis'ethereum'. This is contradictory and may block valid GWEI inputs.- if (['bitcoin', 'ethereum'].includes(networkType) && !isInteger(value)) { - return translationString('CUSTOM_FEE_IS_NOT_INTEGER'); - } + // Only require an integer for Bitcoin; allow decimals for Ethereum. + if (networkType === 'bitcoin' && !isInteger(value)) { + return translationString('CUSTOM_FEE_IS_NOT_INTEGER'); + }
78-83:⚠️ Potential issueAdd error handling for missing normal fee level.
The code assumes that a 'normal' fee level always exists in
feeInfo.levels. If this level is missing,normalLevelwould be undefined, leading to runtime errors when accessingnormalLevel.maxPriorityFeePerGasandnormalLevel.baseFeePerGas.- const normalLevel = feeInfo.levels.filter(level => level.label === 'normal')[0]; + const normalLevel = feeInfo.levels.find(level => level.label === 'normal'); + + if (!normalLevel) { + console.error('Normal fee level not found'); + // Provide fallback or handle the error case + } - const isEip1559 = normalLevel.maxPriorityFeePerGas !== undefined; + const isEip1559 = normalLevel?.maxPriorityFeePerGas !== undefined; - const currentBaseFee = fromWei(Number(normalLevel.baseFeePerGas), 'Gwei'); + const currentBaseFee = normalLevel?.baseFeePerGas + ? fromWei(Number(normalLevel.baseFeePerGas), 'Gwei') + : '0';
84-93:⚠️ Potential issueAdd fallback for empty fee levels.
The
getCurrentFeefunction doesn't handle the case wherefeeInfo.levelsmight be empty. This could cause index out of bounds errors.const getCurrentFee = () => { if (isEip1559) { return `${currentBaseFee}`; } const { levels } = feeInfo; + if (!levels.length) { + return '0'; + } const middleIndex = Math.floor((levels.length - 1) / 2); return levels[middleIndex].feePerUnit; };
🧹 Nitpick comments (7)
suite-common/wallet-config/src/utils.ts (1)
79-88: Nitpick: Fix spelling in doc comment and ensure valid network symbol usage.• The doc comment in line 80 has a small typo: "Check wether" → "Check whether."
• Additionally, ensure that all calls togetNetwork(symbol)are guaranteed to receive a validNetworkSymbolto avoid potential runtime errors.Would you like me to search for any usage of
hasNetworkSettlementLayerin the codebase to confirm that invalid symbols are never passed?packages/suite/src/components/wallet/Fees/StandardFee/EthereumFeeCards.tsx (1)
34-68: Remove unnecessary fragment.The fragment wrapping the
.map(...)block is flagged by static analysis as unnecessary. You can safely remove it and return the mapped array directly to improve readability and satisfy the linter.Apply this diff to remove the redundant fragment:
- showFee && ( - <> - {feeOptions?.map((fee, index) => ( - <FeeCard - key={index} - ... - /> - ))} - </> - ) + showFee && feeOptions?.map((fee, index) => ( + <FeeCard + key={index} + ... + /> + ))🧰 Tools
🪛 Biome (1.9.4)
[error] 35-67: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
packages/suite/src/components/wallet/Fees/CustomFee/CustomFeeEthereum.tsx (2)
42-46: Clarify or remove commented-out error definitions.These lines are placeholders for EIP-1559 error handling but remain commented out. Leaving them in might cause confusion. Consider removing or explicitly documenting why they are commented out if this code will be enabled later.
147-192: Confirm the partial EIP-1559 logic usage.This section includes logic and validation for EIP-1559 fees but appears partially disabled. Verify whether this partial implementation is sufficient or if it should be fully removed until the feature is active to avoid confusion.
Do you want me to provide a script to look for any references or calls that might incorrectly enable this code before it's fully functional?
packages/suite/src/components/wallet/Fees/StandardFee/BitcoinFeeCards.tsx (1)
26-65: Simplify unnecessary Fragment.The Fragment wrapping the mapped fee options is redundant and can be removed.
- showFee && ( - <> - {feeOptions?.map((fee, index) => ( + showFee && + feeOptions?.map((fee, index) => ( <FeeCard key={index} value={fee.value} isSelected={selectedLevel.label === fee.value} changeFeeLevel={changeFeeLevel} topLeftChild={ <span data-testid={`@fee-card/${fee.value}`}>{fee.label}</span> } topRightChild={ <> ~ {formatDurationStrict( feeInfo.blockTime * (fee?.blocks ?? 0) * 60, locale, )} </> } bottomLeftChild={ <FiatValue disableHiddenPlaceholder amount={fee?.networkAmount ?? ''} symbol={symbol} showApproximationIndicator /> } bottomRightChild={ <> <FeeRate feeRate={fee.feePerUnit} networkType={networkType} symbol={symbol} /> {hasInfo ? ` (${transactionInfo.bytes} B)` : ''} </> } /> - ))} - </> - ) + ))🧰 Tools
🪛 Biome (1.9.4)
[error] 26-65: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
packages/product-components/src/components/FeeRate/FeeRate.tsx (1)
16-32: The fee calculation logic is now properly structured.The code now correctly handles different network types and considers whether the network symbol has a settlement layer. The operator precedence issue has been fixed with proper parentheses in the Ethereum case.
However, there's still room for defensive programming:
- const numOfDecimalPlaces = hasNetworkSettlementLayer(symbol) ? 4 : 2; + // Set decimal places based on network type and settlement layer + const numOfDecimalPlaces = hasNetworkSettlementLayer(symbol) ? 4 : 2; const multiplier = Math.pow(10, numOfDecimalPlaces); switch (networkType) { case 'ethereum': fee = (Math.ceil((Number(feeRate) || 0) * multiplier) / multiplier).toFixed( numOfDecimalPlaces, ); break; case 'bitcoin': fee = typeof feeRate === 'string' ? new BigNumber(feeRate).toFixed(2) : feeRate.toFixed(2); break; default: - fee = typeof feeRate === 'string' ? feeRate : feeRate.toString(); + // Ensure we handle null or undefined feeRate gracefully + fee = typeof feeRate === 'string' ? feeRate : (feeRate?.toString() || '0'); }packages/suite/src/components/wallet/Fees/StandardFee/StandardFee.tsx (1)
85-92: Consider optimizing the Text wrapper.The Text component is used as a wrapper with
as="div", but its purpose might be confusing. Consider if a simpler structure would be more appropriate.- return ( - <Text data-testid="@wallet/fee-details" as="div"> - <Row gap={spacings.md} justifyContent="space-evenly"> - <FeeCardsWrapper $columns={columns} ref={ref}> - <FeeCardsComponent {...props} /> - </FeeCardsWrapper> - </Row> - </Text> - ); + return ( + <div data-testid="@wallet/fee-details"> + <Row gap={spacings.md} justifyContent="space-evenly"> + <FeeCardsWrapper $columns={columns} ref={ref}> + <FeeCardsComponent {...props} /> + </FeeCardsWrapper> + </Row> + </div> + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
packages/product-components/src/components/FeeRate/FeeRate.tsx(1 hunks)packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewSummary.tsx(3 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/BasicTxDetails.tsx(2 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/CancelTransaction/CancelTransaction.tsx(1 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/ChangeFee/ChangeFee.tsx(1 hunks)packages/suite/src/components/wallet/Fees/CustomFee.tsx(0 hunks)packages/suite/src/components/wallet/Fees/CustomFee/CurrentFee.tsx(1 hunks)packages/suite/src/components/wallet/Fees/CustomFee/CustomFee.tsx(1 hunks)packages/suite/src/components/wallet/Fees/CustomFee/CustomFeeEthereum.tsx(1 hunks)packages/suite/src/components/wallet/Fees/CustomFee/CustomFeeMisc.tsx(1 hunks)packages/suite/src/components/wallet/Fees/CustomFee/CustomFeeWrapper.tsx(1 hunks)packages/suite/src/components/wallet/Fees/CustomFee/index.tsx(1 hunks)packages/suite/src/components/wallet/Fees/FeeDetails.tsx(0 hunks)packages/suite/src/components/wallet/Fees/Fees.tsx(8 hunks)packages/suite/src/components/wallet/Fees/StandardFee/BitcoinFeeCards.tsx(1 hunks)packages/suite/src/components/wallet/Fees/StandardFee/EthereumFeeCards.tsx(1 hunks)packages/suite/src/components/wallet/Fees/StandardFee/FeeCard.tsx(1 hunks)packages/suite/src/components/wallet/Fees/StandardFee/MiscFeeCards.tsx(1 hunks)packages/suite/src/components/wallet/Fees/StandardFee/StandardFee.tsx(1 hunks)packages/suite/src/components/wallet/Fees/StandardFee/index.tsx(1 hunks)packages/suite/src/support/messages.ts(4 hunks)suite-common/wallet-config/src/utils.ts(1 hunks)
💤 Files with no reviewable changes (2)
- packages/suite/src/components/wallet/Fees/CustomFee.tsx
- packages/suite/src/components/wallet/Fees/FeeDetails.tsx
🚧 Files skipped from review as they are similar to previous changes (11)
- packages/suite/src/components/wallet/Fees/StandardFee/index.tsx
- packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/BasicTxDetails.tsx
- packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/CancelTransaction/CancelTransaction.tsx
- packages/suite/src/components/wallet/Fees/CustomFee/index.tsx
- packages/suite/src/components/wallet/Fees/CustomFee/CustomFeeMisc.tsx
- packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/ChangeFee/ChangeFee.tsx
- packages/suite/src/components/wallet/Fees/StandardFee/MiscFeeCards.tsx
- packages/suite/src/components/wallet/Fees/CustomFee/CustomFeeWrapper.tsx
- packages/suite/src/components/wallet/Fees/CustomFee/CurrentFee.tsx
- packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewSummary.tsx
- packages/suite/src/components/wallet/Fees/Fees.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
packages/suite/src/components/wallet/Fees/StandardFee/BitcoinFeeCards.tsx
[error] 26-65: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
packages/suite/src/components/wallet/Fees/StandardFee/EthereumFeeCards.tsx
[error] 35-67: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: run-desktop-tests (@group=passphrase, trezor-user-env-unix)
- GitHub Check: EAS Update
- GitHub Check: build-web
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: prepare_android_test_app
🔇 Additional comments (10)
packages/suite/src/components/wallet/Fees/StandardFee/FeeCard.tsx (1)
1-46: Looks good!Implementation is clear and straightforward. No issues or concerns found.
packages/suite/src/components/wallet/Fees/StandardFee/BitcoinFeeCards.tsx (1)
38-38: Remove stray tilde character.There's an unexpected tilde character that should be removed.
- <> - ~ + <>packages/suite/src/support/messages.ts (8)
3421-3424: New translation key added for network base fee.This new message will be used to display the current network base fee information, which is part of the EIP-1559 fee model for Ethereum transactions.
3425-3428: New translation key added for max base fee per gas.Adding support for the EIP-1559 max base fee per gas parameter, which allows users to set the maximum base fee they're willing to pay.
3429-3432: New translation key for custom max base fee option.This adds a user-friendly message for choosing to use the network's current base fee rather than a custom value.
3433-3436: New translation key for gas limit validation.This message will be shown when a gas limit value is required but not set, improving form validation feedback.
5578-5581: New translation key for fee explanation link.This adds a "Why fees?" link text that will help users understand transaction fees better.
5607-5610: New translation key for medium fee level.Adding a "Medium" option to the existing fee levels (Low, Normal, High), providing more granular fee selection.
5611-5618: New translation keys for EIP-1559 priority fee components.These two messages support the EIP-1559 fee model by adding labels for "Priority fee" and "Max fee" fields, which are the two key components of the new fee structure.
5632-5635: New translation key for custom base fee validation.This message will provide feedback when a user attempts to set a custom base fee that's below the current network base fee, which would likely result in a transaction not being processed.
| useEffect(() => { | ||
| if (!ref.current) return; | ||
|
|
||
| resizeObserver(feeOptions, setColumns).observe(ref.current); | ||
|
|
||
| return () => resizeObserver(feeOptions, setColumns).disconnect(); | ||
| }, [feeOptions, setColumns]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve cleanup for ResizeObserver.
The current cleanup function creates a new ResizeObserver during cleanup, which is inefficient. It would be better to store the observer instance and disconnect it directly.
useEffect(() => {
if (!ref.current) return;
- resizeObserver(feeOptions, setColumns).observe(ref.current);
+ const observer = resizeObserver(feeOptions, setColumns);
+ observer.observe(ref.current);
- return () => resizeObserver(feeOptions, setColumns).disconnect();
+ return () => observer.disconnect();
}, [feeOptions, setColumns]);📝 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.
| useEffect(() => { | |
| if (!ref.current) return; | |
| resizeObserver(feeOptions, setColumns).observe(ref.current); | |
| return () => resizeObserver(feeOptions, setColumns).disconnect(); | |
| }, [feeOptions, setColumns]); | |
| useEffect(() => { | |
| if (!ref.current) return; | |
| const observer = resizeObserver(feeOptions, setColumns); | |
| observer.observe(ref.current); | |
| return () => observer.disconnect(); | |
| }, [feeOptions, setColumns]); |
enjojoy
force-pushed
the
feat/refactor-fees-component
branch
from
89a12cd to
9991cd3
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
packages/suite/src/components/wallet/Fees/StandardFee/BitcoinFeeCards.tsx (1)
38-38:⚠️ Potential issueRemove stray tilde character.
There's an unexpected tilde character that should be removed.
- <> - ~ + <>packages/suite/src/components/wallet/Fees/CustomFee/CustomFee.tsx (2)
67-70:⚠️ Potential issueFix the integer-only check to allow decimals for Ethereum.
The comment at line 67 mentions allowing decimals in ETH, but the code checks for integer-only when
networkTypeis'ethereum'. This is contradictory and may block valid GWEI inputs.// Allow decimals in ETH since GWEI is not a satoshi. validate: (value: string) => { - if (['bitcoin', 'ethereum'].includes(networkType) && !isInteger(value)) { + if (networkType === 'bitcoin' && !isInteger(value)) { return translationString('CUSTOM_FEE_IS_NOT_INTEGER'); } },
76-81:⚠️ Potential issueAdd fallback for empty fee levels.
The
getCurrentFeefunction doesn't handle the case wherefeeInfo.levelsmight be empty. This could cause index out of bounds errors.const getCurrentFee = () => { const { levels } = feeInfo; + if (!levels.length) { + return '0'; + } const middleIndex = Math.floor((levels.length - 1) / 2); return levels[middleIndex].feePerUnit; };
🧹 Nitpick comments (8)
packages/suite/src/components/wallet/Fees/StandardFee/EthereumFeeCards.tsx (1)
16-44: Remove unnecessary Fragment.The Fragment wrapper is unnecessary since it contains only one child element (the mapping operation). This was also flagged by the static analysis tool.
- showFee && ( - <> - {feeOptions?.map((fee, index) => ( + showFee && feeOptions?.map((fee, index) => ( <FeeCard key={index} value={fee.value} isSelected={selectedLevel.label === fee.value} changeFeeLevel={changeFeeLevel} topLeftChild={<span data-testid={`@fee-card/${fee.value}`}>{fee.label}</span>} topRightChild="" bottomLeftChild={ <FiatValue disableHiddenPlaceholder amount={fee.networkAmount || ''} symbol={symbol} showApproximationIndicator /> } bottomRightChild={ <FeeRate feeRate={fee?.feePerUnit} networkType={networkType} symbol={symbol} /> } /> - ))} - </> - ); + ));🧰 Tools
🪛 Biome (1.9.4)
[error] 17-43: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
packages/suite/src/components/wallet/Fees/CustomFee/CustomFeeMisc.tsx (2)
30-32: Consider using TypeScript generics more effectively.The type assertion could be improved for better type safety. Instead of using
unknownas an intermediary, consider updating the component's generic constraints.- // Type assertion allowing to make the component reusable, see https://stackoverflow.com/a/73624072. - const { getValues } = props as unknown as UseFormReturn<FormState>; - const errors = props.errors as unknown as FieldErrors<FormState>; + // Use generics to ensure type safety + const { getValues } = props as UseFormReturn<TFieldValues>; + const errors = props.errors as FieldErrors<TFieldValues>;
82-96: Consider extracting the NumberInput configuration into a separate variable.The NumberInput component has many props, making it harder to read. Extracting the configuration into a separate variable would improve readability.
+ const numberInputProps = { + locale, + control, + inputState: getInputState(feePerUnitError), + innerAddon: ( + <Text variant="tertiary" typographyStyle="label"> + {feeUnits} + </Text> + ), + name: FEE_PER_UNIT, + 'data-testid': FEE_PER_UNIT, + rules: feeRules, + bottomText: feePerUnitError?.message || null, + }; return ( <> <input type="hidden" {...register(FEE_LIMIT as FieldPath<TFieldValues>)} /> - <NumberInput - locale={locale} - control={control} - inputState={getInputState(feePerUnitError)} - innerAddon={ - <Text variant="tertiary" typographyStyle="label"> - {feeUnits} - </Text> - } - name={FEE_PER_UNIT} - data-testid={FEE_PER_UNIT} - rules={feeRules} - bottomText={feePerUnitError?.message || null} - /> + <NumberInput {...numberInputProps} /> {feeDifferenceWarning && <Note>{feeDifferenceWarning}</Note>} </> );packages/suite/src/components/wallet/Fees/StandardFee/BitcoinFeeCards.tsx (1)
24-66: Simplify return statement and remove unnecessary Fragment.The component can be simplified by removing the unnecessary Fragment and handling the null case at the beginning.
- return ( - showFee && ( - <> - {feeOptions?.map((fee, index) => ( + if (!showFee || !feeOptions?.length) { + return null; + } + + return feeOptions.map((fee, index) => ( <FeeCard key={index} value={fee.value} isSelected={selectedLevel.label === fee.value} changeFeeLevel={changeFeeLevel} topLeftChild={ <span data-testid={`@fee-card/${fee.value}`}>{fee.label}</span> } topRightChild={ <> {formatDurationStrict( feeInfo.blockTime * (fee?.blocks ?? 0) * 60, locale, )} </> } bottomLeftChild={ <FiatValue disableHiddenPlaceholder amount={fee?.networkAmount ?? ''} symbol={symbol} showApproximationIndicator /> } bottomRightChild={ <> <FeeRate feeRate={fee.feePerUnit} networkType={networkType} symbol={symbol} /> {hasInfo ? ` (${transactionInfo.bytes} B)` : ''} </> } /> - ))} - </> - ) - ); + ));🧰 Tools
🪛 Biome (1.9.4)
[error] 26-65: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
packages/suite/src/components/wallet/Fees/StandardFee/StandardFee.tsx (2)
78-83: Consider adding logging for unknown network types.For network types not explicitly handled, the component silently falls back to MiscFeeCards. Adding debug logging when an unknown network type is encountered would help with troubleshooting.
const feeCardsComponentMap: Partial<Record<NetworkType, React.FC<StandardFeeProps>>> = { bitcoin: BitcoinFeeCards, ethereum: EthereumFeeCards, }; - const FeeCardsComponent = feeCardsComponentMap[networkType] ?? MiscFeeCards; + const FeeCardsComponent = feeCardsComponentMap[networkType]; + if (!FeeCardsComponent) { + console.debug(`No specific fee card component for network type: ${networkType}, using MiscFeeCards`); + } + const FinalFeeCardsComponent = FeeCardsComponent ?? MiscFeeCards;
85-93: Consider adding accessibility attributes.The fee details component might benefit from additional accessibility attributes to improve usability for screen readers.
return ( - <Text data-testid="@wallet/fee-details" as="div"> + <Text data-testid="@wallet/fee-details" as="div" aria-label="Fee options"> <Row gap={spacings.md} justifyContent="space-evenly"> <FeeCardsWrapper $columns={columns} ref={ref}> <FeeCardsComponent {...props} /> </FeeCardsWrapper> </Row> </Text> );packages/suite/src/components/wallet/Fees/CustomFee/CustomFee.tsx (2)
86-93: Add error handling for cases when fee info is incomplete.The component assumes that the fee information is always correctly structured and contains the necessary data. Consider adding defensive coding to handle cases when fee information might be incomplete.
return ( <CustomFeeWrapper> <CurrentFee networkType={networkType} feeIconName={feeIconName} - currentFee={getCurrentFee()} + currentFee={feeInfo?.levels?.length ? getCurrentFee() : '0'} />
93-119: Extract component selection logic into a separate function.The conditional rendering of either
CustomFeeEthereumorCustomFeeMisccould be extracted into a separate function to improve readability and maintainability, especially if more network types need to be supported in the future.+ const getFeeComponent = () => { + const commonProps = { + ...props, + networkType, + feeInfo, + register, + control, + composedFeePerByte, + feeUnits, + translationString, + locale, + sharedRules, + }; + + if (networkType === 'ethereum') { + return <CustomFeeEthereum {...commonProps} />; + } + + return <CustomFeeMisc {...commonProps} />; + }; return ( <CustomFeeWrapper> <CurrentFee networkType={networkType} feeIconName={feeIconName} currentFee={getCurrentFee()} /> - {networkType === 'ethereum' ? ( - <CustomFeeEthereum - {...props} - networkType={networkType} - feeInfo={feeInfo} - register={register} - control={control} - composedFeePerByte={composedFeePerByte} - feeUnits={feeUnits} - translationString={translationString} - locale={locale} - sharedRules={sharedRules} - /> - ) : ( - <CustomFeeMisc - {...props} - networkType={networkType} - feeInfo={feeInfo} - register={register} - control={control} - composedFeePerByte={composedFeePerByte} - feeUnits={feeUnits} - translationString={translationString} - locale={locale} - sharedRules={sharedRules} - /> - )} + {getFeeComponent()} </CustomFeeWrapper> );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
packages/product-components/src/components/FeeRate/FeeRate.tsx(1 hunks)packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewSummary.tsx(2 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/BasicTxDetails.tsx(2 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/CancelTransaction/CancelTransaction.tsx(1 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/ChangeFee/ChangeFee.tsx(1 hunks)packages/suite/src/components/wallet/Fees/CustomFee.tsx(0 hunks)packages/suite/src/components/wallet/Fees/CustomFee/CurrentFee.tsx(1 hunks)packages/suite/src/components/wallet/Fees/CustomFee/CustomFee.tsx(1 hunks)packages/suite/src/components/wallet/Fees/CustomFee/CustomFeeEthereum.tsx(1 hunks)packages/suite/src/components/wallet/Fees/CustomFee/CustomFeeMisc.tsx(1 hunks)packages/suite/src/components/wallet/Fees/CustomFee/CustomFeeWrapper.tsx(1 hunks)packages/suite/src/components/wallet/Fees/CustomFee/index.tsx(1 hunks)packages/suite/src/components/wallet/Fees/FeeDetails.tsx(0 hunks)packages/suite/src/components/wallet/Fees/Fees.tsx(6 hunks)packages/suite/src/components/wallet/Fees/StandardFee/BitcoinFeeCards.tsx(1 hunks)packages/suite/src/components/wallet/Fees/StandardFee/EthereumFeeCards.tsx(1 hunks)packages/suite/src/components/wallet/Fees/StandardFee/FeeCard.tsx(1 hunks)packages/suite/src/components/wallet/Fees/StandardFee/MiscFeeCards.tsx(1 hunks)packages/suite/src/components/wallet/Fees/StandardFee/StandardFee.tsx(1 hunks)packages/suite/src/components/wallet/Fees/StandardFee/index.tsx(1 hunks)packages/suite/src/support/messages.ts(1 hunks)suite-common/wallet-config/src/utils.ts(1 hunks)
💤 Files with no reviewable changes (2)
- packages/suite/src/components/wallet/Fees/CustomFee.tsx
- packages/suite/src/components/wallet/Fees/FeeDetails.tsx
🚧 Files skipped from review as they are similar to previous changes (14)
- packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/CancelTransaction/CancelTransaction.tsx
- packages/suite/src/components/wallet/Fees/StandardFee/index.tsx
- packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/BasicTxDetails.tsx
- packages/suite/src/components/wallet/Fees/CustomFee/index.tsx
- packages/suite/src/components/wallet/Fees/CustomFee/CurrentFee.tsx
- packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/TxDetailModal/ChangeFee/ChangeFee.tsx
- packages/suite/src/components/wallet/Fees/CustomFee/CustomFeeWrapper.tsx
- packages/suite/src/components/wallet/Fees/StandardFee/FeeCard.tsx
- packages/suite/src/components/wallet/Fees/CustomFee/CustomFeeEthereum.tsx
- packages/suite/src/components/wallet/Fees/Fees.tsx
- suite-common/wallet-config/src/utils.ts
- packages/suite/src/components/wallet/Fees/StandardFee/MiscFeeCards.tsx
- packages/suite/src/support/messages.ts
- packages/suite/src/components/suite/modals/ReduxModal/TransactionReviewModal/TransactionReviewSummary.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
packages/suite/src/components/wallet/Fees/StandardFee/EthereumFeeCards.tsx
[error] 17-43: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
packages/suite/src/components/wallet/Fees/StandardFee/BitcoinFeeCards.tsx
[error] 26-65: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: prepare_android_test_app
- GitHub Check: run-desktop-tests (@group=passphrase, trezor-user-env-unix)
- GitHub Check: EAS Update
- GitHub Check: build-web
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: Setup and Cache Dependencies
🔇 Additional comments (5)
packages/suite/src/components/wallet/Fees/StandardFee/EthereumFeeCards.tsx (1)
8-15: Add null check for feeOptions at the beginning of component.Adding an early return will make the code more readable and avoid potential issues if feeOptions is null or empty.
export const EthereumFeeCards = ({ showFee, feeOptions, selectedLevel, changeFeeLevel, symbol, networkType, -}: StandardFeeProps) => +}: StandardFeeProps) => { + if (!showFee || !feeOptions?.length) { + return null; + } + + return feeOptions.map((fee, index) => (packages/product-components/src/components/FeeRate/FeeRate.tsx (2)
16-32: Define numOfDecimalPlaces within each case for better readability.The numOfDecimalPlaces calculation can be moved into each case for better clarity, especially since it's only relevant to specific network types.
- const numOfDecimalPlaces = hasNetworkSettlementLayer(symbol) ? 4 : 2; - const multiplier = Math.pow(10, numOfDecimalPlaces); switch (networkType) { case 'ethereum': + const ethNumOfDecimalPlaces = hasNetworkSettlementLayer(symbol) ? 4 : 2; + const ethMultiplier = Math.pow(10, ethNumOfDecimalPlaces); - fee = (Math.ceil((Number(feeRate) || 0) * multiplier) / multiplier).toFixed( - numOfDecimalPlaces, + fee = (Math.ceil((Number(feeRate) || 0) * ethMultiplier) / ethMultiplier).toFixed( + ethNumOfDecimalPlaces, ); break; case 'bitcoin': fee = typeof feeRate === 'string' ? new BigNumber(feeRate).toFixed(2) : feeRate.toFixed(2); break; default: fee = typeof feeRate === 'string' ? feeRate : feeRate.toString(); }
1-1:❓ Verification inconclusive
Verify all components properly pass the symbol prop to FeeRate.
Since this component now requires a
symbolprop, ensure that all implementations across the codebase are updated to provide this parameter.
🏁 Script executed:
#!/bin/bash # Description: Verify that all uses of FeeRate component include the required symbol prop # Search for FeeRate component usage rg '<FeeRate' --type=ts --type=tsx -A 5 | grep -v 'symbol='Length of output: 87
Below is a revised review comment. To ensure complete confidence, please re-run the search with a corrected command (or verify manually) because the previous script did not recognize TSX files. For example, you might try:
#!/bin/bash # Re-run FeeRate usage check including TSX file type. rg --type-add 'tsx:*.tsx' '<FeeRate' -A 5 | grep -v 'symbol='Ensure all FeeRate component usages provide the required
symbolprop.Since the component now mandates a
symbolprop, double-check that every instance across the codebase is updated to include it. In particular, verify that no FeeRate usage omits thesymbolparameter—even in TSX files—by either manually confirming the changes or re-running an updated search command as shown above.packages/suite/src/components/wallet/Fees/StandardFee/StandardFee.tsx (2)
70-76: Improve cleanup for ResizeObserver.The current cleanup function creates a new ResizeObserver during cleanup, which is inefficient. It would be better to store the observer instance and disconnect it directly.
useEffect(() => { if (!ref.current) return; - resizeObserver(feeOptions, setColumns).observe(ref.current); + const observer = resizeObserver(feeOptions, setColumns); + observer.observe(ref.current); - return () => resizeObserver(feeOptions, setColumns).disconnect(); + return () => observer.disconnect(); }, [feeOptions, setColumns]);
86-86: Review the Text wrapper implementation.Using Text with
as="div"may be confusing. Consider whether this is the intended approach or if a more semantic wrapper would be appropriate.
|
@coderabbitai pause |
enjojoy
force-pushed
the
feat/refactor-fees-component
branch
from
9991cd3 to
0837ba1
Compare
enjojoy
changed the title
| <FeeRate feeRate={currentFee} networkType={networkType} symbol={symbol} /> | ||
| {getFeeUnits(networkType)} |
There was a problem hiding this comment.
thanks, forgot it has unit in it already
| const selectedLevel = | ||
| feeInfo.levels.find(level => level.label === selectedOption) || | ||
| feeInfo.levels.find(level => level.label === 'normal')!; |
There was a problem hiding this comment.
here is something what was not there before
There was a problem hiding this comment.
Since I added FeeOption[] type to output of buildFeeLevels, TS was complaining that selectedLevel can be undefined, even though the functionality didn't change... This fixed it.
| <> | ||
| {feeOptions?.map((fee, index) => ( | ||
| <FeeCard |
There was a problem hiding this comment.
| <> | |
| {feeOptions?.map((fee, index) => ( | |
| <FeeCard | |
| {feeOptions.map((fee, index) => ( | |
| <FeeCard |
| networkType={networkType} | ||
| symbol={symbol} | ||
| /> | ||
| {hasInfo ? ` (${transactionInfo.bytes} B)` : ''} |
There was a problem hiding this comment.
is it something we need there? we do not show gas limit either
| return ''; | ||
| }; | ||
|
|
||
| return ( |
There was a problem hiding this comment.
? and React fragment is not needed
| if (!feeOptions?.length) return null; | ||
|
|
||
| const feeOption = feeOptions[0]; // in the future Solana should have it's own Details component | ||
| const feeAmount = networkType === 'solana' ? feeOption.feePerTx : feeOption.feePerUnit; | ||
|
|
||
| return ( | ||
| showFee && ( |
There was a problem hiding this comment.
| if (!feeOptions?.length) return null; | |
| const feeOption = feeOptions[0]; // in the future Solana should have it's own Details component | |
| const feeAmount = networkType === 'solana' ? feeOption.feePerTx : feeOption.feePerUnit; | |
| return ( | |
| showFee && ( | |
| if (!feeOptions?.length || showFee) return null; | |
| const feeOption = feeOptions[0]; // in the future Solana should have it's own Details component | |
| const feeAmount = networkType === 'solana' ? feeOption.feePerTx : feeOption.feePerUnit; | |
| return ( |
There was a problem hiding this comment.
!showFee, but yes agreed
| feeOptions, | ||
| symbol, | ||
| changeFeeLevel, | ||
| }: StandardFeeProps) => { |
There was a problem hiding this comment.
nit:
| }: StandardFeeProps) => { | |
| }: FeeProps) => { |
There was a problem hiding this comment.
I like it more verbose so it's more obvious that it's something on the same level with CustomFeeProps
| const FeeCardsComponent = feeCardsComponentMap[networkType] ?? MiscFeeCards; | ||
|
|
||
| return ( | ||
| <Text data-testid="@wallet/fee-details" as="div"> |
There was a problem hiding this comment.
Does it need to be Text? @enjojoy maybe you can remove this wrapper completely?
| @@ -0,0 +1,7 @@ | |||
| import { BitcoinFeeCards } from './BitcoinFeeCards'; | |||
There was a problem hiding this comment.
I would move this file one folder down
enjojoy
force-pushed
the
feat/refactor-fees-component
branch
from
0837ba1 to
3601053
Compare
enjojoy
force-pushed
the
feat/refactor-fees-component
branch
from
3601053 to
053bedb
Compare
Description
Refactor fees components