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

WalletAccount: silent instantiation #1257

Closed

Conversation

PhilippeR26
Copy link
Collaborator

Motivation and Resolution

This draft PR is a proposal of alternative to the PR #1245 of Dhruv about removal of address reading from the WalletAccount class constructor.

Usage related changes

The PR#1245 is introducing a big breaking change : new WalletAccount(...) is replaced by WalletAccount.connect(...).

  • The wallet address is immediately accessible.
  • If the Wallet is locked or not connected, the Wallet UI will be opened immediately, not necessarily at the best moment to have the best user experience.

My alternative is not changing the creation of a WalletAccount (still `new WalletAccount()`) and is not trying to read the address in the constructor.
  • Nevertheless, it also introduce a breaking change : the address is not available from start, and need either usage of myWalletAccount.getAdress(), or to use a function that needs the address (here I just coded as example the getNonce function.
  • Once the instance created, you can use the methods that are not using the address property (mainly properties from Provider class), without opening up the Wallet UI. So, you are not opening the Wallet UI just to read Provider stuff. In PR 1245, you need to add a RpcProvider instance to reach the same result.

This draft PR has been created to initiate the discussion about the pro/con of each solution, especially in term of DAPP devs/final user experiences.

Development related changes

As .address is sync, I can't override it with an async API call. This is why there is a breaking change in this proposal.
All Account methods that are using .address are overrided to first read the address in the API (if needed).

Checklist:

  • Performed a self-review of the code
  • Rebased to the last commit of the target branch (or merged it into my branch)
  • Linked the issues which this PR resolves
  • Documented the changes in code (API docs will be generated automatically)
  • Updated the tests
  • All tests are passing

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.

1 participant