WA-232 Refactor add & remove owners DOM tests (from 55 seconds to 5 seconds)

src/routes/safe/component/AddOwner/index.jsx

@@ -35,6 +35,18 @@ const getOwnerAddressesFrom = (owners: List<Owner>) => {
   return owners.map((owner: Owner) => owner.get('address'))
 }
 
+export const addOwner = async (values: Object, safe: Safe, threshold: number, executor: string) => {
+  const nonce = Date.now()
+  const newThreshold = values[INCREASE_PARAM] ? threshold + 1 : threshold
+  const newOwnerAddress = values[OWNER_ADDRESS_PARAM]
+  const newOwnerName = values[NAME_PARAM]
+  const safeAddress = safe.get('address')
+  const gnosisSafe = await getSafeEthereumInstance(safeAddress)
+  const data = gnosisSafe.contract.addOwnerWithThreshold.getData(newOwnerAddress, newThreshold)
+  await createTransaction(safe, `Add Owner ${newOwnerName}`, safeAddress, 0, nonce, executor, data)
+  setOwners(safeAddress, safe.get('owners').push(makeOwner({ name: newOwnerName, address: newOwnerAddress })))
+}
+
 class AddOwner extends React.Component<Props, State> {
   state = {
     done: false,
@@ -45,15 +57,7 @@ class AddOwner extends React.Component<Props, State> {
       const {
         safe, threshold, userAddress, fetchTransactions,
       } = this.props
-      const nonce = Date.now()
+      await addOwner(values, safe, threshold, userAddress)
-      const newThreshold = values[INCREASE_PARAM] ? threshold + 1 : threshold
-      const newOwnerAddress = values[OWNER_ADDRESS_PARAM]
-      const newOwnerName = values[NAME_PARAM]
-      const safeAddress = safe.get('address')
-      const gnosisSafe = await getSafeEthereumInstance(safeAddress)
-      const data = gnosisSafe.contract.addOwnerWithThreshold.getData(newOwnerAddress, newThreshold)
-      await createTransaction(safe, `Add Owner ${newOwnerName}`, safeAddress, 0, nonce, userAddress, data)
-      setOwners(safeAddress, safe.get('owners').push(makeOwner({ name: newOwnerName, address: newOwnerAddress })))
       fetchTransactions()
       this.setState({ done: true })
     } catch (error) {

src/routes/safe/component/RemoveOwner/index.jsx

@@ -27,11 +27,33 @@ type State = {
 const SENTINEL_ADDRESS = '0x0000000000000000000000000000000000000001'
 export const REMOVE_OWNER_RESET_BUTTON_TEXT = 'RESET'
 
-const initialValuesFrom = (decreaseMandatory: boolean = false) => ({
+export const initialValuesFrom = (decreaseMandatory: boolean = false) => ({
   [DECREASE_PARAM]: decreaseMandatory,
 })
 
-const shouldDecrease = (numOwners: number, threshold: number) => threshold === numOwners
+export const shouldDecrease = (numOwners: number, threshold: number) => threshold === numOwners
+
+export const removeOwner = async (
+  values: Object,
+  safe: Safe,
+  threshold: number,
+  userToRemove: string,
+  name: string,
+  executor: string,
+) => {
+  const nonce = Date.now()
+  const newThreshold = values[DECREASE_PARAM] ? threshold - 1 : threshold
+  const safeAddress = safe.get('address')
+  const gnosisSafe = await getSafeEthereumInstance(safeAddress)
+
+  const storedOwners = await gnosisSafe.getOwners()
+  const index = storedOwners.findIndex(ownerAddress => ownerAddress === userToRemove)
+  const prevAddress = index === 0 ? SENTINEL_ADDRESS : storedOwners[index - 1]
+  const data = gnosisSafe.contract.removeOwner.getData(prevAddress, userToRemove, newThreshold)
+
+  const text = name || userToRemove
+  return createTransaction(safe, `Remove Owner ${text}`, safeAddress, 0, nonce, executor, data)
+}
 
 class RemoveOwner extends React.Component<Props, State> {
   state = {
@@ -43,19 +65,7 @@ class RemoveOwner extends React.Component<Props, State> {
       const {
         safe, threshold, executor, fetchTransactions, userToRemove, name,
       } = this.props
-      const nonce = Date.now()
+      await removeOwner(values, safe, threshold, userToRemove, executor, name)
-      const newThreshold = values[DECREASE_PARAM] ? threshold - 1 : threshold
-      const safeAddress = safe.get('address')
-      const gnosisSafe = await getSafeEthereumInstance(safeAddress)
-
-      const storedOwners = await gnosisSafe.getOwners()
-      const index = storedOwners.findIndex(ownerAddress => ownerAddress === userToRemove)
-      const prevAddress = index === 0 ? SENTINEL_ADDRESS : storedOwners[index - 1]
-      const data = gnosisSafe.contract.removeOwner.getData(prevAddress, userToRemove, newThreshold)
-
-      const text = name || userToRemove
-      await createTransaction(safe, `Remove Owner ${text}`, safeAddress, 0, nonce, executor, data)
-
       fetchTransactions()
       this.setState({ done: true })
     } catch (error) {

src/routes/safe/test/Safe.owners.test.js

@@ -1,187 +0,0 @@
-// @flow
-import { aNewStore } from '~/store'
-import { aDeployedSafe } from '~/routes/safe/store/test/builder/deployedSafe.builder'
-import { getWeb3 } from '~/wallets/getWeb3'
-import { sleep } from '~/utils/timer'
-import { type Match } from 'react-router-dom'
-import { promisify } from '~/utils/promisify'
-import { processTransaction } from '~/routes/safe/component/Transactions/processTransactions'
-import { confirmationsTransactionSelector, safeSelector, safeTransactionsSelector } from '~/routes/safe/store/selectors/index'
-import { getTransactionFromReduxStore } from '~/routes/safe/test/testMultisig'
-import { buildMathPropsFrom } from '~/test/utils/buildReactRouterProps'
-import { createTransaction } from '~/wallets/createTransactions'
-import fetchTransactions from '~/routes/safe/store/actions/fetchTransactions'
-import { type GlobalState } from '~/store/index'
-import { type Safe } from '~/routes/safe/store/model/safe'
-import { type Transaction } from '~/routes/safe/store/model/transaction'
-import { getGnosisSafeInstanceAt } from '~/wallets/safeContracts'
-
-const getSafeFrom = (state: GlobalState, safeAddress: string): Safe => {
-  const match: Match = buildMathPropsFrom(safeAddress)
-  const safe = safeSelector(state, { match })
-  if (!safe) throw new Error()
-
-  return safe
-}
-
-describe('React DOM TESTS > Add and remove owners', () => {
-  const assureExecuted = (transaction: Transaction) => {
-    expect(transaction.get('tx')).not.toBe(null)
-    expect(transaction.get('tx')).not.toBe(undefined)
-    expect(transaction.get('tx')).not.toBe('')
-  }
-
-  const assureThresholdIs = async (gnosisSafe, threshold: number) => {
-    const safeThreshold = await gnosisSafe.getThreshold()
-    expect(Number(safeThreshold)).toEqual(threshold)
-  }
-
-  const assureOwnersAre = async (gnosisSafe, ...owners) => {
-    const safeOwners = await gnosisSafe.getOwners()
-    expect(safeOwners.length).toEqual(owners.length)
-    for (let i = 0; i < owners.length; i += 1) {
-      expect(safeOwners[i]).toBe(owners[i])
-    }
-  }
-
-  it('adds owner without increasing the threshold', async () => {
-    // GIVEN
-    const numOwners = 2
-    const threshold = 1
-    const store = aNewStore()
-    const address = await aDeployedSafe(store, 10, threshold, numOwners)
-    const accounts = await promisify(cb => getWeb3().eth.getAccounts(cb))
-    const safe = getSafeFrom(store.getState(), address)
-    const gnosisSafe = await getGnosisSafeInstanceAt(address)
-
-    // WHEN
-    await assureThresholdIs(gnosisSafe, 1)
-    await assureOwnersAre(gnosisSafe, accounts[0], accounts[1])
-    const nonce = Date.now()
-    const accountIndex = 5
-    const data = gnosisSafe.contract.addOwnerWithThreshold.getData(accounts[accountIndex], 1)
-    await createTransaction(safe, `Add Owner with index ${accountIndex}`, address, 0, nonce, accounts[0], data)
-    await sleep(1500)
-    await store.dispatch(fetchTransactions())
-
-    // THEN
-    const transactions = safeTransactionsSelector(store.getState(), { safeAddress: address })
-    expect(transactions.count()).toBe(1)
-    const tx = transactions.get(0)
-    if (!tx) throw new Error()
-    assureExecuted(tx)
-    await assureOwnersAre(gnosisSafe, accounts[5], accounts[0], accounts[1])
-    await assureThresholdIs(gnosisSafe, 1)
-  })
-
-  it('adds owner increasing the threshold', async () => {
-    // GIVEN
-    const numOwners = 2
-    const threshold = 1
-    const store = aNewStore()
-    const address = await aDeployedSafe(store, 10, threshold, numOwners)
-    const accounts = await promisify(cb => getWeb3().eth.getAccounts(cb))
-    const safe = getSafeFrom(store.getState(), address)
-    const gnosisSafe = await getGnosisSafeInstanceAt(address)
-
-    // WHEN
-    await assureThresholdIs(gnosisSafe, 1)
-    await assureOwnersAre(gnosisSafe, accounts[0], accounts[1])
-    const nonce = Date.now()
-    const accountIndex = 5
-    const data = gnosisSafe.contract.addOwnerWithThreshold.getData(accounts[accountIndex], 2)
-    await createTransaction(safe, `Add Owner with index ${accountIndex}`, address, 0, nonce, accounts[0], data)
-    await sleep(1500)
-    await store.dispatch(fetchTransactions())
-
-    // THEN
-    const transactions = safeTransactionsSelector(store.getState(), { safeAddress: address })
-    expect(transactions.count()).toBe(1)
-    const tx = transactions.get(0)
-    if (!tx) throw new Error()
-    assureExecuted(tx)
-    await assureOwnersAre(gnosisSafe, accounts[accountIndex], accounts[0], accounts[1])
-    await assureThresholdIs(gnosisSafe, 2)
-  })
-
-  const processOwnerModification = async (store, safeAddress, executor) => {
-    const tx = getTransactionFromReduxStore(store, safeAddress)
-    if (!tx) throw new Error()
-    const confirmed = confirmationsTransactionSelector(store.getState(), { transaction: tx })
-    const data = tx.get('data')
-    expect(data).not.toBe(null)
-    expect(data).not.toBe(undefined)
-    expect(data).not.toBe('')
-
-    await processTransaction(safeAddress, tx, confirmed, executor)
-    await sleep(1800)
-  }
-
-  it('remove owner without decreasing the threshold', async () => {
-    // GIVEN
-    const numOwners = 3
-    const threshold = 2
-    const store = aNewStore()
-    const address = await aDeployedSafe(store, 10, threshold, numOwners)
-    const accounts = await promisify(cb => getWeb3().eth.getAccounts(cb))
-    const safe = getSafeFrom(store.getState(), address)
-    const gnosisSafe = await getGnosisSafeInstanceAt(address)
-
-    // WHEN
-    await assureThresholdIs(gnosisSafe, 2)
-    await assureOwnersAre(gnosisSafe, accounts[0], accounts[1], accounts[2])
-    const nonce = Date.now()
-    const accountIndex = 2
-    const data = gnosisSafe.contract.removeOwner.getData(accounts[accountIndex - 1], accounts[accountIndex], 2)
-    await createTransaction(safe, `Remove owner Address 3 ${nonce}`, address, 0, nonce, accounts[0], data)
-    await sleep(1500)
-    await assureOwnersAre(gnosisSafe, accounts[0], accounts[1], accounts[2])
-    await store.dispatch(fetchTransactions())
-
-
-    processOwnerModification(store, address, accounts[1])
-    await sleep(3000)
-    await store.dispatch(fetchTransactions())
-    await sleep(3000)
-    const tx = getTransactionFromReduxStore(store, address)
-    if (!tx) throw new Error()
-    const txHash = tx.get('tx')
-    expect(txHash).not.toBe('')
-    await assureThresholdIs(gnosisSafe, 2)
-    await assureOwnersAre(gnosisSafe, accounts[0], accounts[1])
-  })
-
-  it('remove owner decreasing the threshold', async () => {
-    // GIVEN
-    const numOwners = 2
-    const threshold = 2
-    const store = aNewStore()
-    const address = await aDeployedSafe(store, 10, threshold, numOwners)
-    const accounts = await promisify(cb => getWeb3().eth.getAccounts(cb))
-    const safe = getSafeFrom(store.getState(), address)
-    const gnosisSafe = await getGnosisSafeInstanceAt(address)
-
-    // WHEN
-    await assureThresholdIs(gnosisSafe, 2)
-    await assureOwnersAre(gnosisSafe, accounts[0], accounts[1])
-    const nonce = Date.now()
-    const accountIndex = 1
-    const data = gnosisSafe.contract.removeOwner.getData(accounts[accountIndex - 1], accounts[accountIndex], 1)
-    await createTransaction(safe, `Remove owner Address 2 ${nonce}`, address, 0, nonce, accounts[0], data)
-    await sleep(1500)
-    await assureOwnersAre(gnosisSafe, accounts[0], accounts[1])
-    await store.dispatch(fetchTransactions())
-
-
-    processOwnerModification(store, address, accounts[1])
-    await sleep(3000)
-    await store.dispatch(fetchTransactions())
-    await sleep(3000)
-    const tx = getTransactionFromReduxStore(store, address)
-    if (!tx) throw new Error()
-    const txHash = tx.get('tx')
-    expect(txHash).not.toBe('')
-    await assureThresholdIs(gnosisSafe, 1)
-    await assureOwnersAre(gnosisSafe, accounts[0])
-  })
-})

src/test/builder/safe.redux.builder.js

@@ -98,11 +98,14 @@ export const aMinedSafe = async (
     [FIELD_NAME]: 'Safe Name',
     [FIELD_CONFIRMATIONS]: `${threshold}`,
     [FIELD_OWNERS]: `${owners}`,
-    [getOwnerNameBy(0)]: 'Adolfo 1 Eth Account',
-    [getOwnerAddressBy(0)]: accounts[0],
     [FIELD_DAILY_LIMIT]: `${dailyLimit}`,
   }
 
+  for (let i = 0; i < owners; i += 1) {
+    form[getOwnerNameBy(i)] = `Adol ${i + 1} Eth Account`
+    form[getOwnerAddressBy(i)] = accounts[i]
+  }
+
   const addSafeFn: any = (...args) => store.dispatch(addSafe(...args))
   const openSafeProps: OpenState = await createSafe(form, accounts[0], addSafeFn)
 

src/test/safe.redux.owners.test.js

@@ -0,0 +1,190 @@
+// @flow
+import { aNewStore } from '~/store'
+import { getWeb3 } from '~/wallets/getWeb3'
+import { type Match } from 'react-router-dom'
+import { promisify } from '~/utils/promisify'
+import { processTransaction } from '~/routes/safe/component/Transactions/processTransactions'
+import { confirmationsTransactionSelector, safeSelector } from '~/routes/safe/store/selectors/index'
+import { getTransactionFromReduxStore } from '~/routes/safe/test/testMultisig'
+import { buildMathPropsFrom } from '~/test/utils/buildReactRouterProps'
+import fetchTransactions from '~/routes/safe/store/actions/fetchTransactions'
+import { type GlobalState } from '~/store/index'
+import { type Safe } from '~/routes/safe/store/model/safe'
+import { getGnosisSafeInstanceAt } from '~/wallets/safeContracts'
+import { aMinedSafe } from '~/test/builder/safe.redux.builder'
+import { NAME_PARAM, OWNER_ADDRESS_PARAM, INCREASE_PARAM } from '~/routes/safe/component/AddOwner/AddOwnerForm'
+import { addOwner } from '~/routes/safe/component/AddOwner/index'
+import fetchSafe from '~/routes/safe/store/actions/fetchSafe'
+import { removeOwner, shouldDecrease, initialValuesFrom } from '~/routes/safe/component/RemoveOwner'
+import { DECREASE_PARAM } from '~/routes/safe/component/RemoveOwner/RemoveOwnerForm/index'
+
+const getSafeFrom = (state: GlobalState, safeAddress: string): Safe => {
+  const match: Match = buildMathPropsFrom(safeAddress)
+  const safe = safeSelector(state, { match })
+  if (!safe) throw new Error()
+
+  return safe
+}
+
+describe('React DOM TESTS > Add and remove owners', () => {
+  const processOwnerModification = async (store, safeAddress, executor) => {
+    const tx = getTransactionFromReduxStore(store, safeAddress)
+    if (!tx) throw new Error()
+    const confirmed = confirmationsTransactionSelector(store.getState(), { transaction: tx })
+    const data = tx.get('data')
+    expect(data).not.toBe(null)
+    expect(data).not.toBe(undefined)
+    expect(data).not.toBe('')
+    return processTransaction(safeAddress, tx, confirmed, executor)
+  }
+
+  const assureThresholdIs = async (gnosisSafe, threshold: number) => {
+    const safeThreshold = await gnosisSafe.getThreshold()
+    expect(Number(safeThreshold)).toEqual(threshold)
+  }
+
+  const assureOwnersAre = async (gnosisSafe, ...owners) => {
+    const safeOwners = await gnosisSafe.getOwners()
+    expect(safeOwners.length).toEqual(owners.length)
+    for (let i = 0; i < owners.length; i += 1) {
+      expect(safeOwners[i]).toBe(owners[i])
+    }
+  }
+
+  const getAddressesFrom = (safe: Safe) => safe.get('owners').map(owner => owner.get('address'))
+
+  it('adds owner without increasing the threshold', async () => {
+    // GIVEN
+    const numOwners = 2
+    const threshold = 1
+    const store = aNewStore()
+    const address = await aMinedSafe(store, numOwners, threshold, 10)
+    const accounts = await promisify(cb => getWeb3().eth.getAccounts(cb))
+    const gnosisSafe = await getGnosisSafeInstanceAt(address)
+
+    const values = {
+      [NAME_PARAM]: 'Adol 3 Metamask',
+      [OWNER_ADDRESS_PARAM]: accounts[2],
+      [INCREASE_PARAM]: false,
+    }
+
+    // WHEN
+    let safe = getSafeFrom(store.getState(), address)
+    await addOwner(values, safe, threshold, accounts[0])
+
+    // THEN
+    await assureThresholdIs(gnosisSafe, 1)
+    await assureOwnersAre(gnosisSafe, accounts[2], accounts[0], accounts[1])
+
+    await store.dispatch(fetchSafe(safe))
+    safe = getSafeFrom(store.getState(), address)
+    expect(safe.get('owners').count()).toBe(3)
+    await assureOwnersAre(gnosisSafe, ...getAddressesFrom(safe))
+  })
+
+  it('adds owner increasing the threshold', async () => {
+    // GIVEN
+    const numOwners = 2
+    const threshold = 1
+    const store = aNewStore()
+    const address = await aMinedSafe(store, numOwners, threshold, 10)
+    const accounts = await promisify(cb => getWeb3().eth.getAccounts(cb))
+    const gnosisSafe = await getGnosisSafeInstanceAt(address)
+
+    const values = {
+      [NAME_PARAM]: 'Adol 3 Metamask',
+      [OWNER_ADDRESS_PARAM]: accounts[2],
+      [INCREASE_PARAM]: true,
+    }
+
+    // WHEN
+    let safe = getSafeFrom(store.getState(), address)
+    await addOwner(values, safe, threshold, accounts[0])
+
+    // THEN
+    await assureThresholdIs(gnosisSafe, 2)
+    await assureOwnersAre(gnosisSafe, accounts[2], accounts[0], accounts[1])
+
+    await store.dispatch(fetchSafe(safe))
+    safe = getSafeFrom(store.getState(), address)
+    expect(safe.get('owners').count()).toBe(3)
+    await assureOwnersAre(gnosisSafe, ...getAddressesFrom(safe))
+  })
+
+  it('remove owner decreasing owner automatically', async () => {
+    const numOwners = 2
+    const threshold = 2
+    const store = aNewStore()
+    const address = await aMinedSafe(store, numOwners, threshold, 10)
+    const accounts = await promisify(cb => getWeb3().eth.getAccounts(cb))
+    const gnosisSafe = await getGnosisSafeInstanceAt(address)
+
+    const decrease = shouldDecrease(numOwners, threshold)
+    const values = initialValuesFrom(decrease)
+    expect(values[DECREASE_PARAM]).toBe(true)
+
+    let safe = getSafeFrom(store.getState(), address)
+    await removeOwner(values, safe, threshold, accounts[1], 'Adol Metamask 2', accounts[0])
+    await store.dispatch(fetchTransactions())
+    await processOwnerModification(store, address, accounts[1])
+
+    await assureThresholdIs(gnosisSafe, 1)
+    await assureOwnersAre(gnosisSafe, accounts[0])
+
+    await store.dispatch(fetchSafe(safe))
+    safe = getSafeFrom(store.getState(), address)
+    expect(safe.get('owners').count()).toBe(1)
+    await assureOwnersAre(gnosisSafe, ...getAddressesFrom(safe))
+  })
+
+  it('remove owner decreasing threshold', async () => {
+    const numOwners = 3
+    const threshold = 2
+    const store = aNewStore()
+    const address = await aMinedSafe(store, numOwners, threshold, 10)
+    const accounts = await promisify(cb => getWeb3().eth.getAccounts(cb))
+    const gnosisSafe = await getGnosisSafeInstanceAt(address)
+
+    const decrease = true
+    const values = initialValuesFrom(decrease)
+
+    let safe = getSafeFrom(store.getState(), address)
+    await removeOwner(values, safe, threshold, accounts[2], 'Adol Metamask 3', accounts[0])
+    await store.dispatch(fetchTransactions())
+    await processOwnerModification(store, address, accounts[1])
+
+    await assureThresholdIs(gnosisSafe, 1)
+    await assureOwnersAre(gnosisSafe, accounts[0], accounts[1])
+
+    await store.dispatch(fetchSafe(safe))
+    safe = getSafeFrom(store.getState(), address)
+    expect(safe.get('owners').count()).toBe(2)
+    await assureOwnersAre(gnosisSafe, ...getAddressesFrom(safe))
+  })
+
+  it('remove owner without decreasing threshold', async () => {
+    const numOwners = 3
+    const threshold = 2
+    const store = aNewStore()
+    const address = await aMinedSafe(store, numOwners, threshold, 10)
+    const accounts = await promisify(cb => getWeb3().eth.getAccounts(cb))
+    const gnosisSafe = await getGnosisSafeInstanceAt(address)
+
+    const decrease = shouldDecrease(numOwners, threshold)
+    const values = initialValuesFrom(decrease)
+    expect(values[DECREASE_PARAM]).toBe(false)
+
+    let safe = getSafeFrom(store.getState(), address)
+    await removeOwner(values, safe, threshold, accounts[2], 'Adol Metamask 3', accounts[0])
+    await store.dispatch(fetchTransactions())
+    await processOwnerModification(store, address, accounts[1])
+
+    await assureThresholdIs(gnosisSafe, 2)
+    await assureOwnersAre(gnosisSafe, accounts[0], accounts[1])
+
+    await store.dispatch(fetchSafe(safe))
+    safe = getSafeFrom(store.getState(), address)
+    expect(safe.get('owners').count()).toBe(2)
+    await assureOwnersAre(gnosisSafe, ...getAddressesFrom(safe))
+  })
+})