Skip to content
This repository has been archived by the owner on Jan 15, 2021. It is now read-only.

Fix issue approving tokens #1618

Merged
merged 1 commit into from
Nov 13, 2020
Merged

Fix issue approving tokens #1618

merged 1 commit into from
Nov 13, 2020

Conversation

anxolin
Copy link
Contributor

@anxolin anxolin commented Nov 13, 2020

This solves Anna's issue with approving tokens using Wallet Connect.

TLDR: The issue, is that the estimation is failing if we provide a gasPrice

yes, really. I'm betting this has to do with yesterday hardfork in some way. https://www.coindesk.com/ethereums-hard-fork-disruption

So this is how we estimate. We just send the same params we would use in the tx, to the estimateGas function:

web3.eth.estimateGas({data: "0x095ea7b30000000000000000000000006f400810b62df8e13fded51be75ff5393eaa841fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffdec0de02120132", from: "0xd5cbc5e664813c7f1ce0a136aa154ce525e97e21", to: "0x6810e776880c02933d47db1b9fc05908e5386b96", gasPrice: "0xcce416bb4"}, console.log)

Throws error: react_devtools_backend.js:2450 MetaMask - RPC Error: gas required exceeds allowance (160) {code: -32000, message: "gas required exceeds allowance (160)"}

The original params has a priceEstimation, so it goes with the data of the estimation too.
You would think that if the estimation don't need that info, they would ignore it. It seems is not the case! at least now, I bet it used to be and not anymore!

https://web3js.readthedocs.io/en/v1.2.0/web3-eth-contract.html#methods-mymethod-estimategas

web3.eth.estimateGas({data: "0x095ea7b30000000000000000000000006f400810b62df8e13fded51be75ff5393eaa841fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffdec0de02120132", from: "0xd5cbc5e664813c7f1ce0a136aa154ce525e97e21", to: "0x6810e776880c02933d47db1b9fc05908e5386b96"}, console.log)

Returns: 44374

Only difference, I removed the price estimation

@gnosis-info
Copy link

Travis automatic deployment:

@anxolin anxolin changed the title Test to remove some fields from estimation Fix issue approving tokens Nov 13, 2020
@anxolin anxolin changed the base branch from master to hotfix/1.6.2 November 13, 2020 15:11
Copy link
Contributor

@Velenir Velenir left a comment

Choose a reason for hiding this comment

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

Great job debugging it 👍
Though I would have expected tsConfig to be sanitized inside web3 or provider

@anxolin
Copy link
Contributor Author

anxolin commented Nov 13, 2020

Though I would have expected tsConfig to be sanitized inside web3 or provider

But gasPrice is not even part of the web3 params, so I don't get why they use it.
More over, what has silently changed. Or changed not silently and I'm unaware.

This is another interesting test, if I send any prop they don't know about, like gasPrice2 in the following example, it works. So it ignores the extra properties as it should. Is just the gasPrice is now being used for some reason! (and is not part of the params)

web3.eth.estimateGas({data: "0x095ea7b30000000000000000000000006f400810b62df8e13fded51be75ff5393eaa841fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffdec0de02120132", from: "0xd5cbc5e664813c7f1ce0a136aa154ce525e97e21", to: "0x6810e776880c02933d47db1b9fc05908e5386b96", gasPrice2: "0xcce416bb4"}, console.log)

@Velenir
Copy link
Contributor

Velenir commented Nov 13, 2020

Though I would have expected tsConfig to be sanitized inside web3 or provider

But gasPrice is not even part of the web3 params, so I don't get why they use it.
More over, what has silently changed. Or changed not silently and I'm unaware.

This is another interesting test, if I send any prop they don't know about, like gasPrice2 in the following example, it works. So it ignores the extra properties as it should. Is just the gasPrice is now being used for some reason! (and is not part of the params)

web3.eth.estimateGas({data: "0x095ea7b30000000000000000000000006f400810b62df8e13fded51be75ff5393eaa841fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffdec0de02120132", from: "0xd5cbc5e664813c7f1ce0a136aa154ce525e97e21", to: "0x6810e776880c02933d47db1b9fc05908e5386b96", gasPrice2: "0xcce416bb4"}, console.log)

types allow it, though
estimateGas > TransactionConfig

@anxolin anxolin merged commit 76b2b0c into hotfix/1.6.2 Nov 13, 2020
@Velenir Velenir mentioned this pull request Nov 18, 2020
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.

None yet

3 participants