Fix: use estimations v2 endpoint for tx nonce #3201

DiogoSoaress · 2021-12-20T23:35:09Z

What it solves

Resolves #3190

How this PR fixes it

Uses the estimations endpoint v2 to fetch the next recommended txNonce

A Cancel transaction is used if we are only interested in fetching the recommendedNonce
POST body:

{
    "to": "0xc1e55e565a40BB76686C94E0688be8F00cbE9d60",
    "value": "0",
    "data": "0x",
    "operation": 0
}

github-actions · 2021-12-20T23:35:22Z

CLA Assistant Lite All Contributors have signed the CLA.

DiogoSoaress · 2021-12-20T23:36:44Z

Waiting for the safe-react-gateway-sdk to publish a new version.

github-actions · 2021-12-20T23:39:24Z

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}

coveralls · 2021-12-20T23:42:04Z

Pull Request Test Coverage Report for Build 1610849559

5 of 18 (27.78%) changed or added relevant lines in 5 files are covered.
13 unchanged lines in 5 files lost coverage.
Overall coverage increased (+0.1%) to 32.197%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/logic/safe/api/fetchSafeTxGasEstimation.ts	3	4	75.0%
src/logic/safe/transactions/gas.ts	0	2	0.0%
src/routes/safe/container/hooks/useTransactionParameters.ts	2	4	50.0%
src/logic/safe/store/actions/createTransaction.ts	0	4	0.0%
src/logic/safe/store/actions/processTransaction.ts	0	4	0.0%

Files with Coverage Reduction	New Missed Lines	%
src/logic/safe/store/actions/createTransaction.ts	1	3.23%
src/logic/safe/store/actions/processTransaction.ts	1	1.08%
src/logic/notifications/store/reducer/notifications.ts	3	6.9%
src/routes/safe/components/Apps/hooks/appList/useRemoteSafeApps.ts	3	75.0%
src/logic/notifications/store/actions/enqueueSnackbar.tsx	5	27.27%

Totals
Change from base Build 1610811340:	0.1%
Covered Lines:	3080
Relevant Lines:	8496

💛 - Coveralls

github-actions · 2021-12-20T23:43:52Z

Deployment links

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

github-actions · 2021-12-20T23:52:05Z

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

Failed tests:

❌ Read-only transaction creation and review Read-only transaction creation and review

iamacook · 2021-12-21T08:05:16Z

src/logic/safe/api/fetchSafeTxGasEstimation.ts

-    ({ safeTxGas }) => safeTxGas,
-  )
+}: FetchSafeTxGasEstimationProps): Promise<SafeTransactionEstimationV2> => {
+  return postSafeGasEstimationV2(GATEWAY_URL, _getChainId(), checksumAddress(safeAddress), body).then((data) => data)


You don't need the then() method here.

katspaugh · 2021-12-21T08:18:47Z

src/logic/safe/api/fetchSafeTxGasEstimation.ts

+import {
+  postSafeGasEstimationV2,
+  SafeTransactionEstimationRequest,
+  SafeTransactionEstimationV2,


This V2 looks off. Let's rename the old one to SafeTransactionEstimationV1 and call the new one simply SafeTransactionEstimation.

Is SafeTransactionEstimation v0 or v2? I'd stay as explicit as possible or rename old v1 to something else

We are only using what is now v2 in frontend. Do you think it would be necessary to keep the 'legacy' function/type? Our docs would only show the v2 response, from my understanding.

katspaugh · 2021-12-22T09:16:52Z

src/logic/safe/api/fetchSafeTxGasEstimation.ts

+    safeAddress,
+    value: '0',
+    operation: Operation.CALL,
+    // Workaround: use a cancellation transaction to fetch only the recommendedNonce


In future, we should make just one request – to fetch the actual safeTxGas and the nonce. They are displayed on the same Review screen.

katspaugh · 2021-12-22T09:17:59Z

src/logic/safe/store/actions/createTransaction.ts

-    const nextNonce = await getNewTxNonce(lastTxNonce, safeInstance)
+    let nextNonce: string
+    try {
+      nextNonce = (await getRecommendedNonce(safeAddress)).toString()


There's already a toString() on line 128.

katspaugh

LGTM 👍

katspaugh · 2021-12-22T09:51:58Z

I'll merge this to dev because it's broken after Safe Apps were moved to the SDK. This new version fixes it.

…ns-v2-endpoint-for-tx-nonce

Diogo Soares added 2 commits December 21, 2021 00:05

chore: Update safe-react-gateway-sdk dependency

d5b2f09

build: Use the new estimation endpoint v2 to calculate the next txNonce

893caee

iamacook reviewed Dec 21, 2021

View reviewed changes

katspaugh reviewed Dec 21, 2021

View reviewed changes

Diogo Soares added 4 commits December 21, 2021 11:10

fix: Update the cgw sdk version and method calls

257b5d6

build: Use a cancel tx to fetch only the recommended nonce

87663bc

Add try catch in case backend request fails

b404fba

Replace getting next Tx nonce from the store by the tx estimation call

f7bb237

DiogoSoaress marked this pull request as ready for review December 21, 2021 11:55

DiogoSoaress requested a review from katspaugh December 21, 2021 11:55

katspaugh reviewed Dec 22, 2021

View reviewed changes

katspaugh approved these changes Dec 22, 2021

View reviewed changes

katspaugh changed the title ~~Use estimations v2 endpoint for tx nonce~~ Fix: use estimations v2 endpoint for tx nonce Dec 22, 2021

Merge branch 'main' into use-estimations-v2-endpoint-for-tx-nonce

1f50be2

iamacook linked an issue Dec 22, 2021 that may be closed by this pull request

Incorrect nonce after transaction submission #3190

Closed

DiogoSoaress changed the base branch from main to dev December 22, 2021 09:54

katspaugh and others added 4 commits December 22, 2021 11:05

Merge branch 'dev' into use-estimations-v2-endpoint-for-tx-nonce

70b1db1

Merge branch 'dev' of github.com:gnosis/safe-react into use-estimatio…

a34ad9e

…ns-v2-endpoint-for-tx-nonce

fix: remove unused imports

5e468fe

fix: remove v1 form the GATEWAY_URL

6e983ee

DiogoSoaress merged commit 7c4b469 into dev Dec 22, 2021

DiogoSoaress deleted the use-estimations-v2-endpoint-for-tx-nonce branch December 22, 2021 10:26

github-actions bot locked and limited conversation to collaborators Dec 22, 2021

Fix: use estimations v2 endpoint for tx nonce #3201

Fix: use estimations v2 endpoint for tx nonce #3201

Uh oh!

Conversation

DiogoSoaress commented Dec 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What it solves

How this PR fixes it

Uh oh!

github-actions bot commented Dec 20, 2021

Uh oh!

DiogoSoaress commented Dec 20, 2021

Uh oh!

github-actions bot commented Dec 20, 2021 • 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 Dec 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 1610849559

💛 - Coveralls

Uh oh!

github-actions bot commented Dec 20, 2021

Deployment links

Uh oh!

github-actions bot commented Dec 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iamacook Dec 21, 2021

Choose a reason for hiding this comment

Uh oh!

katspaugh Dec 21, 2021

Choose a reason for hiding this comment

Uh oh!

mmv08 Dec 21, 2021 • 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!

iamacook Dec 21, 2021

Choose a reason for hiding this comment

Uh oh!

katspaugh Dec 22, 2021

Choose a reason for hiding this comment

Uh oh!

katspaugh Dec 22, 2021

Choose a reason for hiding this comment

Uh oh!

katspaugh left a comment

Choose a reason for hiding this comment

Uh oh!

katspaugh commented Dec 22, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

DiogoSoaress commented Dec 20, 2021 •

edited

Loading

github-actions bot commented Dec 20, 2021 •

edited

Loading

coveralls commented Dec 20, 2021 •

edited

Loading

github-actions bot commented Dec 20, 2021 •

edited

Loading

mmv08 Dec 21, 2021 •

edited

Loading