From c13711906db24e67445dda45a5741a229ec07a60 Mon Sep 17 00:00:00 2001 From: apanizo Date: Wed, 4 Jul 2018 15:36:27 +0200 Subject: [PATCH] WA-232 Refactor add & remove owners DOM tests (from 55 seconds to 5 seconds) --- src/routes/safe/component/AddOwner/index.jsx | 22 +- .../safe/component/RemoveOwner/index.jsx | 40 ++-- src/routes/safe/test/Safe.owners.test.js | 187 ----------------- src/test/builder/safe.redux.builder.js | 7 +- src/test/safe.redux.owners.test.js | 190 ++++++++++++++++++ 5 files changed, 233 insertions(+), 213 deletions(-) delete mode 100644 src/routes/safe/test/Safe.owners.test.js create mode 100644 src/test/safe.redux.owners.test.js diff --git a/src/routes/safe/component/AddOwner/index.jsx b/src/routes/safe/component/AddOwner/index.jsx index a0812731..dcc3fb53 100644 --- a/src/routes/safe/component/AddOwner/index.jsx +++ b/src/routes/safe/component/AddOwner/index.jsx @@ -35,6 +35,18 @@ const getOwnerAddressesFrom = (owners: List) => { 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 { state = { done: false, @@ -45,15 +57,7 @@ class AddOwner extends React.Component { const { safe, threshold, userAddress, fetchTransactions, } = this.props - 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, userAddress, data) - setOwners(safeAddress, safe.get('owners').push(makeOwner({ name: newOwnerName, address: newOwnerAddress }))) + await addOwner(values, safe, threshold, userAddress) fetchTransactions() this.setState({ done: true }) } catch (error) { diff --git a/src/routes/safe/component/RemoveOwner/index.jsx b/src/routes/safe/component/RemoveOwner/index.jsx index b6955b59..eea7dcfe 100644 --- a/src/routes/safe/component/RemoveOwner/index.jsx +++ b/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 { state = { @@ -43,19 +65,7 @@ class RemoveOwner extends React.Component { const { safe, threshold, executor, fetchTransactions, userToRemove, name, } = this.props - 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 - await createTransaction(safe, `Remove Owner ${text}`, safeAddress, 0, nonce, executor, data) - + await removeOwner(values, safe, threshold, userToRemove, executor, name) fetchTransactions() this.setState({ done: true }) } catch (error) { diff --git a/src/routes/safe/test/Safe.owners.test.js b/src/routes/safe/test/Safe.owners.test.js deleted file mode 100644 index de44e8d0..00000000 --- a/src/routes/safe/test/Safe.owners.test.js +++ /dev/null @@ -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]) - }) -}) diff --git a/src/test/builder/safe.redux.builder.js b/src/test/builder/safe.redux.builder.js index 4407cebc..9c467981 100644 --- a/src/test/builder/safe.redux.builder.js +++ b/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) diff --git a/src/test/safe.redux.owners.test.js b/src/test/safe.redux.owners.test.js new file mode 100644 index 00000000..375c1434 --- /dev/null +++ b/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)) + }) +})