Skip to content
This repository has been archived by the owner on Jun 24, 2022. It is now read-only.

Reset ens on redux state when changing accounts #2340

Merged
merged 1 commit into from
Jan 28, 2022

Conversation

alfetopito
Copy link
Contributor

Summary

ENS names now properly reset when account is changed

To Test

  1. On this PR, on either mainnet or rinkeby, look for claims for vitalik.eth
  2. Then, change account and look for claims for another account
  • The account name (top left of claims page) should have been updated

The same does not happen right now on staging, the account name displayed will remain the ens name.

@alfetopito alfetopito self-assigned this Jan 27, 2022
@alfetopito alfetopito requested review from a team January 27, 2022 22:12
@github-actions
Copy link
Contributor

  • 🔭 GP Swap: CoW Protocol v2 Swap UI

@elena-zh
Copy link

@alfetopito , this works when change accounts using 'Change account' buttons.
But is does not work when change an account in the connected wallet. Se e the video:
https://watch.screencastify.com/v/qX9SN1y7jUbNT6AjezrW

Copy link
Contributor

@nenadV91 nenadV91 left a comment

Choose a reason for hiding this comment

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

Approved because this seems like it should be there but I couldn't actually reproduce the issue on staging because once you actually change the addres/ens to search and press the check claimable vcow button and call the handleCheckClaim the ens is changed in that method.
https://watch.screencastify.com/v/bksGdR4yx5on8e8FAG3N

@nenadV91
Copy link
Contributor

@elena-zh In my opinion that is not supposed to happen once you search for some address and then change the wallet. You should still see the results for the address you search for and not results for the new wallet you just changed/connected to.

@elena-zh
Copy link

elena-zh commented Jan 28, 2022

@nenadV91 , yes, thanks!
My main concern is that behavior is different:

  1. If we never press on the 'Change wallet' button, and change wallets in the connected wallet, data on the Claim form is reset to the connected account
  2. BUT: if we once press on the 'Change wallet' button, and search for an account data, then change wallets in the connected wallet, we still be able to see previous wallet data, not for the currently connected one.

@anxolin
Copy link
Contributor

anxolin commented Jan 28, 2022

I believe changing using the link is what this PR was intending to fix.

If you change using MM, it changes the connected wallet, and ALSO the claiming wallet when you were claiming for yourself. However, Elena, i find it ok (and is even nice) that it remembers the external wallet your are claiming for when you change wallet in MM

@@ -96,6 +96,7 @@ export default function Claim() {
// handle change account
const handleChangeAccount = () => {
setActiveClaimAccount('')
setActiveClaimAccountENS('')
Copy link
Contributor

Choose a reason for hiding this comment

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

yes! that was my lucky guess :P

Copy link
Contributor

Choose a reason for hiding this comment

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

ah no, my lucky guess was the resetClaimUi

@alfetopito alfetopito merged commit 3198060 into release/1.10 Jan 28, 2022
@alfetopito alfetopito deleted the fix-claim-ens-not-updated branch January 28, 2022 17:39
alfetopito added a commit that referenced this pull request Jan 28, 2022
ramirotw pushed a commit that referenced this pull request Feb 18, 2022
* fix: only try SafeApp connection in an iframe

Improves non-iframe pageload by 300ms. Fixes #2338.

The Gnosis check for a SafeApp races a postMessage and a 300ms timeout [1]. The SafeApp embeds the interface in an iframe, so this avoids the check when not in iframes.

[1]: https://github.com/gnosis/safe-apps-sdk/blob/f224869dd5ae810db8cecad08fbbcfaa6c046d9d/packages/safe-apps-web3-react/src/connector.ts#L52

* refactor: IS_IN_IFRAME const
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants