-
Notifications
You must be signed in to change notification settings - Fork 38
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
Conversation
WalkthroughThe updates across various frontend components and services of a wallet extension indicate a shift from using Changes
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 ? TipsChat with CodeRabbit Bot (
|
There was a problem hiding this 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
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 theprovider
parameter and now directly uses theethereum
object. Ensure that this change is reflected wherevergetToken
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 theethereum.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 thesdk
object to connect and fetch accounts. Ensure thatsdk?.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 thesdk
object for account fetching. Ensure that thesdk?.connect()
method is used correctly and that the error handling is comprehensive.151-154: The
useEffect
hook dependency array now includessdk
. Confirm that this is the only dependency that should trigger theinitialize
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
andethers
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
withdebug
set tofalse
andsdkOptions
includingdappMetadata
withname
andurl
properties is consistent with the PR objectives to integrate the MetaMask SDK. Ensure that thename
andurl
provided indappMetadata
are accurate and meet the requirements for identifying the dApp within the MetaMask ecosystem.48-63: The introduction of
MetaMaskProvider
wrapping aroundWalletConnectionProvider
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 thedappMetadata
object'surl
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 aprovider
parameter and directly uses theethereum
object. Ensure that theethereum
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 ifethereum
is not present or if it's not MetaMask. This change assumes thatethereum
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 takeaccounts
andtoken
as parameters. Ensure that the removal of theprovider
parameter does not impact the ability to authenticate accounts with the Ten Gateway, as the function now relies on theaccountIsAuthenticated
API call.91-106: The
getAccounts
function has been refactored to remove theprovider
parameter and now directly checks for the presence of theethereum
object and the length of theaccounts
array. Ensure that the logic correctly handles scenarios whereethereum
is not present or accounts are not found.125-125: The
getAccounts
function now callsethService.formatAccounts
directly with theaccounts
andtoken
. Verify that thetoken
is always available and valid when this function is called, as it is critical for theformatAccounts
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 ofprovider
and the addition ofsdk
are consistent with the rest of the application's codebase and that all necessary functionality previously relying onprovider
is now correctly usingsdk
.Verification successful
The verification process has revealed that:
- The
provider
removed fromuseWalletConnection
in thewalletextension
frontend does not seem to affect other parts of the application, asprovider
is still used in theobscuroscan_v3
frontend independently.- The
sdk
fromuseSDK
is being utilized in thewalletextension
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 asconnectToTenTestnet
, have been negatively impacted by the removal ofprovider
.Based on this information, the changes in
useGatewayService.ts
are consistent with the PR's objectives and do not seem to introduce issues within thewalletextension
frontend.* 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.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
Verification successful
The
isMetamaskConnected
function usessdk.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 ofToastType.DESTRUCTIVE
indicates the severity of the issue appropriately.* 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.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.tsLength of output: 616
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 fromWalletConnectionContextType
aligns with the PR's goal to integrate the MetaMask SDK. Ensure that all references to theprovider
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 theprovider
property within theWalletConnectionContextType
interface. Additionally, I will userg
to search specifically within thewalletextension
directory for any usage of theprovider
property that might still exist in the codebase and could be affected by its removal from theWalletConnectionContextType
interface.
I will generate a script to check the contents of the TypeScript files that import
useWalletConnection
fromwallet-provider
to verify if they are using theprovider
property.
Based on the results, it appears that the removal of the
provider
property from theWalletConnectionContextType
interface has been handled correctly in thewalletextension
tool. There are no indications of theprovider
property being used in the components that importuseWalletConnection
.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 tsLength 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 10Length of output: 11394
tools/walletextension/frontend/src/components/modules/home/connected.tsx
Outdated
Show resolved
Hide resolved
93f2550
to
bc21f9b
Compare
There was a problem hiding this 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
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 theuseWalletConnection
hook is a good example of how to surface dynamic information in the UI. Ensure that theversion
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 theprovider
has been removed from the destructuring assignment in favor of using the MetaMask SDK.54-54:
Verify whether thegetToken
function requires aprovider
orsdk
object to function correctly, as it is being called without any arguments. If it does require one, the call togetToken
needs to be updated accordingly.
tools/walletextension/frontend/src/components/providers/wallet-provider.tsx
Show resolved
Hide resolved
tools/walletextension/frontend/src/components/providers/wallet-provider.tsx
Show resolved
Hide resolved
tools/walletextension/frontend/src/components/providers/wallet-provider.tsx
Show resolved
Hide resolved
tools/walletextension/frontend/src/components/providers/wallet-provider.tsx
Show resolved
Hide resolved
tools/walletextension/frontend/src/components/providers/wallet-provider.tsx
Show resolved
Hide resolved
@coderabbitai review |
There was a problem hiding this 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
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
PR checks pre-merging
Please indicate below by ticking the checkbox that you have read and performed the required
PR checks