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

feat: improve WalletAccount class instantiation #1245

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

dhruvkelawala
Copy link
Collaborator

@dhruvkelawala dhruvkelawala commented Oct 10, 2024

Motivation and Resolution

Currently, the WalletAccount class in the account.ts file has the connection logic and account address retrieval mixed with the object construction process in the constructor. This violates the principle of separation of concerns and can lead to potential issues such as race conditions and difficulty in testing.

To address this concern, the connection logic and account address retrieval have been separated from the constructor and moved into static connect and connectSilent methods. This change ensures that the object construction process remains synchronous and focused on initializing the object's state, while the connection logic is handled separately.

RPC version (if applicable)

N/A

Usage related changes

No changes to the usage of the WalletAccount class from the user's perspective.

Development related changes

  • The WalletAccount class constructor no longer includes any asynchronous operations or connection logic. It now focuses solely on initializing the object's state and setting up event listeners.

  • The connect and connectSilent static methods have been introduced to handle the connection logic and account address retrieval from the wallet provider.

  • Developers should now use the connect or connectSilent methods to create instances of WalletAccount with the appropriate account address retrieved from the wallet provider.

  • This approach facilitates better integration with libraries like starknetkit and starknet-react, which handle the connection logic internally. After establishing a connection, these libraries can return a WalletAccount instance without attempting to connect again, as the connection logic is now separated from the object construction process.

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

@tabaktoni
Copy link
Collaborator

This will be breaking change with connect() for existing users.

Is there an imminent issue with this one, so should it be forced into the current version?
If so can we for this version leave auto-connect width a delay for connect let's say 2000 ms, and in v7 merge it with connect?

@dhruvkelawala
Copy link
Collaborator Author

No hurry for merging, but let's make sure it's there in the next major version

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