Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Gateway] enable metamask desktop and mobile support #1697

Merged
merged 8 commits into from
Dec 15, 2023

Conversation

Jennievon
Copy link
Contributor

@Jennievon Jennievon commented Dec 13, 2023

Why this change is needed

Please provide a description and a link to the underlying ticket

https://github.com/ten-protocol/ten-internal/issues/2598

What changes were made as part of this PR

Please provide a high level list of the changes made

  • enable support for metamask desktop and mobile support

PR checks pre-merging

Please indicate below by ticking the checkbox that you have read and performed the required
PR checks

  • PR checks reviewed and performed

@Jennievon Jennievon requested review from zkokelj and 0xjba December 13, 2023 23:28
@Jennievon Jennievon self-assigned this Dec 13, 2023
Copy link

coderabbitai bot commented Dec 13, 2023

Walkthrough

The updates across various frontend components and services of a wallet extension indicate a shift from using ethers directly to integrating with the Metamask SDK. The changes involve removing the provider argument and reliance on the ethereum object, instead opting for the SDK's useSDK hook for connecting to Metamask. This refactoring simplifies the connection process and account management. Additionally, the global styles have been tweaked, and new color variables have been introduced.

Changes

File(s) Change Summary
.../globals.css Updated font-weight for "CloudSoft" and added new color variables.
.../ethRequests.ts, .../services/ethService.ts, .../types/interfaces/WalletInterfaces.ts Removed ethers import and usage, refactored to remove provider argument.
.../home/connected.tsx Restructured conditional rendering and refactored account mapping logic.
.../providers/wallet-provider.tsx, .../pages/_app.tsx Integrated @metamask/sdk-react, replaced direct ethereum usage with useSDK, and updated Metamask connection logic.
.../useGatewayService.ts Updated import formatting, removed provider, added sdk, and refactored Metamask connection checks.

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 X ?


Tips

Chat with CodeRabbit Bot (@coderabbitai)

  • You can reply to a review comment made by CodeRabbit.
  • You can tag CodeRabbit on specific lines of code or files in the PR by tagging @coderabbitai in a comment.
  • You can tag @coderabbitai in a PR comment and ask one-off questions about the PR and the codebase. Use quoted replies to pass the context for follow-up questions.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • The JSON schema for the configuration file is available here.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

@Jennievon Jennievon changed the title refactor: use metamask sdk refactor: use metamask sdk - desktop and mobile support Dec 13, 2023
@Jennievon Jennievon changed the title refactor: use metamask sdk - desktop and mobile support Use metamask sdk - desktop and mobile support Dec 13, 2023
@Jennievon Jennievon changed the title Use metamask sdk - desktop and mobile support [Gateway] Use metamask sdk - desktop and mobile support Dec 13, 2023
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Status

Actionable comments generated: 7

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 7c2a4df and 93f2550.
Files ignored due to filter (2)
  • tools/walletextension/frontend/package-lock.json
  • tools/walletextension/frontend/package.json
Files selected for processing (8)
  • tools/obscuroscan_v3/frontend/styles/globals.css (5 hunks)
  • tools/walletextension/frontend/src/api/ethRequests.ts (2 hunks)
  • tools/walletextension/frontend/src/components/modules/home/connected.tsx (2 hunks)
  • tools/walletextension/frontend/src/components/providers/wallet-provider.tsx (6 hunks)
  • tools/walletextension/frontend/src/pages/_app.tsx (2 hunks)
  • tools/walletextension/frontend/src/services/ethService.ts (6 hunks)
  • tools/walletextension/frontend/src/services/useGatewayService.ts (1 hunks)
  • tools/walletextension/frontend/src/types/interfaces/WalletInterfaces.ts (2 hunks)
Additional comments: 20
tools/walletextension/frontend/src/api/ethRequests.ts (2)
  • 84-97: The getToken function has been refactored to remove the provider parameter and now directly uses the ethereum object. Ensure that this change is reflected wherever getToken is called, and that the error handling is consistent with other functions in the module.

  • 91-93: The use of getRandomIntAsString to generate a random number for the ethereum.request call introduces non-determinism. Verify if this behavior is intentional and whether it could have any unintended side effects.

tools/walletextension/frontend/src/components/providers/wallet-provider.tsx (6)
  • 19-19: The introduction of useSDK from @metamask/sdk-react is consistent with the PR's objective to integrate the MetaMask SDK.

  • 43-59: The initialize function now uses the sdk object to connect and fetch accounts. Ensure that sdk?.connect() is the correct method and that it's being used as intended by the SDK's API. Also, verify that the error handling is robust and that all possible exceptions from the SDK are being caught.

  • 121-131: The fetchUserAccounts function has been updated to use the sdk object for account fetching. Ensure that the sdk?.connect() method is used correctly and that the error handling is comprehensive.

  • 151-154: The useEffect hook dependency array now includes sdk. Confirm that this is the only dependency that should trigger the initialize function and that no other dependencies are missing.

  • 161-166: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [156-166]

The walletConnectionContextValue object is being returned with the same properties as before. Confirm that all properties are still relevant and that no additional properties are needed due to the integration of the MetaMask SDK.

  • 5-6: The AI-generated summary mentions the removal of ethereum and ethers imports, but these are not visible in the provided hunks. Since the hunks do not display removed lines, we cannot review these changes directly. However, it's important to ensure that the removal of these imports does not affect other parts of the codebase that may rely on them.
tools/walletextension/frontend/src/pages/_app.tsx (3)
  • 48-63: The configuration of MetaMaskProvider with debug set to false and sdkOptions including dappMetadata with name and url properties is consistent with the PR objectives to integrate the MetaMask SDK. Ensure that the name and url provided in dappMetadata are accurate and meet the requirements for identifying the dApp within the MetaMask ecosystem.

  • 48-63: The introduction of MetaMaskProvider wrapping around WalletConnectionProvider and its children is a significant structural change. It is crucial to perform thorough testing to ensure that this new structure integrates seamlessly with the existing application and does not introduce any regressions or unexpected behavior, especially considering the application's state management and lifecycle events.

  • 9-9: The variable tenGatewayAddress is imported but not used within the provided hunk. Verify if it's used elsewhere in the file or consider removing it if it's an unnecessary import.

Verification successful

The variable tenGatewayAddress is indeed used within the _app.tsx file, specifically as part of the dappMetadata object's url property. This confirms that the import is necessary for the current implementation.

Scripts Executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Search for usage of `tenGatewayAddress` within the `_app.tsx` file.
rg 'tenGatewayAddress' -- 'tools/walletextension/frontend/src/pages/_app.tsx'

Length of output: 187

tools/walletextension/frontend/src/services/ethService.ts (5)
  • 13-16: The function checkIfMetamaskIsLoaded has been refactored to no longer require a provider parameter and directly uses the ethereum object. Ensure that the ethereum object is always available when this function is called, as it now relies on it being present in the global scope.

  • 47-50: The handleEthereum function now exits early if ethereum is not present or if it's not MetaMask. This change assumes that ethereum is globally available and is MetaMask. Verify that this assumption holds true across all environments where the application runs.

  • 75-78: The formatAccounts function has been simplified to only take accounts and token as parameters. Ensure that the removal of the provider parameter does not impact the ability to authenticate accounts with the Ten Gateway, as the function now relies on the accountIsAuthenticated API call.

  • 91-106: The getAccounts function has been refactored to remove the provider parameter and now directly checks for the presence of the ethereum object and the length of the accounts array. Ensure that the logic correctly handles scenarios where ethereum is not present or accounts are not found.

  • 125-125: The getAccounts function now calls ethService.formatAccounts directly with the accounts and token. Verify that the token is always available and valid when this function is called, as it is critical for the formatAccounts function to work correctly.

tools/walletextension/frontend/src/services/useGatewayService.ts (3)
  • 5-20: The refactoring of import statements and the integration of useSDK from @metamask/sdk-react align with the PR objectives to integrate the MetaMask SDK. Ensure that the removal of provider and the addition of sdk are consistent with the rest of the application's codebase and that all necessary functionality previously relying on provider is now correctly using sdk.
Verification successful

The verification process has revealed that:

  • The provider removed from useWalletConnection in the walletextension frontend does not seem to affect other parts of the application, as provider is still used in the obscuroscan_v3 frontend independently.
  • The sdk from useSDK is being utilized in the walletextension frontend, with appropriate checks and error handling in place.
  • The changes appear to be localized to the walletextension frontend, and there is no indication that other functions, such as connectToTenTestnet, have been negatively impacted by the removal of provider.

Based on this information, the changes in useGatewayService.ts are consistent with the PR's objectives and do not seem to introduce issues within the walletextension frontend.

Scripts Executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Verify that no other parts of the application are using the removed `provider` from `useWalletConnection`.
rg 'useWalletConnection\(\)' --files-with-matches | xargs rg 'provider'
# Ensure that `sdk` is being used correctly wherever `useSDK` is imported.
rg 'useSDK\(\)' --files-with-matches | xargs rg 'sdk'

Length of output: 2621

* 22-31: The `isMetamaskConnected` function now uses `sdk.connect()` to check for connected accounts. Verify that the `sdk.connect()` method is correctly implemented and that the error handling is adequate. Additionally, ensure that the error message is informative and that the error is logged or handled appropriately.
Verification successful

The isMetamaskConnected function uses sdk.connect() to check for connected accounts, and the error handling around this method is consistent and informative. Errors are communicated to the user through toast notifications and are re-thrown for potential further handling. The error messages are clear, and the use of ToastType.DESTRUCTIVE indicates the severity of the issue appropriately.

Scripts Executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Verify that `sdk.connect()` is implemented correctly across the codebase.
rg 'sdk.connect\(\)' --files-with-matches
# Check for consistent error handling where `sdk.connect()` is used.
rg 'sdk.connect\(\)' --files-with-matches | xargs rg 'catch'

Length of output: 218



Script:

#!/bin/bash
# Extract the full context of the catch blocks where `sdk.connect()` is used.
rg -C 5 'catch' --files-with-matches | xargs rg 'sdk.connect\(\)'

Length of output: 174



Script:

#!/bin/bash
# Extract the surrounding lines of the `catch` blocks in the `useGatewayService.ts` file to verify the error handling.
rg -C 5 'catch' tools/walletextension/frontend/src/services/useGatewayService.ts

Length of output: 616

* 18-31: Given the removal of `provider`, confirm that none of the functions called within `connectToTenTestnet` rely on `provider` being passed down from `useGatewayService`. This is crucial to ensure that the functionality remains intact after the refactor.
tools/walletextension/frontend/src/types/interfaces/WalletInterfaces.ts (1)
  • 6-11: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [1-11]

The removal of the provider property from WalletConnectionContextType aligns with the PR's goal to integrate the MetaMask SDK. Ensure that all references to the provider property in the codebase have been updated or removed to prevent runtime errors.

Verification successful

I will now correct the syntax error in the ast-grep pattern and rerun the command to search for references to the provider property within the WalletConnectionContextType interface. Additionally, I will use rg to search specifically within the walletextension directory for any usage of the provider property that might still exist in the codebase and could be affected by its removal from the WalletConnectionContextType interface.


I will generate a script to check the contents of the TypeScript files that import useWalletConnection from wallet-provider to verify if they are using the provider property.


Based on the results, it appears that the removal of the provider property from the WalletConnectionContextType interface has been handled correctly in the walletextension tool. There are no indications of the provider property being used in the components that import useWalletConnection.

Scripts Executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Search for references to the `provider` property within the `WalletConnectionContextType` interface.
ast-grep --lang typescript --pattern $'WalletConnectionContextType { $$$ provider: $_ $$$ }'
# Search for any usage of the `provider` property that might still exist in the codebase.
rg 'provider' --type ts

Length of output: 3657



Script:

#!/bin/bash
# Correct the syntax error in the ast-grep pattern and search for references to the `provider` property within the `WalletConnectionContextType` interface.
ast-grep --lang typescript --pattern $'interface WalletConnectionContextType { $$$ provider: $_ $$$ }'

# Search specifically within the `walletextension` directory for any usage of the `provider` property.
rg 'provider' --type ts --glob 'tools/walletextension/**'

Length of output: 7223



Script:

#!/bin/bash
# Check the contents of the TypeScript files that import `useWalletConnection` from `wallet-provider`.
rg 'useWalletConnection' --type ts --glob 'tools/walletextension/**' -A 10

Length of output: 11394

tools/obscuroscan_v3/frontend/styles/globals.css Outdated Show resolved Hide resolved
tools/walletextension/frontend/src/api/ethRequests.ts Outdated Show resolved Hide resolved
tools/obscuroscan_v3/frontend/styles/globals.css Outdated Show resolved Hide resolved
tools/walletextension/frontend/src/api/ethRequests.ts Outdated Show resolved Hide resolved
tools/obscuroscan_v3/frontend/styles/globals.css Outdated Show resolved Hide resolved
tools/obscuroscan_v3/frontend/styles/globals.css Outdated Show resolved Hide resolved
@Jennievon Jennievon force-pushed the jennifer/2598-testnet-gateway-mobile-support branch from 93f2550 to bc21f9b Compare December 15, 2023 00:25
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Status

Actionable comments generated: 13

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 6605017 and bc21f9b.
Files selected for processing (6)
  • tools/walletextension/frontend/src/api/ethRequests.ts (2 hunks)
  • tools/walletextension/frontend/src/components/layouts/footer.tsx (2 hunks)
  • tools/walletextension/frontend/src/components/modules/home/connected.tsx (1 hunks)
  • tools/walletextension/frontend/src/components/providers/wallet-provider.tsx (4 hunks)
  • tools/walletextension/frontend/src/services/ethService.ts (2 hunks)
  • tools/walletextension/frontend/src/services/useGatewayService.ts (2 hunks)
Files skipped from review as they are similar to previous changes (2)
  • tools/walletextension/frontend/src/components/modules/home/connected.tsx
  • tools/walletextension/frontend/src/services/ethService.ts
Additional comments: 4
tools/walletextension/frontend/src/components/layouts/footer.tsx (2)
  • 14-16: The refactoring of the Footer component's layout and styling appears to be consistent and well-structured, with the classes indicating a more responsive design.

  • 39-42: The display of the version fetched from the useWalletConnection hook is a good example of how to surface dynamic information in the UI. Ensure that the version variable is always populated correctly, especially after the refactor.

tools/walletextension/frontend/src/services/useGatewayService.ts (2)
  • 5-16:
    The import statements have been reformatted for better readability, and the provider has been removed from the destructuring assignment in favor of using the MetaMask SDK.

  • 54-54:
    Verify whether the getToken function requires a provider or sdk object to function correctly, as it is being called without any arguments. If it does require one, the call to getToken needs to be updated accordingly.

@Jennievon Jennievon changed the title [Gateway] Use metamask sdk - desktop and mobile support [Gateway] enable metamask desktop and mobile support Dec 15, 2023
@Jennievon
Copy link
Contributor Author

@coderabbitai review

Copy link
Contributor

@zkokelj zkokelj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍 + tested on dev-testnet

@Jennievon Jennievon merged commit 5c4e75d into main Dec 15, 2023
3 of 4 checks passed
@Jennievon Jennievon deleted the jennifer/2598-testnet-gateway-mobile-support branch December 15, 2023 07:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants