refactor: rely on Onboard for provider watching #3364

iamacook · 2022-01-24T15:41:03Z

Known bugs that resulted in reopening

Multiple GA event tracking: one is occuring per provider action and when switching network it sometimes tracks incorrectly.
ENS name is not resolving correctly, as explained here.
Disconnecting and refreshing reconnects the user automatically.
No network switch prompt when connecting.

May likely fix

Avalanche network switching message: The network change is required message is shown when the network was changed in wallet for Avalanche #3285
Did fix: ~~Wallet Connect Infura errors: Wallet connect - infura error in console #2093~~
Fixed a separate PR: ~~Wallet Connect not working on Gnosis Chain: WC doesn't work on GChain ( connect an owner via WC) #3372~~

What it solves

The removal of an unnecessary interval and, in turn, numerous requests. Also, a reduction of ENS errors in the console.

How this PR fixes it

In researching how to reduce ENS console errors from our provider watcher, it became apparent that we were relying on a legacy method of 'watching' the Onboard provider. Onboard, however, has built-in subscriptions that are a better alternative than the interval we were using. (One of which resolves ENS domains on wallet connection).

Instead of using an interval that polls every few seconds, this PR migrates to using the wallet, address, network and ens subscriptions from ENS, responsible for:

Wallet changes
Account changes
Network changes
ENS resolutions (when the above three change)

The legacy watcher interval has been removed, as well as the associated reducer/middleware adjusted. The reducer handles the subscriptions based on updates instead and wallet disconnection is now handled internally via Onboard, removing reliance on a provider removal method. Logic that was previously required in the middleware: instantiating contracts and saving the current provider for re-connection is now also handled by Onboard.

The notification system has been moved from provider 'fetching' (which was previously happening every time per interval) to the middleware, which is also subscription-based.

A few unnecessary utility functions have also be replaced or substituted for their Onboard counterparts.

How to test it

Nothing should change on a UI/UX level. Connection may seem a bit quicker. Areas to check are the following (cross-wallet):

Wallet connection should correctly update the providers store data correctly, i.e. EOA details, ENS domain, hardware/smart contract wallet flags, etc.
Disconnection should clear the providers store.
Provider-related notifications should work as before.

github-actions · 2022-01-24T15:41:21Z

CLA Assistant Lite All Contributors have signed the CLA.

github-actions · 2022-01-24T15:45:05Z

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type	Occurrences	Fixable
Errors	0	0
Warnings	0	0
Ignored	1	N/A

Result: ✅ success
Annotations: 0 total

^{Report generated by eslint-plus-action}

github-actions · 2022-01-24T15:47:41Z

Deployment links

🟠 Rinkeby	⚪ Mainnet	🟣 Polygon	🟡 BSC	⚫ Arbitrum	🟢 Gnosis Chain

github-actions · 2022-01-24T15:55:10Z

E2E Tests Failed
Check the results here: https://github.com/gnosis/safe-react-e2e-tests/actions/runs/1859214340

Failed tests:

❌ Address book Address book
❌ Read-only transaction creation and review Read-only transaction creation and review

coveralls · 2022-01-24T16:30:12Z

Pull Request Test Coverage Report for Build 1859171848

69 of 152 (45.39%) changed or added relevant lines in 17 files are covered.
4 unchanged lines in 4 files lost coverage.
Overall coverage increased (+0.03%) to 33.613%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/components/AppLayout/Header/index.tsx	0	1	0.0%
src/logic/wallets/getWeb3.ts	1	2	50.0%
src/logic/wallets/utils/network.ts	1	2	50.0%
src/logic/wallets/store/selectors/index.ts	10	14	71.43%
src/routes/CreateSafePage/steps/OwnersAndConfirmationsNewSafeStep.tsx	11	16	68.75%
src/logic/wallets/store/reducer/index.ts	4	11	36.36%
src/logic/wallets/utils/walletList.ts	3	14	21.43%
src/logic/wallets/store/middleware/index.ts	12	28	42.86%
src/logic/wallets/patchedWalletConnect.ts	4	21	19.05%
src/logic/wallets/onboard.ts	6	26	23.08%

Files with Coverage Reduction	New Missed Lines	%
src/components/AppLayout/Header/index.tsx	1	0%
src/logic/wallets/onboard.ts	1	12.07%
src/logic/wallets/utils/network.ts	1	76.32%
src/logic/wallets/utils/walletList.ts	1	13.64%

Totals
Change from base Build 1858621660:	0.03%
Covered Lines:	3238
Relevant Lines:	8625

💛 - Coveralls

DiogoSoaress

The PR looks good to me and it is easier to follow the logic with the subscriptions. ~~I am not sure I have enough understanding of all the underlying problems to say it's ready for QA.~~

iamacook · 2022-01-31T07:27:34Z

I am not sure I have enough understanding of all the underlying problems to say it's ready for QA.

What problems did you find?

dasanra

Nice refactor!

github-actions · 2022-02-08T16:10:14Z

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type	Occurrences	Fixable
Errors	0	0
Warnings	0	0
Ignored	2	N/A

Result: ✅ success
Annotations: 0 total

^{Report generated by eslint-plus-action}

* fix: checksum address before ens validation * fix: checksum `verifiedAddress` * refactor: lookup ENS w/ form init

* fix: use field-level owner validation * fix: test * fix: forward compare previous owners * fix: checksum address from Onboard * fix: keep store logic in reducer

* fix: increase block polling interval * fix: patch balance `stateSyncer` out of WC * fix: upgrade `bnc-onboard` + patch `web3-provider-engine` * fix: cleanup patch * fix: cleanup patch * fix: exchange patch for custom WC module instead

liliya-soroka · 2022-02-14T14:30:59Z

Tested on Rinkeby with

Ledger nano X - works both signing and execution
Trezor T - only signing works. Execution is not available with trezor T

francovenica · 2022-02-17T02:00:01Z

Issue with WC connection. Tried in dev and prod and it doesn't happen there
#3528

francovenica · 2022-02-17T02:32:01Z

The trezor causes 2 calls to GA when connecting it, one when the modal open to select an account from the dropdown and one when you actually confirm it. This also happens in dev, but given the 1st checkbox in the description of this ticket I wanted to know if this ticket is supposed to fix these repeated calls to GA or not.

francovenica · 2022-02-17T02:36:11Z

Tested in Rinkeby so far. Signing tx with MM chrome extension, WC with MM phone app, trezor one and ledger nano S worked fine. Also safe creation
I'll give it a try to other networks tomorrow

iamacook · 2022-02-17T08:20:44Z

Issue with WC connection. Tried in dev and prod and it doesn't happen there #3528

I'm a bit confused as you're following comment:

Tested in Rinkeby so far. Signing tx with MM chrome extension, WC with MM phone app, trezor one and ledger nano S worked fine.

suggests it works? Can you provide any more details?

I wanted to know if this ticket is supposed to fix these repeated calls to GA or not

Not here. I am working on the Analytics epic where it will be sorted.

francovenica · 2022-02-17T12:48:13Z

Let me summarize my comments:

2 issues I found recently: Onboard refactor: Safe not loading when WC is used to connect to the safe. #3528 and Onboard refactor: Console warnings when opening the safe list on the sidebar #3529. The "dev" comment is that I always check dev (and sometimes prod) to see if the issue are happening there as well, but in this case both of those issues only happen in this PR.
Lili seems to have issues with the "Trezor T" to sign txs
I made a comment regarding GA in trezor, but you are saying that is not expected to be fixed here, so I'll ignore it
Despite the issues Lili and I found, on my side I had no problem creating safes, signing and executing tx with the MM extension, WC with the MM phone app and the devices I have: a trezor one and a ledger nano S.

github-actions · 2022-02-17T14:28:00Z

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type	Occurrences	Fixable
Errors	0	0
Warnings	0	0
Ignored	2	N/A

Result: ✅ success
Annotations: 0 total

^{Report generated by eslint-plus-action}

iamacook · 2022-02-17T14:56:31Z

* 2 issues I found recently: [Onboard refactor: Safe not loading when WC is used to connect to the safe. #3528](https://github.com/gnosis/safe-react/issues/3528) and [Onboard refactor: Console warnings when opening the safe list on the sidebar #3529](https://github.com/gnosis/safe-react/issues/3529). The "dev" comment is that I always check dev (and sometimes prod) to see if the issue are happening there as well, but in this case both of those issues only happen in this PR.

Just confirmed with @francovenica that these were solved by merging dev into this branch (649cfd0)

* Lili seems to have issues with the "Trezor T" to sign txs

Awaiting confirmation on whether this is related to an older ticket.

Looks like we can merge this soon 👀

iamacook added 10 commits January 20, 2022 09:50

refactor: rely on onboard for provider watching

e3c8a2e

chore: remove key check

d714c36

fix: ENS support + notifications after full load

db82eea

feat: use fallthroughs + remove console error

b2b1879

fix: reset web3 on wallet disconnection

7ed5afd

fix: wallet is loaded when one flag is true

c2b7420

fix: default fallthrough logic

bd8707b

chore: cleanup code structure

b0f9e14

Merge branch 'dev' into cleanup-provider-watcher

f605716

fix: always dispatch wallet store actions

f8fdecd

iamacook self-assigned this Jan 24, 2022

iamacook changed the title ~~feat: rely on Onboard for provider watching~~ refactor: rely on Onboard for provider watching Jan 24, 2022

iamacook mentioned this pull request Jan 24, 2022

refactor: rely on Onboard for provider watching #3333

Merged

3 tasks

iamacook mentioned this pull request Jan 24, 2022

Revert "refactor: rely on Onboard for provider watching" #3365

Merged

iamacook added 2 commits January 24, 2022 20:56

fix: track wallet once + clear on disconnect

99fa31d

fix: chain-dependant ENS resolution

1f01db5

iamacook requested review from katspaugh and usame-algan and removed request for katspaugh January 24, 2022 20:11

iamacook marked this pull request as ready for review January 24, 2022 20:22

iamacook mentioned this pull request Jan 25, 2022

The network change is required message is shown when the network was changed in wallet for Avalanche #3285

Closed

iamacook marked this pull request as draft January 25, 2022 18:20

fix: don't reset web3 object

e64d1f2

katspaugh requested a review from dasanra January 26, 2022 13:38

DiogoSoaress reviewed Jan 30, 2022

View reviewed changes

DiogoSoaress approved these changes Jan 31, 2022

View reviewed changes

dasanra approved these changes Jan 31, 2022

View reviewed changes

katspaugh mentioned this pull request Jan 31, 2022

Wallet connect - infura error in console #2093

Closed

Merge branch 'dev' into cleanup-provider-watcher

b9cb7bf

This was referenced Feb 8, 2022

ENS name not being autocompleted in safe creation #3459

Closed

Duplicated owner in safe creation #3460

Closed

Merge branch 'dev' into cleanup-provider-watcher

62422e1

iamacook added 5 commits February 9, 2022 13:34

fix: lookup ENS domains when intialising form (#3465)

9bcedc4

* fix: checksum address before ens validation * fix: checksum `verifiedAddress` * refactor: lookup ENS w/ form init

fix: use field-level owner validation (#3470)

4f634ba

* fix: use field-level owner validation * fix: test * fix: forward compare previous owners * fix: checksum address from Onboard * fix: keep store logic in reducer

Merge branch 'dev' into cleanup-provider-watcher

5faefb7

fix: reduce infura requests (#3479)

400b52e

* fix: increase block polling interval * fix: patch balance `stateSyncer` out of WC * fix: upgrade `bnc-onboard` + patch `web3-provider-engine` * fix: cleanup patch * fix: cleanup patch * fix: exchange patch for custom WC module instead

Merge branch 'dev' into cleanup-provider-watcher

cb02d19

iamacook mentioned this pull request Feb 14, 2022

Switching network will not switch network of Safe App RPC #3497

Closed

francovenica mentioned this pull request Feb 17, 2022

Onboard refactor: Safe not loading when WC is used to connect to the safe. #3528

Closed

francovenica mentioned this pull request Feb 17, 2022

Onboard refactor: Console warnings when opening the safe list on the sidebar #3529

Closed

Merge branch 'dev' into cleanup-provider-watcher

649cfd0

iamacook merged commit 74e980f into dev Feb 17, 2022

iamacook deleted the cleanup-provider-watcher branch February 17, 2022 16:07

github-actions bot locked and limited conversation to collaborators Feb 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: rely on Onboard for provider watching #3364

refactor: rely on Onboard for provider watching #3364

iamacook commented Jan 24, 2022 •

edited

Loading

github-actions bot commented Jan 24, 2022

github-actions bot commented Jan 24, 2022 •

edited

Loading

github-actions bot commented Jan 24, 2022

github-actions bot commented Jan 24, 2022 •

edited

Loading

coveralls commented Jan 24, 2022 •

edited

Loading

DiogoSoaress left a comment •

edited

Loading

iamacook commented Jan 31, 2022

dasanra left a comment

github-actions bot commented Feb 8, 2022 •

edited

Loading

liliya-soroka commented Feb 14, 2022

francovenica commented Feb 17, 2022

francovenica commented Feb 17, 2022

francovenica commented Feb 17, 2022

iamacook commented Feb 17, 2022

francovenica commented Feb 17, 2022

github-actions bot commented Feb 17, 2022

iamacook commented Feb 17, 2022

refactor: rely on Onboard for provider watching #3364

refactor: rely on Onboard for provider watching #3364

Conversation

iamacook commented Jan 24, 2022 • edited Loading

Known bugs that resulted in reopening

May likely fix

What it solves

How this PR fixes it

How to test it

github-actions bot commented Jan 24, 2022

github-actions bot commented Jan 24, 2022 • edited Loading

ESLint Summary View Full Report

github-actions bot commented Jan 24, 2022

Deployment links

github-actions bot commented Jan 24, 2022 • edited Loading

coveralls commented Jan 24, 2022 • edited Loading

Pull Request Test Coverage Report for Build 1859171848

💛 - Coveralls

DiogoSoaress left a comment • edited Loading

Choose a reason for hiding this comment

iamacook commented Jan 31, 2022

dasanra left a comment

Choose a reason for hiding this comment

github-actions bot commented Feb 8, 2022 • edited Loading

ESLint Summary View Full Report

liliya-soroka commented Feb 14, 2022

francovenica commented Feb 17, 2022

francovenica commented Feb 17, 2022

francovenica commented Feb 17, 2022

iamacook commented Feb 17, 2022

francovenica commented Feb 17, 2022

github-actions bot commented Feb 17, 2022

ESLint Summary View Full Report

iamacook commented Feb 17, 2022

iamacook commented Jan 24, 2022 •

edited

Loading

github-actions bot commented Jan 24, 2022 •

edited

Loading

github-actions bot commented Jan 24, 2022 •

edited

Loading

coveralls commented Jan 24, 2022 •

edited

Loading

DiogoSoaress left a comment •

edited

Loading

github-actions bot commented Feb 8, 2022 •

edited

Loading