feat: Add Human readable descriptions #2371

usame-algan · 2023-08-08T15:01:45Z

What it solves

First part of the EPIC #2011

Resolves #2448

Depends on safe-global/safe-client-gateway#631

How this PR fixes it

Displays human-readable descriptions for nonce replacement and notifications

ToDos

Update gateway-typescript-sdk type for TransactionInfo
Check remaining dispatch functions if they need a txDescription

How to test it

Queue a transaction (transfer, settings change, custom transaction)
Create a new transaction
Open the nonce replacement form
Observe human-readable descriptions
If there is no human-readable description observe the same message as before
Sign or execute a transaction
Observe a human-readable description inside notifications and the notification center
If there is no human-readable description observe a generic fallback saying Transaction as the Notification title

Screenshots

Checklist

I've tested the branch on mobile 📱
I've documented how it affects the analytics (if at all) 📊
I've written a unit/e2e test for it (if applicable) 🧑‍💻

github-actions · 2023-08-08T15:02:06Z

Branch preview

✅ Deploy successful!

https://human_readable_txs--walletweb.review-wallet-web.5afe.dev

github-actions · 2023-08-08T15:03:06Z

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	0	N/A

Result: ✅ success
Annotations: 0 total

^{Report generated by eslint-plus-action}

src/config/constants.ts

.vscode/settings.json

…her place

src/components/tx-flow/common/TxNonce/index.tsx

…acement text

github-actions · 2023-08-17T14:06:39Z

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	0	N/A

Result: ✅ success
Annotations: 0 total

^{Report generated by eslint-plus-action}

…istory

katspaugh · 2023-08-28T09:26:22Z

src/components/tx/SignOrExecuteForm/hooks.ts

+  addToBatch: (safeTx?: SafeTransaction, origin?: string, humanDescription?: string) => Promise<string>
+  signTx: (safeTx?: SafeTransaction, txId?: string, origin?: string, humanDescription?: string) => Promise<string>


Passing humanDescription to these two methods seems oddly specific. I would suggest passing the whole txDetails.

I've changed the signature to accept a transaction object instead.

katspaugh · 2023-08-28T09:27:52Z

src/hooks/useTxNotifications.ts


        dispatch(
          showNotification({
+            title: humanDescription,


What will it look like w/o a title?

I went through each tx event that we show a notification for and made sure to add a humanDescription there, either from the CGW response or an appropriate fallback i.e. Transaction #<nonce> or Module transaction

katspaugh · 2023-08-28T09:28:39Z

src/services/tx/tx-sender/dispatch.ts

+      txDispatch(TxEvent.SIGNATURE_PROPOSE_FAILED, {
+        txId,
+        error: asError(error),
+      })


Unnecessary formatting

Suggested change

txDispatch(TxEvent.SIGNATURE_PROPOSE_FAILED, {

txId,

error: asError(error),

})

txDispatch(TxEvent.SIGNATURE_PROPOSE_FAILED, { txId, error: asError(error) })

src/hooks/useTransactionType.ts

iamacook · 2023-08-28T11:16:43Z

src/hooks/useTxNotifications.ts

+  [TxEvent.SIGN_FAILED]: 'Failed to sign. Please try again.',
+  [TxEvent.PROPOSED]: 'Successfully added to queue.',
+  [TxEvent.PROPOSE_FAILED]: 'Failed to add to queue. Please try again.',
+  [TxEvent.SIGNATURE_PROPOSED]: 'Successfully signed.',
+  [TxEvent.SIGNATURE_PROPOSE_FAILED]: 'Failed to send signature. Please try again.',
+  [TxEvent.EXECUTING]: 'Confirm the execution in your wallet.',
+  [TxEvent.PROCESSING]: 'Validating...',
+  [TxEvent.PROCESSING_MODULE]: 'Validating module interaction...',
+  [TxEvent.ONCHAIN_SIGNATURE_REQUESTED]: 'Confirm on-chain signature in your wallet.',
+  [TxEvent.ONCHAIN_SIGNATURE_SUCCESS]: 'On-chain signature request confirmed.',
+  [TxEvent.PROCESSED]: 'Successfully validated. Indexing...',
+  [TxEvent.REVERTED]: 'Reverted. Please check your gas settings.',
+  [TxEvent.SUCCESS]: 'Successfully executed.',
+  [TxEvent.FAILED]: 'Failed.',


Why are these changes necessary? If there's no humanDescription, we now lose some context.

humanDescription will be always there unless the feature is disabled for technical reasons.

If the signature hash does not match on the gateway the humanDescription will be null. You can test this with spending limits atm:

Good point, I think we should add a fallback for the title. Maybe we can include the nonce, something like Transaction #113

iamacook · 2023-08-28T14:48:12Z

src/components/tx/SignOrExecuteForm/ExecuteForm.tsx

@@ -50,6 +52,8 @@ const ExecuteForm = ({
  const { setTxFlow } = useContext(TxModalContext)
  const { needsRiskConfirmation, isRiskConfirmed, setIsRiskIgnored } = useContext(TxSecurityContext)

+  const tx = useAppSelector((state) => selectQueuedTransactionById(state, txId))


If the transaction dispatchers (e.g. executeTx) were thunks, you could select this inside it as you already have the txId. It's an opinionated change though so I won't request it be changed.

src/components/tx/SignOrExecuteForm/hooks.ts

src/services/tx/tx-sender/dispatch.ts

src/config/constants.ts

src/services/tx/tx-sender/dispatch.ts

src/store/txHistorySlice.ts

src/services/tx/tx-sender/dispatch.ts

francovenica · 2023-08-29T03:42:12Z

This message is the same as the one in stg currently. in this case I tried to execute a batch tx (2 fully signed tx in queue, and using the Execute batch button), but these tx are incompatible so is expected for the batch to fail. (for example creating 2 tx that removes the same owner)

Idk if something more user friendly can be set, or if the information in the tx means something for the user.

francovenica · 2023-08-29T03:48:30Z

just "send"? in the description it shows that it should show what token you are sending, how much and to who.
In this case I'm testing in Goerli, sending GoerliETH

…ide of dispatch calls

usame-algan · 2023-08-29T08:49:14Z

@francovenica thanks for testing! I've fixed the "Send" issue. This was a regression, nice find! For Batch Executions I now added a static description so it should always say "Batch transaction" as the title.

usame-algan · 2023-08-29T09:15:19Z

@iamacook @katspaugh what do you think about removing all the custom fallbacks and only having one fallback to "Transaction" inside useTxNotifications. We would lose some information like the nonce but I don't think its too critical since thats how it was before.

katspaugh · 2023-08-29T09:15:48Z

src/store/txHistorySlice.ts

+          const HUMAN_DESCRIPTION_FALLBACK =
+            'Transaction ' +
+            (isMultisigExecutionInfo(result.transaction.executionInfo)
+              ? `#${result.transaction.executionInfo.nonce}`
+              : '')


Suggested change

const HUMAN_DESCRIPTION_FALLBACK =

'Transaction ' +

(isMultisigExecutionInfo(result.transaction.executionInfo)

? `#${result.transaction.executionInfo.nonce}`

: '')

const HUMAN_DESCRIPTION_FALLBACK =

'Transaction' +

(isMultisigExecutionInfo(result.transaction.executionInfo)

? ` #${result.transaction.executionInfo.nonce}`

: '')

I've reverted this change and use a generic fallback now.

iamacook · 2023-08-29T10:24:56Z

@iamacook @katspaugh what do you think about removing all the custom fallbacks and only having one fallback to "Transaction" inside useTxNotifications. We would lose some information like the nonce but I don't think its too critical since thats how it was before.

Having a SSOT makes the most sense.

github-actions · 2023-08-29T10:48:42Z

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	0	N/A

Result: ✅ success
Annotations: 0 total

^{Report generated by eslint-plus-action}

iamacook

There is a failing test.

francovenica · 2023-08-30T01:18:58Z

I tried the tx that Usame shared with me.
I was able to see most of them

I wasn't able to trigger "ONCHAIN_SIGNATURE_REQUESTED" and "ONCHAIN_SIGNATURE_SUCCESS". I'm gonna throw that, these probably were used back when every signature was on chain, even the ones that proposed a tx without execution.

The issue I reported about the nonce replacement message now show the tx name correctly

check the size of the nonce input tx display for mobile, looks fine as well:

Looks good to me

francovenica · 2023-08-30T01:42:04Z

EDIT:
Found an issue a last minute. a error message that show a "GS013" error

To reproduce this create 2 tx and batch them (the new feature "batch tx", not the "Execute batch" button in the queue). Make the 1st tx to send a small amout of any tokens from the safe and the 2nd one to send the Max amount. These 2 transactions cannot be executed in batch so the error happens

usame-algan · 2023-08-30T07:09:17Z

Found an issue a last minute. a error message that show a "GS013" error

Nice find! There was an issue with setting the fallback title. It should be fixed now and display Transaction at the top of the notification.

francovenica · 2023-08-30T13:29:25Z

Yes, it has it now:

Display human readable description if it exists

b1472a9

iamacook reviewed Aug 9, 2023

View reviewed changes

src/config/constants.ts Outdated Show resolved Hide resolved

iamacook reviewed Aug 9, 2023

View reviewed changes

.vscode/settings.json Outdated Show resolved Hide resolved

usame-algan added 2 commits August 10, 2023 12:09

Add some spacing

15db3c5

Add readable description to nonce replace form, remove it in every ot…

9f0bcb6

…her place

usame-algan marked this pull request as ready for review August 11, 2023 15:22

Show txDescription in notifications and notification center

f231cd7

usame-algan force-pushed the human-readable-txs branch from 5e29717 to f231cd7 Compare August 11, 2023 15:23

usame-algan changed the title ~~[PoC] Display human readable descriptions~~ Display human readable descriptions Aug 11, 2023

usame-algan requested review from iamacook and katspaugh August 11, 2023 15:23

usame-algan added 3 commits August 14, 2023 13:01

Merge remote-tracking branch 'origin/main' into human-readable-txs

ec3cdcf

Update gateway-sdk package, rename to humanDescription

c7e2653

Merge remote-tracking branch 'origin/dev' into human-readable-txs

e854d06

katspaugh reviewed Aug 17, 2023

View reviewed changes

src/components/tx-flow/common/TxNonce/index.tsx Show resolved Hide resolved

Display human description in txSummary if possible, update nonce repl…

89bdefc

…acement text

usame-algan added 3 commits August 18, 2023 10:43

fix: Adjust history column layout, display transfer descriptions in h…

1cea04a

…istory

Revert env change, hide human description in queue

82b12bb

Merge remote-tracking branch 'origin/dev' into human-readable-txs

9ececeb

usame-algan changed the title ~~Display human readable descriptions~~ feat: Add Human readable descriptions Aug 28, 2023

fix: Human-readable notifications

0154d0b

usame-algan force-pushed the human-readable-txs branch from 159e941 to 0154d0b Compare August 28, 2023 09:17

fix: Failing tests

31fc3fe

usame-algan requested a review from katspaugh August 28, 2023 09:26

katspaugh reviewed Aug 28, 2023

View reviewed changes

iamacook reviewed Aug 28, 2023

View reviewed changes

revert: Remove human descriptions from tx history

e761fd2

usame-algan requested a review from iamacook August 28, 2023 14:33

fix: Failing tests

4accc6b

iamacook reviewed Aug 28, 2023

View reviewed changes

fix: fallback label when replacing nonce, move humanDescriptions outs…

88c6a13

…ide of dispatch calls

usame-algan force-pushed the human-readable-txs branch from 57104ff to af34a83 Compare August 29, 2023 08:51

usame-algan requested a review from iamacook August 29, 2023 08:52

fix: Revert constants, add fallback for pending tx notifications

7749282

usame-algan force-pushed the human-readable-txs branch from af34a83 to 7749282 Compare August 29, 2023 09:02

fix: Add generic humanDescription fallback to notifications

286b398

katspaugh reviewed Aug 29, 2023

View reviewed changes

iamacook approved these changes Aug 29, 2023

View reviewed changes

fix: Remove custom fallbacks and use one fallback inside Notifications

2c33674

usame-algan force-pushed the human-readable-txs branch from 4dc8a28 to 2c33674 Compare August 29, 2023 11:00

revert: Changes to the tx history

c748e45

fix: Notification title fallback if human description is undefined

a7d513f

usame-algan added 2 commits August 30, 2023 11:34

Merge remote-tracking branch 'origin/dev' into human-readable-txs

3220be0

fix: Add bigger timeout to batch tx e2e test

83799b6

usame-algan merged commit 7659ab4 into dev Aug 30, 2023
9 checks passed

usame-algan deleted the human-readable-txs branch August 30, 2023 13:39

github-actions bot locked and limited conversation to collaborators Aug 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add Human readable descriptions #2371

feat: Add Human readable descriptions #2371

usame-algan commented Aug 8, 2023 •

edited

Loading

github-actions bot commented Aug 8, 2023 •

edited

Loading

github-actions bot commented Aug 8, 2023 •

edited

Loading

github-actions bot commented Aug 17, 2023 •

edited

Loading

katspaugh Aug 28, 2023

usame-algan Aug 28, 2023

katspaugh Aug 28, 2023

usame-algan Aug 28, 2023

katspaugh Aug 28, 2023

iamacook Aug 28, 2023

katspaugh Aug 28, 2023

iamacook Aug 28, 2023 •

edited

Loading

usame-algan Aug 28, 2023

iamacook Aug 28, 2023

francovenica commented Aug 29, 2023 •

edited

Loading

francovenica commented Aug 29, 2023

usame-algan commented Aug 29, 2023

usame-algan commented Aug 29, 2023 •

edited

Loading

katspaugh Aug 29, 2023

usame-algan Aug 29, 2023

iamacook commented Aug 29, 2023

github-actions bot commented Aug 29, 2023 •

edited

Loading

iamacook left a comment

francovenica commented Aug 30, 2023

francovenica commented Aug 30, 2023 •

edited

Loading

usame-algan commented Aug 30, 2023

francovenica commented Aug 30, 2023

		addToBatch: (safeTx?: SafeTransaction, origin?: string, humanDescription?: string) => Promise<string>
		signTx: (safeTx?: SafeTransaction, txId?: string, origin?: string, humanDescription?: string) => Promise<string>

feat: Add Human readable descriptions #2371

feat: Add Human readable descriptions #2371

Conversation

usame-algan commented Aug 8, 2023 • edited Loading

What it solves

How this PR fixes it

ToDos

How to test it

Screenshots

Checklist

github-actions bot commented Aug 8, 2023 • edited Loading

Branch preview

github-actions bot commented Aug 8, 2023 • edited Loading

ESLint Summary View Full Report

github-actions bot commented Aug 17, 2023 • edited Loading

ESLint Summary View Full Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iamacook Aug 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

francovenica commented Aug 29, 2023 • edited Loading

francovenica commented Aug 29, 2023

usame-algan commented Aug 29, 2023

usame-algan commented Aug 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iamacook commented Aug 29, 2023

github-actions bot commented Aug 29, 2023 • edited Loading

ESLint Summary View Full Report

iamacook left a comment

Choose a reason for hiding this comment

francovenica commented Aug 30, 2023

francovenica commented Aug 30, 2023 • edited Loading

usame-algan commented Aug 30, 2023

francovenica commented Aug 30, 2023

usame-algan commented Aug 8, 2023 •

edited

Loading

github-actions bot commented Aug 8, 2023 •

edited

Loading

github-actions bot commented Aug 8, 2023 •

edited

Loading

github-actions bot commented Aug 17, 2023 •

edited

Loading

iamacook Aug 28, 2023 •

edited

Loading

francovenica commented Aug 29, 2023 •

edited

Loading

usame-algan commented Aug 29, 2023 •

edited

Loading

github-actions bot commented Aug 29, 2023 •

edited

Loading

francovenica commented Aug 30, 2023 •

edited

Loading