Skip to content
This repository was archived by the owner on Nov 10, 2023. It is now read-only.

Conversation

@iamacook
Copy link
Contributor

@iamacook iamacook commented Feb 10, 2022

What it solves

Resolves #3435

How this PR fixes it

isSmartContractWallet now checks via the readonly web3 object instead of that from the provider. Using the standard getWeb3() object, returned from the provider, was causing web3 interaction in the case of ,eth.getCode() to hang when using the https://rpc.xdaichain.com/oe-only/ RPC.

This fix facilitates switching back to the OE RPC for Gnosis Chain instead. This fixes the gas estimation issue. The isSmartContractWallet() tweak fixes the WalletConnect issue which made us migrate RPC in the first place.

How to test it

  • Use the temporary Gnosis Chain on the staging CGW (it has a different RPC than on prod.)
  • Test that estimations do not fail on < 1.3.0 Safes on Gnosis Chain
  • Test that Safe-Safe WC works, setting the smartContractWallet flag correctly in the providers store

Before release

  • Change the Gnosis Chain RPC back to https://rpc.xdaichain.com/oe-only/

@iamacook iamacook requested review from katspaugh and mmv08 February 10, 2022 14:25
@github-actions
Copy link

CLA Assistant Lite All Contributors have signed the CLA.

@github-actions
Copy link

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
Copy link

Deployment links

🟠 Rinkeby Mainnet 🟣 Polygon 🟡 BSC Arbitrum 🟢 Gnosis Chain

@coveralls
Copy link

Pull Request Test Coverage Report for Build 1824281873

  • 2 of 3 (66.67%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 33.682%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/logic/wallets/getWeb3.ts 2 3 66.67%
Totals Coverage Status
Change from base Build 1819173832: 0.0%
Covered Lines: 3234
Relevant Lines: 8576

💛 - Coveralls

@github-actions
Copy link

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

Failed tests:

  • ❌ Read-only transaction creation and review Read-only transaction creation and review
  • ❌ Safe Balances Safe Balances

Copy link
Contributor

@mmv08 mmv08 left a comment

Choose a reason for hiding this comment

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

THE MVP

Copy link
Member

@katspaugh katspaugh left a comment

Choose a reason for hiding this comment

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

Hallelujah!

@francovenica
Copy link
Contributor

1 - Trying to create a tx with a safe as the owner of the safe were the tx is being created makes that all fields are equal to 0

I see no errors in the "estimation" call in the network tab and the errors in the console are not about estimations either
image
image

With a regular user (a MM account) I have no issues with the estimation

2 - Question: in this PR I got the call to the RPC constantly, by just staying in the tx tab. I've check prod and there are calls, but are not done all the time like in this PR. Is this expected?
image

@iamacook
Copy link
Contributor Author

iamacook commented Feb 13, 2022

2 - Question: in this PR I got the call to the RPC constantly, by just staying in the tx tab. I've check prod and there are calls, but are not done all the time like in this PR. Is this expected?

Can you provide some more details? If you're using WalletConnect then this is a known problem (solved here #3479).

@francovenica
Copy link
Contributor

No, just by being there I get those calls. I don't even need to be connected
https://pr3477--safereact.review-safe.gnosisdev.com/app/gno:0x1dA9748A8Bd95e9D17E7B8976174dA6A9E85B342/transactions/history
image

That being said, I don't see the issue in the 3479, so is fixed there I guess

@iamacook iamacook merged commit 41b3bcf into dev Feb 14, 2022
@iamacook iamacook deleted the xdai-oe-wc branch February 14, 2022 13:19
@github-actions github-actions bot locked and limited conversation to collaborators Feb 14, 2022
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.

Gnosis Chain - Gas estimation fails

6 participants