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

fix: remove ensure wallet in argent mobile and argent webwallet #126

Merged
merged 1 commit into from
Aug 20, 2024

Conversation

bluecco
Copy link
Contributor

@bluecco bluecco commented Aug 19, 2024

Issue / feature description

Remove argent mobile and argent webwallet popups when a request is performed (just throw an exception and the dapp will have to manage it)

required for being compatible with starknet-react, since it will always make a request while checking if the connector is connected (and, in this case, it shouldn't open mobile popup login / webwallet)

Changes

  • remove this.ensureWallet() for Argent Mobile connector
  • remove this.ensureWallet() for Argent Webwallet connector

Checklist

  • Rebased to the last commit of the target branch (or merged)
  • Code self-reviewed
  • Code self-tested
  • Tests updated (if needed)
  • All tests are passing locally

Please keep your pull request as small as possible. If you need to make multiple changes, please create separate pull requests for each change. Create a draft pull request if you need early feedback. This will mark the pull request as a work in progress and prevent it from being reviewed or merged until you are ready.

Please move drafts to the ready for review (regular PR) when you are ready to start the review process and other developers will pick it up from there.

@@ -56,18 +56,19 @@ export class ArgentMobileBaseConnector extends Connector {
}

async ready(): Promise<boolean> {
// check if session is valid and retrieve the wallet
// if no sessions, it will show the login modal
await this.ensureWallet()

Choose a reason for hiding this comment

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

Without this code the _wallet property will not be initialized unless connect is called - will this be good enough? I wonder if the ensureWallet method shouldn't still initialize the _wallet property but not try to call enable

Copy link
Contributor Author

@bluecco bluecco Aug 19, 2024

Choose a reason for hiding this comment

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

Without this code the _wallet property will not be initialized unless connect is called - will this be good enough?

yes, it follows the same behaviour as webwallet

  • if the wallet is not initialized, the dapp would display a connect button
  • if the dapp has an autoconnect function, it will be initialized

if the dapp perform the request while not connected, it will receive the exception that will be managed on dapp side (like showing the connect modal)

managing ensureWallet without enable could be a little tricky at the moment, but can be a good point to improve in a future release

Choose a reason for hiding this comment

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

Sounds good to me - just wanted to make sure it won't break our normal flows 👍

@bluecco bluecco merged commit 2d6a64e into develop Aug 20, 2024
1 check passed
@bluecco bluecco deleted the fix/argent-ww-mobile-connectors branch August 20, 2024 07:54
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