(Feature) New spending limit modal by fernandomg · Pull Request #1218 · 5afe/safe-react

fernandomg · 2020-08-07T00:46:58Z

This PR closes #690, by creating modal with form and field validations according to mocks vs current styles.

For ease of testing, defaults to an AllowanceModule contract deployed in Rinkeby: https://github.com/gnosis/safe-react/blob/a3b9ee2435e523632609b8324bd5846415d6ec88/src/utils/constants.ts#L18 (this will be removed and should be set in CI variables when the integration branch is merged to development)
Users can set a new Spending Limit by selecting the beneficiary from the address book, pick a token from the available in the Safe, and set an arbitrary amount.
Also can set a reset time.
If, the Spending Limit created already exists (that is: beneficiary<->token, then the user will be prompted with a hard to avoid warning message. Later can -and should- be enhanced).

Note: I'm marking this to be reviewed, as I need feedback for the functionality/flow.

There are snippets of code that need to be refactored, like methods that request module's info, utility functions, etc. that I'll reorganize with the next PR when I build the table of spending limits.

On the safe-react-components side, I recreated some components in the app to cover my needs, but I need to create the issues to solve that in the library.

…w-SpendingLimit

- marked `isValid` as optional, with default `true` value - marked `initialValue` as optional - migrated to hooks for material-ui styles

- migrated to hooks for material-ui styles - make `label` parametrized and optional, with default value - make `setIsValidAddress` optional

- migrated to hooks for material-ui styles

…w-SpendingLimit # Conflicts: # src/routes/safe/components/Apps/index.tsx

github-actions · 2020-08-07T00:47:14Z

CLA Assistant Lite All Contributors have signed the CLA.

github-actions · 2020-08-07T13:25:14Z

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

Result: ✅ success
Annotations: 6 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/store/actions/fetchSafe.ts Line 57 - Missing return type on function.
src/logic/safe/store/actions/fetchSafe.ts Line 57 - Argument 'latestMasterContractVersion' should be typed with a non-any type.
src/logic/safe/store/actions/fetchSafe.ts Line 170 - Missing return type on function.
src/logic/safe/store/actions/fetchSafe.ts Line 170 - Argument 'dispatch' should be typed.
src/logic/safe/store/actions/fetchSafe.ts Line 170 - Argument 'getState' should be typed.
src/routes/safe/components/Settings/SpendingLimit/utils.ts Line 21 - Missing return type on function.

^{Report generated by eslint-plus-action}

Also: - fixed styles - handled form state while going back and forth between create and review - had to change `Review` Button as submitting wasn't triggered on the first click.

- also refactored `ScanQRWrapper` so it uses the specified icon

ghost · 2020-08-11T19:42:09Z

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

…w-SpendingLimit # Conflicts: # package.json # src/routes/safe/components/Apps/index.tsx # yarn.lock

- if the module is not enabled it will be enabled on the first tx - beneficiary is added to the delegate list in the contract - allowance is set Still several things to address, like informing the user about an existing allowance, code cleanup

ghost · 2020-08-16T00:40:07Z

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

- create - verify existent and inform the user

…dingLimit state issue

…w-SpendingLimit

nicosampler · 2020-08-19T15:24:48Z

src/routes/safe/components/Settings/SpendingLimit/BeneficiarySelect.tsx

+      role="button"
+      aria-pressed="false"
+      tabIndex={0}
+      onKeyDown={(e) => {


can you extract this to a constant?

nicosampler · 2020-08-19T15:25:45Z

src/routes/safe/components/Settings/SpendingLimit/BeneficiarySelect.tsx

+import { getNameFromAdbk } from 'src/logic/addressBook/utils'
+import AddressBookInput from 'src/routes/safe/components/Balances/SendModal/screens/AddressBookInput'
+
+const KEYCODES = {


this could be used in another placer, perhaps moving it to a generic place is a good idea.

Ok, I'll take this suggestion to move all the generic functions to a utils file, but inside the SpendingLimit/ directory for now

nicosampler · 2020-08-19T15:28:59Z

src/routes/safe/components/Settings/SpendingLimit/NewSpendingLimit.tsx

+    'resetTimeOption resetTimeOption';
+`
+
+const YetAnotherButton = styled(GnoButton)`


Is this just a disabled state? seems like we should support it in safe-react-components.

No, there's another issue with the SRC Button. By setting its type to submit I need to "double-click" it to actually submit the form.

nicosampler · 2020-08-19T15:31:59Z

src/routes/safe/components/Settings/SpendingLimit/NewSpendingLimit.tsx

+  }
+`
+
+// TODO: propose refactor in safe-react-components based on this requirements


not sure to understand

mh! after the refactor that TODO lost its target. 😄

nicosampler · 2020-08-19T18:22:41Z

src/components/ScanQRModal/ScanQRWrapper/index.tsx

-        src={QRIcon}
-        testId="qr-icon"
-      />
+      <button onClick={openQrModal} className={classes.qrCodeBtn} title="Scan QR" data-testid="qr-icon" type="button">


can we use Button from src?

nicosampler · 2020-08-19T18:25:10Z

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

+  </Field>
+)
+
+const Switch = ({ label, name }: { label: string; name: string }): React.ReactElement => (


Can we move this to SRC in the next iteration, we have a checkbox but the radio button is missing.

I just added a more detailed TODO for the Switch (that actually exists in the SRC library, but there are missing props that I need to pass to properly integrate it with RFF).

The RadioButton is in the library, but I added some styles here, that we may need to move over too.

ghost · 2020-08-19T20:42:04Z

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

- used for..of for better readability

dasanra · 2020-08-20T11:51:02Z

Is only me having trouble to add ETH allowance?

Address 0x000 is not valid.
Is this in scope? If it doesn't maybe we need to remove it from the list

fernandomg · 2020-08-20T13:23:17Z

Is only me having trouble to add ETH allowance?

Address 0x000 is not valid.
Is this in scope? If it doesn't maybe we need to remove it from the list

It should work, it was me who failed to handle this scenario. Thanks for the catch!

ghost · 2020-08-20T13:30:49Z

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

ghost · 2020-08-20T14:11:24Z

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

this is due to the lack of global variable in the build

ghost · 2020-08-20T15:23:47Z

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

dasanra · 2020-08-20T12:23:21Z

src/routes/safe/components/Settings/SpendingLimit/index.tsx

+
+    // prepare the setAllowance tx
+    const web3 = getWeb3()
+    const spendingLimit = new web3.eth.Contract(SpendingLimitModule.abi as any, SPENDING_LIMIT_MODULE_ADDRESS)


Why create a double instance of the smart contract?

Inside previous if we are doing the exact same new (maybe we should anticipate this general one, because is something we will need regardless of the if result)

I was expecting to do this later in the integration branch, but given this requirement, I moved the SpendingLimit requests to fetchSafe.

Data now lives in the Safe's store, so it's requested via selectors.

dasanra · 2020-08-20T15:04:57Z

src/components/ScanQRModal/ScanQRWrapper/index.tsx

 import { makeStyles } from '@material-ui/core/styles'
-import { useState } from 'react'
-import * as React from 'react'
+import React from 'react'


I usually prefer to have import React, { useState } so is explicit that we use useState

Oh, I prefer the most verbose/explicit way: React.anyHook, so it's easier to follow while reading the code if it's a React's hook or a different one. Whichever the final decision is, this should be added and enforced in a style-guide for sure.

dasanra · 2020-08-20T15:13:11Z

src/routes/safe/components/Settings/SpendingLimit/BeneficiarySelect.tsx

+
+  const [pristine, setPristine] = React.useState<boolean>(!initialValues?.beneficiary)
+
+  React.useEffect(() => {


I would go for explicit import of React hooks

dasanra · 2020-08-20T15:15:00Z

src/routes/safe/components/Settings/SpendingLimit/BeneficiarySelect.tsx

+  const { initialValues } = useFormState()
+  const { mutators } = useForm()
+
+  const [selectedEntry, setSelectedEntry] = React.useState<{ address: string; name: string } | null>({


Another hook that is used twice.

I prefer explicit import

dasanra · 2020-08-20T15:23:31Z

src/routes/safe/components/Settings/SpendingLimit/BeneficiarySelect.tsx

+  const addressBook = useSelector(getAddressBook)
+
+  const handleScan = (value, closeQrModal) => {
+    let scannedAddress = value


This can be a oneliner. replace returns a new string.

const scannedAddress = value.startsWith('ethereum:') ? value.replace('ethereum:', '') : value

dasanra · 2020-08-20T15:49:40Z

src/routes/safe/components/Settings/SpendingLimit/index.tsx

+  const dispatch = useDispatch()
+
+  const [step, setStep] = React.useState<'create' | 'review'>('create')
+  const [values, setValues] = React.useState()


Also in this occurrences, explicit import of useState is preferred

dasanra · 2020-08-20T15:52:05Z

src/routes/safe/components/Settings/SpendingLimit/index.tsx

+
+  // TODO: Refactor `delegates` for better performance. This is just to verify allowance works
+  const safeAddress = useSelector(safeParamAddressFromStateSelector)
+  const [delegates, setDelegates] = React.useState({ results: [], next: '' })


Also explicit import of useState and useEffect on next line

dasanra · 2020-08-20T15:55:29Z

src/routes/safe/components/Settings/SpendingLimit/utils.ts

+export const toTokenUnit = (amount: string, decimals: string | number): string =>
+  new BigNumber(amount).times(`1e${decimals}`).toFixed()
+
+export const currentMinutes = () => Math.floor(Date.now() / (1000 * 60))


This function returns type number, isn't it?

Yes, this was shamelessly copied from the Allowances contract's tests 😬

- also, refactored components for better readability

ghost · 2020-08-21T01:32:01Z

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

fernandomg · 2020-08-22T00:17:26Z

Changes in this PR were already merged into #1261

I've covered all the requirements.

The only exception is for the implicit vs explicit use of React's hook: #1218 (comment) @dasanra

fernandomg added 11 commits July 30, 2020 16:47

Merge branch 'development' into feature/#413-SpendingLimit-in-app

9b3c973

fix: Card component requires className instead of styles

5e24cfc

update safe-react-components

f669a3a

change Spending Limit icon from allowances to fuelIndicator

2583e57

Merge branch 'feature/#413-SpendingLimit-in-app' into feature/#690-ne…

5fca120

…w-SpendingLimit

add types to TokenSelectField component

df6547a

- marked `isValid` as optional, with default `true` value - marked `initialValue` as optional - migrated to hooks for material-ui styles

refactor AddressBookInput

7616e8f

- migrated to hooks for material-ui styles - make `label` parametrized and optional, with default value - make `setIsValidAddress` optional

add types to GnoModal component

32e5d50

- migrated to hooks for material-ui styles

update safe-react-components

d0b2c7d

WIP: New Spending Limit form

cea0817

Merge branch 'feature/#413-SpendingLimit-in-app' into feature/#690-ne…

b603512

…w-SpendingLimit # Conflicts: # src/routes/safe/components/Apps/index.tsx

fernandomg self-assigned this Aug 7, 2020

fernandomg added 2 commits August 6, 2020 21:51

fix compatibility with new safe-react-components version

83a0478

update safe-react-components version

66c1e85

fernandomg added 3 commits August 10, 2020 21:45

fix styles for TokenSelectField's placeholder

debf1ce

WIP: add review screen

7c109f0

Also: - fixed styles - handled form state while going back and forth between create and review - had to change `Review` Button as submitting wasn't triggered on the first click.

WIP: fix styles/layout

b854053

- also refactored `ScanQRWrapper` so it uses the specified icon

fernandomg added 4 commits August 13, 2020 10:58

disable Spending Limit button for non-owners

3c9543d

Merge branch 'feature/#413-SpendingLimit-in-app' into feature/#690-ne…

ec4408e

…w-SpendingLimit # Conflicts: # package.json # src/routes/safe/components/Apps/index.tsx # yarn.lock

WIP: Add new spending limit

a3b9ee2

- if the module is not enabled it will be enabled on the first tx - beneficiary is added to the delegate list in the contract - allowance is set Still several things to address, like informing the user about an existing allowance, code cleanup

WIP: verify module is enabled and works

b5aadc8

fernandomg added 5 commits August 18, 2020 16:00

WIP: complete New Spending Limit flow

1fea250

- create - verify existent and inform the user

WIP: properly handle tokens' decimals also fix typos and existentSpen…

4bd62a6

…dingLimit state issue

refactor: split code into components

53f9e4a

Merge branch 'feature/#413-SpendingLimit-in-app' into feature/#690-ne…

8307abd

…w-SpendingLimit

fix reference after merge

9be7cd7

nicosampler reviewed Aug 19, 2020

View reviewed changes

fernandomg added 5 commits August 19, 2020 17:02

rename TextField to SRCTextField, and GnoTextField to TextField

54b45ff

extract inline event handlers

43bf62c

extract utility functions

ea27f97

add TODO for YetAnotherButton

9781dee

add TODO for StyledSwitch

e131e25

fernandomg added 2 commits August 19, 2020 20:55

fix delegates retrieval

68d8181

- used for..of for better readability

fix logic for one-time allowance

d90b029

support ETH Spending Limit

72ab5f9

add back SPENDING_LIMIT_MODULE_ADDRESS default value in constants file

20fd94a

this is due to the lack of global variable in the build

dasanra suggested changes Aug 20, 2020

View reviewed changes

fernandomg added 2 commits August 20, 2020 22:16

refactor: store SpendingLimit in the store

8be577d

- also, refactored components for better readability

refactor: convert scannedAddress to a oneliner

157f539

fernandomg requested a review from dasanra August 21, 2020 14:59

fernandomg closed this Aug 22, 2020

fernandomg deleted the feature/#690-new-SpendingLimit branch August 22, 2020 00:17

github-actions bot locked and limited conversation to collaborators Aug 22, 2020


		const [pristine, setPristine] = React.useState<boolean>(!initialValues?.beneficiary)

		React.useEffect(() => {

Conversation

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

Uh oh!

Uh oh!

github-actions bot commented Aug 7, 2020

Uh oh!

github-actions bot commented Aug 7, 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

Uh oh!

ghost commented Aug 11, 2020

Uh oh!

ghost commented Aug 16, 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!

ghost commented Aug 19, 2020

Uh oh!

dasanra commented Aug 20, 2020

Uh oh!

fernandomg commented Aug 20, 2020

Uh oh!

ghost commented Aug 20, 2020

Uh oh!

ghost commented Aug 20, 2020

Uh oh!

ghost commented Aug 20, 2020

Uh oh!

dasanra Aug 20, 2020 • 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!

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 Aug 20, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dasanra Aug 20, 2020 • 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!

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!

ghost commented Aug 21, 2020

fernandomg commented Aug 7, 2020 •

edited

Loading

github-actions bot commented Aug 7, 2020 •

edited

Loading

dasanra Aug 20, 2020 •

edited

Loading

fernandomg Aug 20, 2020 •

edited

Loading

dasanra Aug 20, 2020 •

edited

Loading