(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

@@ -26,7 +26,7 @@ export interface StepperPageFormProps {
 }
 
 interface StepperPageProps {
-  validate?: (...args: unknown[]) => undefined | string[] | Promise<undefined | Record<string, string>>
+  validate?: (...args: unknown[]) => undefined | Record<string, string> | Promise<undefined | Record<string, string>>
   component: (
     ...args: unknown[]
   ) => (controls: React.ReactElement, formProps: StepperPageFormProps) => React.ReactElement

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"
           >
             <StepperPage component={SafeNameField} />
-            <StepperPage component={SafeOwnersPage} />
+            <StepperPage component={SafeOwnersPage} validate={validateOwnersForm} />
             <StepperPage network={network} userAccount={userAccount} component={Review} />
           </Stepper>
         </Block>

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<string, string>} values
+ * @return Object<string, string>
+ */
+export const validateOwnersForm = (values: Record<string, string>): Record<string, string> => {
+  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<string, string>): Record<string, string> =>
+  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<string, string>)
+
 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}`}
                 />
               </Col>

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',
+    })
+  })
+})