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] fix: loading screen and connect accounts on reload #1703

Merged
merged 2 commits into from
Dec 15, 2023

Conversation

Jennievon
Copy link
Contributor

Why this change is needed

Please provide a description and a link to the underlying ticket

What changes were made as part of this PR

Please provide a high level list of the changes made

  • no MM loading screen
  • fetch connected accounts on reload

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

- no MM loading screen
- fetch connected accounts on reload
@Jennievon Jennievon requested a review from zkokelj December 15, 2023 09:34
@Jennievon Jennievon self-assigned this Dec 15, 2023
Copy link

coderabbitai bot commented Dec 15, 2023

Walkthrough

The update to the wallet provider involves modifying the initialize function to accept a providerInstance parameter, which is used to reference the Ethereum provider throughout the function. The update also includes changes to the event listener callback for ethereum.on, where initialize is invoked with the new providerInstance argument.

Changes

File Path Change Summary
.../wallet-provider.tsx Updated initialize function to accept providerInstance argument and use it within the function. Modified ethereum.on callback to call initialize with providerInstance.

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

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: 1

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 5c4e75d and 9ace858.
Files selected for processing (1)
  • tools/walletextension/frontend/src/components/providers/wallet-provider.tsx (2 hunks)
Additional comments: 2
tools/walletextension/frontend/src/components/providers/wallet-provider.tsx (2)
  • 46-68: The update to the initialize function to accept a providerInstance parameter is a good change for explicit dependency injection, which can make the function easier to test and maintain. Ensure that all calls to initialize throughout the codebase have been updated to include the new providerInstance parameter.

  • 187-198: The ethereum.on event listener for accountsChanged calls fetchUserAccounts, which may need to be updated to use the new providerInstance if it previously relied on the old provider state. Verify that fetchUserAccounts is correctly using the new providerInstance and not the old provider state.

Verification successful

The fetchUserAccounts function uses the provider from the useWalletConnection context, which is updated to the new providerInstance within the useEffect hook in wallet-provider.tsx. This confirms that fetchUserAccounts indirectly uses the new providerInstance, aligning with the changes made in the pull request.

Scripts Executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Search for `fetchUserAccounts` function definition to ensure it uses the new `providerInstance`.
ast-grep --lang typescript --pattern $'fetchUserAccounts($_)'

Length of output: 62



Script:

#!/bin/bash
# Search for the `fetchUserAccounts` function definition in a broader scope without specifying arguments.
ast-grep --lang typescript --pattern $'fetchUserAccounts($$$)'

Length of output: 166



Script:

#!/bin/bash
# Extract the content of the file where `fetchUserAccounts` is being called to analyze the function definition.
rg 'fetchUserAccounts' ./tools/walletextension/frontend/src/services/useGatewayService.ts

Length of output: 183



Script:

#!/bin/bash
# Search for the definition of `fetchUserAccounts` within the file `useGatewayService.ts`.
ast-grep --lang typescript --pattern $'fetchUserAccounts($$$)' --file-pattern 'useGatewayService.ts'

Length of output: 295



Script:

#!/bin/bash
# Search for the definition of `fetchUserAccounts` within the file `useGatewayService.ts` without using the unsupported argument.
ast-grep --lang typescript --pattern $'fetchUserAccounts($$$)'

Length of output: 166



Script:

#!/bin/bash
# Use `rg` to extract the function definition of `fetchUserAccounts` from `useGatewayService.ts`.
rg 'function fetchUserAccounts' ./tools/walletextension/frontend/src/services/useGatewayService.ts -A 10

Length of output: 106



Script:

#!/bin/bash
# Use `rg` to extract the broader function definition of `fetchUserAccounts` from `useGatewayService.ts`.
rg 'fetchUserAccounts' ./tools/walletextension/frontend/src/services/useGatewayService.ts -A 10 -B 10

Length of output: 1226



Script:

#!/bin/bash
# Search for the definition of `useWalletConnection` to find where `fetchUserAccounts` is provided from.
rg 'useWalletConnection' -A 10 -B 10

Length of output: 25322



Script:

#!/bin/bash
# Use `rg` to extract the implementation of `fetchUserAccounts` from `wallet-provider.tsx`.
rg 'fetchUserAccounts' ./tools/walletextension/frontend/src/components/providers/wallet-provider.tsx -A 10 -B 10

Length of output: 1621

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: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 9ace858 and ae24f51.
Files selected for processing (1)
  • tools/walletextension/frontend/src/components/providers/wallet-provider.tsx (2 hunks)
Files skipped from review as they are similar to previous changes (1)
  • tools/walletextension/frontend/src/components/providers/wallet-provider.tsx

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

@Jennievon Jennievon merged commit b528fcb into main Dec 15, 2023
2 of 3 checks passed
@Jennievon Jennievon deleted the jennifer/fix-non-mm-screen branch December 15, 2023 11:10
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