Instantiate contracts via `chainId` from loaded config #3313

iamacook · 2022-01-17T11:03:52Z

What it solves

Resolves #3311

How this PR fixes it

Contracts were being instantiated via the web3 object, meaning that the chainId could be stale, i.e. the user is connected to Rinkeby but the unified app is on Avalanche.

Instead of relying on the chainId from the web3 object, _chainId (a local mirror of that from the store) is used instead. This is kept in sync with the current network displayed in the UI via the config middleware (where the contracts are initiated).

How to test it

Open a Safe on Rinkeby.
Create a MultiSend transaction via the Transaction Builder.
Switch to Avalanche (which uses a different addressed MultiSendOnly contract when compared to Rinkeby) via the network selector in the top right but do not connect via MetaMask.
Submit the transaction and observe that it does not show an unexpected deligate call, instead the known one.

Screenshots

github-actions · 2022-01-17T11:08:19Z

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	3	N/A

Result: ✅ success
Annotations: 0 total

^{Report generated by eslint-plus-action}

coveralls · 2022-01-17T11:09:48Z

Pull Request Test Coverage Report for Build 1711915775

9 of 34 (26.47%) changed or added relevant lines in 6 files are covered.
3 unchanged lines in 2 files lost coverage.
Overall coverage increased (+0.02%) to 32.385%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/components/Root/index.tsx	0	1	0.0%
src/logic/safe/utils/safeVersion.ts	0	1	0.0%
src/routes/opening/index.tsx	0	1	0.0%
src/logic/contracts/safeContracts.ts	2	4	50.0%
src/logic/wallets/store/middlewares/providerWatcher.ts	6	26	23.08%

Files with Coverage Reduction	New Missed Lines	%
src/logic/contracts/safeContracts.ts	1	36.84%
src/components/App/index.tsx	2	0%

Totals
Change from base Build 1687885328:	0.02%
Covered Lines:	3117
Relevant Lines:	8561

💛 - Coveralls

github-actions · 2022-01-17T11:13:49Z

Deployment links

🟠 Safe Rinkeby	⚪ Safe Mainnet	🟣 Safe Polygon	🟡 Safe BSC	⚫ Safe Arbitrum	🟢 Safe xDai

github-actions · 2022-01-17T11:19:38Z

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

Failed tests:

❌ Add an existing safe Add an existing safe
❌ Safe Balances Safe Balances

liliya-soroka · 2022-01-17T12:42:05Z

@iamacook , for rinkeby -> avalanche I can not reproduce, but for avalanche->xdai I still see the issue

Avalanche multisendonly contract is used for xdai after network switching

src/logic/config/utils/index.ts

src/components/Root/index.tsx

src/logic/config/utils/index.ts

src/components/Root/index.tsx

src/components/App/index.tsx

src/components/Root/index.tsx

src/logic/config/store/middleware/index.ts

katspaugh · 2022-01-17T17:07:22Z

src/logic/wallets/store/middlewares/providerWatcher.ts

-        watcherInterval = setInterval(async () => {
-          const web3 = getWeb3()
-          const providerInfo = await getProviderInfo(web3)
+          instantiateSafeContracts()


katspaugh

💪

usame-algan

🚀

francovenica · 2022-01-18T16:05:13Z

looks good.
I kept switching between rinkeby, xdai, avalanche, optimism and arbitrum.
Optimism and avalanche use 0xA1dabEF33b3B82c7814B6D82A79e50F4AC44102B
Rinkeby, Xdai and arbitrum use 0x40A2aCCbd92BCA938b02010E17A5b8929b49130D

I created multisend tx with the Tx builder and checking in the MM popUp that the correct address was showing

Created 2 tx, one in xdai and the very next one in optimism and they both have the correct multisend address

Kept going back and forth, creating tx in rinkeby, then avalanche, then xdai, then optimism. Basically going from a network with one address to another network with the other address. It worked fine

fix: Use chain id from chains config

fb01de6

iamacook linked an issue Jan 17, 2022 that may be closed by this pull request

[BUG] transaction builder on AVAX uses mainnet deployment of MultiSend not the AVAX deployment #3311

Closed

iamacook requested a review from katspaugh January 17, 2022 11:20

build: init Safe contracts with chains + id set

2411c00

katspaugh reviewed Jan 17, 2022

View reviewed changes

src/logic/config/utils/index.ts Outdated Show resolved Hide resolved

build: Set chain id + contract init in middleware

4a95fd1

iamacook requested a review from katspaugh January 17, 2022 14:31

katspaugh reviewed Jan 17, 2022

View reviewed changes

src/components/Root/index.tsx Outdated Show resolved Hide resolved

fix: Remove unnecessary contract init

27d445b

iamacook marked this pull request as draft January 17, 2022 15:09

usame-algan reviewed Jan 17, 2022

View reviewed changes

src/logic/config/utils/index.ts Outdated Show resolved Hide resolved

fix: Dispatch chain id setting with App init

0c1da16

katspaugh reviewed Jan 17, 2022

View reviewed changes

src/components/Root/index.tsx Outdated Show resolved Hide resolved

src/components/App/index.tsx Outdated Show resolved Hide resolved

src/components/App/index.tsx Outdated Show resolved Hide resolved

build: Move contract init to provider watcher

e0a873f

iamacook requested review from katspaugh and usame-algan January 17, 2022 17:03

iamacook assigned usame-algan Jan 17, 2022

iamacook marked this pull request as ready for review January 17, 2022 17:03

katspaugh reviewed Jan 17, 2022

View reviewed changes

src/components/Root/index.tsx Show resolved Hide resolved

katspaugh reviewed Jan 17, 2022

View reviewed changes

src/logic/config/store/middleware/index.ts Outdated Show resolved Hide resolved

katspaugh reviewed Jan 17, 2022

View reviewed changes

build: Separate local chainId from store

e90fc54

katspaugh approved these changes Jan 18, 2022

View reviewed changes

usame-algan approved these changes Jan 18, 2022

View reviewed changes

katspaugh mentioned this pull request Jan 18, 2022

Release 3.17.1 #3322

Merged

katspaugh merged commit 2b67598 into main Jan 18, 2022

katspaugh deleted the instantiate-contracts-via-chains branch January 18, 2022 17:05

Instantiate contracts via chainId from loaded config #3313

Instantiate contracts via chainId from loaded config #3313

Uh oh!

Conversation

iamacook commented Jan 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What it solves

How this PR fixes it

How to test it

Screenshots

Uh oh!

github-actions bot commented Jan 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ESLint Summary View Full Report

Uh oh!

coveralls commented Jan 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 1711915775

💛 - Coveralls

Uh oh!

github-actions bot commented Jan 17, 2022

Deployment links

Uh oh!

github-actions bot commented Jan 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

liliya-soroka commented Jan 17, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

katspaugh Jan 17, 2022

Choose a reason for hiding this comment

Uh oh!

katspaugh left a comment

Choose a reason for hiding this comment

Uh oh!

usame-algan left a comment

Choose a reason for hiding this comment

Uh oh!

francovenica commented Jan 18, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Instantiate contracts via `chainId` from loaded config #3313

Instantiate contracts via `chainId` from loaded config #3313

iamacook commented Jan 17, 2022 •

edited

Loading

github-actions bot commented Jan 17, 2022 •

edited

Loading

coveralls commented Jan 17, 2022 •

edited

Loading

github-actions bot commented Jan 17, 2022 •

edited

Loading