Conversation
Release: v2.7.0
WalkthroughMultiple Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (18)
✅ Files skipped from review due to trivial changes (4)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code Graph Analysis (2)packages/hw-wallets/src/trezor/solana/index.ts (3)
packages/extension/src/providers/ethereum/libs/transaction/index.ts (2)
🪛 Biome (1.9.4)packages/hw-wallets/src/trezor/trezorConnect.ts[error] 4-4: Change to an optional chain. Unsafe fix: Change to an optional chain. (lint/complexity/useOptionalChain) ⏰ Context from checks skipped due to timeout of 90000ms (3)
🔇 Additional comments (13)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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}
Devop/trezor conditional
devop: to address field
Add safe buffer to Rootstock collective staking gas estimation
kvhnuke
force-pushed
the
devop/release-v-2-7-0
branch
from
013aedf to
48c972f
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 6
🧹 Nitpick comments (7)
packages/extension/src/providers/ethereum/ui/send-transaction/components/send-address-input.vue (1)
74-74: Good UX improvement for short address readability.This change appropriately displays short addresses in full without truncation, improving readability when ellipsis would be unnecessary.
Consider extracting the magic number
13to a named constant for better maintainability:+const SHORT_ADDRESS_THRESHOLD = 13; + const visibleAddress = computed(() => { let address = props.value; if (props.network.isAddress(address)) address = props.network.displayAddress(props.value); if (isFocus.value) return address; - if (address.length < 13) return address; + if (address.length < SHORT_ADDRESS_THRESHOLD) return address; return replaceWithEllipsis(address, 6, 6); });packages/extension/src/ui/action/views/swap/components/send-address-input.vue (1)
83-83: Consistent UX improvement across components.This change aligns with the same improvement made in the Ethereum component, ensuring consistent address display behavior across different UI contexts.
For consistency with the recommendation in the Ethereum component, consider extracting the threshold to a shared constant that could be imported across all address input components.
packages/extension/src/providers/solana/ui/send-transaction/components/send-address-input.vue (1)
71-75: Excellent refactoring with clear conditional logic.This implementation is the clearest of the three address input components. The explicit conditional structure makes it immediately obvious when the full address is displayed (focused OR short) versus when truncation applies.
Consider applying this clearer conditional pattern to the other address input components for consistency. Also, extract the magic number
13to a shared constant:+const SHORT_ADDRESS_THRESHOLD = 13; + const address = computed({ get: () => { if (isFocus.value) return solAddress.value; - if (solAddress.value.length < 13) return solAddress.value; + if (solAddress.value.length < SHORT_ADDRESS_THRESHOLD) return solAddress.value; return replaceWithEllipsis(solAddress.value, 6, 6); }, set: value => emit('update:inputAddress', value), });packages/extension/src/providers/common/ui/styles/verify-transaction.less (1)
177-181: Consider alternatives to !important declarations.While the styling achieves the desired visual effect, the extensive use of
!importantflags suggests potential CSS specificity issues. This can make future maintenance more difficult.Consider refactoring to increase specificity naturally instead of using
!important:&-to { font-style: normal; - font-weight: 400 !important; - font-size: 16px !important; - line-height: 20px !important; + font-weight: 400; + font-size: 16px; + line-height: 20px; letter-spacing: 0.5px; - color: @black !important; + color: @black; word-break: break-all; }If overriding is necessary, consider increasing the selector specificity or restructuring the CSS hierarchy.
packages/hw-wallets/src/trezor/trezorConnect.ts (1)
4-4: Improve safety with optional chaining.The chrome runtime detection can be made safer using optional chaining as suggested by the static analysis tool.
- if (chrome && chrome.runtime && chrome.runtime.getPlatformInfo) { + if (chrome?.runtime?.getPlatformInfo) {🧰 Tools
🪛 Biome (1.9.4)
[error] 4-4: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/extension/src/providers/ethereum/libs/transaction/gas-utils.ts (1)
182-195: Consider making API URLs configurable.The configuration structure is well-organized. However, hardcoding external API URLs could make maintenance difficult if Blockscout endpoints change or become unavailable.
Consider moving these URLs to environment variables or a configuration file for easier maintenance and environment-specific deployments.
packages/extension/src/providers/ethereum/libs/transaction/index.ts (1)
112-118: Consider adding caching and fallback mechanisms.The staking gas adjustment relies on external API availability, which could impact reliability and performance.
Consider implementing:
- Response caching: Cache successful gas values for a short period to reduce API calls
- Circuit breaker pattern: Temporarily disable the feature if APIs are consistently failing
- Metrics/monitoring: Track API response times and failures to ensure good user experience
🛑 Comments failed to post (6)
packages/extension/src/providers/ethereum/ui/eth-verify-transaction.vue (1)
203-203: 🛠️ Refactor suggestion
Potential issue with empty string initialization.
The
identiconTois initialized with an empty string passed tonetwork.value.identicon(''). This could potentially cause issues if the identicon function doesn't handle empty strings gracefully.Consider initializing with a safer default:
-const identiconTo = ref<string>(network.value.identicon('')); +const identiconTo = ref<string>('');Or ensure the identicon function handles empty strings properly before initialization.
📝 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 identiconTo = ref<string>(network.value.identicon('')); + const identiconTo = ref<string>('');🤖 Prompt for AI Agents
In packages/extension/src/providers/ethereum/ui/eth-verify-transaction.vue at line 203, the identiconTo variable is initialized by calling network.value.identicon with an empty string, which may cause issues if the identicon function does not handle empty strings properly. To fix this, either initialize identiconTo with a safer default value that the identicon function can handle or modify the identicon function to safely process empty string inputs before this initialization.packages/hw-wallets/src/trezor/bitcoin/index.ts (1)
18-18:
⚠️ Potential issueFix incorrect class name.
The class is named
TrezorEthereumbut this file implements Bitcoin functionality. This appears to be a copy-paste error.-class TrezorEthereum implements HWWalletProvider { +class TrezorBitcoin implements HWWalletProvider {Also update the export statement:
-export default TrezorEthereum; +export default TrezorBitcoin;📝 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.// ... other imports and code ... class TrezorBitcoin implements HWWalletProvider { // ... existing implementation ... } export default TrezorBitcoin;🤖 Prompt for AI Agents
In packages/hw-wallets/src/trezor/bitcoin/index.ts at line 18, the class is incorrectly named TrezorEthereum, which does not match the Bitcoin functionality of this file. Rename the class to TrezorBitcoin to reflect the correct implementation. Additionally, update the export statement to export TrezorBitcoin instead of TrezorEthereum.packages/hw-wallets/src/trezor/solana/index.ts (2)
62-67: 🛠️ Refactor suggestion
Add error handling for transaction signing.
Similar to the address retrieval, the transaction signing lacks error handling present in other implementations.
return this.TrezorConnect.solanaSignTransaction({ path: options.pathType.path.replace(`{index}`, options.pathIndex), serializedTx: (options.transaction as SolSignTransaction).solTx.toString( "hex", ), - }).then((result) => (result.payload as any).signature); + }).then((result) => { + if (!result.success) throw new Error((result.payload as any).error); + return (result.payload as any).signature; + });📝 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.return this.TrezorConnect.solanaSignTransaction({ path: options.pathType.path.replace(`{index}`, options.pathIndex), serializedTx: (options.transaction as SolSignTransaction).solTx.toString( "hex", ), }).then((result) => { if (!result.success) throw new Error((result.payload as any).error); return (result.payload as any).signature; });🤖 Prompt for AI Agents
In packages/hw-wallets/src/trezor/solana/index.ts around lines 62 to 67, the transaction signing call lacks error handling. Modify the promise chain to check if the result indicates success; if not, throw or handle the error appropriately. This can be done by inspecting the result's success or error properties before returning the signature, ensuring any failure is caught and managed similarly to the address retrieval method.
35-42: 🛠️ Refactor suggestion
Add error handling for consistency and safety.
The Solana implementation is missing error handling that exists in the Bitcoin and Ethereum implementations. This could lead to runtime errors if the Trezor operation fails.
const res = await this.TrezorConnect.solanaGetAddress({ path: options.pathType.path.replace(`{index}`, options.pathIndex), showOnTrezor: options.confirmAddress, }); + if (!res.payload) { + throw new Error("popup failed to open"); + } + if (!res.success) throw new Error((res.payload as any).error); return { address: bufferToHex(base58.decode((res.payload as any).address)), publicKey: bufferToHex(base58.decode((res.payload as any).address)), };📝 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 res = await this.TrezorConnect.solanaGetAddress({ path: options.pathType.path.replace(`{index}`, options.pathIndex), showOnTrezor: options.confirmAddress, }); if (!res.payload) { throw new Error("popup failed to open"); } if (!res.success) { throw new Error((res.payload as any).error); } return { address: bufferToHex(base58.decode((res.payload as any).address)), publicKey: bufferToHex(base58.decode((res.payload as any).address)), };🤖 Prompt for AI Agents
In packages/hw-wallets/src/trezor/solana/index.ts between lines 35 and 42, add error handling around the call to this.TrezorConnect.solanaGetAddress to check if the response indicates success before accessing res.payload. If the call fails, throw an error or handle it appropriately to prevent runtime exceptions, ensuring consistency with the Bitcoin and Ethereum implementations.packages/extension/src/providers/ethereum/libs/transaction/gas-utils.ts (1)
197-242: 🛠️ Refactor suggestion
Enhance error handling and add timeout controls.
The gas calculation logic is sound, but there are several areas for improvement:
- Generic error handling: The catch block (lines 239-241) silently returns the original estimate for any error
- No timeout protection: External API calls could hang indefinitely
- Sequential API calls: Could impact transaction preparation performance
Consider these improvements:
-const safeGasForStaking = async (chainID: string, estimatedGas: number) => { +const safeGasForStaking = async (chainID: string, estimatedGas: number): Promise<number> => { const gasConfig = collectiveGasConfig[chainID]; + const controller = new AbortController(); + const timeout = setTimeout(() => controller.abort(), 5000); // 5s timeout + try { - const stakingResponse = await fetch(gasConfig.stakingUrl); + const stakingResponse = await fetch(gasConfig.stakingUrl, { signal: controller.signal }); if (!stakingResponse.ok) { + console.warn(`Failed to fetch staking history: ${stakingResponse.status}`); return estimatedGas; }Also consider adding more specific error logging to help with debugging issues in production.
📝 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 safeGasForStaking = async (chainID: string, estimatedGas: number): Promise<number> => { const gasConfig = collectiveGasConfig[chainID]; const controller = new AbortController(); const timeout = setTimeout(() => controller.abort(), 5000); // 5s timeout try { const stakingResponse = await fetch(gasConfig.stakingUrl, { signal: controller.signal }); if (!stakingResponse.ok) { console.warn(`Failed to fetch staking history: ${stakingResponse.status}`); return estimatedGas; } const stakingTxHistory = await stakingResponse.json(); for (const tx of stakingTxHistory.items) { // find stake tx if (tx.method === gasConfig.method) { const txDetailsResponse = await fetch(`${gasConfig.txUrl}/${tx.transaction_hash}`); if (!txDetailsResponse.ok) { return estimatedGas; } const txDetail = await txDetailsResponse.json(); // tx must be successfull if (txDetail.status === 'ok') { const txGasLimit = Number(txDetail.gas_limit); const txGasUsed = Number(txDetail.gas_used); // if web3 estimate gas is less than previous stake tx gas used // This could cause tx to fail due to less gas // Bump the gas limit particularly in this case if (estimatedGas < txGasUsed) { // Check if need to add buffer const usedGasPercentage = txGasUsed / txGasLimit * 100; if (usedGasPercentage > 80) { // add a safe buffer of 30% return Math.max(txGasUsed + (txGasUsed * 0.30), txGasLimit); } // safe buffer not required, just use gas limit return txGasLimit; } // safe to use max of estimate or tx gas limit return Math.max(estimatedGas, txGasLimit); } } } // No previous stake tx found, just return web3 estimated gas. return estimatedGas; } catch { return estimatedGas; } };🤖 Prompt for AI Agents
In packages/extension/src/providers/ethereum/libs/transaction/gas-utils.ts between lines 197 and 242, improve error handling by catching and logging specific errors instead of silently returning the estimated gas. Add timeout controls to the fetch calls to prevent hanging on external API requests. Refactor the sequential fetch calls to run in parallel where possible to enhance performance, such as fetching transaction details concurrently after retrieving the staking transaction history. Ensure all fetch calls handle errors gracefully and log meaningful messages for easier debugging in production.packages/extension/src/providers/ethereum/libs/transaction/index.ts (1)
115-118: 🛠️ Refactor suggestion
⚠️ Potential issueImprove type safety and code formatting.
The logic is correct, but there are type safety and formatting concerns:
- Type overflow risk: Converting hex string to
Numbercould cause issues with large gas values- Inconsistent formatting: Missing spaces and inconsistent indentation
Apply this diff to address the issues:
- if ([rsk.chainID, rskTestnet.chainID].includes(this.tx.chainId) && this.tx.data && - this.tx.data.startsWith(collectiveGasConfig[this.tx.chainId].methodId)) { - gasLimit = (numberToHex(await safeGasForStaking(this.tx.chainId, Number(gasLimit))) as `0x${string}`) - } + if ( + [rsk.chainID, rskTestnet.chainID].includes(this.tx.chainId) && + this.tx.data && + this.tx.data.startsWith(collectiveGasConfig[this.tx.chainId].methodId) + ) { + const currentGasLimit = parseInt(gasLimit, 16); + if (!isNaN(currentGasLimit)) { + gasLimit = numberToHex( + await safeGasForStaking(this.tx.chainId, currentGasLimit) + ) as `0x${string}`; + } + }This ensures safe hex-to-number conversion and improves readability.
📝 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.if ( [rsk.chainID, rskTestnet.chainID].includes(this.tx.chainId) && this.tx.data && this.tx.data.startsWith(collectiveGasConfig[this.tx.chainId].methodId) ) { const currentGasLimit = parseInt(gasLimit, 16); if (!isNaN(currentGasLimit)) { gasLimit = numberToHex( await safeGasForStaking(this.tx.chainId, currentGasLimit) ) as `0x${string}`; } }🤖 Prompt for AI Agents
In packages/extension/src/providers/ethereum/libs/transaction/index.ts around lines 115 to 118, fix the type safety issue by replacing the direct Number conversion of the hex gasLimit with a safer hex-to-number conversion method to avoid overflow risks. Also, improve code formatting by adding necessary spaces and consistent indentation for better readability.
5 participants
Summary by CodeRabbit
New Features
Chores