(Feature) [Spending Limits] Transaction List details by fernandomg · Pull Request #1271 · 5afe/safe-react

fernandomg · 2020-08-25T12:34:13Z

This PR closes #691, by:

identifying and displaying tx details for Spending Limit txs

also displays hex data for those txs that don't have decodedData and wraps its content to avoid overflow of text:

github-actions · 2020-08-25T12:34:30Z

CLA Assistant Lite All Contributors have signed the CLA.

github-actions · 2020-08-25T12:39: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	9	0
Ignored	5	N/A

Result: ✅ success
Annotations: 9 total

[warning] @typescript-eslint/explicit-module-boundary-types

Require explicit return and argument types on exported functions' and classes' public class methods

src/routes/safe/components/Balances/SendModal/screens/SendFunds/index.tsx Line 71 - Object pattern argument should be typed.
src/routes/safe/components/Transactions/TxsTable/ExpandedTxRow/IncomingTx/index.tsx Line 7 - Missing return type on function.
src/routes/safe/components/Transactions/TxsTable/ExpandedTxRow/IncomingTx/index.tsx Line 7 - Object pattern argument should be typed.
src/routes/safe/components/Transactions/TxsTable/ExpandedTxRow/IncomingTxDescription/index.tsx Line 36 - Missing return type on function.
src/routes/safe/components/Transactions/TxsTable/ExpandedTxRow/IncomingTxDescription/index.tsx Line 36 - Object pattern argument should be typed.
src/routes/safe/components/Transactions/TxsTable/ExpandedTxRow/OutgoingTx/index.tsx Line 8 - Missing return type on function.
src/routes/safe/components/Transactions/TxsTable/ExpandedTxRow/OutgoingTx/index.tsx Line 8 - Object pattern argument should be typed.

[warning] @typescript-eslint/no-non-null-assertion

Disallows non-null assertions using the ! postfix operator

src/routes/safe/components/Transactions/TxsTable/TxType/index.tsx Line 78 - Forbidden non-null assertion.
src/routes/safe/components/Transactions/TxsTable/TxType/index.tsx Line 78 - Forbidden non-null assertion.

^{Report generated by eslint-plus-action}

ghost · 2020-08-25T12:45:14Z

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

ghost · 2020-08-25T20:40:31Z

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

ghost · 2020-08-26T15:45:01Z

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

- also set a default value for oldTxs

- create moduleTxSelector and fill the required data to build txTable

- also, renamed the `ExpandedTx` directory to `ExpandedTxRow` - the component exported from `ExpandedTxRow` now decides whether to display `ExpandedTx` or `ExpandedModuleTx`

…txList-SpendingLimit

ghost · 2020-08-29T00:05:56Z

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

…txList-SpendingLimit

github-actions · 2020-09-01T15:05:47Z

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

Result: ✅ success
Annotations: 10 total

[warning] @typescript-eslint/explicit-module-boundary-types

Require explicit return and argument types on exported functions' and classes' public class methods

src/routes/safe/components/Balances/SendModal/screens/ReviewTx/index.tsx Line 40 - Missing return type on function.
src/routes/safe/components/Balances/SendModal/screens/ReviewTx/index.tsx Line 40 - Object pattern argument should be typed.
src/routes/safe/components/Transactions/TxsTable/ExpandedTxRow/IncomingTx/index.tsx Line 7 - Missing return type on function.
src/routes/safe/components/Transactions/TxsTable/ExpandedTxRow/IncomingTx/index.tsx Line 7 - Object pattern argument should be typed.
src/routes/safe/components/Transactions/TxsTable/ExpandedTxRow/IncomingTxDescription/index.tsx Line 36 - Missing return type on function.
src/routes/safe/components/Transactions/TxsTable/ExpandedTxRow/IncomingTxDescription/index.tsx Line 36 - Object pattern argument should be typed.
src/routes/safe/components/Transactions/TxsTable/ExpandedTxRow/OutgoingTx/index.tsx Line 8 - Missing return type on function.
src/routes/safe/components/Transactions/TxsTable/ExpandedTxRow/OutgoingTx/index.tsx Line 8 - Object pattern argument should be typed.

[warning] @typescript-eslint/no-non-null-assertion

Disallows non-null assertions using the ! postfix operator

src/routes/safe/components/Transactions/TxsTable/TxType/index.tsx Line 78 - Forbidden non-null assertion.
src/routes/safe/components/Transactions/TxsTable/TxType/index.tsx Line 78 - Forbidden non-null assertion.

^{Report generated by eslint-plus-action}

ghost · 2020-09-01T15:12:19Z

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

mmv08 · 2020-09-02T10:04:44Z

src/logic/contracts/methodIds.ts

+  EXECUTE_ALLOWANCE_TRANSFER: 'executeAllowanceTransfer',
+}
+
+export const SPENDING_LIMIT_METHOD_TO_ID = {


this seems like id to a method, because the keys are IDs

yes, that messed with my head too. That means that the METHOD_TO_ID const for Safe's methods, should be renamed as well. Isn't it?

refactored a bit, moved these objects to the type file and renamed

mmv08 · 2020-09-02T10:08:32Z

src/logic/safe/store/actions/transactions/fetchTransactions/index.ts

+
+    const moduleTransactions = await loadModuleTransactions(safeAddress)
+
+    if (moduleTransactions) {


the default response for this is [], I think you wanted to check .length

fixed, thanks

mmv08 · 2020-09-02T10:09:24Z

src/logic/safe/store/actions/transactions/fetchTransactions/loadModuleTransactions.ts

+  const state = store.getState()
+
+  if (!safeAddress) {
+    return defaultResponse
+  }
+
+  const safe = state[SAFE_REDUCER_ID].getIn(['safes', safeAddress])
+
+  if (!safe) {
+    return defaultResponse
+  }


why are we accessing state in a function whose responsibility is just to fetch transactions from the backend?

no need to access it, thanks

mmv08 · 2020-09-02T10:11:14Z

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

@@ -0,0 +1,32 @@
+import { handleActions } from 'redux-actions'


why do we need a separate reducer? what was wrong with the previous one we used for transactions?

it's a different endpoint, and the returned data is quite different. Same strategy as with incomingTransactions

But in the end, we get to merge them to a single list

But in the end, we get to merge them to a single list

yes, but closer to the component.

Fortunately, this will work differently with the allTransactions endpoint approach.

In fact, I explored the strategy of storing the plain DTO in the store and then calculate data in the selector, it ended up working quite decently.

Then should've gone for it 👍

mmv08 · 2020-09-02T10:12:03Z

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

+      const oldModuleTxs = state[safeAddress] ?? []
+      const oldModuleTxsHashes = oldModuleTxs.map(({ transactionHash }) => transactionHash)
+      // filtering in this level happens, because backend is returning the whole list of txs
+      // so, to avoid re-storing all the txs, those already stored are filtered out


why does it matter if we restore them?

the problem is not the "re-store", but the duplicates

In that case, I would change the comment "to avoid duplicates..."

mmv08 · 2020-09-02T10:21:03Z

src/routes/safe/components/Balances/SendModal/screens/ReviewTx/index.tsx

-  const txToken = tokens.find((token) => token.address === tx.token)
-  const isSendingETH = txToken.address === ETH_ADDRESS
-  const txRecipient = isSendingETH ? tx.recipientAddress : txToken.address
+  const txToken = React.useMemo(() => tokens.find((token) => token.address === tx.token), [tokens, tx.token])


tx.token is an object, will equality check for this case? if tx changes then it will be re-run I think

I have to double-check this, it's a string:

mmv08 · 2020-09-02T10:22:21Z

src/routes/safe/components/Balances/SendModal/screens/ReviewTx/index.tsx

+          notifiedTransaction: TX_NOTIFICATION_TYPES.STANDARD_TX,
+          enqueueSnackbar,
+          closeSnackbar,
+        } as any),


is it possible to remove as any? createTransaction method is typed

also it's now not needed to pass closeSnackbar/enqueueSnackbar

done, removed and fixed

mmv08 · 2020-09-02T10:23:36Z

src/routes/safe/components/Balances/SendModal/screens/SendFunds/index.tsx

                    </Paragraph>
                    <ButtonLink
-                      onClick={() => mutators.setMax(selectedTokenRecord.balance)}
+                      onClick={() =>


The component is getting really big, would be good to split it

mmv08 · 2020-09-02T10:25:29Z

src/routes/safe/components/Transactions/TxsTable/index.tsx

          {(sortedData) =>
            sortedData.map((row, index) => (
-              <React.Fragment key={index}>
+              <React.Fragment key={`${row.tx.safeTxHash}-${index}`}>


The index is not required in that case

oops! fixed, thanks

mmv08 · 2020-09-02T10:38:54Z

src/routes/safe/components/Transactions/TxsTable/columns.tsx

    }

-    return getTransactionTableData(tx, cancelTxs.get(`${tx.nonce}`))
+    return getTransactionTableData(tx as Transaction, cancelTxs.get(`${tx.nonce}`))


what was the error there that required as?

getTransactionTableData has issues with tx being of SafeModuleTransaction type.

I tried helping TS by using type guards, but it didn't work either: https://www.typescriptlang.org/docs/handbook/advanced-types.html#type-guards-and-differentiating-types

Yeah we should explore using type guards, this is getting a problem for tokens also

…txList-SpendingLimit

- also reorganized imports

…ons`

…on call

ghost · 2020-09-03T17:30:57Z

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

ghost · 2020-09-03T17:40:31Z

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

ghost · 2020-09-03T18:15:27Z

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

…List-SpendingLimit

ghost · 2020-09-03T18:35:58Z

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

mmv08

Please address those small changes before merging, otherwise good job :)

mmv08 · 2020-09-04T12:23:53Z

src/logic/contracts/methodIds.ts

 export const isSetAllowanceMethod = (data: string): boolean => {
-  const methodId = data.slice(0, 10) as keyof typeof SPENDING_LIMIT_METHOD_TO_ID
-  return SPENDING_LIMIT_METHOD_TO_ID[methodId] === SPENDING_LIMIT_METHODS_NAMES.SET_ALLOWANCE
+  const methodId = data.slice(0, 10) as keyof typeof SPENDING_LIMIT_METHOD_ID_TO_NAME


Would be good to export this keyof typeof thing to a separate type

src/logic/safe/store/actions/transactions/fetchTransactions/loadModuleTransactions.ts

mmv08 · 2020-09-04T12:25:33Z

src/logic/safe/store/actions/transactions/fetchTransactions/index.ts

+    const moduleTransactions = await loadModuleTransactions(safeAddress)
+
+    if (moduleTransactions.length) {
+      dispatch(addModuleTransactions({ modules: moduleTransactions, safeAddress }))


Little inconsistency in naming, transactions get passed as modules

mmv08 · 2020-09-04T12:26:09Z

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

@@ -0,0 +1,32 @@
+import { handleActions } from 'redux-actions'


Then should've gone for it 👍

mmv08 · 2020-09-04T12:27:52Z

src/routes/safe/components/Transactions/TxsTable/columns.tsx

    }

-    return getTransactionTableData(tx, cancelTxs.get(`${tx.nonce}`))
+    return getTransactionTableData(tx as Transaction, cancelTxs.get(`${tx.nonce}`))


Yeah we should explore using type guards, this is getting a problem for tokens also

francovenica · 2020-09-04T19:02:42Z

https://pr1271--safereact.review.gnosisdev.com/app/#/safes/0x9913B9180C20C6b0F21B6480c84422F6ebc4B808/transactions
The user that has the spending limit 0x6E45d69a383CECa3d54688e833Bd0e1388747e6B

If I open a tx "Spending limit" created by sending funds using the user with the spending limit I get a console error and the page crashes

francovenica · 2020-09-04T19:03:56Z

Minor issue:
When changing the checkbox to choose between a regular transaction or using spending limit the amount should be checked if it is valid
https://www.loom.com/share/743778ae0373493a8245e534c5ae70ab

EDIT:
It seems that's the way the form works, this issue is also present if you set an amount, and then change tokens that has less than the amount that was input previously

The system will not let you send more tokens than you have in the safe so is not critical.

…List-SpendingLimit # Conflicts: # src/logic/safe/store/actions/transactions/fetchTransactions/fetchTransactions.ts # src/logic/safe/store/selectors/index.ts # src/routes/safe/components/Balances/SendModal/screens/ReviewTx/index.tsx # src/routes/safe/components/Transactions/TxsTable/ExpandedTxRow/TxDescription/CustomDescription.tsx # src/routes/safe/components/Transactions/TxsTable/columns.tsx # src/routes/safe/components/Transactions/TxsTable/index.tsx

fernandomg · 2020-09-08T21:52:08Z

@francovenica, thanks for the catch. Now ETH spending limit txs are properly displayed.

ghost · 2020-09-08T22:03:21Z

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

fernandomg self-assigned this Aug 25, 2020

fernandomg marked this pull request as draft August 25, 2020 12:34

fernandomg added 12 commits August 27, 2020 15:20

WIP: first attempt to send funds using spending limit module

7ca838d

fix: avoid signature request

bbacc8b

refactor: remove duplicated type file

d228c03

add missing type to parameters

3380816

load module transactions in the store

96d0776

refactor: move logic and types from routes to logic

6411d5d

update module's reducer to only add new txs

e100876

- also set a default value for oldTxs

define type for SafeModuleTransaction

7d5cd59

add moduleTxs to the list of txs

f7f3dc2

- create moduleTxSelector and fill the required data to build txTable

add ExpandedModuleTx component

0aa49d9

- also, renamed the `ExpandedTx` directory to `ExpandedTxRow` - the component exported from `ExpandedTxRow` now decides whether to display `ExpandedTx` or `ExpandedModuleTx`

identify spendingLimitTxs

a7b5953

Merge branch 'feature/#693-sendFunds-SpendingLimit' into feature/#691-…

0ea23b3

…txList-SpendingLimit

fernandomg requested review from Agupane, dasanra, mmv08 and nicosampler August 28, 2020 23:51

fernandomg marked this pull request as ready for review August 28, 2020 23:52

Merge branch 'feature/#693-sendFunds-SpendingLimit' into feature/#691-…

91167fc

…txList-SpendingLimit

mmv08 suggested changes Sep 2, 2020

View reviewed changes

Merge branch 'feature/#693-sendFunds-SpendingLimit' into feature/#691-…

08faadb

…txList-SpendingLimit

fernandomg added 5 commits September 3, 2020 10:41

fix: properly verify existence of moduleTransactions

80d0948

- also reorganized imports

fix: remove unnecessary state access from within `loadModuleTransacti…

c141d52

…ons`

fix: rewrite explanation why there's a filter in the module's tx reducer

a549155

fix: remove as any and unnecessary params in createTransaction acti…

f2668fa

…on call

fix: remove index from key

b138c4d

refactor: rename constants

ef439dd

fernandomg requested a review from mmv08 September 3, 2020 18:03

Merge branch 'feature/#413-SpendingLimit-in-app' into feature/#691-tx…

5840ae9

…List-SpendingLimit

mmv08 approved these changes Sep 4, 2020

View reviewed changes

fernandomg added 3 commits September 8, 2020 15:21

fix: support ETH for spending limit transactions

ce681b8

fix types after merge

b58b648

mmv08 merged commit 9936794 into feature/#413-SpendingLimit-in-app Sep 9, 2020

mmv08 deleted the feature/#691-txList-SpendingLimit branch September 9, 2020 12:16

github-actions bot locked and limited conversation to collaborators Sep 9, 2020


		const moduleTransactions = await loadModuleTransactions(safeAddress)

		if (moduleTransactions) {

		@@ -0,0 +1,32 @@
		import { handleActions } from 'redux-actions'

Conversation

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

Uh oh!

Uh oh!

github-actions bot commented Aug 25, 2020

Uh oh!

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

Uh oh!

ESLint Summary View Full Report

[warning] @typescript-eslint/explicit-module-boundary-types

[warning] @typescript-eslint/no-non-null-assertion

Uh oh!

ghost commented Aug 25, 2020

Uh oh!

ghost commented Aug 25, 2020

Uh oh!

ghost commented Aug 26, 2020

Uh oh!

ghost commented Aug 29, 2020

Uh oh!

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

Uh oh!

ESLint Summary View Full Report

[warning] @typescript-eslint/explicit-module-boundary-types

[warning] @typescript-eslint/no-non-null-assertion

Uh oh!

ghost commented Sep 1, 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!

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!

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fernandomg commented Aug 25, 2020 •

edited

Loading

github-actions bot commented Aug 25, 2020 •

edited

Loading

github-actions bot commented Sep 1, 2020 •

edited

Loading

francovenica commented Sep 4, 2020 •

edited

Loading