Skip to content

feat: conditional qr modal ep #5799

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

Open
wants to merge 7 commits into
base: v2.0
Choose a base branch
from

Conversation

ganchoradkov
Copy link
Member

@ganchoradkov ganchoradkov commented Mar 24, 2025

Description

Added new util method to ethereum-provider, updateConfigOptions which lets devs update the provider configuration after its initialization. This allows for conditional QR modal behaviour where devs might choose to enable the modal even if they had disabled it during init. This wasn't possible before and required complex work-arounds such as running duplicate provider instances - one with modal enabled and one with the modal disabled

Type of change

  • Chore (non-breaking change that addresses non-functional tasks, maintenance, or code quality improvements)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Draft PR (breaking/non-breaking change which needs more work for having a proper functionality [Mark this PR as ready to review only when completely ready])
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

How has this been tested?

tests, dogfooding

Checklist

  • I have performed a self-review of my own code
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules

Additional Information (Optional)

Please include any additional information that may be useful for the reviewer.

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces a new utility method, updateConfigOptions, to allow dynamic updates to the Ethereum provider configuration—including toggling the QR modal—without reinitializing the provider.

  • Added updateConfigOptions to update provider settings after initialization
  • Extended modal loading logic by awaiting loadWalletConnectModal in various authentication flows
  • Added tests covering the update and preservation of configuration options

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
providers/ethereum-provider/test/index.spec.ts Added tests for updating configuration options
providers/ethereum-provider/src/EthereumProvider.ts Introduced updateConfigOptions and adjusted modal loading behavior
Comments suppressed due to low confidence (2)

providers/ethereum-provider/src/EthereumProvider.ts:293

  • The call to await loadWalletConnectModal does not explicitly handle the case where the modal fails to load, which could leave this.modal undefined and lead to unexpected behavior when subscribing to modal events. Consider adding error handling or a fallback mechanism to ensure consistent behavior if modal loading fails.
await this.loadWalletConnectModal();

providers/ethereum-provider/src/EthereumProvider.ts:438

  • [nitpick] Consider adding tests to cover edge cases for updateConfigOptions, for example, when partial options are provided or when nested configuration fields (like rpcMap) are updated. This will help ensure that existing settings are preserved and that the method behaves as expected under various scenarios.
public updateConfigOptions(opts: Partial<EthereumProviderOptions>) {

Comment on lines 293 to 294
await this.loadWalletConnectModal();
this.modal?.subscribeModal((state: { open: boolean }) => {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe refactor loadWalletConnectModal to be a "get instance" method and return the modal instance? Then you don't need to manually load the modal when you need it, because the only way to obtain it is to load it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants