From f1618759022a36afb0c7e0d22eedd39ac18be52a Mon Sep 17 00:00:00 2001 From: Fernando Date: Wed, 9 Dec 2020 08:55:55 -0300 Subject: [PATCH] (Fix) Duplicated Owner Addresses on Safe Creation (#1666) * add form level validation for OwnersForm - also fixed `calculateValuesAfterRemoving` function that removed an owner's row by clicking on the trash icon * add tests for `calculateValuesAfterRemoving` function * reformat with prettier --- src/components/Stepper/index.tsx | 2 +- src/routes/open/components/Layout.tsx | 4 +- .../SafeOwnersConfirmationsForm/index.tsx | 79 ++++++++++++++----- .../calculateValuesAfterRemoving.test.ts | 49 ++++++++++++ 4 files changed, 111 insertions(+), 23 deletions(-) create mode 100644 src/routes/open/components/SafeOwnersConfirmationsForm/tests/calculateValuesAfterRemoving.test.ts diff --git a/src/components/Stepper/index.tsx b/src/components/Stepper/index.tsx index 0147677f..07ee8497 100644 --- a/src/components/Stepper/index.tsx +++ b/src/components/Stepper/index.tsx @@ -26,7 +26,7 @@ export interface StepperPageFormProps { } interface StepperPageProps { - validate?: (...args: unknown[]) => undefined | string[] | Promise> + validate?: (...args: unknown[]) => undefined | Record | Promise> component: ( ...args: unknown[] ) => (controls: React.ReactElement, formProps: StepperPageFormProps) => React.ReactElement diff --git a/src/routes/open/components/Layout.tsx b/src/routes/open/components/Layout.tsx index 78e5a43f..45385f05 100644 --- a/src/routes/open/components/Layout.tsx +++ b/src/routes/open/components/Layout.tsx @@ -9,7 +9,7 @@ import Row from 'src/components/layout/Row' import { instantiateSafeContracts } from 'src/logic/contracts/safeContracts' import { Review } from 'src/routes/open/components/ReviewInformation' import SafeNameField from 'src/routes/open/components/SafeNameForm' -import { SafeOwnersPage } from 'src/routes/open/components/SafeOwnersConfirmationsForm' +import { SafeOwnersPage, validateOwnersForm } from 'src/routes/open/components/SafeOwnersConfirmationsForm' import { FIELD_CONFIRMATIONS, FIELD_CREATION_PROXY_SALT, @@ -133,7 +133,7 @@ export const Layout = (props: LayoutProps): React.ReactElement => { testId="create-safe-form" > - + diff --git a/src/routes/open/components/SafeOwnersConfirmationsForm/index.tsx b/src/routes/open/components/SafeOwnersConfirmationsForm/index.tsx index 76494f54..a1469842 100644 --- a/src/routes/open/components/SafeOwnersConfirmationsForm/index.tsx +++ b/src/routes/open/components/SafeOwnersConfirmationsForm/index.tsx @@ -4,7 +4,6 @@ import { makeStyles } from '@material-ui/core/styles' import CheckCircle from '@material-ui/icons/CheckCircle' import * as React from 'react' import { styles } from './style' -import { getAddressValidator } from './validators' import QRIcon from 'src/assets/icons/qrcode.svg' import trash from 'src/assets/icons/trash.svg' @@ -21,6 +20,7 @@ import { noErrorsOn, required, minMaxLength, + ADDRESS_REPEATED_ERROR, } from 'src/components/forms/validator' import Block from 'src/components/layout/Block' import Button from 'src/components/layout/Button' @@ -44,30 +44,70 @@ const { useState } = React export const ADD_OWNER_BUTTON = '+ Add another owner' -export const calculateValuesAfterRemoving = (index, notRemovedOwners, values) => { - const initialValues = { ...values } +/** + * Validates the whole OwnersForm, specially checks for non-repeated addresses + * + * If finds a repeated address, marks it as invalid + * @param {Object} values + * @return Object + */ +export const validateOwnersForm = (values: Record): Record => { + const { errors } = Object.keys(values).reduce( + (result, key) => { + if (/owner\d+Address/.test(key)) { + const address = values[key].toLowerCase() - const numOwnersAfterRemoving = notRemovedOwners - 1 + if (result.addresses.includes(address)) { + result.errors[key] = ADDRESS_REPEATED_ERROR + } - for (let i = index; i < numOwnersAfterRemoving; i += 1) { - initialValues[getOwnerNameBy(i)] = values[getOwnerNameBy(i + 1)] - initialValues[getOwnerAddressBy(i)] = values[getOwnerAddressBy(i + 1)] - } - - if (+values[FIELD_CONFIRMATIONS] === notRemovedOwners) { - initialValues[FIELD_CONFIRMATIONS] = numOwnersAfterRemoving.toString() - } - - delete initialValues[getOwnerNameBy(index)] - delete initialValues[getOwnerAddressBy(index)] - - return initialValues + result.addresses.push(address) + } + return result + }, + { addresses: [] as string[], errors: {} }, + ) + return errors } +export const calculateValuesAfterRemoving = (index: number, values: Record): Record => + Object.keys(values) + .sort() + .reduce((newValues, key) => { + const ownerRelatedField = /owner(\d+)(Name|Address)/ + + if (!ownerRelatedField.test(key)) { + // no owner-related field + newValues[key] = values[key] + return newValues + } + + const ownerToRemove = new RegExp(`owner${index}(Name|Address)`) + + if (ownerToRemove.test(key)) { + // skip, doing anything with the removed field + return newValues + } + + // we only have the owner-related fields to work with + // we must reduce the index value for those owners that come after the deleted owner row + const [, ownerOrder, ownerField] = key.match(ownerRelatedField) as RegExpMatchArray + + if (Number(ownerOrder) > index) { + // reduce by one the order of the owner + newValues[`owner${Number(ownerOrder) - 1}${ownerField}`] = values[key] + } else { + // previous owners to the deleted row + newValues[key] = values[key] + } + + return newValues + }, {} as Record) + const useStyles = makeStyles(styles) const SafeOwnersForm = (props): React.ReactElement => { - const { errors, form, otherAccounts, values } = props + const { errors, form, values } = props const classes = useStyles() const validOwners = getNumOwnersFrom(values) @@ -87,7 +127,7 @@ const SafeOwnersForm = (props): React.ReactElement => { } const onRemoveRow = (index) => () => { - const initialValues = calculateValuesAfterRemoving(index, numOwners, values) + const initialValues = calculateValuesAfterRemoving(index, values) form.reset(initialValues) setNumOwners(numOwners - 1) @@ -171,7 +211,6 @@ const SafeOwnersForm = (props): React.ReactElement => { name={addressName} placeholder="Owner Address*" text="Owner Address" - validators={[getAddressValidator(otherAccounts, index)]} testId={`create-safe-address-field-${index}`} /> diff --git a/src/routes/open/components/SafeOwnersConfirmationsForm/tests/calculateValuesAfterRemoving.test.ts b/src/routes/open/components/SafeOwnersConfirmationsForm/tests/calculateValuesAfterRemoving.test.ts new file mode 100644 index 00000000..a305388c --- /dev/null +++ b/src/routes/open/components/SafeOwnersConfirmationsForm/tests/calculateValuesAfterRemoving.test.ts @@ -0,0 +1,49 @@ +import { calculateValuesAfterRemoving } from 'src/routes/open/components/SafeOwnersConfirmationsForm' + +describe('calculateValuesAfterRemoving', () => { + it(`should properly remove the last owner row`, () => { + // Given + const formContent = { + name: 'My Safe', + owner0Name: 'Owner 0', + owner0Address: '0x90F8bf6A479f320ead074411a4B0e7944Ea8c9C1', + owner1Name: 'Owner 1', + owner1Address: '0xFFcf8FDEE72ac11b5c542428B35EEF5769C409f0', + } + + // When + const newFormContent = calculateValuesAfterRemoving(1, formContent) + + // Then + expect(newFormContent).toStrictEqual({ + name: 'My Safe', + owner0Name: 'Owner 0', + owner0Address: '0x90F8bf6A479f320ead074411a4B0e7944Ea8c9C1', + }) + }) + + it(`should properly remove an owner and recalculate fields indices`, () => { + // Given + const formContent = { + name: 'My Safe', + owner0Name: 'Owner 0', + owner0Address: '0x90F8bf6A479f320ead074411a4B0e7944Ea8c9C1', + owner1Name: 'Owner 1', + owner1Address: '0xFFcf8FDEE72ac11b5c542428B35EEF5769C409f0', + owner2Name: 'Owner 2', + owner2Address: '0x22d491Bde2303f2f43325b2108D26f1eAbA1e32b', + } + + // When + const newFormContent = calculateValuesAfterRemoving(1, formContent) + + // Then + expect(newFormContent).toStrictEqual({ + name: 'My Safe', + owner0Name: 'Owner 0', + owner0Address: '0x90F8bf6A479f320ead074411a4B0e7944Ea8c9C1', + owner1Name: 'Owner 2', + owner1Address: '0x22d491Bde2303f2f43325b2108D26f1eAbA1e32b', + }) + }) +})