Fix: deep-linked tx status and internal state #3156

DiogoSoaress · 2021-12-10T00:26:37Z

What it solves

How this PR fixes it

When adding queued transactions, it no longer mutates the current state, instead setting it to what backend returns. The "deeplinked" transaction details page persists the loaded transaction in state (if the automatic polling doesn't return queued transactions).

Client-side "PENDING"/PENDING_FAILED statuses have been moved to their own localStatuses key of the store, so as to not pollute the data returned from backend.

Note: the URL structure for deeplinked transactions has been reverted to using the safeTxHash, as follows:

.../app/{shortName}:{safeAddress}/transactions/{safeTxHash}

How to test it

Immediate transaction

Create a transaction that executes immediately.
Expect it to open the deeplinked transaction.
Observe the pending spinner that eventually changed to success.
After successful execution, check that it is only present in the history list.

Queued transaction

Open a confirmed, deeplink transaction.
Execute the transaction and expect points 3-4 above.

github-actions · 2021-12-10T00:26:52Z

CLA Assistant Lite All Contributors have signed the CLA.

github-actions · 2021-12-10T00:31: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	3	N/A

Result: ✅ success
Annotations: 0 total

^{Report generated by eslint-plus-action}

github-actions · 2021-12-10T00:33:06Z

Deployment links

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

coveralls · 2021-12-10T00:34:04Z

Pull Request Test Coverage Report for Build 1601361648

13 of 132 (9.85%) changed or added relevant lines in 21 files are covered.
14 unchanged lines in 9 files lost coverage.
Overall coverage increased (+0.03%) to 32.278%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/logic/safe/transactions/awaitingTransactions.ts	0	1	0.0%
src/logic/safe/utils/aboutToExecuteTx.ts	0	1	0.0%
src/routes/safe/components/Transactions/TxList/TxQueueRow.tsx	0	1	0.0%
src/routes/safe/components/Transactions/TxList/TxCollapsedActions.tsx	0	2	0.0%
src/logic/safe/store/reducer/localTransactions.ts	2	5	40.0%
src/routes/safe/components/Transactions/TxList/TxCollapsed.tsx	0	3	0.0%
src/routes/safe/components/Transactions/TxList/TxDetails.tsx	0	3	0.0%
src/routes/safe/components/Transactions/TxList/TxExpandedActions.tsx	1	4	25.0%
src/routes/safe/components/Transactions/TxList/hooks/useActionButtonsHandlers.ts	0	3	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.28%
src/logic/safe/store/middleware/notificationsMiddleware.ts	1	28.57%
src/routes/safe/components/Transactions/TxList/TxCollapsed.tsx	1	5.94%
src/routes/safe/components/Transactions/TxList/TxDetails.tsx	1	5.77%
src/routes/safe/components/Transactions/TxList/TxOwners.tsx	1	5.41%
src/routes/safe/components/Transactions/TxList/TxQueueCollapsed.tsx	1	8.7%
src/routes/safe/components/Transactions/TxList/TxSingularDetails.tsx	2	1.85%
src/logic/safe/store/actions/utils.ts	3	87.1%
src/logic/safe/store/reducer/gatewayTransactions.ts	3	2.3%

Totals
Change from base Build 1588198876:	0.03%
Covered Lines:	3083
Relevant Lines:	8488

💛 - Coveralls

github-actions · 2021-12-10T00:40:20Z

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

iamacook

Looking good. I found a few things and it seems the Polygon colour has been modified in the CGW.

src/logic/safe/store/reducer/gatewayTransactions.ts

iamacook · 2021-12-10T14:41:35Z

@DiogoSoaress, as this is your PR I cannot request your review. Please let me know what you think and "I" will approve it on your behalf.

katspaugh

Good to see a massive chunk of code to be gone!

iamacook

Approving after confirmation from @DiogoSoaress.

DiogoSoaress · 2021-12-13T08:37:27Z

Looks good @iamacook 👍

francovenica · 2021-12-14T03:41:39Z

Tx are shown only on the history tab. That works fine.

Still there are issues with the tx in the "Deep link view" that were not there before.
The tx reloads a couple of times while is executing and when it ends it shows a "cancelled" status.

@iamacook @DiogoSoaress is this re-rendering of the tx something we should worry about? or we can tackle it in another ticket?

Gif: is over 1 minute long

iamacook · 2021-12-15T10:44:27Z

is this re-rendering of the tx something we should worry about? or we can tackle it in another ticket?

As they are so tightly linked, I included the fixes for #3071 in this ticket (I will update the desc. when all is working). The remaining issue of the cancelled status I will include in this PR and, if it is too much of a rabbit hole, will open a separate issue for the re-rendering. I hope to include that in this PR however.

katspaugh · 2021-12-16T17:45:59Z

src/routes/safe/components/Transactions/TxList/TxCollapsed.tsx

  // attaching ref to a div element as it was causing troubles to add a `ref` to a FunctionComponent
  const txCollapsedStatus = (
    <div className="tx-status" ref={sameString(lastItemId, transaction.id) ? ref : null}>
-      {transaction?.txStatus === 'PENDING' || transaction?.txStatus === 'PENDING_FAILED' ? (


Dunno why it was also showing a loader for PENDING_FAILED. 🤷

iamacook

Just a couple of findings

src/logic/safe/store/models/types/gateway.d.ts

src/store/index.ts

iamacook · 2021-12-17T10:37:33Z

When a transaction immediately executes, a notification is shown about confirmation still being required and no confirmations are shown in the owners stepper:

I'm pretty sure this is shown because the transaction passed to the reducer retains the 'stale' status and has an empty confirmations array. Maybe it does instead make more sense to keep the local status in the transaction object and, if it has a client-side "PENDING" status, fill the confirmations array with the current user?

The client-side statuses also never clear from the local state. They should likely be removed when the transaction is successful (both of the following are successful transactions):

katspaugh · 2021-12-20T13:02:45Z

src/logic/safe/store/middleware/notificationsMiddleware.ts

+          }
+
+          const hasLocalStatus = transactions.some((tx) =>
+            localStatusedSafeTxHashes.some((safeTxHash) => tx.id.includes(safeTxHash)),


Isn't tx.id different than safeTxHash? Or in this case it's same?

Transaction summaries do not return the safeTxHash. The id, however, contains it.

Is it in a random place or can we extract the exact safeTxHash part and use it for a direct hash map lookup?

From my understanding, it is the last part of the id but I am not 100% sure. It would be ideal if the safeTxHash was returned as part of the summary but that is something we'd have to discuss with backend in the new year.

katspaugh · 2021-12-20T13:03:26Z

src/logic/safe/store/middleware/notificationsMiddleware.ts

+            break
+          }
+
+          const hasLocalStatus = transactions.some((tx) =>


Why do you need to search the array or keys instead of just looking up the key in the hash map?

We don't have the safeTxHash. Please check my other response.

francovenica · 2021-12-21T04:18:29Z

Let me know if I can report these here or I should create new tickets.

If you create a tx out of nonce order you see the tx ready to be executed. The execute button should be disabled.
Only the execute button is wrong, the Reject button is fine to be available
To reproduce it, in a safe with a policy of 1 out of X create a tx 1 nonce higher than the one the safe expect to execute.

iamacook · 2021-12-21T08:13:20Z

If you create a tx out of nonce order you see the tx ready to be executed. The execute button should be disabled. Only the execute button is wrong, the Reject button is fine to be available To reproduce it, in a safe with a policy of 1 out of X create a tx 1 nonce higher than the one the safe expect to execute.

This looks like a bug relating to non-owner execution of confirmed transactions. Can you confirm whether that was tested in that issue?

francovenica · 2021-12-21T13:47:30Z

This looks like a bug relating to non-owner execution of confirmed transactions. Can you confirm whether that was tested in that issue?

I tested it in the #3151 and is not happening there. when a tx out of order is created the confirm button is disabled which is correct

Diogo Soares added 2 commits December 10, 2021 01:19

Enable navigate to TxDetails

e388b44

Remove duplicated txs in the queue queue

dd75f0c

iamacook reviewed Dec 10, 2021

View reviewed changes

src/logic/safe/store/reducer/gatewayTransactions.ts Outdated Show resolved Hide resolved

src/logic/safe/store/reducer/gatewayTransactions.ts Outdated Show resolved Hide resolved

src/logic/safe/store/reducer/gatewayTransactions.ts Outdated Show resolved Hide resolved

iamacook reviewed Dec 10, 2021

View reviewed changes

src/logic/safe/store/reducer/gatewayTransactions.ts Outdated Show resolved Hide resolved

iamacook added 2 commits December 10, 2021 15:13

chore: Overwrite queued state in action

0e4f943

chore: Restore history action

86d3a3d

iamacook requested a review from katspaugh December 10, 2021 14:40

iamacook marked this pull request as ready for review December 10, 2021 14:40

katspaugh approved these changes Dec 10, 2021

View reviewed changes

iamacook approved these changes Dec 13, 2021

View reviewed changes

fix: Send pending flag with transaction creation

125e1bb

iamacook marked this pull request as draft December 15, 2021 10:44

iamacook and others added 3 commits December 16, 2021 11:23

build: Deeplinking details loading

a7a3c19

Don't re-render on queue poll

a1cac28

Refactor hooks

6ac43b0

iamacook mentioned this pull request Dec 16, 2021

Remove client-side transaction statuses safe-global/safe-gateway-typescript-sdk#40

Closed

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

Deep link - Inconsistent data while the a tx is being executed #3071

Closed

iamacook changed the title ~~Duplicated tx in queued~~ Inconsistent data on executing transactions, duplicated across queue/history Dec 16, 2021

katspaugh added 3 commits December 16, 2021 18:01

txStatus

cd9e46d

Merge with dev

c844524

Fix tests

de2f702

Fix TransactionStatus enum

79563ab

katspaugh reviewed Dec 16, 2021

View reviewed changes

Add a comment

0e3d8d3

katspaugh marked this pull request as ready for review December 16, 2021 17:52

katspaugh changed the title ~~Inconsistent data on executing transactions, duplicated across queue/history~~ Fix: deep-linked tx status and internal state Dec 16, 2021

iamacook reviewed Dec 16, 2021

View reviewed changes

src/logic/safe/store/models/types/gateway.d.ts Show resolved Hide resolved

src/store/index.ts Show resolved Hide resolved

katspaugh added 3 commits December 17, 2021 09:24

useLocalTxStatus hook

609582f

Clean up

9a39ff2

Fix status flicker

95002df

fix: Prevent sign notification when executing

0eb232a

iamacook requested a review from katspaugh December 20, 2021 10:04

iamacook mentioned this pull request Dec 20, 2021

Persist/sanitise local transaction statuses #3194

Closed

katspaugh reviewed Dec 20, 2021

View reviewed changes

iamacook mentioned this pull request Dec 21, 2021

Deep Link - Out of order parallel txs are dissapearing #3202

Closed

iamacook merged commit 8815f8b into dev Dec 21, 2021

iamacook deleted the duplicated-tx-in-queued branch December 21, 2021 14:20

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

Fix: deep-linked tx status and internal state #3156

Fix: deep-linked tx status and internal state #3156

Uh oh!

Conversation

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

Uh oh!

What it solves

How this PR fixes it

How to test it

Immediate transaction

Queued transaction

Uh oh!

github-actions bot commented Dec 10, 2021

Uh oh!

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

Uh oh!

ESLint Summary View Full Report

Uh oh!

github-actions bot commented Dec 10, 2021

Deployment links

Uh oh!

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

Uh oh!

Pull Request Test Coverage Report for Build 1601361648

💛 - Coveralls

Uh oh!

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

Uh oh!

Uh oh!

iamacook left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

iamacook commented Dec 10, 2021

Uh oh!

katspaugh left a comment

Choose a reason for hiding this comment

Uh oh!

iamacook left a comment

Choose a reason for hiding this comment

Uh oh!

DiogoSoaress commented Dec 13, 2021

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

katspaugh Dec 16, 2021

Choose a reason for hiding this comment

Uh oh!

iamacook left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

iamacook commented Dec 17, 2021

Uh oh!

katspaugh Dec 20, 2021

Choose a reason for hiding this comment

Uh oh!

iamacook Dec 20, 2021

Choose a reason for hiding this comment

Uh oh!

katspaugh Dec 20, 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 20, 2021

Choose a reason for hiding this comment

Uh oh!

katspaugh Dec 20, 2021

Choose a reason for hiding this comment

Uh oh!

iamacook Dec 20, 2021

Choose a reason for hiding this comment

DiogoSoaress commented Dec 10, 2021 •

edited by iamacook

Loading

github-actions bot commented Dec 10, 2021 •

edited

Loading

coveralls commented Dec 10, 2021 •

edited

Loading

github-actions bot commented Dec 10, 2021 •

edited

Loading

francovenica commented Dec 14, 2021 •

edited

Loading

iamacook commented Dec 15, 2021 •

edited

Loading

katspaugh Dec 20, 2021 •

edited

Loading

francovenica commented Dec 21, 2021 •

edited

Loading