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.

Sign up for GitHub

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

Jump to bottom

wallet-ext,wallet-adapters,wallet-kit-core: handle disconnecting dapp using wallet #7258

Merged
merged 4 commits into from
Jan 13, 2023
Merged

wallet-ext,wallet-adapters,wallet-kit-core: handle disconnecting dapp using wallet #7258

merged 4 commits into from
Jan 13, 2023

Conversation

pchrysochoidis
Copy link
Contributor

@pchrysochoidis pchrysochoidis commented Jan 9, 2023 •
edited
Loading

fix the case where user disconnects from a dapp using the wallet and wallet-kit was staying in connected state

  • wallet-ext: emit events when connected accounts change
  • wallet-adapters: emit events when connected status changes
  • wallet-kit-core: listen for connection status events and update state
Screen.Recording.2023-01-11.at.17.49.49.mov

part of APPS-308

@vercel
Copy link

vercel bot commented Jan 9, 2023 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated
explorer ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Jan 13, 2023 at 7:19PM (UTC)
wallet-adapter ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Jan 13, 2023 at 7:19PM (UTC)
1 Ignored Deployment
Name Status Preview Comments Updated
explorer-storybook ⬜️ Ignored (Inspect) Jan 13, 2023 at 7:19PM (UTC)

@pchrysochoidis pchrysochoidis force-pushed the pc-wallet-ext-apps-308-disconnected-event branch from 2edfa1e to ee40c67 Compare January 9, 2023 21:24
@vercel vercel bot temporarily deployed to Preview – wallet-adapter January 9, 2023 21:25 Inactive
@pchrysochoidis pchrysochoidis force-pushed the pc-wallet-ext-apps-308-disconnected-event branch from ee40c67 to c4b7f7d Compare January 11, 2023 11:23
@vercel vercel bot temporarily deployed to Preview – wallet-adapter January 11, 2023 11:25 Inactive
@pchrysochoidis pchrysochoidis force-pushed the pc-wallet-ext-apps-308-disconnected-event branch from c4b7f7d to 2cdce7c Compare January 11, 2023 17:06
@vercel vercel bot temporarily deployed to Preview – wallet-adapter January 11, 2023 17:07 Inactive
@MystenLabs MystenLabs deleted a comment from github-actions bot Jan 11, 2023
@pchrysochoidis pchrysochoidis changed the title (wip) wallet-ext: disconnected event wallet-ext,wallet-adapters,wallet-kit-core: handle disconnecting dapp using wallet Jan 11, 2023
@pchrysochoidis pchrysochoidis marked this pull request as ready for review January 11, 2023 17:28
@pchrysochoidis pchrysochoidis force-pushed the pc-wallet-ext-apps-308-disconnected-event branch from 2cdce7c to f77f88a Compare January 11, 2023 20:15
@vercel vercel bot temporarily deployed to Preview – wallet-adapter January 11, 2023 20:17 Inactive
Jordan-Mysten
Jordan-Mysten reviewed Jan 11, 2023
View reviewed changes
sdk/wallet-adapter/adapters/wallet-standard-adapter/src/StandardWalletAdapter.ts Outdated
#wallet: StandardWalletAdapterWallet;

constructor({ wallet }: StandardWalletAdapterConfig) {
this.#wallet = wallet;
this.#wallet.features["standard:events"].on(
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not convinced this should be listened for here vs in wallet kit. I'm curious why you ended up listening here?

Also, if we do keep the listen here, it probably makes more sense to set this up in connect instead of the constructor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to do it to update the connected field. But also WalletAdapter interface seems to kind of isolate the wallet from wallet-kit, and thought maybe it's better to add the extra WalletAdapterEvents so adapters can have extra events instead of just exposing the wallet feature. But happy to change it if you have something else in mind.

Also, if we do keep the listen here, it probably makes more sense to set this up in connect instead of the constructor?

I will move it

@pchrysochoidis pchrysochoidis force-pushed the pc-wallet-ext-apps-308-disconnected-event branch from f77f88a to 3873fb4 Compare January 12, 2023 14:11
@vercel vercel bot temporarily deployed to Preview – wallet-adapter January 12, 2023 14:12 Inactive
@pchrysochoidis pchrysochoidis force-pushed the pc-wallet-ext-apps-308-disconnected-event branch from 3873fb4 to 0076cd3 Compare January 12, 2023 14:18
@vercel vercel bot temporarily deployed to Preview – wallet-adapter January 12, 2023 14:19 Inactive
Jordan-Mysten
Jordan-Mysten approved these changes Jan 13, 2023
View reviewed changes
Copy link
Contributor

@Jordan-Mysten Jordan-Mysten left a comment

Choose a reason for hiding this comment

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

Looks good to me.

sdk/wallet-adapter/adapters/base-adapter/src/index.ts Outdated
SignableTransaction,
SuiAddress,
SuiTransactionResponse,
} from "@mysten/sui.js";

export interface WalletAdapterEvents {
changed(changes: { connected?: boolean; accounts?: SuiAddress[] }): void;
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe change to align more with tense of other events?

@pchrysochoidis
wallet-ext: emit connected account change events
4841f1b
@pchrysochoidis
wallet-adapters: adapter events
872e533 
* emit change events when connected status changes
@pchrysochoidis
wallet-kit-core: handle wallet disconnect
07b42df 
* handle the case when user disconnects from the dapp using the wallet, and update the wallet-kit state as well
@pchrysochoidis
wallet-ext: rename wallet adapter event to change
8b966e3 
* address PR comment
@pchrysochoidis pchrysochoidis force-pushed the pc-wallet-ext-apps-308-disconnected-event branch from 0076cd3 to 8b966e3 Compare January 13, 2023 19:18
@vercel vercel bot temporarily deployed to Preview – wallet-adapter January 13, 2023 19:19 Inactive
@pchrysochoidis pchrysochoidis merged commit 65fd8ac into main Jan 13, 2023
@pchrysochoidis pchrysochoidis deleted the pc-wallet-ext-apps-308-disconnected-event branch January 13, 2023 19:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Jordan-Mysten Jordan-Mysten Jordan-Mysten approved these changes

@Jibz1 Jibz1 Awaiting requested review from Jibz1

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@pchrysochoidis @Jordan-Mysten