(Feature) Spending Limit by fernandomg · Pull Request #1637 · 5afe/safe-react

fernandomg · 2020-11-20T00:58:41Z

This PR is a replacement for #1351. Given it's a big feature and it was there for around 4 months, its history became very hard to follow.

This PR closes #413, by:

supporting Spending Limits module as a Safe's feature
Allowing to add/edit/remove a beneficiary
Identifying and listing Spending Limit transactions in the transactions table

NOTE:
Before merging this feature, we need to remove the hardcoded address in constants.ts file and set the global variable REACT_APP_SPENDING_LIMIT_MODULE_ADDRESS to the audited module address.

How to test

"Allowances" was replaced by "Spending Limit"
Consider that feature mockups were made way before the new Safe's designs, so there will be differences.

This is an ordered list of the feature's sub-tasks.
Implementation details are in the PRs description.

task	issue	PR	important feedback
Spending Limit Settings tab	#689	#1151	#1151 (comment) #1151 (comment)
New Spending Limit	#690	#1218
Remove Spending Limit	#692	#1261
List of Spending Limit	#1039	#1261	#1261 (comment) #1261 (comment)
Send funds	#693	#1276	#1276 (comment) #1276 (comment)
Transaction list	#691	#1271	#1271 (comment)

What this feature adds to the safe:

Spending Limit as a Setting option

New Spending Limit

setup	review	listing beneficiaries

Identify Spending Limit Transactions

first ever spending limit	new beneficiary	already known beneficiary

Action 1: Enable Module Action 2: Add Beneficiary Action 3: Set the Spending Limit	Action 1: Add Beneficiary Action 2: Set the Spending Limit	Set the Spending Limit

Use the Spending Limit allowance

send	transaction details

github-actions · 2020-11-20T00:58:55Z

CLA Assistant Lite All Contributors have signed the CLA.

github-actions · 2020-11-20T01:03:43Z

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

Result: ✅ success
Annotations: 3 total

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

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

src/logic/safe/utils/upgradeSafe.ts Line 52 - Argument 'createTransaction' 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}

- also added typings for the contract

- also fixed ADDRESSBOOK_EDIT_ENTRY key

- also used hooks for styles - and safe-react-components wherever is possible

- create spending limit - list spending limit - delete spending limit

ghost · 2020-11-20T16:41:39Z

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

ghost · 2020-11-20T16:41:56Z

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

ghost · 2020-11-20T17:23:44Z

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

ghost · 2020-11-20T17:26:36Z

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

dasanra · 2020-11-24T08:34:09Z

src/routes/safe/components/Settings/SpendingLimit/FormFields/ResetTime.tsx

+  return (
+    <>
+      <ResetTimeLabel>
+        <Text size="xl">Set a reset-time to have the allowance automatically refill after a defined time-period.</Text>


"Set a reset time so the allowance automatically refills after the defined time period"

fernandomg · 2020-11-24T19:47:33Z

@francovenica, frow what we discussed in the walkthrough with the team. The only visible addition was the explanatory text next to the reset time toggle button in the "new spending limit" modal.

ghost · 2020-11-24T20:00:10Z

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

francovenica · 2020-11-24T21:21:51Z

Should we show the exact number here for < 0.0001 allowed tokens to use?

francovenica · 2020-11-24T21:29:51Z

If you disable the token in the assets tab it also goes away in the spending limit.
I don't know what the solution could it be here.
Could we cash the token symbol if a spending limit is created. Or show a default symbol for the tokens and at least show the name and limit set

francovenica · 2020-11-24T22:23:26Z

We have to deal with editing the spending limit to an amount lower than what was already used. Disable the "Spending limit" radiobutton or something. Maybe for the 2.17 release

Also deal with this inconsistency for that scenario

francovenica · 2020-11-24T23:41:51Z

I gave spending limits to an user and then I removed him as owner of the safe.
Still, I was able to go to etherscan and execute the contract to use those tokens.
I don't know if that's an issue, since it was the idea that non owner also should be able to use the spending limits, but since we decided that for now only owners should be allowed to use it I just want to be sure.

It respects the limit allowed still, so at least that user won't be able to use more of what it was set to him.

The safe: https://pr1637--safereact.review.gnosisdev.com/rinkeby/app/#/safes/0x7f58E04c2949CDA2A61E7413E9d8DA93b1726D7F/transactions
This 2 tx were created by that user as non owner from etherscan

lukasschor · 2020-11-25T07:28:49Z

I gave spending limits to an user and then I removed him as owner of the safe.
Still, I was able to go to etherscan and execute the contract to use those tokens.
I don't know if that's an issue, since it was the idea that non owner also should be able to use the spending limits, but since we decided that for now only owners should be allowed to use it I just want to be sure.

It respects the limit allowed still, so at least that user won't be able to use more of what it was set to him.

The safe: https://pr1637--safereact.review.gnosisdev.com/rinkeby/app/#/safes/0x7f58E04c2949CDA2A61E7413E9d8DA93b1726D7F/transactions
This 2 tx were created by that user as non owner from etherscan

Yeah that's fine. It should be possible to use the spending limit as a non-owner. Just not via our UI yet. :)

ghost · 2020-11-25T07:56:30Z

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

… manage list

fernandomg · 2020-11-25T12:20:55Z

@francovenica,

Should we show the exact number here for < 0.0001 allowed tokens to use?

❓ may a tooltip be helpful here?

If you disable the token in the assets tab it also goes away in the spending limit.
I don't know what the solution could it be here.
Could we cash the token symbol if a spending limit is created. Or show a default symbol for the tokens > and at least show the name and limit set

✔️ fixed

We have to deal with editing the spending limit to an amount lower than what was already used. Disable the "Spending limit" radiobutton or something. Maybe for the 2.17 release

🗒️ exactly. As it was discussed with the team during the walkthrough.

ghost · 2020-11-25T12:32:25Z

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

ghost · 2020-11-25T12:32:38Z

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

ghost · 2020-11-25T12:35:10Z

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

ghost · 2020-11-25T12:35:20Z

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

… coin - also removed the `useMemo` hook

…feature/413-spendingLimit

ghost · 2020-11-25T13:38:43Z

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

ghost · 2020-11-25T13:41:26Z

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

francovenica · 2020-11-25T18:17:57Z

The issue with the token not showing when disabling them in the assets tab was fixed
An issue where the value of the token was not properly being processed by the input was fixed

The other reports can be left for improvements for the 2.17 version

An owner being able to use tokens from outside of the safe while not being an owner is not an issue then.

Tested:

Set the spending limit. The 1st tx will add the module
To delete the module we need to include this fix. (the issue with deleting the module only happens if 2 or more modules are enabled) (Fix) remove module feature #1646
Use the tokens with single signature
Use only the amount set
Update the spending limit
Check the refresh of the limit (tested with the daily refresh)
Remove the spending limit from the user
Tried to trick the system to spend more than intended and checking that such tx fails to do so
Use the spending limit from outside of the safe with the limitations applied
"Send max" button in the send funds form takes in account if spending limit is used or not
Spending limit pagination and sorting

It looks good to me

ghost · 2020-11-25T18:49:31Z

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

ghost · 2020-11-25T18:50:12Z

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

fernandomg self-assigned this Nov 20, 2020

fernandomg requested review from Agupane, dasanra, francovenica, matextrem and nicosampler November 20, 2020 01:01

fernandomg added 15 commits November 20, 2020 13:21

add spendingLimit contract artifact

e47e39e

- also added typings for the contract

add notifications for SpendingLimit

57628d6

- also fixed ADDRESSBOOK_EDIT_ENTRY key

add label parameter to AddressBookInput

2937a6a

add typing to TokenSelectField

8100755

- also used hooks for styles - and safe-react-components wherever is possible

create helper functions for spendingLimit

ef12de1

add enableModule method

37c4564

fetch and store spendingLimit information

bafc75e

add spendingLimit module address

38a051f

add SpendingLimit in Advanced settings

85fcfc8

- create spending limit - list spending limit - delete spending limit

add spendingLimit feature to the SendFunds modal

f6efa60

refactor methods decodes

c5d6f11

add typings

09afbd5

identify module txs

0b89465

add module selector

e9ab1c3

adapt tx list to spending limit

c135a04

fernandomg force-pushed the feature/413-spendingLimit branch from bc68b4c to c135a04 Compare November 20, 2020 16:24

Merge branch 'development' into feature/413-spendingLimit

9fa795d

dasanra reviewed Nov 24, 2020

View reviewed changes

fernandomg added 2 commits November 24, 2020 16:41

add an explanatory text next to 'reset time' toggle

0e64bb0

add an explanatory text next to 'reset time' toggle

b15a531

fix to display token info despite it has being disabled in the assets…

b4f0aff

… manage list

Merge branch 'development' into feature/413-spendingLimit

aad9ba9

fernandomg added 3 commits November 25, 2020 09:57

fix unit conversion

3012125

fix token selector, by adding a new selector that includes the native…

92bfd10

… coin - also removed the `useMemo` hook

Merge remote-tracking branch 'origin/feature/413-spendingLimit' into …

95e75b3

…feature/413-spendingLimit

francovenica approved these changes Nov 25, 2020

View reviewed changes

Merge branch 'development' into feature/413-spendingLimit

f04dccb

fernandomg merged commit fc1250d into development Nov 25, 2020

fernandomg deleted the feature/413-spendingLimit branch November 25, 2020 18:59

github-actions bot locked and limited conversation to collaborators Nov 25, 2020

Conversation

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

Uh oh!

How to test

Spending Limit as a Setting option

New Spending Limit

Identify Spending Limit Transactions

Use the Spending Limit allowance

Uh oh!

github-actions bot commented Nov 20, 2020

Uh oh!

github-actions bot commented Nov 20, 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 Nov 20, 2020

Uh oh!

ghost commented Nov 20, 2020

Uh oh!

ghost commented Nov 20, 2020

Uh oh!

ghost commented Nov 20, 2020

Uh oh!

dasanra Nov 24, 2020

Choose a reason for hiding this comment

Uh oh!

fernandomg commented Nov 24, 2020

Uh oh!

ghost commented Nov 24, 2020

Uh oh!

francovenica commented Nov 24, 2020

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

francovenica commented Nov 24, 2020

Uh oh!

lukasschor commented Nov 25, 2020

Uh oh!

ghost commented Nov 25, 2020

Uh oh!

fernandomg commented Nov 25, 2020

Uh oh!

ghost commented Nov 25, 2020

Uh oh!

ghost commented Nov 25, 2020

Uh oh!

ghost commented Nov 25, 2020

Uh oh!

ghost commented Nov 25, 2020

Uh oh!

ghost commented Nov 25, 2020

Uh oh!

ghost commented Nov 25, 2020

Uh oh!

francovenica commented Nov 25, 2020

Uh oh!

ghost commented Nov 25, 2020

Uh oh!

ghost commented Nov 25, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

fernandomg commented Nov 20, 2020 •

edited

Loading

github-actions bot commented Nov 20, 2020 •

edited

Loading

francovenica commented Nov 24, 2020 •

edited

Loading

francovenica commented Nov 24, 2020 •

edited

Loading