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

feat/264 - Handle third-party extension events #310

Merged
merged 6 commits into from
Jul 5, 2023
Merged

feat/264 - Handle third-party extension events #310

merged 6 commits into from
Jul 5, 2023

Conversation

jurevans
Copy link
Collaborator

@jurevans jurevans commented Jun 15, 2023 •
edited
Loading

Resolves #264

  • Interface responds to Keplr account changes
  • Interface responds to Metamask account changes

Testing

Keplr

  • Add a second (or more) account(s) in Keplr
  • In Namada interface, select "Cosmos Hub", and make sure extension is connected
  • In Keplr, select a different account, and the interface should reflect this

Metamask

  • Make sure you have more than one account in Metamask
  • In Namada Interface, select "Goerli Testnet"
  • In Metamask, in the "accounts" menu (icon at top-right) select a different account (NOTE If this is a new account, you will need to first click "connect" on the account name) and the interface should update the address

TODO

  • There is a display bug that has been introduced somewhere in the interface, where the listing of the Ethereum asset is broken (only the address displays) - This will be updated in a separate PR if I don't figure it out quickly

@jurevans jurevans added this to the Release 1.0 (Q4 2022) milestone Jun 15, 2023
@jurevans jurevans self-assigned this Jun 15, 2023
@github-actions
Copy link

github-actions bot commented Jun 15, 2023 •
edited
Loading

🚀 Deployed on https://pull-request-310--wallet-development-heliax-dev.netlify.app

@github-actions github-actions bot temporarily deployed to pull request June 15, 2023 15:08 Inactive
@github-actions github-actions bot temporarily deployed to pull request June 15, 2023 15:54 Inactive
@github-actions github-actions bot temporarily deployed to pull request June 16, 2023 08:14 Inactive
@github-actions github-actions bot temporarily deployed to pull request June 29, 2023 12:23 Inactive
@jurevans jurevans changed the title WIP - feat/264 - Handle third-party extension events feat/264 - Handle third-party extension events Jun 30, 2023
@jurevans jurevans marked this pull request as ready for review June 30, 2023 11:07
@github-actions github-actions bot temporarily deployed to pull request June 30, 2023 11:12 Inactive
@github-actions github-actions bot temporarily deployed to pull request June 30, 2023 12:14 Inactive
@jurevans jurevans mentioned this pull request Jun 30, 2023
Closed
2 tasks
emccorson
emccorson approved these changes Jul 4, 2023
View reviewed changes
Copy link
Collaborator

@emccorson emccorson left a comment

Choose a reason for hiding this comment

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

Left a couple of minor comments, but code looks good to me and account switching works nicely 👍

packages/types/src/events.ts Outdated
// Metamask extension window.ethereum events
export enum MetamaskEvents {
AccountChanged = "accountsChanged",
NetworkChanged = "networkChanged",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I can't see the networkChanged event in the Metamask docs (although I did see it in a gist somewhere). Should this be chainChanged?.
https://docs.metamask.io/wallet/reference/provider-api/#chainchanged

It doesn't look like it's used anyway so it could also be removed potentially.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, that was a typo on my part. As we're not really concerned with it, I'll just remove it! chainsChanged would be appropriate, but yeah, not used at the moment.

apps/namada-interface/src/services/extensionEvents/handlers/keplr.ts
}
if (accounts) {
dispatch(addAccounts(accounts));
dispatch(fetchBalances());
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is just a question, but are these guaranteed to run one after the other i.e. is it possible balances will be fetched for the wrong account?

Copy link
Collaborator

Choose a reason for hiding this comment

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

AFAIK in this case it will work because addAccounts is synchronous

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ahhh, I see it now. I just assumed addAccounts was also asynchronous. Thanks!

mateuszjasiuk
mateuszjasiuk approved these changes Jul 4, 2023
View reviewed changes
Copy link
Collaborator

@mateuszjasiuk mateuszjasiuk left a comment

Choose a reason for hiding this comment

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

Tested! Works nice!

apps/namada-interface/src/services/extensionEvents/handlers/keplr.ts
}
if (accounts) {
dispatch(addAccounts(accounts));
dispatch(fetchBalances());
Copy link
Collaborator

Choose a reason for hiding this comment

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

AFAIK in this case it will work because addAccounts is synchronous

@github-actions github-actions bot temporarily deployed to pull request July 5, 2023 08:37 Inactive
@jurevans jurevans merged commit e1d9af1 into main Jul 5, 2023
5 checks passed
@jurevans jurevans deleted the feat/264-extension-events branch July 5, 2023 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emccorson emccorson emccorson approved these changes

@mateuszjasiuk mateuszjasiuk mateuszjasiuk approved these changes

Assignees

@jurevans jurevans

Labels
None yet
Projects
None yet
Milestone
Public Testnet Release (2023)
Development

Successfully merging this pull request may close these issues.

Interface: Handle 3rd-party extension events
3 participants
@jurevans @mateuszjasiuk @emccorson