(Feature) #1515 Gas estimation for NON-GETH nodes by Agupane · Pull Request #1523 · 5afe/safe-react

Agupane · 2020-10-24T01:28:30Z

Closes #1515, by:

Handling ~~non-geth~~ OpenEthereum/Parity nodes errors
Refactoring estimateSafeTxGas
Throwing errors when the transaction may fail, this may be useful for There should be a warning if we know that a Safe tx will fail #1048

Notes:

The main point for understanding the implementation is to know that GETH/Nethermind nodes will return the response of the call as a response and OpenEthereum nodes will always return the response as an error, to know if the error contains a valid value we need to check if the error data contains the string Reverted <resultHash>, in that case, we known that the transaction was successful and we can extract the hash result, otherwise the transaction has failed.
Please also check the comments on the code
To test this is better to use xDai because the rpc we use is OpenEthereum https://rpc.xdaichain.com/ / https://dai.poa.network/:
Post request:

{
    "jsonrpc": "2.0",
    "method": "web3_clientVersion",
    "id": 1
}

Result:

{
    "jsonrpc": "2.0",
    "result": "OpenEthereum//v3.0.1-stable-8ca8089-20200601/x86_64-alpine-linux-musl/rustc1.43.1",
    "id": 1
}

github-actions · 2020-10-24T01:28:44Z

CLA Assistant Lite All Contributors have signed the CLA.

github-actions · 2020-10-24T01:33:34Z

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 · 2020-10-24T01:43:09Z

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

rmeissner · 2020-10-26T10:40:03Z

src/logic/safe/transactions/gas.ts

+  const web3 = getWeb3()
+  for (const additionalGasIterator of additionalGasBatches) {
+    try {
+      await web3.eth.call({


Why did we remove the batch request support?

Because for batch requests we need to use web3.eth.call.request() instead of web3.eth.call(). The first method does not returns the error as data that we need.

Responses examples:

Batch request:
Error: Returned error: Internal JSON-RPC error.

Non-batch request:

Error: Internal JSON-RPC error. { "code": -32015, "message": "VM execution error.", "data": "Reverted 0x08c379a0000000000000000000000000000000000000000000000000000000000000002000000000000000000000000000000000000000000000000000000000000000200000000000000000000000000000000000000000000000000000000000006457" }

🤔 hmmm maybe in this case we should adjust it for all ... while we might have to do 1 or 2 more request the code overhead might not be worth it.

what do you think? @fernandomg

@rmeissner, by that you mean to get rid of the batchRequest approach?

I'm perfectly fine with that.

It's easy to follow, in the worst-case scenario we'll do 10 RPC requests (one after the other, not triggered in parallel), and the user may experiment a delay (we should think of adding something around "calculating gas..." statement somewhere, but in a different issue).

yes ... but maybe @dasanra or @mikheevm have a better idea ... but since batched requests truncate the response it is not really possible to always use it.

we'll do 10 RPC requests (one after the other, not triggered in parallel)

that's because we are using await, right? if we make use of promise.all() it should not be the case, isn't it?
@fernandomg

I think not doing them in parallel might be better here, as in most cases we will probably only need 1 or 2 requests, perform all 10 for this always would be quite some additional overhead.

I removed the batch request and simplified the logic, now it doesn't matter which node it's running the code, the function getGasEstimationTxResponse will take care of it and return a result, then I replaced calculateGasForOpenEthereumNodes / calculateGasForGethAndNethermindNodes with the function calculateMinimumGasForTransaction. I think it's more clear now, please give it a check

rmeissner · 2020-10-26T10:41:48Z

src/logic/safe/transactions/gas.ts

+  const errorAsJSON = JSON.parse(error.join(''))
+
+  if (errorAsJSON?.data) {
+    const [, dataResult] = errorAsJSON.data.split('Reverted ')


This only works for OpenEthereum/Parity ... but not for Nethermind

Yes you are right, I check this again and replaced non-geth comments/checks with OpenEthereum because it's the one failing. On Geth and Nethermind nodes, the initial implementation it's working (nethermind returns the result the same way as get, not as an error like openEthereum).

src/logic/safe/transactions/gas.ts

rmeissner

I would really like to see unit tests for the parsing of the error message here. (E.g. getNonGethErrorDataResult and if possible also extract the logic for the other case into a method too and test it)

src/logic/safe/transactions/gas.ts

ghost · 2020-10-26T13:36:56Z

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

ghost · 2020-10-26T14:54:38Z

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

rmeissner · 2020-10-26T17:07:47Z

src/logic/safe/transactions/gas.ts

+  const errorAsJSON = JSON.parse(error.join(''))
+
+  if (errorAsJSON?.data) {
+    const [, dataResult] = errorAsJSON.data.split('Reverted ')


Maybe instead if split by Reverted we split by (space), this way we also handle Nethermind (which uses revert <hex data>

Do you have a nethermind endpoint rpc that I can use for testing the response?

Edit: https://rpc.xdaichain.com/ uses OpenEthereum not Nethermind:

Hmmm seems they switched back .... annoying ... so we can only test this in our tests with the data from the issue ... should be enough for now :/

I refactored the file to support nethermind and added a test case for it :)

Tests have been added

…to feature/1515-safe-tx-estimation

nicosampler · 2020-10-27T12:40:30Z

src/logic/safe/transactions/gas.ts

  }
 }

+const getOpenEthereumErrorDataResult = (errorMessage: string): string | undefined => {


does it make sense to move this function to a more generic place?

I'm not sure because it will only be used here, do you have any ideas?

src/logic/safe/transactions/gas.ts

… gas or throws errors based on the current node

Updates tests

ghost · 2020-10-27T15:35:49Z

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

ghost · 2020-10-27T15:56:27Z

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

fernandomg

LGTM

ghost · 2020-10-28T12:26:27Z

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

ghost · 2020-10-28T16:01:37Z

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

mmv08

Good job! It was a pleasure to review that after reading a nicely-written description, keep it up 💪

Agupane · 2020-10-29T12:08:31Z

Good job! It was a pleasure to review that after reading a nicely-written description, keep it up 💪

Thank you! 💪

Agupane added 2 commits October 23, 2020 22:09

Remove unnecesary await

28c3737

Implement gas calculation for NON-GETH nodes

94871f5

Agupane requested review from dasanra, francovenica, mmv08, nicosampler and rmeissner October 24, 2020 01:28

Agupane assigned fernandomg and Agupane Oct 24, 2020

rmeissner reviewed Oct 26, 2020

View reviewed changes

src/logic/safe/transactions/gas.ts Outdated Show resolved Hide resolved

rmeissner previously requested changes Oct 26, 2020

View reviewed changes

src/logic/safe/transactions/gas.ts Outdated Show resolved Hide resolved

Agupane added 2 commits October 26, 2020 10:21

Use await on return

d5e1bc3

Reorder functions

1d608d6

Replaces non-geth wit openEthereum

70d9e61

rmeissner reviewed Oct 26, 2020

View reviewed changes

Add tests

7f1e4d4

Merge branch 'development' of https://github.com/gnosis/safe-react in…

9a09ff7

…to feature/1515-safe-tx-estimation

nicosampler reviewed Oct 27, 2020

View reviewed changes

src/logic/safe/transactions/gas.ts Show resolved Hide resolved

Agupane added 2 commits October 27, 2020 12:01

Refactor estimateSafeTxGas: now getGasEstimationTxResponse calculates…

5908a03

… gas or throws errors based on the current node

Refactor getOpenEthereumErrorDataResult to make it works with nethermind

78a6587

Updates tests

Agupane requested a review from fernandomg October 27, 2020 15:23

Agupane requested review from nicosampler and rmeissner October 27, 2020 15:23

Improve commets

9c9f7a3

fernandomg approved these changes Oct 27, 2020

View reviewed changes

nicosampler approved these changes Oct 27, 2020

View reviewed changes

Merge branch 'development' into feature/1515-safe-tx-estimation

fad36fb

dasanra approved these changes Oct 28, 2020

View reviewed changes

Disable Volta build for this branch

050061f

dasanra merged commit 551db13 into development Oct 28, 2020

dasanra deleted the feature/1515-safe-tx-estimation branch October 28, 2020 16:03

github-actions bot locked and limited conversation to collaborators Oct 28, 2020

dasanra restored the feature/1515-safe-tx-estimation branch October 28, 2020 16:06

mmv08 reviewed Oct 29, 2020

View reviewed changes

dasanra deleted the feature/1515-safe-tx-estimation branch October 29, 2020 08:28

Conversation

Agupane commented Oct 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Oct 24, 2020

Uh oh!

github-actions bot commented Oct 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ESLint Summary View Full Report

Uh oh!

ghost commented Oct 24, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fernandomg Oct 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicosampler Oct 27, 2020 • edited by rmeissner Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rmeissner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ghost commented Oct 26, 2020

Uh oh!

ghost commented Oct 26, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Agupane Oct 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ghost commented Oct 27, 2020

Uh oh!

ghost commented Oct 27, 2020

Uh oh!

fernandomg left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Oct 28, 2020

Uh oh!

ghost commented Oct 28, 2020

Uh oh!

mmv08 left a comment

Choose a reason for hiding this comment

Uh oh!

Agupane commented Oct 29, 2020

Uh oh!

Reviewers

Assignees

Agupane commented Oct 24, 2020 •

edited

Loading

github-actions bot commented Oct 24, 2020 •

edited

Loading

fernandomg Oct 26, 2020 •

edited

Loading

nicosampler Oct 27, 2020 •

edited by rmeissner

Loading

Agupane Oct 26, 2020 •

edited

Loading