From 3e4961a4f3315a206c02cfe81898051cb260fc98 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Kosi=C5=84ski?= Date: Mon, 23 Apr 2018 15:40:26 +0200 Subject: [PATCH 1/2] Refactor requestFactory tests --- test/request-factory-tests/requestFactory.js | 375 +++++-------------- 1 file changed, 95 insertions(+), 280 deletions(-) diff --git a/test/request-factory-tests/requestFactory.js b/test/request-factory-tests/requestFactory.js index bcb88c567..8b426002a 100644 --- a/test/request-factory-tests/requestFactory.js +++ b/test/request-factory-tests/requestFactory.js @@ -19,72 +19,86 @@ const NULL_ADDR = "0x0000000000000000000000000000000000000000" // Note - these tests were checked very well and should never be wrong. // If they start failing - look in the contracts. contract("Request factory", async (accounts) => { - it("should create a request with provided properties", async () => { - // Get the instance of the deployed RequestLib - const requestLib = await RequestLib.deployed() - expect(requestLib.address).to.exist - - // Get the current block - const curBlock = await config.web3.eth.getBlockNumber() - - // Set up the data for our transaction request - const claimWindowSize = 255 - const fee = 12345 - const bounty = 54321 - const freezePeriod = 10 - const windowStart = curBlock + 20 - const windowSize = 511 - const reservedWindowSize = 16 - const temporalUnit = 1 - const callValue = 123456789 - const callGas = 1000000 - const gasPrice = 1000000 - const requiredDeposit = 1000000 - const testCallData = "this-is-call-data" + let requestLib + let transactionRequestCore + let requestFactory + + const transactionRequest = { + claimWindowSize: 255, + fee: 12345, + bounty: 54321, + freezePeriod: 10, + windowSize: 511, + reservedWindowSize: 16, + temporalUnit: 1, + callValue: 123456789, + callGas: 1000000, + gasPrice: 1000000, + requiredDeposit: 1000000, + testCallData: "this-is-call-data", + endowment: 10 ** 18 + } + + const getWindowStart = async () => (await config.web3.eth.getBlockNumber()) + 20 + + const createValidationParams = async (properties = {}) => { + const request = Object.assign( + {}, + transactionRequest, + { windowStart: await getWindowStart() }, + properties + ) - // Validate the data with the RequestLib + return [ + request.fee, + request.bounty, + request.claimWindowSize, + request.freezePeriod, + request.reservedWindowSize, + request.temporalUnit, + request.windowSize, + request.windowStart, + request.callGas, + request.callValue, + ] + } + + const validate = async ({ + endowment = transactionRequest.endowment, + properties, + to = accounts[2] + }) => { + const paramsForValidation = await createValidationParams(properties) const isValid = await requestLib.validate( - [accounts[0], accounts[0], accounts[1], accounts[2]], - [ - fee, - bounty, - claimWindowSize, - freezePeriod, - reservedWindowSize, - temporalUnit, - windowSize, - windowStart, - callGas, - callValue, - ], - "this-is-call-data", - config.web3.utils.toWei("10") // endowment calculate actual endowment + [accounts[0], accounts[0], accounts[1], to], + paramsForValidation, + transactionRequest.testCallData, + endowment ) - isValid.forEach(bool => expect(bool).to.be.true) + return { paramsForValidation, isValid } + } + + before(async () => { + requestLib = await RequestLib.deployed() + expect(requestLib.address).to.exist - // We need a transaction request core for the factory - const transactionRequestCore = await TransactionRequestCore.deployed() + transactionRequestCore = await TransactionRequestCore.deployed() expect(transactionRequestCore.address).to.exist - // Pass the request tracker to the factory - const requestFactory = await RequestFactory.new(transactionRequestCore.address) + requestFactory = await RequestFactory.new(transactionRequestCore.address) expect(requestFactory.address).to.exist + }) - const params = [ - fee, - bounty, - claimWindowSize, - freezePeriod, - reservedWindowSize, - temporalUnit, - windowSize, - windowStart, - callGas, - callValue, - gasPrice, - requiredDeposit - ] + it("should create a request with provided properties", async () => { + const windowStart = await getWindowStart() + const { paramsForValidation, isValid } = await validate({ properties: { windowStart } }) + + isValid.forEach(bool => expect(bool).to.be.true) + + const params = paramsForValidation + params.push(transactionRequest.gasPrice) + params.push(transactionRequest.requiredDeposit) // Create a request with the same args we validated const createTx = await requestFactory.createRequest( @@ -94,14 +108,16 @@ contract("Request factory", async (accounts) => { accounts[2], // to ], params, - testCallData + transactionRequest.testCallData ) + expect(createTx.receipt).to.exist const logRequestCreated = createTx.logs.find(e => e.event === "RequestCreated") expect(logRequestCreated.args.request).to.exist expect(logRequestCreated.args.params.length).to.equal(12) + logRequestCreated.args.params.forEach((el, idx) => expect(el.toNumber()).to.equal(params[idx])) const bucket = calculateBlockBucket(windowStart) @@ -127,35 +143,37 @@ contract("Request factory", async (accounts) => { expect(requestData.claimData.paymentModifier).to.equal(0) - expect(requestData.paymentData.fee).to.equal(fee) + expect(requestData.paymentData.fee).to.equal(transactionRequest.fee) expect(requestData.paymentData.feeRecipient).to.equal(accounts[1]) expect(requestData.paymentData.feeOwed).to.equal(0) - expect(requestData.paymentData.bounty).to.equal(bounty) + expect(requestData.paymentData.bounty).to.equal(transactionRequest.bounty) expect(requestData.paymentData.bountyBenefactor).to.equal(NULL_ADDR) expect(requestData.paymentData.bountyOwed).to.equal(0) - expect(requestData.schedule.claimWindowSize).to.equal(claimWindowSize) + expect(requestData.schedule.claimWindowSize).to.equal(transactionRequest.claimWindowSize) - expect(requestData.schedule.freezePeriod).to.equal(freezePeriod) + expect(requestData.schedule.freezePeriod).to.equal(transactionRequest.freezePeriod) expect(requestData.schedule.windowStart).to.equal(windowStart) - expect(requestData.schedule.reservedWindowSize).to.equal(reservedWindowSize) + expect(requestData.schedule.reservedWindowSize).to.equal(transactionRequest.reservedWindowSize) expect(requestData.schedule.temporalUnit).to.equal(1) expect(requestData.txData.toAddress).to.equal(accounts[2]) - expect(await txRequest.callData()).to.equal(ethUtil.bufferToHex(Buffer.from(testCallData))) + const expectedCallData = ethUtil.bufferToHex(Buffer.from(transactionRequest.testCallData)) + const callData = await txRequest.callData() + expect(callData).to.equal(expectedCallData) - expect(requestData.txData.callValue).to.equal(callValue) + expect(requestData.txData.callValue).to.equal(transactionRequest.callValue) - expect(requestData.txData.callGas).to.equal(callGas) + expect(requestData.txData.callGas).to.equal(transactionRequest.callGas) // Lastly, we just make sure that the transaction request // address is a known request for the factory. @@ -165,40 +183,8 @@ contract("Request factory", async (accounts) => { }) it("should test request factory insufficient endowment validation error", async () => { - const curBlock = await config.web3.eth.getBlockNumber() - - const requestLib = await RequestLib.deployed() - expect(requestLib.address).to.exist - - const claimWindowSize = 255 - const fee = 12345 - const bounty = 54321 - const freezePeriod = 10 - const windowStart = curBlock + 20 - const windowSize = 255 - const reservedWindowSize = 16 - const temporalUnit = 1 - const callValue = 123456789 - const callGas = 1000000 - // Validate the data with the RequestLib - const isValid = await requestLib.validate( - [accounts[0], accounts[0], accounts[1], accounts[2]], - [ - fee, - bounty, - claimWindowSize, - freezePeriod, - reservedWindowSize, - temporalUnit, - windowSize, - windowStart, - callGas, - callValue, - ], - "this-is-call-data", - 1 // endowment ATTENTION THIS IS TOO SMALL, HENCE WHY IT FAILS - ) + const { isValid } = await validate({ endowment: 1 }) expect(isValid[0]).to.be.false @@ -206,219 +192,48 @@ contract("Request factory", async (accounts) => { }) it("should test request factory throws validation error on too large of a reserve window", async () => { - const curBlock = await config.web3.eth.getBlockNumber() - - const requestLib = await RequestLib.deployed() - expect(requestLib.address).to.exist - - const claimWindowSize = 255 - const fee = 12345 - const bounty = 54321 - const freezePeriod = 10 - const windowStart = curBlock + 20 - const windowSize = 255 - const reservedWindowSize = 255 + 2 // 2 more than window size - const temporalUnit = 1 - const callValue = 123456789 - const callGas = 1000000 - - // Validate the data with the RequestLib - const isValid = await requestLib.validate( - [accounts[0], accounts[0], accounts[1], accounts[2]], - [ - fee, - bounty, - claimWindowSize, - freezePeriod, - reservedWindowSize, - temporalUnit, - windowSize, - windowStart, - callGas, - callValue, - ], - "this-is-call-data", - config.web3.utils.toWei("10") // endowment - ) + const reservedWindowSize = transactionRequest.windowSize + 2 + const { isValid } = await validate({ properties: { reservedWindowSize } }) expect(isValid[1]).to.be.false - expect(isValid[0]).to.be.true - isValid.slice(2).forEach(bool => expect(bool).to.be.true) }) it("should test request factory throws invalid temporal unit validation error", async () => { - const curBlock = await config.web3.eth.getBlockNumber() - - const requestLib = await RequestLib.deployed() - expect(requestLib.address).to.exist - - const claimWindowSize = 255 - const fee = 12345 - const bounty = 54321 - const freezePeriod = 10 - const windowStart = curBlock + 20 - const windowSize = 255 - const reservedWindowSize = 16 - const temporalUnit = 3 // Only 1 and 2 are supported - const callValue = 123456789 - const callGas = 1000000 - - // Validate the data with the RequestLib - const isValid = await requestLib.validate( - [accounts[0], accounts[0], accounts[1], accounts[2]], - [ - fee, - bounty, - claimWindowSize, - freezePeriod, - reservedWindowSize, - temporalUnit, - windowSize, - windowStart, - callGas, - callValue, - ], - "this-is-call-data", - config.web3.utils.toWei("10") // endowment - ) + const temporalUnit = 100 + const { isValid } = await validate({ properties: { temporalUnit } }) expect(isValid[2]).to.be.false - expect(isValid[3]).to.be.false - isValid.slice(0, 2).forEach(bool => expect(bool).to.be.true) isValid.slice(4).forEach(bool => expect(bool).to.be.true) }) it("should test request factory too soon execution window validation error", async () => { - const curBlock = await config.web3.eth.getBlockNumber() - - const requestLib = await RequestLib.deployed() - expect(requestLib.address).to.exist - - const claimWindowSize = 255 - const fee = 12345 - const bounty = 54321 - const freezePeriod = 11 // more than the blocks between now and the window start - const windowStart = curBlock + 10 - const windowSize = 255 - const reservedWindowSize = 16 - const temporalUnit = 1 - const callValue = 123456789 - const callGas = 1000000 - - // Validate the data with the RequestLib - const isValid = await requestLib.validate( - [accounts[0], accounts[0], accounts[1], accounts[2]], - [ - fee, - bounty, - claimWindowSize, - freezePeriod, - reservedWindowSize, - temporalUnit, - windowSize, - windowStart, - callGas, - callValue, - ], - "this-is-call-data", - config.web3.utils.toWei("10") // endowment - ) + const windowStart = await getWindowStart() + const freezePeriod = windowStart + 1 + const { isValid } = await validate({ properties: { windowStart, freezePeriod } }) expect(isValid[3]).to.be.false - isValid.slice(0, 3).forEach(bool => expect(bool).to.be.true) isValid.slice(4).forEach(bool => expect(bool).to.be.true) }) it("should test request factory has too high call gas validation error", async () => { - const curBlock = await config.web3.eth.getBlockNumber() - - const requestLib = await RequestLib.deployed() - expect(requestLib.address).to.exist - - const claimWindowSize = 255 - const fee = 12345 - const bounty = 54321 - const freezePeriod = 10 - const windowStart = curBlock + 20 - const windowSize = 255 - const reservedWindowSize = 16 - const temporalUnit = 1 - const callValue = 123456789 const callGas = 8.8e8 // cannot be over gas limit - - // Validate the data with the RequestLib - const isValid = await requestLib.validate( - [accounts[0], accounts[0], accounts[1], accounts[2]], - [ - fee, - bounty, - claimWindowSize, - freezePeriod, - reservedWindowSize, - temporalUnit, - windowSize, - windowStart, - callGas, - callValue, - ], - "this-is-call-data", - config.web3.utils.toWei("10") // endowment - ) + const { isValid } = await validate({ properties: { callGas } }) expect(isValid[4]).to.be.false - isValid.slice(0, 4).forEach(bool => expect(bool).to.be.true) isValid.slice(5).forEach(bool => expect(bool).to.be.true) }) it("should test null to address validation error", async () => { - const curBlock = await config.web3.eth.getBlockNumber() - - const requestLib = await RequestLib.deployed() - expect(requestLib.address).to.exist - - const claimWindowSize = 255 - const fee = 12345 - const bounty = 54321 - const freezePeriod = 10 - const windowStart = curBlock + 20 - const windowSize = 255 - const reservedWindowSize = 16 - const temporalUnit = 1 - const callValue = 123456789 - const callGas = 1000000 - - // Validate the data with the RequestLib - const isValid = await requestLib.validate( - [ - accounts[0], - accounts[0], - accounts[1], - NULL_ADDR, // TO ADDRESS - ], - [ - fee, - bounty, - claimWindowSize, - freezePeriod, - reservedWindowSize, - temporalUnit, - windowSize, - windowStart, - callGas, - callValue, - ], - "this-is-call-data", - config.web3.utils.toWei("10") // endowment - ) + const to = NULL_ADDR // cannot be over gas limit + const { isValid } = await validate({ to }) expect(isValid[5]).to.be.false - isValid.slice(0, 5).forEach(bool => expect(bool).to.be.true) }) }) From 3dadbb894ca2dc404fdcb75e25df61874e4a738d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Kosi=C5=84ski?= Date: Mon, 23 Apr 2018 15:58:55 +0200 Subject: [PATCH 2/2] Fix + tests for reinitialization --- contracts/TransactionRequestCore.sol | 4 ++ test/request-factory-tests/requestFactory.js | 42 +++++++++++++++++++- 2 files changed, 44 insertions(+), 2 deletions(-) diff --git a/contracts/TransactionRequestCore.sol b/contracts/TransactionRequestCore.sol index 691968c25..991d2eea4 100644 --- a/contracts/TransactionRequestCore.sol +++ b/contracts/TransactionRequestCore.sol @@ -9,6 +9,7 @@ contract TransactionRequestCore is TransactionRequestInterface { using RequestScheduleLib for RequestScheduleLib.ExecutionWindow; RequestLib.Request txnRequest; + bool private initialized = false; /* * addressArgs[0] - meta.createdBy @@ -36,7 +37,10 @@ contract TransactionRequestCore is TransactionRequestInterface { ) public payable { + require(!initialized); + txnRequest.initialize(addressArgs, uintArgs, callData); + initialized = true; } /* diff --git a/test/request-factory-tests/requestFactory.js b/test/request-factory-tests/requestFactory.js index 8b426002a..bf80151bf 100644 --- a/test/request-factory-tests/requestFactory.js +++ b/test/request-factory-tests/requestFactory.js @@ -67,7 +67,7 @@ contract("Request factory", async (accounts) => { endowment = transactionRequest.endowment, properties, to = accounts[2] - }) => { + } = {}) => { const paramsForValidation = await createValidationParams(properties) const isValid = await requestLib.validate( [accounts[0], accounts[0], accounts[1], to], @@ -178,7 +178,6 @@ contract("Request factory", async (accounts) => { // Lastly, we just make sure that the transaction request // address is a known request for the factory. expect(await requestFactory.isKnownRequest(NULL_ADDR)).to.be.false // sanity check - expect(await requestFactory.isKnownRequest(txRequest.address)).to.be.true }) @@ -236,4 +235,43 @@ contract("Request factory", async (accounts) => { expect(isValid[5]).to.be.false isValid.slice(0, 5).forEach(bool => expect(bool).to.be.true) }) + + it("should not allow to reinitialize the scheduled transaction", async () => { + const { paramsForValidation } = await validate() + + const params = paramsForValidation + params.push(transactionRequest.gasPrice) + params.push(transactionRequest.requiredDeposit) + + // Create a request with the same args we validated + const createTx = await requestFactory.createRequest( + [ + accounts[0], + accounts[1], // fee recipient + accounts[2], // to + ], + params, + transactionRequest.testCallData + ) + + const logRequestCreated = createTx.logs.find(e => e.event === "RequestCreated") + const txRequest = await TransactionRequestCore.at(logRequestCreated.args.request) + + let requestData = await parseRequestData(txRequest) + expect(requestData.txData.toAddress).to.equal(accounts[2]) + + await txRequest.initialize( + [ + accounts[0], + accounts[0], + accounts[1], // fee recipient + accounts[3], // hijacking recipient + ], + params, + transactionRequest.testCallData + ).should.be.rejectedWith("VM Exception while processing transaction: revert") + + requestData = await parseRequestData(txRequest) + expect(requestData.txData.toAddress).to.equal(accounts[2]) + }) })