Devop/trezor conditional#705
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (3)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis update includes dependency version bumps across multiple package.json files, affecting both runtime and development dependencies. It introduces a new dynamic initialization pattern for TrezorConnect in hardware wallet integrations, refactoring related source files. Additionally, the Ethereum transaction verification UI now displays the recipient ("To") address and identicon, with corresponding style updates. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ExtensionUI
participant TrezorClass
participant getTrezorConnect
participant TrezorConnectInstance
User->>ExtensionUI: Initiate hardware wallet action (e.g., sign transaction)
ExtensionUI->>TrezorClass: Call init()
TrezorClass->>getTrezorConnect: Await getTrezorConnect()
getTrezorConnect->>TrezorConnectInstance: Dynamically import & initialize TrezorConnect
getTrezorConnect-->>TrezorClass: Return TrezorConnectInstance
TrezorClass->>TrezorConnectInstance: Call method (e.g., signTransaction)
TrezorConnectInstance-->>TrezorClass: Return result
TrezorClass-->>ExtensionUI: Return result
ExtensionUI-->>User: Show result
sequenceDiagram
participant User
participant ExtensionUI
User->>ExtensionUI: Review Ethereum transaction
ExtensionUI->>ExtensionUI: Decode transaction
ExtensionUI->>ExtensionUI: Generate identicon for "From" address
ExtensionUI->>ExtensionUI: Generate identicon for "To" address (new)
ExtensionUI-->>User: Display "From" and "To" sections with addresses and identicons
Possibly related PRs
Suggested reviewers
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}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (4)
packages/extension/src/providers/common/ui/styles/verify-transaction.less (1)
177-182: Consider refactoring to avoid !important flags.The extensive use of
!importantflags suggests CSS specificity issues. This can make the styles harder to maintain and override in the future.Consider increasing the specificity of the selector instead:
- &-to { - font-style: normal; - font-weight: 400 !important; - font-size: 16px !important; - line-height: 20px !important; - letter-spacing: 0.5px; - color: @black !important; - word-break: break-all; - } + &-to { + font-style: normal; + font-weight: 400; + font-size: 16px; + line-height: 20px; + letter-spacing: 0.5px; + color: @black; + word-break: break-all; + }Then increase the selector specificity in the parent selector if needed.
packages/hw-wallets/src/trezor/trezorConnect.ts (1)
4-4: Simplify the environment detection with optional chaining.The static analysis tool correctly identifies that this chained conditional check can be simplified.
Apply this diff to use optional chaining:
- 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/hw-wallets/src/trezor/ethereum/index.ts (1)
45-45: Type casting in error handling could mask type issuesThe consistent use of
(result.payload as any).errortype casting throughout error handling might mask potential typing issues.Consider defining proper error payload types instead of using
any:+interface TrezorErrorPayload { + error: string; +} -if (!rootPub.success) throw new Error((rootPub.payload as any).error); +if (!rootPub.success) throw new Error((rootPub.payload as TrezorErrorPayload).error);Also applies to: 79-79, 103-103, 123-123
packages/hw-wallets/src/trezor/solana/index.ts (1)
19-19: Consider adding initialization safeguards.The
TrezorConnectproperty is not initialized in the constructor. Consider adding runtime checks in methods that use this property to ensureinit()has been called first, or add JSDoc comments documenting the initialization requirement.async getAddress(options: getAddressRequest): Promise<AddressResponse> { + if (!this.TrezorConnect) { + throw new Error("TrezorConnect not initialized. Call init() first."); + } if (!supportedPaths[this.network])
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (21)
package.json(1 hunks)packages/extension-bridge/package.json(2 hunks)packages/extension/package.json(5 hunks)packages/extension/src/providers/common/ui/styles/verify-transaction.less(2 hunks)packages/extension/src/providers/ethereum/ui/eth-verify-transaction.vue(5 hunks)packages/hw-wallets/package.json(4 hunks)packages/hw-wallets/src/trezor/bitcoin/index.ts(6 hunks)packages/hw-wallets/src/trezor/ethereum/index.ts(7 hunks)packages/hw-wallets/src/trezor/solana/index.ts(4 hunks)packages/hw-wallets/src/trezor/trezorConnect.ts(1 hunks)packages/keyring/package.json(2 hunks)packages/name-resolution/package.json(2 hunks)packages/request/package.json(2 hunks)packages/signers/bitcoin/package.json(2 hunks)packages/signers/ethereum/package.json(2 hunks)packages/signers/kadena/package.json(2 hunks)packages/signers/polkadot/package.json(2 hunks)packages/storage/package.json(2 hunks)packages/swap/package.json(2 hunks)packages/types/package.json(2 hunks)packages/utils/package.json(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/hw-wallets/src/trezor/solana/index.ts (3)
packages/hw-wallets/src/types.ts (3)
getAddressRequest(74-76)AddressResponse(31-34)SolSignTransaction(61-63)packages/hw-wallets/src/trezor/solana/configs.ts (1)
supportedPaths(8-8)packages/utils/src/index.ts (1)
bufferToHex(38-38)
🪛 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)
- GitHub Check: buildAll
- GitHub Check: test
- GitHub Check: test
🔇 Additional comments (30)
packages/extension/src/providers/common/ui/styles/verify-transaction.less (1)
10-16: LGTM! Clean label styling added.The new label styles are well-defined and consistent with the design system. The properties are appropriate for a secondary label element.
packages/extension/src/providers/ethereum/ui/eth-verify-transaction.vue (5)
14-16: LGTM! Good template organization.The addition of the "From" comment and label improves the template structure and consistency with the new "To" section.
40-40: LGTM! Explicit image sizing is a good practice.Adding explicit width and height attributes helps prevent layout shifts and improves performance.
83-96: LGTM! Well-implemented "To" section.The new "To" section follows the same pattern as the existing "From" section, maintaining consistency in the UI. The use of the
identiconToreactive reference and proper fallback withdecodedTx?.toAddress ?? '~'shows good defensive programming.
203-203: LGTM! Proper reactive reference initialization.The
identiconToreference is properly initialized with a default empty identicon, ensuring no undefined state.
237-239: LGTM! Correct identicon generation for recipient.The identicon is properly generated for the recipient address using the lowercase address, which is consistent with Ethereum address handling best practices.
package.json (1)
37-37: Approve devDependency bump of @swc/core
Updated @swc/core from ^1.11.24 to ^1.11.29 to stay current with maintenance upgrades across the monorepo.packages/types/package.json (1)
27-30: Approve devDependency version bumps in @enkryptcom/types
The updates to @types/node, @typescript-eslint/eslint-plugin, @typescript-eslint/parser, eslint, tsup, and typescript-eslint align this package’s development tooling with the rest of the monorepo.Also applies to: 39-39, 41-41
packages/request/package.json (1)
34-38: Approve devDependency version bumps in @enkryptcom/request
Upgraded @types/node, ESLint plugins/parsers, tsup, typescript-eslint, and vitest to ensure consistency and leverage the latest patches.Also applies to: 46-46, 48-50
packages/signers/bitcoin/package.json (1)
35-38: Approve devDependency version bumps in @enkryptcom/signer-bitcoin
Bumped TypeScript and lint/test tooling (including vitest and tsup) to sync with monorepo standards.Also applies to: 47-47, 49-50
packages/signers/ethereum/package.json (1)
35-38: Approve devDependency version bumps in @enkryptcom/signer-ethereum
Aligned devDependencies (ESLint, tsup, vitest, etc.) with other packages for consistent development environment.Also applies to: 47-47, 49-50
packages/swap/package.json (1)
42-45: Consistent devDependency version bump
All development dependencies have been updated to the new patch/minor versions, aligning with the monorepo’s tooling standards. These are non-breaking updates to linting, build, and testing tools.Also applies to: 54-54, 56-57
packages/storage/package.json (1)
30-33: Consistent devDependency version bump
The updated devDependencies match the versions used across other packages in the repo, ensuring consistency in linting, build, and test tools. No breaking changes are expected.Also applies to: 42-42, 44-45
packages/extension-bridge/package.json (1)
47-47: DevDependency version updates for TypeScript tooling and linting
All listed devDependencies—including TypeScript ESLint plugins,bumpp, andtsup—have been bumped to the latest patch/minor releases, matching the rest of the monorepo. These changes standardize the development environment.Also applies to: 49-52, 61-61, 64-64
packages/name-resolution/package.json (1)
25-28: DevDependency version bump to align with monorepo
The ESLint, TypeScript,tsup, andvitestdevDependencies have been updated to the same versions used across other packages, keeping the build and test setup consistent.Also applies to: 37-37, 39-40
packages/signers/polkadot/package.json (2)
27-28: Patch bump of @polkadot/util and @polkadot/util-crypto
The runtime dependencies for Polkadot utilities have been upgraded from^13.4.xto^13.5.1, matching other signer packages. This is a non-breaking minor/patch update.
34-37: Consistent devDependency version updates
DevDependencies for TypeScript, ESLint,tsup, andvitesthave been bumped to the same versions used across the monorepo, standardizing the development tooling.Also applies to: 46-46, 48-49
packages/keyring/package.json (1)
32-32: LGTM! Dependency version bumps look good.These coordinated dependency updates across the monorepo are appropriate maintenance changes that should bring bug fixes and improvements without breaking compatibility.
Also applies to: 37-39, 40-40, 49-49, 51-52
packages/utils/package.json (1)
27-27: LGTM! Consistent dependency maintenance across the monorepo.These version bumps align with the coordinated dependency updates across other packages, maintaining consistency in the monorepo.
Also applies to: 33-36, 45-45, 48-49
packages/signers/kadena/package.json (1)
31-31: LGTM! Completing the coordinated dependency updates.These updates complete the pattern of coordinated dependency maintenance across the monorepo packages.
Also applies to: 33-36, 45-45, 47-48
packages/extension/package.json (2)
26-26: LGTM: Routine dependency updatesThe dependency version bumps appear to be routine maintenance updates with mostly patch and minor version increments, which should be safe.
Also applies to: 41-41, 43-43, 46-54, 81-81
102-102: LGTM: Development dependency updatesThe development dependency updates look good and maintain consistency with tooling versions across the monorepo.
Also applies to: 107-109, 119-119, 121-121, 127-127, 130-134, 140-140
packages/hw-wallets/package.json (2)
3-3: Significant version bump reflects the TrezorConnect refactoringThe version jump from 0.0.4 to 0.0.9 appropriately reflects the significant changes in the TrezorConnect integration pattern.
63-65: Addition of @trezor/connect-web supports new dynamic import patternThe new @trezor/connect-web dependency is correctly added to support the conditional TrezorConnect loading based on runtime environment.
packages/hw-wallets/src/trezor/bitcoin/index.ts (2)
5-5: LGTM: Clean import refactoringThe refactoring from static import to type-only import and dynamic loading via
getTrezorConnectis well-structured.Also applies to: 16-16
16-16: 💡 Verification agent🧩 Analysis chain
Verify getTrezorConnect implementation
Ensure the
getTrezorConnectfunction properly handles environment detection and initialization.
🏁 Script executed:
#!/bin/bash # Verify the getTrezorConnect implementation exists and check its logic cat packages/hw-wallets/src/trezor/trezorConnect.tsLength of output: 1017
Refine getTrezorConnect environment detection and init
Directly referencing
chromecan throw a ReferenceError in non-extension contexts and the web manifest is using an insecure URL. Please update as follows:• Safely guard against an undefined global:
- if (chrome && chrome.runtime && chrome.runtime.getPlatformInfo) { + if (typeof chrome !== 'undefined' && chrome.runtime && typeof chrome.runtime.getPlatformInfo === 'function') {• Ensure the web manifest uses HTTPS:
await TrezorConnect.default.init({ lazyLoad: true, manifest: { email: "info@enkrypt.com", - appUrl: "http://www.myetherwallet.com", + appUrl: "https://www.myetherwallet.com", }, });• (Optional) Confirm your
transportslist covers all needed options for your target environments.With these changes,
getTrezorConnectwill reliably detect extension vs. web contexts and use secure URLs.Likely an incorrect or invalid review comment.
packages/hw-wallets/src/trezor/ethereum/index.ts (1)
6-6: LGTM: Consistent refactoring patternThe refactoring follows the same clean pattern as the bitcoin implementation, maintaining consistency across Trezor integrations.
Also applies to: 16-16
packages/hw-wallets/src/trezor/solana/index.ts (3)
1-1: LGTM! Good TypeScript practices with type imports.The refactoring to use type imports and dynamic initialization via
getTrezorConnectfollows TypeScript best practices and enables runtime-specific TrezorConnect variant selection.Also applies to: 15-15
1-80: Consistent refactoring pattern successfully applied.The refactoring from static imports to instance-based TrezorConnect usage is consistently applied throughout the file and aligns well with the architectural goal of supporting different TrezorConnect variants based on runtime environment.
40-42:Details
❓ Verification inconclusive
Improve type safety and verify address/publicKey logic.
Several concerns with this implementation:
- Type casting to
anybypasses TypeScript safety and could hide runtime errors- Using the same decoded address value for both
addressandpublicKeyproperties seems incorrect- The logic converts a base58 address to hex, but
publicKeyshould typically be the raw public keyPlease verify the correct structure of the Trezor Solana response and whether address and publicKey should have the same value. Run this script to check the response structure:
🏁 Script executed:
#!/bin/bash # Check the Trezor Connect Solana API documentation and response structure ast-grep --pattern 'solanaGetAddress($_)' -A 10Length of output: 49
Improve type safety and correct address vs publicKey mapping
I couldn’t locate a
solanaGetAddressdefinition in the codebase, so please verify against the official Trezor Connect Solana API docs that the response payload actually includes bothaddressandpublicKeyfields, and that we’re decoding the right values.• Replace the
anycasts with the proper TrezorConnect typings (e.g.TrezorConnect.SolanaGetAddress['payload']).
• Ensure you decodepayload.addressfor the Solana address andpayload.publicKeyfor the raw public key, rather than using the same field twice.Suggested diff:
- address: bufferToHex(base58.decode((res.payload as any).address)), - publicKey: bufferToHex(base58.decode((res.payload as any).address)), + // Use the correct payload type instead of `any` + const { address, publicKey } = (res.payload as TrezorConnect.SolanaGetAddress['payload']); + + address: bufferToHex(base58.decode(address)), + publicKey: bufferToHex(base58.decode(publicKey)),Please confirm the exact payload structure in Trezor’s Solana API docs to ensure we map each field correctly.
| class TrezorEthereum implements HWWalletProvider { | ||
| network: NetworkNames; | ||
|
|
||
| TrezorConnect: TrezorConnect; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add null checking for TrezorConnect instance
The TrezorConnect property is not initialized in the constructor and could cause runtime errors if other methods are called before init().
Consider adding null checks in methods that use this.TrezorConnect:
async getAddress(options: getAddressRequest): Promise<AddressResponse> {
+ if (!this.TrezorConnect) {
+ throw new Error("TrezorConnect not initialized. Call init() first.");
+ }
if (!supportedPaths[this.network])
return Promise.reject(new Error("trezor-bitcoin: Invalid network name"));Apply similar checks to signPersonalMessage and signTransaction methods.
Also applies to: 29-30
🤖 Prompt for AI Agents
In packages/hw-wallets/src/trezor/bitcoin/index.ts around lines 20 and 29-30,
the TrezorConnect property is declared but not initialized in the constructor,
which can cause runtime errors if methods like signPersonalMessage and
signTransaction are called before init(). To fix this, add null or undefined
checks for this.TrezorConnect at the start of these methods and handle the case
where it is not initialized, such as throwing an error or returning early, to
prevent runtime exceptions.
| class TrezorEthereum implements HWWalletProvider { | ||
| network: NetworkNames; | ||
|
|
||
| TrezorConnect: TrezorConnect; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add initialization checks for TrezorConnect
Same issue as in the bitcoin implementation - the TrezorConnect property could be undefined if methods are called before init().
Add null checks consistently across all methods that use this.TrezorConnect:
async getAddress(options: getAddressRequest): Promise<AddressResponse> {
+ if (!this.TrezorConnect) {
+ throw new Error("TrezorConnect not initialized. Call init() first.");
+ }
if (!supportedPaths[this.network])
return Promise.reject(new Error("trezor-ethereum: Invalid network name"));Also applies to: 29-30
🤖 Prompt for AI Agents
In packages/hw-wallets/src/trezor/ethereum/index.ts around lines 20 and 29-30,
the TrezorConnect property may be undefined if methods are called before init().
To fix this, add null or undefined checks for this.TrezorConnect at the start of
all methods that use it, and handle the case where it is not initialized, such
as throwing an error or returning early. This ensures safe usage and prevents
runtime errors.
1 participant
Summary by CodeRabbit
New Features
Style
Bug Fixes
Chores