WalletConnect Solana adapter #17360
WalletConnect Solana adapter #17360
Conversation
|
No dependency changes detected. Learn more about Socket for GitHub ↗︎ 👍 No dependency changes detected in pull request |
|
🚀 Expo preview is ready!
|
martykan
force-pushed
the
feat/walletconnect-solana
branch
2 times, most recently
from
cce06e5 to
939fb8c
Compare
martykan
force-pushed
the
feat/walletconnect-solana
branch
from
939fb8c to
9331fff
Compare
WalkthroughThe overall changes span multiple modules, focusing on enhanced fee estimation, transaction processing, UI adjustments, and WalletConnect integration. In the fee estimation modules, an optional fee payer property is added and function parameters are updated to work directly with decompiled transaction messages. The modifications streamline operations by removing obsolete decompilation functions and modifying internal logic for clarity and efficiency. In the UI components, a new property is introduced to allow select menus to adjust their width based on content. WalletConnect integrations are expanded through added adapter methods that generate chain IDs, construct namespaces, and process namespaces, alongside updates in thunks and proposal approval logic to incorporate preferential account ordering. Additionally, new interfaces support structured account grouping. Overall, the changes update types, function signatures, and dependency management across blockchain processing, UI, and WalletConnect adapters, ensuring consistency in API usage and supporting network-specific requirements. ✨ 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:
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 (9)
suite-common/walletconnect/src/adapters/solana.ts (4)
5-25: Centralizing methods is a good design choice.Defining the
methodsarray for Solana-specific functionality keeps logic tidy and consistent with other adapters.
27-84: Promote clearer error messaging and handle possible unsuccessful fee estimates gracefully.
- If
estimatedFee.successisfalse, the code throws a generic error. Including the actual error fromestimatedFee.payload.error(if present) can aid debugging.- The fallback for
feePayer(line 51) is helpful, but double-check if there's ever a scenario whereestimatedFee.payload.levelsis empty.- throw new Error('Failed to estimate fee'); + throw new Error(`Failed to estimate fee: ${estimatedFee.payload.error ?? 'unknown error'}`);
127-129: Update error messages for Solana push transaction.The error references "eth_sendTransaction", which may be a copy-paste leftover. Rename it to clarify it is a Solana push error.
- console.error('eth_sendTransaction push error', pushResponse); - throw new Error('eth_sendTransaction push error'); + console.error('solana_signAndSendTransaction push error', pushResponse); + throw new Error('solana_signAndSendTransaction push error');
166-201: Robust namespace processing.Checks for unsupported, inactive, and active networks are well structured. Make sure the
hasAccountslogic aligns with future expansions (e.g., multi-account requirements).suite-common/walletconnect/src/walletConnectTypes.ts (1)
5-6: Interfaces and new methods appear coherent.
getChainId,getNamespace, andprocessNamespacesprovide clear hooks for multi-chain context.- Consider clarifying the
requiredparameter's meaning inprocessNamespaces(e.g., rename toareNamespacesMandatory) for improved readability.Also applies to: 14-21
packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/WalletConnectProposalModal.tsx (1)
71-89: Account ordering logic is coherent.
Sorting by device, then account type, then index is well-structured. Potentially factor it out as a utility for reusability if needed across the codebase.suite-common/walletconnect/src/walletConnectThunks.ts (1)
128-128: Helpful console logging for error diagnostics.
We see an inline console.error; if it’s just for debugging, consider adding a consistent error handling approach.suite-common/walletconnect/src/adapters/ethereum.ts (2)
20-25: Confirm method coverage.
Consider verifying whether other Ethereum methods likeeth_signare needed. Otherwise, keeping only these methods is fine if your dApps and internal logic don't require additional calls.
219-254: Namespace processing approach is generally good.
This adds robust logic to categorize networks asunsupported,active, orinactive. Consider how this might be extended or unified if you add multi-chain or non-EVM support.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (11)
packages/blockchain-link-types/src/responses.ts(1 hunks)packages/blockchain-link/src/workers/solana/fee.ts(2 hunks)packages/blockchain-link/src/workers/solana/index.ts(3 hunks)packages/components/src/components/form/Select/Select.tsx(5 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/WalletConnectProposalModal.tsx(2 hunks)suite-common/walletconnect/package.json(1 hunks)suite-common/walletconnect/src/adapters/ethereum.ts(2 hunks)suite-common/walletconnect/src/adapters/index.ts(2 hunks)suite-common/walletconnect/src/adapters/solana.ts(1 hunks)suite-common/walletconnect/src/walletConnectThunks.ts(7 hunks)suite-common/walletconnect/src/walletConnectTypes.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
suite-common/walletconnect/src/adapters/index.ts
[error] 26-26: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
suite-common/walletconnect/src/walletConnectThunks.ts
[error] 279-279: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
⏰ Context from checks skipped due to timeout of 90000ms (14)
- GitHub Check: Releases revision Checks
- GitHub Check: PR-check / node-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: PR-check / node-override init-api-flaky
- GitHub Check: PR-check / web-override init-api-flaky
- GitHub Check: PR-check / web-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: build-deploy
- GitHub Check: build-deploy
- GitHub Check: EAS Update
- GitHub Check: test
- GitHub Check: build-web
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: transport-e2e-test
- GitHub Check: prepare_android_test_app
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (46)
suite-common/walletconnect/package.json (1)
21-22:✅ Verification successful
Dependency addition for Solana support
Adding the
bs58package is appropriate for Solana integration as it's required for Base58 encoding/decoding, which is the standard format for Solana addresses and transaction data.
🏁 Script executed:
#!/bin/bash # Check if bs58 is a stable package and its usage in the codebase # Check latest version and download stats to verify it's a maintained package npm view bs58 --json | jq '{version: .version, downloads: .["dist-tags"], license: .license, maintainers: .maintainers}' # Look for actual usage of bs58 in the codebase to verify it's being utilized rg --type ts "bs58" -A 3 -B 3Length of output: 24341
Dependency Addition Verified: bs58 is appropriately added for Solana support.
After verification, we confirmed that:
- bs58 (v6.0.0) is stable, MIT licensed, and maintained.
- It is actively used in the codebase (e.g., in
suite-common/walletconnect/src/adapters/solana.tsfor encoding signatures) as well as in multiple other modules for Base58 encoding/decoding.The addition of the
bs58package insuite-common/walletconnect/package.jsonis thus correctly justified to support Solana’s standard for address and transaction data encoding.packages/blockchain-link-types/src/responses.ts (1)
101-101:✅ Verification successful
Valid addition of optional feePayer property
Adding the optional
feePayerproperty to the EstimateFee interface is appropriate for Solana support, as Solana transactions require explicit identification of the account that will pay the transaction fees.
🏁 Script executed:
#!/bin/bash # Check if the feePayer property is used in the Solana-related code # Look for usage of feePayer in Solana adapter implementations rg "feePayer" --type ts -A 2 -B 2 "./packages/blockchain-link/src/workers/solana"Length of output: 547
Approved: Addition of optional feePayer property is valid
The new
feePayer?: string;property in the EstimateFee interface is properly justified, as it aligns with Solana’s requirement to explicitly designate the fee payer. Verification confirms that the property is used in the Solana adapter (e.g., inpackages/blockchain-link/src/workers/solana/index.tswherefeePayeris accessed).packages/blockchain-link/src/workers/solana/index.ts (4)
24-25: Import new decompilation function for lookup tablesAdding the
decompileTransactionMessageFetchingLookupTablesimport is aligned with Solana's recent addition of Address Lookup Tables, which helps with transaction space optimization.
491-496: Updated priority fee calculation with decompiledTransactionMessageProperly passing the decompiledTransactionMessage to getPriorityFee ensures accurate fee computation for Solana transactions, handling both the decompiled message and compiled message for different aspects of fee calculation.
515-515: Added feePayer address to fee estimation payloadAdding the feePayer address to the payload provides important information for transaction construction and signing, especially for wallet connect integration where the client needs to know which account will pay the fees.
486-489:✅ Verification successful
Using more robust transaction message decompilation
The switch to
decompileTransactionMessageFetchingLookupTablesis a good improvement as it properly handles Address Lookup Tables, which are important for complex Solana transactions.
🏁 Script executed:
#!/bin/bash # Verify the implementation and usage of the decompilation function # Check how this new function is implemented in @solana/web3.js rg "decompileTransactionMessageFetchingLookupTables" --type ts -A 10 -B 2 "./node_modules/@solana" # Check for other usages of this function in the codebase to ensure consistency rg "decompileTransactionMessageFetchingLookupTables" --type ts -A 2 -B 2Length of output: 1159
Improvement Validated – Transaction Message Decompilation Upgrade Approved
The updated implementation using
decompileTransactionMessageFetchingLookupTablesconsistently handles Address Lookup Tables, ensuring robust decompilation of complex Solana transactions. Our search confirmed its correct usage inpackages/blockchain-link/src/workers/solana/index.tswithout conflicting references. Great work on aligning with best practices.packages/components/src/components/form/Select/Select.tsx (5)
119-119: Added menu width control property to WrapperPropsAdding the
$menuFitContenttransient property to the styled component props is the correct pattern for controlling CSS from component props.
285-285: Added menuFitContent to SelectProps interfaceThe property is correctly added to the SelectProps interface, maintaining type safety for component consumers.
299-299: Updated Select component parametersThe menuFitContent parameter is added to the function signature, allowing consumers to use this feature when needed.
359-359: Passed menuFitContent to styled wrapper componentThe menuFitContent prop is correctly passed to the styled component as a transient prop ($menuFitContent), following React's best practices for passing props to styled components.
234-238:✅ Verification successful
Dynamic menu width styling implementation
The conditional styling for the select menu width is implemented correctly. When
$menuFitContentis true, the menu will size itself according to content instead of filling the available space.Let's examine how this feature works with the Solana account selection in the WalletConnect integration:
🏁 Script executed:
#!/bin/bash # Check how menuFitContent is used in WalletConnect components # Look for usage of menuFitContent in WalletConnect components rg "menuFitContent" --type ts -A 2 -B 2 "./packages/suite/src/components/suite/modals" rg "menuFitContent" --type ts -A 2 -B 2 "./suite-common/walletconnect"Length of output: 1006
Dynamic menu width styling implementation verified in WalletConnect integration
The conditional styling in
Select.tsxworks as expected. When$menuFitContentis true, the menu sizes itself based on its content. Our grep search inWalletConnectProposalModal.tsxconfirms that the WalletConnect integration correctly passes and uses this prop for displaying the Solana account selection menu. No changes are required.packages/blockchain-link/src/workers/solana/fee.ts (2)
4-4: Good adoption of theCompilableTransactionMessagetype.This helps simplify fee calculation by removing the need to call
decompileTransactionMessage.
59-64: Ensure robust handling of empty or undefined instructions.While this approach to retrieve “writable” accounts likely works in most scenarios, consider verifying that
decompiledMessage.instructionsis neither undefined nor empty before processing.suite-common/walletconnect/src/adapters/solana.ts (4)
1-3: Imports look correct.All imported packages are relevant and align with the project's stated dependencies.
136-141: Chain ID logic appears consistent.Mapping testnet/mainnet to distinct chain IDs is straightforward. Verify that these IDs match your supported network references within the app and external docs.
142-164: Efficiently constructing a Solana namespace object.The approach is concise and ensures each chain ID is included only once.
203-209: Adapter structure is well-defined.The
solanaAdapterneatly ties everything together, adhering to theWalletConnectAdapterinterface.suite-common/walletconnect/src/adapters/index.ts (3)
1-2: Imports look good.
No issues spotted; these imports are straightforward.
6-7: Adapter additions are correctly integrated.
The inclusion ofsolanaAdapteris consistent, and relevant types are correctly imported.
28-36: Solid modular design for processing namespaces.
Calls each adapter’sprocessNamespacesto build out the final results in a centralized manner. This is clear and maintainable.packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/WalletConnectProposalModal.tsx (12)
1-2: React and React-Select imports are valid.
No issues with the new imports or their usage here.
4-7: Imports from shared modules look consistent.
All references to suite-common and wallet-core types appear in order.
13-14: WalletConnect types imported successfully.
This aligns well with usage in the component.
15-25: UI component imports are correct.
No code smell found; these additions look good.
33-34: Account-related imports are relevant.
No issues with referencing the new components.
40-46: Newly introducedAccountGroupinterface is intuitive.
The design neatly groups accounts by device and metadata; no immediate concerns.
51-53: Storing selected accounts in local state.
Tracking selected accounts here is appropriate for the new selection UI.
56-56: PassingselectedDefaultAccountsto the thunk.
Well-handled approach for user-selected accounts; the logic is straightforward.
63-69: Single-account-per-symbol logic.
Filtering out previously selected accounts of the same symbol ensures only one account is retained per symbol. Verify this aligns with your desired UX when multiple accounts share the same symbol.
91-97: Default account selection effect.
The code synchronizes selected accounts with active networks as intended.
99-122: Comprehensive grouping function.
Groups accounts by device and account type. Good for a multi-device environment.
197-205: Solana-specific handling via Select component.
Smart approach to let the user pick a default account for Solana networks. ThemenuFitContentfeature is a nice enhancement for UI.Also applies to: 209-239, 242-247
suite-common/walletconnect/src/walletConnectThunks.ts (6)
13-13: Network import well-used for chain retrieval logic.
No additional notes; looks fine.
18-23: Centralized adapter imports.
Makes the code more readable and maintainable.
100-101: Namespace processing is cleanly delegated.
LeveragesprocessNamespacesto handle both required and optional. Good reuse of logic.
155-159: Expanded thunk signature for chosen default accounts.
Allows more granular control of the accounts used in the final session.
163-185: Ordering approach for priority accounts.
AppendingselectedDefaultAccountsfirst, then other accounts, meets user preference logic. Well done.
242-256: Chain ID retrieval from adapters.
Ensures correct chain-based events if thechainIdis available. This is a sensible approach for multi-network support.suite-common/walletconnect/src/adapters/ethereum.ts (8)
2-2: Import usage looks appropriate.
This is a straightforward typed import for WalletConnect proposals. No further issues.
5-5: Double-check network availability.
Ensure that all Ethereum networks innetworksCollectionhave validchainIdproperties so calls togetNetworksucceed.
8-8: Standard import, no concerns.
TheAccounttype import is appropriate and aligns with usage in this file.
14-18: Type imports for WalletConnect operations.
These type imports look fine and align with the new methods in this file.
28-28: Handle possible undefined return values.
This thunk might returnstringorundefined. Ensure that callers handle both cases gracefully.
193-193: Validate that the network has a chainId.
Returning\eip155:`is correct for Ethereum networks, but confirm all relevant networks definechainId`.
195-217: Namespace construction is logically sound.
This function correctly collects visible Ethereum accounts. Implementation is clear and consistent.
256-261: Final adapter shape is well-defined.
All the new functions and properties are properly exposed, ensuring consistent usage ofethereumAdapterthroughout the codebase.
| export const getNamespaces = (accounts: Account[]) => | ||
| adapters | ||
| .map(adapter => adapter.getNamespace(accounts)) | ||
| .reduce((acc, val) => ({ ...acc, ...val }), {}); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid repeated object spreading to reduce time complexity.
Object spreading ({ ...acc, ...val }) inside a .reduce() can be O(n^2). Consider using Object.assign or pushing properties directly into acc.
Below is an example diff:
-export const getNamespaces = (accounts: Account[]) =>
- adapters
- .map(adapter => adapter.getNamespace(accounts))
- .reduce((acc, val) => ({ ...acc, ...val }), {});
+export const getNamespaces = (accounts: Account[]) => {
+ return adapters
+ .map(adapter => adapter.getNamespace(accounts))
+ .reduce((acc, val) => {
+ Object.assign(acc, val);
+ return acc;
+ }, {});
+};📝 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.
| export const getNamespaces = (accounts: Account[]) => | |
| adapters | |
| .map(adapter => adapter.getNamespace(accounts)) | |
| .reduce((acc, val) => ({ ...acc, ...val }), {}); | |
| export const getNamespaces = (accounts: Account[]) => { | |
| return adapters | |
| .map(adapter => adapter.getNamespace(accounts)) | |
| .reduce((acc, val) => { | |
| Object.assign(acc, val); | |
| return acc; | |
| }, {}); | |
| }; |
🧰 Tools
🪛 Biome (1.9.4)
[error] 26-26: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
| const namespaces = Object.keys(oldNamespaces).reduce( | ||
| (acc, key) => ({ | ||
| ...acc, | ||
| [key]: { | ||
| ...oldNamespaces[key], | ||
| accounts: updatedNamespaces[key].accounts ?? [], | ||
| chains: updatedNamespaces[key].chains ?? [], | ||
| }, | ||
| }), | ||
| oldNamespaces, | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid repeated object spreads in .reduce() while updating namespaces.
Like in getNamespaces, repeatedly using { ...acc, ...oldNamespaces[key] } can lead to O(n^2) merges. Consider using Object.assign.
Example adjustment:
-const namespaces = Object.keys(oldNamespaces).reduce(
- (acc, key) => ({
- ...acc,
- [key]: {
- ...oldNamespaces[key],
- accounts: updatedNamespaces[key].accounts ?? [],
- chains: updatedNamespaces[key].chains ?? [],
- },
- }),
- oldNamespaces,
-);
+const namespaces = Object.keys(oldNamespaces).reduce((acc, key) => {
+ acc[key] = {
+ ...oldNamespaces[key],
+ accounts: updatedNamespaces[key].accounts ?? [],
+ chains: updatedNamespaces[key].chains ?? [],
+ };
+ return acc;
+}, { ...oldNamespaces });📝 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 namespaces = Object.keys(oldNamespaces).reduce( | |
| (acc, key) => ({ | |
| ...acc, | |
| [key]: { | |
| ...oldNamespaces[key], | |
| accounts: updatedNamespaces[key].accounts ?? [], | |
| chains: updatedNamespaces[key].chains ?? [], | |
| }, | |
| }), | |
| oldNamespaces, | |
| ); | |
| const namespaces = Object.keys(oldNamespaces).reduce((acc, key) => { | |
| acc[key] = { | |
| ...oldNamespaces[key], | |
| accounts: updatedNamespaces[key].accounts ?? [], | |
| chains: updatedNamespaces[key].chains ?? [], | |
| }; | |
| return acc; | |
| }, { ...oldNamespaces }); |
🧰 Tools
🪛 Biome (1.9.4)
[error] 279-279: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
martykan
added
the
build-desktop
Description
Solana support for WalletConnect. Since Solana adapters don't usually allow account switching, this PR also adds account selection to the pairing modal (only for Solana right now).
Only problem is we don't support sign message, since it's not in FW, but most dApps don't rely on it.
Screenshots: