Fix/sending nft fee calc #581
Conversation
WalkthroughThe pull request introduces consistent changes across multiple Ethereum network configuration files, specifically modifying the Changes
Possibly related PRs
Suggested reviewers
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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: 0
🧹 Nitpick comments (3)
packages/extension/src/providers/ethereum/networks/arb-nova.ts (1)
Line range hint
4-24: Consider documenting the relationship between activity handlers and fee calculations.While the implementation of
wrapActivityHandleris consistent across all network configurations, the connection between these changes and NFT fee calculations (mentioned in the PR title) isn't immediately clear. Consider:
- Adding comments explaining how these changes relate to fee calculations
- Documenting any assumptions about the activity handler's role in fee processing
- Including test cases that verify the integration between these components
Would you like help drafting documentation or test cases to clarify these relationships?
packages/extension/src/providers/ethereum/ui/send-transaction/index.vue (2)
228-238: Consider adding a pre-validation stepWhile the validation logic is correct, consider adding a pre-validation step before attempting transactions to provide early feedback to users.
+const canCoverFees = computed(() => { + return !toBN( + toBase( + gasCostValues.value[selectedFee.value].nativeValue, + props.network.decimals, + ), + ).gt(toBN(nativeBalance.value)); +}); +watch(canCoverFees, (newVal) => { + if (!newVal) { + // Disable send button or show warning + } +});Also applies to: 577-578
228-238: Consider implementing fee estimation cachingThe current implementation recalculates fees on every input change. Consider implementing a caching mechanism for fee estimates to improve performance and reduce RPC calls.
- Cache fee estimates with a reasonable TTL (e.g., 30 seconds)
- Only recalculate when:
- The selected fee type changes
- The TTL expires
- The transaction parameters significantly change
Also applies to: 577-578
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/extension/src/providers/ethereum/networks/arb-nova.ts(2 hunks)packages/extension/src/providers/ethereum/networks/bsc.ts(1 hunks)packages/extension/src/providers/ethereum/networks/holesky.ts(2 hunks)packages/extension/src/providers/ethereum/networks/op-bnb.ts(2 hunks)packages/extension/src/providers/ethereum/networks/palm.ts(2 hunks)packages/extension/src/providers/ethereum/ui/send-transaction/index.vue(2 hunks)
🔇 Additional comments (8)
packages/extension/src/providers/ethereum/networks/holesky.ts (2)
4-4: LGTM: Import addition is consistent with other network configurations.
The import statement follows the project's convention using the @/ alias path.
18-18: Verify the wrapper's impact on activity handling.
While the change maintains the same underlying behavior (returning an empty array), we should verify that wrapActivityHandler properly handles promise resolution.
✅ Verification successful
The wrapper properly handles promise resolution for activity handling
The wrapActivityHandler implementation correctly handles promise resolution and caching:
- It properly awaits the promise returned by the wrapped handler using
await - It handles both empty and populated activity states
- It implements caching with proper TTL (5 minutes) and cache invalidation
- The empty array promise in the Holesky network is handled appropriately within this wrapper
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for wrapActivityHandler implementation and usage patterns
ast-grep --pattern 'export default function wrapActivityHandler($_) {
$$$
}'
Length of output: 80
Script:
#!/bin/bash
# Let's try searching for wrapActivityHandler with ripgrep to find its implementation
rg "wrapActivityHandler" -A 5
Length of output: 65711
Script:
#!/bin/bash
# Let's search for the implementation of wrapActivityHandler in the libs directory
rg -g "*.ts" "export.*wrapActivityHandler" -A 10 packages/extension/src/libs/
Length of output: 79
Script:
#!/bin/bash
# Let's try finding the file containing wrapActivityHandler implementation
fd "wrap-activity-handler.ts" packages/extension/src/
Length of output: 121
Script:
#!/bin/bash
# Let's examine the implementation of wrapActivityHandler
cat packages/extension/src/libs/activity-state/wrap-activity-handler.ts
Length of output: 1216
packages/extension/src/providers/ethereum/networks/op-bnb.ts (1)
6-6: Verify integration with NFT handler.
Given that this is a production network with NFT support, we should verify that wrapActivityHandler properly integrates with the existing shNFTHandler for NFT-related activities.
Also applies to: 24-24
✅ Verification successful
NFT handler integration is properly implemented
The verification shows that the integration is correct and safe:
- The
shNFTHandlerfrom simplehash.ts explicitly supports opBNB network (listed asOpBNBinsupportedNetworks) - The
wrapActivityHandleris a generic wrapper that adds caching functionality and works independently of the NFT handler - Both handlers are properly initialized in op-bnb.ts and don't interfere with each other's operation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for interactions between NFT handler and activity handler
rg -A 5 "shNFTHandler.*wrapActivityHandler|wrapActivityHandler.*shNFTHandler"
# Check NFT handler implementation
ast-grep --pattern 'const shNFTHandler = {
$$$
}'
Length of output: 133
Script:
#!/bin/bash
# Let's first check the file content to understand the NFT handler usage
cat packages/extension/src/providers/ethereum/networks/op-bnb.ts
# Also search for NFT handler patterns across the codebase
rg -A 5 "NFTHandler"
# Look for activity handler wrapping patterns
ast-grep --pattern 'wrapActivityHandler($$$)'
Length of output: 50398
Script:
#!/bin/bash
# Let's check the implementation of the NFT handler and activity handler wrapper
cat packages/extension/src/libs/nft-handlers/simplehash.ts
cat packages/extension/src/libs/activity-state/wrap-activity-handler.ts
Length of output: 5210
packages/extension/src/providers/ethereum/networks/arb-nova.ts (1)
6-6: Consistent implementation across network configurations.
The changes follow the same pattern as other network configurations, which is good for maintainability. However, given this PR's focus on NFT fee calculations, we should verify the broader impact.
Consider adding integration tests to verify the interaction between wrapped activity handlers and fee calculations across different networks.
Also applies to: 24-24
packages/extension/src/providers/ethereum/networks/palm.ts (1)
5-5: Verify if empty activity handler is intentional
The activity handler is now returning an empty array. While this implementation is consistent with other network files, please verify if this is intentional as it might affect activity tracking for the Palm network.
Also applies to: 24-24
✅ Verification successful
Empty activity handler is intentional for Palm network
The empty activity handler implementation is intentional and consistent with several other EVM networks in the codebase. Multiple networks like BSC, Holesky, Arbitrum Nova, Klaytn, and others also use wrapActivityHandler(() => Promise.resolve([])). This pattern is used when a network doesn't have a specific activity tracking implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if other network configurations use similar empty activity handlers
rg -A 1 "activityHandler.*wrapActivityHandler"
Length of output: 17630
packages/extension/src/providers/ethereum/networks/bsc.ts (1)
26-26:
Consider impact of removing EtherscanActivity
The change removes actual activity tracking via EtherscanActivity in favor of an empty array. This is a significant change that could affect transaction history visibility.
packages/extension/src/providers/ethereum/ui/send-transaction/index.vue (2)
228-238: Improved fee validation logic
Good addition of native balance check for transaction fees. This prevents transactions from failing due to insufficient funds for gas fees.
577-578: Enhanced input validation
Good addition of BigNumber validation to prevent NaN values and ensure numeric inputs are valid.
Summary by CodeRabbit
New Features
Bug Fixes
Chores