feat: add imToken wallet #3820
feat: add imToken wallet #3820
Conversation
|
@xwartz is attempting to deploy a commit to the OsmoLabs Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe pull request introduces several changes across multiple files to enhance wallet functionality within the application. Key updates include the addition of Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant WalletSelection
participant WalletRegistry
User->>WalletSelection: Initiate wallet selection
WalletSelection->>WalletRegistry: Retrieve available wallets
WalletRegistry-->>WalletSelection: Return wallet list
WalletSelection->>User: Display wallet options
User->>WalletSelection: Select imToken wallet
WalletSelection->>WalletRegistry: Check if imToken supports chain
WalletRegistry-->>WalletSelection: Return support status
WalletSelection->>User: Confirm selection
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
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
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: 1
Outside diff range and nitpick comments (6)
packages/web/config/generate-cosmos-kit-wallet-list.ts (1)
42-42: LGTM: imTokenWalletInfo added to CosmosKitWalletList.The
imTokenWalletInfohas been correctly added to theCosmosKitWalletListarray, consistent with how other wallets are included. This change directly contributes to the PR objective of integrating the imToken wallet into the Osmosis Zone web interface.Consider the order of wallets in the list, as it might affect the presentation to users. You may want to discuss with the team if there's a preferred order for displaying wallet options.
packages/web/config/wallet-registry.ts (3)
165-166: LGTM: Type annotations added and formatting improvedThe changes improve type safety by adding type annotations for
chainId,retryCount, and thegetKeymethod. The formatting changes enhance readability. The logic remains unchanged, which is good.Consider using a more descriptive name for the
okxWallet.keplrproperty, as it might be confusing to see a Keplr-related property in the OKX wallet implementation. If this is intentional due to some compatibility layer, consider adding a comment explaining this.Also applies to: 169-170, 175-176, 178-178, 185-185, 202-202, 204-204, 207-207, 211-211, 218-218, 221-221
284-285: LGTM: Type annotations added and formatting improvedThe changes improve type safety by adding type annotations for
chainIdand thegetKeymethod. The formatting changes enhance readability. The logic remains unchanged, which is good.Similar to the OKX wallet, consider using a more descriptive name for the
xfiWallet.keplrproperty, as it might be confusing to see a Keplr-related property in the XDEFI wallet implementation. If this is intentional due to some compatibility layer, consider adding a comment explaining this.Also applies to: 288-289, 291-291, 296-296
321-322: LGTM: Type annotations added and logic slightly improvedThe changes improve type safety by adding type annotations for
chainIdand thegetChainInfosWithoutEndpointsmethod. The logic has been slightly improved by usingsomeinstead offind, which is more efficient for this use case.Consider using a more descriptive name for the
stationWallet.keplrproperty, as it might be confusing to see a Keplr-related property in the Station wallet implementation. If this is intentional due to some compatibility layer, consider adding a comment explaining this.Also applies to: 325-326, 328-328, 330-331
packages/web/modals/wallet-select/use-selectable-wallets.ts (2)
85-85: Consider renaming_windowto avoid confusionUsing the variable name
_windowmight cause confusion, as it is similar to the globalwindowobject. To improve code clarity, consider renaming it to something more descriptive.Suggested change:
- const _window = window as Record<string, any> + const windowRef = window as Record<string, any>
105-109: Enhance clarity of the code commentThe comment could be rephrased for better readability and to accurately convey the intended meaning.
Suggested rephrasing:
/** - * If on mobile and `imToken` is in `window`, it means that the user enters - * the frontend from imToken's app in app browser. So, there is no need - * to use wallet connect, as it resembles the extension's usage. + * When `imToken` is present in `window` on a mobile device, it indicates that + * the user is accessing the frontend from the imToken in-app browser. + * Therefore, there is no need to use WalletConnect, as it functions like an extension. */
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (3)
packages/web/package.jsonis excluded by!**/*.jsonpackages/web/public/wallets/imtoken.svgis excluded by!**/*.svg,!**/*.svgyarn.lockis excluded by!**/yarn.lock,!**/*.lock,!**/*.lock
Files selected for processing (3)
- packages/web/config/generate-cosmos-kit-wallet-list.ts (3 hunks)
- packages/web/config/wallet-registry.ts (9 hunks)
- packages/web/modals/wallet-select/use-selectable-wallets.ts (4 hunks)
Additional context used
Biome
packages/web/modals/wallet-select/use-selectable-wallets.ts
[error] 148-148: 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 ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
Additional comments not posted (8)
packages/web/config/generate-cosmos-kit-wallet-list.ts (2)
8-8: LGTM: Import statement for imTokenWalletInfo added correctly.The import statement for
imTokenWalletInfofrom@cosmos-kit/imtoken-extensionhas been added correctly, following the same pattern as other wallet imports in the file. This addition aligns with the PR objective of introducing support for the imToken wallet.
Line range hint
1-141: Summary: imToken wallet support successfully added.The changes in this file successfully introduce support for the imToken wallet by importing the necessary information and adding it to the
CosmosKitWalletList. These modifications are consistent with the existing code structure and align well with the PR objectives.The implementation:
- Imports the
imTokenWalletInfofrom the appropriate package.- Adds
imTokenWalletInfoto theCosmosKitWalletListarray.These changes will allow the imToken wallet to be included in the generated TypeScript file containing approved wallets for Cosmos Kit, thereby enabling users to connect their imToken wallets to the Osmosis Zone web interface.
packages/web/config/wallet-registry.ts (5)
29-29: LGTM: Type annotation added and coding style improvedThe changes improve type safety by adding a type annotation for
chainIdand maintain consistency in coding style by removing the semicolon. The logic remains unchanged, which is good.Also applies to: 59-59
81-81: LGTM: Type annotation added and coding style improvedSimilar to the previous change, this update adds a type annotation for
chainIdand removes the semicolon, improving type safety and maintaining consistent coding style. The logic remains intact.Also applies to: 150-150
265-265: LGTM: Type annotation added and coding style improvedThe changes improve type safety by adding a type annotation for
chainIdand maintain consistency in coding style by removing the semicolon. The logic remains unchanged, which is good.Also applies to: 270-271
Line range hint
1-351: Overall assessment: Good improvements with minor suggestionsThe changes in this file significantly improve type safety and consistency across various wallet configurations. The addition of the IMTokenWallet aligns with the PR objectives to introduce support for the imToken wallet.
While the changes are generally good, there are a few minor suggestions for improvements:
- Consider adding explanatory comments for Keplr-related properties in non-Keplr wallet implementations.
- Ensure the IMTokenWallet configuration is complete and consistent with other wallet configurations.
- Verify the imToken integration using the provided script.
These minor adjustments will further enhance the code quality and maintainability.
338-350: LGTM: IMTokenWallet support addedThe addition of
IMTokenWalletconfiguration aligns with the PR objectives and follows the structure of other wallet configurations in the registry. The implementation includes necessary properties and methods, including asupportsChainmethod with proper type annotations.
- Consider renaming the
cdcAvailableChainsvariable toimTokenAvailableChainsfor clarity.- The
mobileDisabledproperty is set tofalse, which differs from some other wallets. Ensure this is intentional and aligns with imToken's mobile support.- Consider adding
stakeUrlandgovernanceUrlproperties if imToken supports these features, to maintain consistency with other wallet configurations.To ensure the imToken wallet integration is complete, please run the following verification script:
Verification successful
Verified: IMTokenWallet integration confirmed
All verification steps have successfully validated the
IMTokenWalletconfiguration:
- Import Statement:
IMTokenWalletis properly imported from@cosmos-kit/imtoken-extension.- Logo File:
/wallets/imtoken.svgexists and is correctly referenced.- Wallet Registration:
IMTokenWalletis included in the wallet lists and selectors.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify imToken wallet integration # Test 1: Check if imToken wallet is properly imported rg --type typescript 'import.*IMTokenWallet.*from.*@cosmos-kit/imtoken-extension' # Test 2: Verify imToken logo file exists test -f "$(fd --type file 'imtoken.svg' | head -n 1)" && echo "imToken logo found" || echo "imToken logo not found" # Test 3: Check if imToken wallet is added to any wallet lists or selectors rg --type typescript 'imtoken|IMTokenWallet'Length of output: 340
Script:
#!/bin/bash # Description: Re-verify imToken wallet integration without using unsupported --type flag # Test 1: Check if imToken wallet is properly imported rg --glob '*.ts' --glob '*.tsx' 'import.*IMTokenWallet.*from.*@cosmos-kit/imtoken-extension' # Test 3: Check if imToken wallet is added to any wallet lists or selectors rg --glob '*.ts' --glob '*.tsx' 'imtoken|IMTokenWallet'Length of output: 549
packages/web/modals/wallet-select/use-selectable-wallets.ts (1)
111-112: Incorrect property access inifconditionIn the condition checking for
imToken, the property access might be incorrect. You're accessing_window?.cosmos?.mode, but it should likely be_window?.imToken?.cosmos?.modeto correctly check thecosmos.modewithin theimTokenobject.Apply this diff to correct the property access:
if ( _window?.imToken && - _window?.cosmos?.mode === mobileWebModeName + _window?.imToken?.cosmos?.mode === mobileWebModeName )Likely invalid or redundant comment.
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@xwartz looks like we have some linting errors preventing the build from passing. Can you fix, rebase, and repush a passing build? Then we can get it reviewed |
The build should now pass successfully. I would appreciate it if you could take a moment to review the changes when you have time. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
packages/web/modals/wallet-select/use-selectable-wallets.ts (1)
105-118: LGTM: Successfully added imToken wallet supportThe new conditional block effectively adds support for the imToken wallet in mobile web mode, aligning perfectly with the PR objectives. The implementation follows the established pattern for other mobile wallets, ensuring consistency in the codebase.
For even better consistency, consider extracting the repeated pattern of filtering and mapping wallets into a separate function. This would reduce code duplication across different wallet checks.
Here's a suggested refactoring:
const filterAndMapWallet = (walletName: string) => array .filter((wallet) => wallet.name === walletName) .map((wallet) => ({ ...wallet, mobileDisabled: false })); // Then use it like this: if (_window?.imToken && _window?.cosmos?.mode === mobileWebModeName) { return filterAndMapWallet(AvailableCosmosWallets.imToken); }This refactoring can be applied to other similar blocks in the function as well.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (3)
packages/web/package.jsonis excluded by!**/*.jsonpackages/web/public/wallets/imtoken.svgis excluded by!**/*.svg,!**/*.svgyarn.lockis excluded by!**/yarn.lock,!**/*.lock,!**/*.lock
📒 Files selected for processing (3)
- packages/web/config/generate-cosmos-kit-wallet-list.ts (3 hunks)
- packages/web/config/wallet-registry.ts (9 hunks)
- packages/web/modals/wallet-select/use-selectable-wallets.ts (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/web/config/generate-cosmos-kit-wallet-list.ts
- packages/web/config/wallet-registry.ts
🧰 Additional context used
🪛 Biome
packages/web/modals/wallet-select/use-selectable-wallets.ts
[error] 148-148: 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 ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
🔇 Additional comments (2)
packages/web/modals/wallet-select/use-selectable-wallets.ts (2)
82-86: LGTM: Improved code readabilityThe removal of the semicolon and the introduction of new variables
_windowandmobileWebModeNameenhance code readability and maintainability. These changes align well with the PR objectives of improving wallet selection for mobile users.
129-129: LGTM: Minor style improvementsThe removal of semicolons after the closing parentheses on lines 129 and 142 is a minor style improvement that enhances code consistency.
Also applies to: 142-142
| } | ||
|
|
||
| /** | ||
| * If user is in a normal mobile browser, show only wallet connect | ||
| */ | ||
| return wallet.name.endsWith("mobile") ? [...acc, wallet] : acc; | ||
| return wallet.name.endsWith("mobile") ? [...acc, wallet] : acc |
There was a problem hiding this comment.
Performance: Avoid spread syntax in .reduce
The use of spread syntax within the .reduce accumulator can lead to O(n^2) time complexity, which may negatively impact performance, especially for larger arrays. This issue was pointed out in a previous review and should be addressed.
Consider refactoring the code as follows to improve performance:
return wallet.name.endsWith("mobile") ? (acc.push(wallet), acc) : acc;This change uses the comma operator to push the wallet to the accumulator and return it in a single expression, maintaining the concise nature of the original code while avoiding the spread syntax.
🧰 Tools
🪛 Biome
[error] 148-148: 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 ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
|
@xwartz Looks like we are getting a type error on the build |
The code has been updated, could you please take a moment to review the changes? |
|
@xwartz can you please share the link of desktop extension. |
I would like to clarify that imToken is primarily a mobile application and currently does not offer a desktop extension or version. Users can connect their imToken wallet via the mobile app, which supports various functionalities for interacting with dApps. |
|
@xwartz just to confirm this is the correct link im mobile app? |
yes |
|
@xwartz not seeing imToken wallet option. |
Hey, could you please share the URL where you are trying to see the imToken wallet option? This will help me understand the context better. |
|
I downloaded the imToken wallet and used the in app browser and used this PR preview link. |
I apologize for the confusion. Currently, the version that supports the Cosmos provider (v2.16.0) has not yet been released and is still in the testing phase. Once it is officially launched, the imToken wallet option will be available. Thank you for your understanding, and please let me know if you have any other questions! imToken.mp4 |
|
@xwartz sounds like we should await it's launch, so that we can test before merging? |
The changes have already been deployed to production and are ready for testing. You can proceed with the testing now. Let me know if you need any assistance or if you find any issues that need to be addressed. |
What is the purpose of the change:
This PR introduces support for the imToken wallet in the Osmosis Zone web interface, allowing users to connect their imToken wallets and interact with the Osmosis decentralized exchange (DEX) and other applications built on the Osmosis blockchain.
By integrating imToken wallet support, we aim to:
Linear Task
N/A
Brief Changelog
Testing and Verifying
This change has been tested locally by rebuilding the website and verified content and links are expected