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

(Fix) Estimate gas for wallet connect #1806

Merged
merged 16 commits into from
Jan 28, 2021

Conversation

Agupane
Copy link
Contributor

@Agupane Agupane commented Jan 22, 2021

Closes #1784 by:

  • Removing web.eth.call as a way for doing the gas estimation, instead now it's using directly an eth_call to infura. The reason it's that depending on the provider implementation the results are different, in some cases, like walletConnect eth_call is not even implemented. I thought that a good way to fix this is just to use our own web3 with infura as provider, but for some reason, if we are using walletConnect the result of the eth_call has no data. So I decided to ignore web3 implementation and use directly infura with a post call.
  • This is nice because we don't care about the provider.
  • This will be available only for networks supported by infura, in case that we are in a not-infura-supported network, we do the old estimation.
  • Also fix some old react warnings like the warning that appears when the sendFunds modal is open

@Agupane Agupane self-assigned this Jan 22, 2021
@Agupane Agupane requested a review from fernandomg January 22, 2021 16:20
@github-actions
Copy link

CLA Assistant Lite All Contributors have signed the CLA.

@github-actions
Copy link

github-actions bot commented Jan 22, 2021

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

@ghost
Copy link

ghost commented Jan 22, 2021

Travis automatic deployment:
https://pr1806--safereact.review.gnosisdev.com/rinkeby/app

@ghost
Copy link

ghost commented Jan 22, 2021

Travis automatic deployment:
https://pr1806--safereact.review.gnosisdev.com/volta/app

Copy link
Contributor

@matextrem matextrem left a comment

Choose a reason for hiding this comment

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

LGTM 👍 . I like the new clearer and simpler implementation!

@ghost
Copy link

ghost commented Jan 25, 2021

Travis automatic deployment:
https://pr1806--safereact.review.gnosisdev.com/rinkeby/app

@ghost
Copy link

ghost commented Jan 25, 2021

Travis automatic deployment:
https://pr1806--safereact.review.gnosisdev.com/volta/app

src/logic/safe/transactions/gas.ts Outdated Show resolved Hide resolved
src/logic/safe/transactions/gas.ts Outdated Show resolved Hide resolved
Comment on lines +78 to +80
const InputAdornmentChildSymbol = ({ symbol }: { symbol?: string }): ReactElement => {
return <>{symbol}</>
}
Copy link
Contributor

Choose a reason for hiding this comment

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

why is it needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

Replaces web3 with web3ReadOnly in estimateGasWithRPCCall
@Agupane Agupane requested a review from fernandomg January 26, 2021 19:01
@ghost
Copy link

ghost commented Jan 26, 2021

Travis automatic deployment:
https://pr1806--safereact.review.gnosisdev.com/rinkeby/app

@ghost
Copy link

ghost commented Jan 26, 2021

Travis automatic deployment:
https://pr1806--safereact.review.gnosisdev.com/volta/app

@ghost
Copy link

ghost commented Jan 26, 2021

Travis automatic deployment:
https://pr1806--safereact.review.gnosisdev.com/rinkeby/app

@ghost
Copy link

ghost commented Jan 26, 2021

Travis automatic deployment:
https://pr1806--safereact.review.gnosisdev.com/volta/app

@ghost
Copy link

ghost commented Jan 26, 2021

Travis automatic deployment:
https://pr1806--safereact.review.gnosisdev.com/rinkeby/app

@ghost
Copy link

ghost commented Jan 26, 2021

Travis automatic deployment:
https://pr1806--safereact.review.gnosisdev.com/volta/app

@francovenica
Copy link
Contributor

francovenica commented Jan 27, 2021

https://pr1806--safereact.review.gnosisdev.com/rinkeby/app/#/safes/0x9913B9180C20C6b0F21B6480c84422F6ebc4B808/transactions

Tested WC with
Regular tx (send funds)
collectibles transfer
Rejecting a tx

Looks good to me, Didn't see the warning message if the tx would succeed. I saw the warning for a Tx builder type of tx that ended up failed (expected outcome)

@ghost
Copy link

ghost commented Jan 27, 2021

Travis automatic deployment:
https://pr1806--safereact.review.gnosisdev.com/rinkeby/app

@dasanra dasanra merged commit 758fc3c into development Jan 28, 2021
@dasanra dasanra deleted the fix/estimate-gas-wallet-connect branch January 28, 2021 08:12
@github-actions github-actions bot locked and limited conversation to collaborators Jan 28, 2021
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.

WallectConnect - Tx failure warning always shows when you try to sign
5 participants