From 1e9199db3238cdae215838a26d082b46b85f9eb3 Mon Sep 17 00:00:00 2001 From: apanizo Date: Tue, 5 Jun 2018 13:27:14 +0200 Subject: [PATCH] WA-238 Adding logic (including tests) for handling change threshold --- config/jest/jest.setup.js | 2 +- .../AddTransaction/createTransactions.js | 16 ++- .../AddTransaction/test/transactions.test.js | 34 ++--- .../AddTransaction/test/transactionsHelper.js | 3 +- .../Transactions/processTransactions.js | 14 ++- src/routes/safe/store/model/transaction.js | 2 + src/routes/safe/test/Safe.threshold.test.js | 117 ++++++++++++++++++ 7 files changed, 157 insertions(+), 31 deletions(-) create mode 100644 src/routes/safe/test/Safe.threshold.test.js diff --git a/config/jest/jest.setup.js b/config/jest/jest.setup.js index 8d9161a7..c97ef811 100644 --- a/config/jest/jest.setup.js +++ b/config/jest/jest.setup.js @@ -1,2 +1,2 @@ // @flow -jest.setTimeout(30000) +jest.setTimeout(45000) diff --git a/src/routes/safe/component/AddTransaction/createTransactions.js b/src/routes/safe/component/AddTransaction/createTransactions.js index 7ac3956b..d0d83569 100644 --- a/src/routes/safe/component/AddTransaction/createTransactions.js +++ b/src/routes/safe/component/AddTransaction/createTransactions.js @@ -47,6 +47,7 @@ export const storeTransaction = ( tx: string, safeAddress: string, safeThreshold: number, + data: string, ) => { const notMinedWhenOneOwnerSafe = confirmations.count() === 1 && !tx if (notMinedWhenOneOwnerSafe) { @@ -54,7 +55,7 @@ export const storeTransaction = ( } const transaction: Transaction = makeTransaction({ - name, nonce, value, confirmations, destination, threshold: safeThreshold, tx, + name, nonce, value, confirmations, destination, threshold: safeThreshold, tx, data, }) const safeTransactions = load(TX_KEY) || {} @@ -82,10 +83,11 @@ const hasOneOwner = (safe: Safe) => { export const createTransaction = async ( safe: Safe, txName: string, - txDestination: string, + txDest: string, txValue: number, nonce: number, user: string, + data: string = '0x', ) => { const web3 = getWeb3() const GnosisSafe = await getGnosisSafeContract(web3) @@ -97,19 +99,21 @@ export const createTransaction = async ( const thresholdIsOne = safe.get('confirmations') === 1 if (hasOneOwner(safe) || thresholdIsOne) { - const txConfirmationData = gnosisSafe.contract.execTransactionIfApproved.getData(txDestination, valueInWei, '0x', CALL, nonce) + const txConfirmationData = + gnosisSafe.contract.execTransactionIfApproved.getData(txDest, valueInWei, data, CALL, nonce) const txHash = await executeTransaction(txConfirmationData, user, safeAddress) checkReceiptStatus(txHash) const executedConfirmations: List = buildExecutedConfirmationFrom(safe.get('owners'), user) - return storeTransaction(txName, nonce, txDestination, txValue, user, executedConfirmations, txHash, safeAddress, safe.get('confirmations')) + return storeTransaction(txName, nonce, txDest, txValue, user, executedConfirmations, txHash, safeAddress, safe.get('confirmations'), data) } - const txConfirmationData = gnosisSafe.contract.approveTransactionWithParameters.getData(txDestination, valueInWei, '0x', CALL, nonce) + const txConfirmationData = + gnosisSafe.contract.approveTransactionWithParameters.getData(txDest, valueInWei, data, CALL, nonce) const txConfirmationHash = await executeTransaction(txConfirmationData, user, safeAddress) checkReceiptStatus(txConfirmationHash) const confirmations: List = buildConfirmationsFrom(safe.get('owners'), user, txConfirmationHash) - return storeTransaction(txName, nonce, txDestination, txValue, user, confirmations, '', safeAddress, safe.get('confirmations')) + return storeTransaction(txName, nonce, txDest, txValue, user, confirmations, '', safeAddress, safe.get('confirmations'), data) } diff --git a/src/routes/safe/component/AddTransaction/test/transactions.test.js b/src/routes/safe/component/AddTransaction/test/transactions.test.js index 367403bf..19c1fdeb 100644 --- a/src/routes/safe/component/AddTransaction/test/transactions.test.js +++ b/src/routes/safe/component/AddTransaction/test/transactions.test.js @@ -33,7 +33,7 @@ describe('Transactions Suite', () => { const txName = 'Buy butteries for project' const nonce: number = 10 const confirmations: List = buildConfirmationsFrom(owners, 'foo', 'confirmationHash') - storeTransaction(txName, nonce, destination, value, 'foo', confirmations, '', safe.get('address'), safe.get('confirmations')) + storeTransaction(txName, nonce, destination, value, 'foo', confirmations, '', safe.get('address'), safe.get('confirmations'), '0x') // WHEN const transactions: Map> = loadSafeTransactions() @@ -45,7 +45,7 @@ describe('Transactions Suite', () => { if (!safeTransactions) { throw new Error() } testSizeOfTransactions(safeTransactions, 1) - testTransactionFrom(safeTransactions, 0, txName, nonce, value, 2, destination, 'foo', 'confirmationHash', owners.get(0), owners.get(1)) + testTransactionFrom(safeTransactions, 0, txName, nonce, value, 2, destination, '0x', 'foo', 'confirmationHash', owners.get(0), owners.get(1)) }) it('adds second confirmation to stored safe with one confirmation', async () => { @@ -55,12 +55,12 @@ describe('Transactions Suite', () => { const safeAddress = safe.get('address') const creator = 'foo' const confirmations: List = buildConfirmationsFrom(owners, creator, 'confirmationHash') - storeTransaction(firstTxName, firstNonce, destination, value, creator, confirmations, '', safeAddress, safe.get('confirmations')) + storeTransaction(firstTxName, firstNonce, destination, value, creator, confirmations, '', safeAddress, safe.get('confirmations'), '0x') const secondTxName = 'Buy printers for project' const secondNonce: number = firstNonce + 100 const secondConfirmations: List = buildConfirmationsFrom(owners, creator, 'confirmationHash') - storeTransaction(secondTxName, secondNonce, destination, value, creator, secondConfirmations, '', safeAddress, safe.get('confirmations')) + storeTransaction(secondTxName, secondNonce, destination, value, creator, secondConfirmations, '', safeAddress, safe.get('confirmations'), '0x') // WHEN const transactions: Map> = loadSafeTransactions() @@ -72,8 +72,8 @@ describe('Transactions Suite', () => { if (!safeTxs) { throw new Error() } testSizeOfTransactions(safeTxs, 2) - testTransactionFrom(safeTxs, 0, firstTxName, firstNonce, value, 2, destination, 'foo', 'confirmationHash', owners.get(0), owners.get(1)) - testTransactionFrom(safeTxs, 1, secondTxName, secondNonce, value, 2, destination, 'foo', 'confirmationHash', owners.get(0), owners.get(1)) + testTransactionFrom(safeTxs, 0, firstTxName, firstNonce, value, 2, destination, '0x', 'foo', 'confirmationHash', owners.get(0), owners.get(1)) + testTransactionFrom(safeTxs, 1, secondTxName, secondNonce, value, 2, destination, '0x', 'foo', 'confirmationHash', owners.get(0), owners.get(1)) }) it('adds second confirmation to stored safe having two safes with one confirmation each', async () => { @@ -82,7 +82,7 @@ describe('Transactions Suite', () => { const safeAddress = safe.address const creator = 'foo' const confirmations: List = buildConfirmationsFrom(owners, creator, 'confirmationHash') - storeTransaction(txName, nonce, destination, value, creator, confirmations, '', safeAddress, safe.get('confirmations')) + storeTransaction(txName, nonce, destination, value, creator, confirmations, '', safeAddress, safe.get('confirmations'), '0x') const secondSafe = SafeFactory.dailyLimitSafe(10, 2) const txSecondName = 'Buy batteris for Beta project' @@ -92,7 +92,7 @@ describe('Transactions Suite', () => { const secondConfirmations: List = buildConfirmationsFrom(secondSafe.get('owners'), secondCreator, 'confirmationHash') storeTransaction( txSecondName, txSecondNonce, destination, value, secondCreator, - secondConfirmations, '', secondSafeAddress, secondSafe.get('confirmations'), + secondConfirmations, '', secondSafeAddress, secondSafe.get('confirmations'), '0x', ) let transactions: Map> = loadSafeTransactions() @@ -112,7 +112,7 @@ describe('Transactions Suite', () => { const txConfirmations: List = buildConfirmationsFrom(owners, creator, 'secondConfirmationHash') storeTransaction( txFirstName, txFirstNonce, destination, value, creator, - txConfirmations, '', safe.get('address'), safe.get('confirmations'), + txConfirmations, '', safe.get('address'), safe.get('confirmations'), '0x', ) transactions = loadSafeTransactions() @@ -125,19 +125,19 @@ describe('Transactions Suite', () => { // Test 2 transactions of first safe testTransactionFrom( transactions.get(safe.address), 0, - txName, nonce, value, 2, destination, + txName, nonce, value, 2, destination, '0x', 'foo', 'confirmationHash', owners.get(0), owners.get(1), ) testTransactionFrom( transactions.get(safe.address), 1, - txFirstName, txFirstNonce, value, 2, destination, + txFirstName, txFirstNonce, value, 2, destination, '0x', 'foo', 'secondConfirmationHash', owners.get(0), owners.get(1), ) // Test one transaction of second safe testTransactionFrom( transactions.get(secondSafe.address), 0, - txSecondName, txSecondNonce, value, 2, destination, + txSecondName, txSecondNonce, value, 2, destination, '0x', '0x03db1a8b26d08df23337e9276a36b474510f0023', 'confirmationHash', secondSafe.get('owners').get(0), secondSafe.get('owners').get(1), ) }) @@ -148,10 +148,10 @@ describe('Transactions Suite', () => { const nonce: number = 10 const creator = 'foo' const confirmations: List = buildConfirmationsFrom(owners, creator, 'confirmationHash') - storeTransaction(txName, nonce, destination, value, creator, confirmations, '', safe.get('address'), safe.get('confirmations')) + storeTransaction(txName, nonce, destination, value, creator, confirmations, '', safe.get('address'), safe.get('confirmations'), '0x') // WHEN - const createTxFnc = () => storeTransaction(txName, nonce, destination, value, creator, confirmations, '', safe.get('address'), safe.get('confirmations')) + const createTxFnc = () => storeTransaction(txName, nonce, destination, value, creator, confirmations, '', safe.get('address'), safe.get('confirmations'), '0x') expect(createTxFnc).toThrow(/Transaction with same nonce/) }) @@ -161,7 +161,7 @@ describe('Transactions Suite', () => { const nonce: number = 10 const creator = 'foo' const confirmations: List = buildConfirmationsFrom(owners, creator, 'confirmationHash') - storeTransaction(txName, nonce, destination, value, creator, confirmations, '', safe.get('address'), safe.get('confirmations')) + storeTransaction(txName, nonce, destination, value, creator, confirmations, '', safe.get('address'), safe.get('confirmations'), '0x') // WHEN const transactions: Map> = loadSafeTransactions() @@ -185,7 +185,7 @@ describe('Transactions Suite', () => { const nonce: number = 10 const tx = '' const confirmations: List = buildExecutedConfirmationFrom(oneOwnerSafe.get('owners'), ownerName) - const createTxFnc = () => storeTransaction(txName, nonce, destination, value, ownerName, confirmations, tx, oneOwnerSafe.get('address'), oneOwnerSafe.get('confirmations')) + const createTxFnc = () => storeTransaction(txName, nonce, destination, value, ownerName, confirmations, tx, oneOwnerSafe.get('address'), oneOwnerSafe.get('confirmations'), '0x') expect(createTxFnc).toThrow(/The tx should be mined before storing it in safes with one owner/) }) @@ -197,7 +197,7 @@ describe('Transactions Suite', () => { const nonce: number = 10 const tx = 'validTxHash' const confirmations: List = buildExecutedConfirmationFrom(oneOwnerSafe.get('owners'), ownerName) - storeTransaction(txName, nonce, destination, value, ownerName, confirmations, tx, oneOwnerSafe.get('address'), oneOwnerSafe.get('confirmations')) + storeTransaction(txName, nonce, destination, value, ownerName, confirmations, tx, oneOwnerSafe.get('address'), oneOwnerSafe.get('confirmations'), '0x') // WHEN const safeTransactions: Map> = loadSafeTransactions() diff --git a/src/routes/safe/component/AddTransaction/test/transactionsHelper.js b/src/routes/safe/component/AddTransaction/test/transactionsHelper.js index 9b92df35..8ad22983 100644 --- a/src/routes/safe/component/AddTransaction/test/transactionsHelper.js +++ b/src/routes/safe/component/AddTransaction/test/transactionsHelper.js @@ -20,7 +20,7 @@ export const testSizeOfTransactions = (safeTxs: List | typeof undef export const testTransactionFrom = ( safeTxs: List | typeof undefined, pos: number, name: string, nonce: number, value: number, threshold: number, destination: string, - creator: string, txHash: string, + data: string, creator: string, txHash: string, firstOwner: Owner | typeof undefined, secondOwner: Owner | typeof undefined, ) => { if (!safeTxs) { throw new Error() } @@ -33,6 +33,7 @@ export const testTransactionFrom = ( expect(tx.get('destination')).toBe(destination) expect(tx.get('confirmations').count()).toBe(2) expect(tx.get('nonce')).toBe(nonce) + expect(tx.get('data')).toBe(data) const confirmations: List = tx.get('confirmations') const firstConfirmation: Confirmation | typeof undefined = confirmations.get(0) diff --git a/src/routes/safe/component/Transactions/processTransactions.js b/src/routes/safe/component/Transactions/processTransactions.js index dfbc64d2..32f0d123 100644 --- a/src/routes/safe/component/Transactions/processTransactions.js +++ b/src/routes/safe/component/Transactions/processTransactions.js @@ -20,9 +20,10 @@ export const updateTransaction = ( tx: string, safeAddress: string, safeThreshold: number, + data: string, ) => { const transaction: Transaction = makeTransaction({ - name, nonce, value, confirmations, destination, threshold: safeThreshold, tx, + name, nonce, value, confirmations, destination, threshold: safeThreshold, tx, data, }) const safeTransactions = load(TX_KEY) || {} @@ -36,7 +37,6 @@ export const updateTransaction = ( localStorage.setItem(TX_KEY, JSON.stringify(safeTransactions)) } -const getData = () => '0x' const getOperation = () => 0 const execTransaction = async ( @@ -45,8 +45,8 @@ const execTransaction = async ( txValue: number, nonce: number, executor: string, + data: string, ) => { - const data = getData() const CALL = getOperation() const web3 = getWeb3() const valueInWei = web3.toWei(txValue, 'ether') @@ -61,8 +61,8 @@ const execConfirmation = async ( txValue: number, nonce: number, executor: string, + data: string, ) => { - const data = getData() const CALL = getOperation() const web3 = getWeb3() const valueInWei = web3.toWei(txValue, 'ether') @@ -110,10 +110,11 @@ export const processTransaction = async ( const txName = tx.get('name') const txValue = tx.get('value') const txDestination = tx.get('destination') + const data = tx.get('data') const txHash = thresholdReached - ? await execTransaction(gnosisSafe, txDestination, txValue, nonce, userAddress) - : await execConfirmation(gnosisSafe, txDestination, txValue, nonce, userAddress) + ? await execTransaction(gnosisSafe, txDestination, txValue, nonce, userAddress, data) + : await execConfirmation(gnosisSafe, txDestination, txValue, nonce, userAddress, data) checkReceiptStatus(txHash) @@ -130,5 +131,6 @@ export const processTransaction = async ( thresholdReached ? txHash : '', safeAddress, threshold, + data, ) } diff --git a/src/routes/safe/store/model/transaction.js b/src/routes/safe/store/model/transaction.js index 83a664d0..e810ed00 100644 --- a/src/routes/safe/store/model/transaction.js +++ b/src/routes/safe/store/model/transaction.js @@ -11,6 +11,7 @@ export type TransactionProps = { confirmations: List, destination: string, tx: string, + data: string, } export const makeTransaction: RecordFactory = Record({ @@ -21,6 +22,7 @@ export const makeTransaction: RecordFactory = Record({ destination: '', tx: '', threshold: 0, + data: '', }) export type Transaction = RecordOf diff --git a/src/routes/safe/test/Safe.threshold.test.js b/src/routes/safe/test/Safe.threshold.test.js new file mode 100644 index 00000000..1ca38066 --- /dev/null +++ b/src/routes/safe/test/Safe.threshold.test.js @@ -0,0 +1,117 @@ +// @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 { 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/buildReactRouterProps' +import { createTransaction } from '~/routes/safe/component/AddTransaction/createTransactions' +import { getGnosisSafeContract } from '~/wallets/safeContracts' +import fetchTransactions from '~/routes/safe/store/actions/fetchTransactions' + +describe('React DOM TESTS > Change threshold', () => { + it('should update the threshold directly if safe has 1 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 match: Match = buildMathPropsFrom(address) + const safe = safeSelector(store.getState(), { match }) + const web3 = getWeb3() + const GnosisSafe = await getGnosisSafeContract(web3) + const gnosisSafe = GnosisSafe.at(address) + + // WHEN + const nonce = Date.now() + const data = gnosisSafe.contract.changeThreshold.getData(2) + await createTransaction(safe, "Change Safe's threshold", 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 thresholdTx: Transaction = transactions.get(0) + expect(thresholdTx.get('tx')).not.toBe(null) + expect(thresholdTx.get('tx')).not.toBe(undefined) + expect(thresholdTx.get('tx')).not.toBe('') + + const safeThreshold = await gnosisSafe.getThreshold() + expect(Number(safeThreshold)).toEqual(2) + }) + + const changeThreshold = async (store, safeAddress, executor) => { + const tx = getTransactionFromReduxStore(store, safeAddress) + 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('should wait for confirmation to update threshold when safe has 1+ 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 match: Match = buildMathPropsFrom(address) + const safe = safeSelector(store.getState(), { match }) + const web3 = getWeb3() + const GnosisSafe = await getGnosisSafeContract(web3) + const gnosisSafe = GnosisSafe.at(address) + + // WHEN + const nonce = Date.now() + const data = gnosisSafe.contract.changeThreshold.getData(3) + await createTransaction(safe, "Change Safe's threshold", address, 0, nonce, accounts[0], data) + await sleep(1500) + await store.dispatch(fetchTransactions()) + + let transactions = safeTransactionsSelector(store.getState(), { safeAddress: address }) + expect(transactions.count()).toBe(1) + + let thresholdTx: Transaction = transactions.get(0) + expect(thresholdTx.get('tx')).toBe('') + let firstOwnerConfirmation = thresholdTx.get('confirmations').get(0) + if (!firstOwnerConfirmation) throw new Error() + expect(firstOwnerConfirmation.get('status')).toBe(true) + let secondOwnerConfirmation = thresholdTx.get('confirmations').get(1) + if (!secondOwnerConfirmation) throw new Error() + expect(secondOwnerConfirmation.get('status')).toBe(false) + + let safeThreshold = await gnosisSafe.getThreshold() + expect(Number(safeThreshold)).toEqual(2) + + // THEN + await changeThreshold(store, address, accounts[1]) + safeThreshold = await gnosisSafe.getThreshold() + expect(Number(safeThreshold)).toEqual(3) + + await store.dispatch(fetchTransactions()) + sleep(1200) + transactions = safeTransactionsSelector(store.getState(), { safeAddress: address }) + expect(transactions.count()).toBe(1) + + thresholdTx = transactions.get(0) + expect(thresholdTx.get('tx')).not.toBe(undefined) + expect(thresholdTx.get('tx')).not.toBe(null) + expect(thresholdTx.get('tx')).not.toBe('') + + firstOwnerConfirmation = thresholdTx.get('confirmations').get(0) + if (!firstOwnerConfirmation) throw new Error() + expect(firstOwnerConfirmation.get('status')).toBe(true) + secondOwnerConfirmation = thresholdTx.get('confirmations').get(1) + if (!secondOwnerConfirmation) throw new Error() + expect(secondOwnerConfirmation.get('status')).toBe(true) + }) +})