fix: base max fee + hw wallet shaded area #565
Conversation
WalkthroughThe changes made in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Transaction
participant FeeCalculator
User->>Transaction: Request to finalize transaction
Transaction->>Transaction: Process transaction
Transaction->>Transaction: Determine transaction type
Transaction->>Transaction: Return finalizedTransaction
User->>FeeCalculator: Request operational fees
FeeCalculator->>Transaction: Get OP fees with finalizedTransaction
Transaction->>FeeCalculator: Return calculated fees
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: 3
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
packages/extension/src/providers/ethereum/libs/transaction/index.ts(7 hunks)
🔇 Additional comments (3)
packages/extension/src/providers/ethereum/libs/transaction/index.ts (3)
175-178: Set defaultHardfork dynamically based on network conditions
Hardcoding defaultHardfork to Hardfork.London may not be suitable for all networks, especially those that have progressed beyond the London hardfork. This could lead to incompatibilities when broadcasting transactions on networks with different hardforks.
Consider retrieving the latest supported hardfork dynamically:
const common = Common.custom({
chainId: BigInt(this.tx.chainId),
- defaultHardfork: Hardfork.London,
+ defaultHardfork: await this.getNetworkHardfork(),
});And implement the getNetworkHardfork method to fetch the appropriate hardfork based on the network:
async getNetworkHardfork(): Promise<Hardfork> {
const networkId = await this.web3.net.getId();
// Map network IDs to their respective hardforks.
// Implement the logic based on the network specifications.
// For example:
// if (networkId === 1) return Hardfork.Merge;
// Add mappings for other networks as needed.
}126-134: Ensure consistent handling of BigInt conversion for chainId
Converting this.tx.chainId to BigInt is necessary for the Common configuration. Verify that this.tx.chainId is always a number or a value compatible with BigInt to prevent potential runtime errors.
Run the following script to check all occurrences of chainId assignments and ensure they are compatible with BigInt:
✅ Verification successful
Based on the search results, I can now provide a final response:
chainId conversion to BigInt is safe in this context
The codebase consistently handles chainId as string values in hexadecimal format (e.g., '0x1', '0x5') through the Ethereum provider interfaces and transaction types. The conversion to BigInt at the specific location is safe because:
- The
EthereumTransactioninterface explicitly typeschainIdas`0x${string}`, ensuring it's always a hex string - All chainId assignments in the codebase use either numeric strings or hex strings, which are valid
BigIntinputs - The conversion is only used for the
Common.custom()configuration, which requires aBigInttype as per the ethereum library requirements
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all assignments to `chainId` and check their types.
# Search for assignments to `chainId` in the codebase.
rg --type-add 'typescript' --files | xargs grep -E "chainId\s*[:=]"
# Check for any non-numeric assignments.
rg --type-add 'typescript' --files | xargs grep -E "chainId\s*[:=]\s*['\"]"
Length of output: 265
Script:
#!/bin/bash
# Search for chainId assignments and usage in typescript files
rg "chainId\s*[:=]" --type ts -A 2
# Search for chainId type definitions
ast-grep --pattern 'interface $_ {
$$$
chainId: $_
$$$
}'
# Search for chainId parameter in function signatures
ast-grep --pattern 'function $_($$$ chainId: $_ $$$) {
$$$
}'
Length of output: 27583
235-266: Verify that finalizedTransaction is correctly obtained in getGasCosts
In the getGasCosts method, ensure that finalizedTransaction is properly returned from finalizeTransaction. Any issues in obtaining finalizedTransaction could affect fee calculations, especially for networks like Optimism where opFee is added.
Run the following script to check for any inconsistencies in the use of finalizedTransaction:
✅ Verification successful
Let me gather more information about the finalizeTransaction implementation and its usage.
Let me check the actual implementation of finalizeTransaction and its return type definitions.
Let me check the usage of finalizedTransaction in the getOPfees method since it's used in the error case.
Based on the gathered information, I can now provide a final response:
finalizedTransaction is correctly obtained and used in getGasCosts
The code correctly obtains finalizedTransaction from finalizeTransaction method, which returns a properly typed object containing either a LegacyTransaction or FeeMarketEIP1559Transaction. The getOPfees method correctly uses this transaction object to calculate Optimism L1 fees by serializing the transaction and calling the price oracle contract. The implementation shows proper error handling and type safety throughout the flow.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for all usages of `finalizedTransaction` to ensure it's properly obtained and used.
rg --type=ts --pcre2 "(const|let|var)\s+\{[^}]*finalizedTransaction[^}]*\}\s+=\s+await\s+this\.finalizeTransaction"
Length of output: 263
Script:
#!/bin/bash
# Search for the implementation of finalizeTransaction
ast-grep --pattern 'finalizeTransaction($$$) {
$$$
}'
# Also check for the interface or type definition
rg --type=ts "finalizeTransaction.*:" -A 3
Length of output: 549
Script:
#!/bin/bash
# Get the implementation of finalizeTransaction
rg --type=ts -A 30 "async finalizeTransaction\("
# Get the type definitions
rg --type=ts "type FinalizedFeeMarketEthereumTransaction|type FinalizedLegacyEthereumTransaction" -A 5
Length of output: 3517
Script:
#!/bin/bash
# Search for getOPfees implementation and its usage of finalizedTransaction
rg --type=ts "getOPfees.*\(" -A 10
# Also check the type definitions for the transaction types used
rg --type=ts "type LegacyTransaction|type FeeMarketEIP1559Transaction" -A 5
Length of output: 3657
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
packages/extension/src/ui/action/views/swap/views/swap-best-offer/index.vue (3)
Line range hint
386-386: Address the "problem" comment in swap executionThere's a comment indicating a known issue in the swap execution logic. This needs to be properly documented or resolved.
Would you like me to help investigate the potential issues in the swap execution logic or create a GitHub issue to track this?
Line range hint
386-434: Enhance error handling in swap executionThe error handling in the swap execution could be improved:
- Replace generic console.error with structured logging
- Add specific error handling for different network types
- Sanitize error messages before displaying to users
Consider refactoring the error handling:
- .catch(err => { - console.error(err); - isTXSendError.value = true; - TXSendErrorMessage.value = err.error ? err.error.message : err.message; + .catch(err => { + const errorContext = { + network: network.value?.name, + provider: pickedTrade.value?.provider, + errorType: err.name + }; + console.error('Swap execution failed:', errorContext, err); + isTXSendError.value = true; + // Sanitize error message for user display + TXSendErrorMessage.value = getSanitizedErrorMessage(err);
Line range hint
196-208: Improve warning system configurationThe warning system uses hardcoded values and could benefit from:
- Moving magic numbers to configuration
- Adding documentation for warning thresholds
- Preparing for internationalization
Consider refactoring:
+const PRICE_DIFFERENCE_THRESHOLD = -0.2; // 20% price impact threshold +const WARNING_CONFIG = { + PRICE_DIFFERENCE_THRESHOLD, + // Add other warning thresholds here +}; - if (1 - Number(swapData.priceDifference) < -0.2) { + if (1 - Number(swapData.priceDifference) < WARNING_CONFIG.PRICE_DIFFERENCE_THRESHOLD) { warning.value = SwapBestOfferWarnings.BAD_PRICE; return; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
packages/extension/src/ui/action/views/swap/components/swap-error/index.vue(1 hunks)packages/extension/src/ui/action/views/swap/components/swap-loading/index.vue(1 hunks)packages/extension/src/ui/action/views/swap/views/swap-best-offer/index.vue(1 hunks)
🔇 Additional comments (3)
packages/extension/src/ui/action/views/swap/components/swap-loading/index.vue (1)
87-87: LGTM! Height change improves responsiveness.
The change from fixed height to 100% makes the component more flexible and responsive. Since this is a loading overlay, full viewport height is appropriate.
Let's verify the component's behavior across different viewport sizes:
✅ Verification successful
Height change is safe and consistent with parent components
The verification shows that the parent components (swap-best-offer/index.vue and index.vue) both use height: 100% and have proper height constraints:
- The main swap view sets a base height of 600px
- The best offer view uses
height: 100%with calculated internal spacing - Other loading/error components in the same directory also use
height: 100%consistently
This confirms that the loading component's switch to height: 100% is well-supported by its parent container structure and follows the established pattern for overlay components in this view.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential viewport/responsive issues
# Look for any media queries or responsive styles that might affect this component
rg -g '*.vue' -g '*.less' "height:|@media" ./packages/extension/src/ui/action/views/swap/
Length of output: 11791
packages/extension/src/ui/action/views/swap/components/swap-error/index.vue (1)
133-133: LGTM! Improved responsive design.
The change from fixed height to 100% makes the component more flexible and responsive. Since this is a modal overlay component, using 100% height is the correct approach.
Let's verify this change doesn't affect the component's appearance in different scenarios:
✅ Verification successful
LGTM! The height change is safe and aligns with the design.
The change from fixed height to 100% in the swap-error component is correct since:
- The parent component
.swap-looking__containeris positioned fixed and covers the full viewport - The component is used as a modal overlay, where 100% height is the standard approach
- No other components rely on its fixed height
- The change aligns with other responsive modal components in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other components that might rely on the fixed height
# Look for any explicit height references or related styling
# Search for any components that might be importing or using this component
rg -l "swap-error"
# Look for related height definitions that might need similar updates
rg "height: 600px" --type css --type vue
Length of output: 143
Script:
#!/bin/bash
# Search for any components that might be importing or using this component
# Using proper file type for Vue files
rg -l "swap-error" --type-add 'vue:*.vue' --type vue
# Look for related height definitions that might need similar updates
rg "height: 600px" --type-add 'vue:*.vue' --type vue
# Check the parent component to understand the layout context
cat "packages/extension/src/ui/action/views/swap/index.vue"
Length of output: 33589
packages/extension/src/ui/action/views/swap/views/swap-best-offer/index.vue (1)
14-14: Verify hardware wallet message component styling
The removal of inline width style (90%) from hardware-wallet-msg component could affect its layout. Ensure that the component's width is properly controlled through CSS classes.
✅ Verification successful
Let me search for the styling of the hardware-wallet-msg component to verify if the width is properly handled.
Let me check the referenced styles to verify how the component's width is handled.
Let me check the actual content of the verify-transaction styles to see how the hardware wallet message component is styled.
Based on the code analysis, I can now provide the final response:
Removal of inline width style is safe and follows proper styling
The hardware wallet message component's width is properly controlled through the CSS class provider-verify-transaction__block which sets width: 100% and includes appropriate padding and margins. The removal of the inline style width: 90% is actually an improvement as it:
- Follows the component's built-in responsive design
- Maintains consistency with other instances of the component across the codebase
- Relies on the proper CSS cascade through class-based styling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the width is handled in component's CSS or parent styles
rg -g '*.vue' -g '*.less' -g '*.css' 'hardware-wallet-msg' -A 5 -B 5
Length of output: 37473
Script:
#!/bin/bash
# Check the hardware-wallet-msg component's styling
rg -g '*.vue' -g '*.less' -g '*.css' 'hardware-wallet-msg' -A 10 -B 10 --context-separator="---" | grep -A 10 -B 10 "style\|class"
# Look for the actual component file to check its internal styling
fd -e vue "hardware-wallet-msg.vue" | xargs cat
Length of output: 50258
Script:
#!/bin/bash
# Check the styles in the referenced less files
cat << 'EOF' | xargs -I {} sh -c 'echo "=== {} ==="; cat {} 2>/dev/null || true'
@action/styles/theme.less
@/providers/ethereum/ui/styles/common-popup.less
@/providers/common/ui/verify-transaction/styles/verify-transaction.less
EOF
# Find any other relevant style files
fd -e less "verify-transaction.less|common-popup.less"
Length of output: 883
Script:
#!/bin/bash
# Check the content of verify-transaction.less files
cat packages/extension/src/providers/common/ui/styles/verify-transaction.less
echo "---"
cat packages/extension/src/providers/bitcoin/ui/styles/verify-transaction.less
echo "---"
cat packages/extension/src/providers/ethereum/ui/styles/common-popup.less
Length of output: 23768
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/extension/src/providers/solana/libs/api.ts (1)
67-67: Consider using a specific tag/commit instead of master branch.While using Solflare's token list is a good choice, referencing the master branch directly could lead to unexpected changes. Consider using a specific tag or commit hash for better stability and predictability.
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/<specific-commit-or-tag>/solana-tokenlist.json'
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
packages/extension/src/providers/ethereum/libs/transaction/gas-utils.ts(1 hunks)packages/extension/src/providers/solana/libs/api.ts(2 hunks)
🔇 Additional comments (2)
packages/extension/src/providers/solana/libs/api.ts (1)
77-77: Verify the impact of increased cache duration.
The increase in cache duration from 1 hour to 6 hours is reasonable for token lists, but let's verify if this affects any time-sensitive features.
✅ Verification successful
Based on the search results, I can now generate the final response:
The 6-hour cache duration for token info is safe to implement
The token info cache is primarily used for:
- Displaying token details (name, symbol, decimals) in transaction decoding
- Asset watching/listing functionality
- Balance display
These use cases are not time-sensitive as:
- Token metadata (name, symbol, decimals) rarely changes
- No swap-related code directly depends on this cached data
- Critical operations like swaps use separate APIs and price feeds
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any code that might depend on fresh token info updates
# Look for references to getTokenInfo that might need fresh data
rg -A 5 "getTokenInfo" --type ts
# Check for any swap-related code that might need fresh token data
rg -A 5 "swap.*token" --type ts
Length of output: 11226
packages/extension/src/providers/ethereum/libs/transaction/gas-utils.ts (1)
65-65: Significant increase in minimum priority fee.
The minimum priority fee has been increased from 0.1 gwei to 1 gwei (10x increase). While this change is reasonable given current network conditions and helps prevent stuck transactions, it will increase the base transaction costs across all fee types.
Let's verify the impact on transaction costs across different fee types:
✅ Verification successful
Based on the search results, I can now provide a conclusive verification of the review comment. The getMinPriorityFee function is used in several critical paths within the gas-utils.ts file:
- As a fallback when fee is zero:
if (fee.eqn(0)) return getMinPriorityFee(); - As a fallback when gas fee history is empty:
if (gasFeeHistory.blocks.length === 0) return getMinPriorityFee(); - As a default case in the gas price type switch statement
Let's verify if there are any gas fee calculations or tests that might be affected by this change:
Minimum priority fee increase is safe and localized
The change to increase the minimum priority fee from 0.1 to 1 gwei is safe because:
- The
getMinPriorityFeefunction is only used within gas-utils.ts as a fallback mechanism - It's used in three specific scenarios: when fee is zero, when gas fee history is empty, or as a default case
- No other files in the codebase directly use the gwei calculations or have hardcoded assertions about priority fees
- The change provides a more realistic floor value that helps prevent stuck transactions in current network conditions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for references to getMinPriorityFee to understand usage impact
# Find direct usage of getMinPriorityFee
echo "Direct usage of getMinPriorityFee:"
rg "getMinPriorityFee" -A 3
# Find related test files that might need updates
echo -e "\nRelated test files:"
fd -e test.ts -e spec.ts | xargs rg "priority.*fee" -l
Length of output: 1779
Script:
#!/bin/bash
# Search for gas fee calculations and tests
echo "Files with gas fee calculations:"
rg -l "toWei.*gwei"
echo -e "\nGas fee test assertions:"
rg "assert.*gwei" -A 2
Length of output: 258
Summary by CodeRabbit
New Features
Bug Fixes
Style