From e8106f04b55846dcd10c5e87b2a9ff750a5e37bc Mon Sep 17 00:00:00 2001 From: Lawrence Forman Date: Mon, 6 Jul 2020 12:29:23 -0400 Subject: [PATCH] `@0x/contracts-zero-ex`: Address review feedback. --- .../src/features/IMetaTransactions.sol | 16 ++--- .../src/features/MetaTransactions.sol | 70 +++++++++++++------ .../src/features/SimpleFunctionRegistry.sol | 2 +- .../src/migrations/FullMigration.sol | 26 +++---- .../src/migrations/InitialMigration.sol | 24 +++---- contracts/zero-ex/package.json | 4 +- contracts/zero-ex/src/migration.ts | 4 +- contracts/zero-ex/test/artifacts.ts | 4 +- .../test/features/meta_transactions_test.ts | 40 +++++++++-- .../test/features/signature_validator_test.ts | 5 +- contracts/zero-ex/test/full_migration_test.ts | 10 +-- .../zero-ex/test/initial_migration_test.ts | 6 +- contracts/zero-ex/test/wrappers.ts | 2 +- contracts/zero-ex/tsconfig.json | 2 +- 14 files changed, 135 insertions(+), 80 deletions(-) diff --git a/contracts/zero-ex/contracts/src/features/IMetaTransactions.sol b/contracts/zero-ex/contracts/src/features/IMetaTransactions.sol index 1052d048cc..16be3986b8 100644 --- a/contracts/zero-ex/contracts/src/features/IMetaTransactions.sol +++ b/contracts/zero-ex/contracts/src/features/IMetaTransactions.sol @@ -36,7 +36,7 @@ interface IMetaTransactions { // Maximum gas price. uint256 maxGasPrice; // MTX is invalid after this time. - uint256 expirationTime; + uint256 expirationTimeSeconds; // Nonce to make this MTX unique. uint256 salt; // Encoded call data to a function on the exchange proxy. @@ -65,33 +65,33 @@ interface IMetaTransactions { /// @dev Execute a single meta-transaction. /// @param mtx The meta-transaction. /// @param signature The signature by `mtx.signer`. - /// @return returnData The ABI-encoded result of the underlying call. + /// @return returnResult The ABI-encoded result of the underlying call. function executeMetaTransaction( MetaTransactionData calldata mtx, bytes calldata signature ) external payable - returns (bytes memory returnData); + returns (bytes memory returnResult); /// @dev Execute multiple meta-transactions. /// @param mtxs The meta-transactions. /// @param signatures The signature by each respective `mtx.signer`. - /// @return returnDatas The ABI-encoded results of the underlying calls. - function executeMetaTransactions( + /// @return returnResults The ABI-encoded results of the underlying calls. + function batchExecuteMetaTransactions( MetaTransactionData[] calldata mtxs, bytes[] calldata signatures ) external payable - returns (bytes[] memory returnDatas); + returns (bytes[] memory returnResults); /// @dev Execute a meta-transaction via `sender`. Privileged variant. /// Only callable from within. /// @param sender Who is executing the meta-transaction.. /// @param mtx The meta-transaction. /// @param signature The signature by `mtx.signer`. - /// @return returnData The ABI-encoded result of the underlying call. + /// @return returnResult The ABI-encoded result of the underlying call. function _executeMetaTransaction( address sender, MetaTransactionData calldata mtx, @@ -99,7 +99,7 @@ interface IMetaTransactions { ) external payable - returns (bytes memory returnData); + returns (bytes memory returnResult); /// @dev Get the block at which a meta-transaction has been executed. /// @param mtx The meta-transaction. diff --git a/contracts/zero-ex/contracts/src/features/MetaTransactions.sol b/contracts/zero-ex/contracts/src/features/MetaTransactions.sol index 856b1831a8..55b73ed92d 100644 --- a/contracts/zero-ex/contracts/src/features/MetaTransactions.sol +++ b/contracts/zero-ex/contracts/src/features/MetaTransactions.sol @@ -73,7 +73,7 @@ contract MetaTransactions is "address sender," "uint256 minGasPrice," "uint256 maxGasPrice," - "uint256 expirationTime," + "uint256 expirationTimeSeconds," "uint256 salt," "bytes callData," "uint256 value," @@ -98,7 +98,7 @@ contract MetaTransactions is returns (bytes4 success) { _registerFeatureFunction(this.executeMetaTransaction.selector); - _registerFeatureFunction(this.executeMetaTransactions.selector); + _registerFeatureFunction(this.batchExecuteMetaTransactions.selector); _registerFeatureFunction(this._executeMetaTransaction.selector); _registerFeatureFunction(this.getMetaTransactionExecutedBlock.selector); _registerFeatureFunction(this.getMetaTransactionHashExecutedBlock.selector); @@ -109,7 +109,7 @@ contract MetaTransactions is /// @dev Execute a single meta-transaction. /// @param mtx The meta-transaction. /// @param signature The signature by `mtx.signer`. - /// @return returnData The ABI-encoded result of the underlying call. + /// @return returnResult The ABI-encoded result of the underlying call. function executeMetaTransaction( MetaTransactionData memory mtx, bytes memory signature @@ -117,7 +117,7 @@ contract MetaTransactions is public payable override - returns (bytes memory returnData) + returns (bytes memory returnResult) { return _executeMetaTransactionPrivate( msg.sender, @@ -129,15 +129,15 @@ contract MetaTransactions is /// @dev Execute multiple meta-transactions. /// @param mtxs The meta-transactions. /// @param signatures The signature by each respective `mtx.signer`. - /// @return returnDatas The ABI-encoded results of the underlying calls. - function executeMetaTransactions( + /// @return returnResults The ABI-encoded results of the underlying calls. + function batchExecuteMetaTransactions( MetaTransactionData[] memory mtxs, bytes[] memory signatures ) public payable override - returns (bytes[] memory returnDatas) + returns (bytes[] memory returnResults) { if (mtxs.length != signatures.length) { LibMetaTransactionsRichErrors.InvalidMetaTransactionsArrayLengthsError( @@ -145,9 +145,9 @@ contract MetaTransactions is signatures.length ).rrevert(); } - returnDatas = new bytes[](mtxs.length); + returnResults = new bytes[](mtxs.length); for (uint256 i = 0; i < mtxs.length; ++i) { - returnDatas[i] = _executeMetaTransactionPrivate( + returnResults[i] = _executeMetaTransactionPrivate( msg.sender, mtxs[i], signatures[i] @@ -160,7 +160,7 @@ contract MetaTransactions is /// @param sender Who is executing the meta-transaction.. /// @param mtx The meta-transaction. /// @param signature The signature by `mtx.signer`. - /// @return returnData The ABI-encoded result of the underlying call. + /// @return returnResult The ABI-encoded result of the underlying call. function _executeMetaTransaction( address sender, MetaTransactionData memory mtx, @@ -170,7 +170,7 @@ contract MetaTransactions is payable override onlySelf - returns (bytes memory returnData) + returns (bytes memory returnResult) { return _executeMetaTransactionPrivate(sender, mtx, signature); } @@ -214,7 +214,7 @@ contract MetaTransactions is mtx.sender, mtx.minGasPrice, mtx.maxGasPrice, - mtx.expirationTime, + mtx.expirationTimeSeconds, mtx.salt, keccak256(mtx.callData), mtx.value, @@ -227,14 +227,14 @@ contract MetaTransactions is /// @param sender Who is executing the meta-transaction.. /// @param mtx The meta-transaction. /// @param signature The signature by `mtx.signer`. - /// @return returnData The ABI-encoded result of the underlying call. + /// @return returnResult The ABI-encoded result of the underlying call. function _executeMetaTransactionPrivate( address sender, MetaTransactionData memory mtx, bytes memory signature ) private - returns (bytes memory returnData) + returns (bytes memory returnResult) { ExecuteState memory state; state.sender = sender; @@ -252,7 +252,7 @@ contract MetaTransactions is // Execute the call based on the selector. state.selector = mtx.callData.readBytes4(0); if (state.selector == ITransformERC20.transformERC20.selector) { - returnData = _executeTransformERC20Call(state); + returnResult = _executeTransformERC20Call(state); } else { LibMetaTransactionsRichErrors .MetaTransactionUnsupportedFunctionError(state.hash, state.selector) @@ -290,12 +290,12 @@ contract MetaTransactions is ).rrevert(); } // Must not be expired. - if (state.mtx.expirationTime <= block.timestamp) { + if (state.mtx.expirationTimeSeconds <= block.timestamp) { LibMetaTransactionsRichErrors .MetaTransactionExpiredError( state.hash, block.timestamp, - state.mtx.expirationTime + state.mtx.expirationTimeSeconds ).rrevert(); } // Must have a valid gas price. @@ -349,12 +349,36 @@ contract MetaTransactions is /// the taker address. function _executeTransformERC20Call(ExecuteState memory state) private - returns (bytes memory returnData) + returns (bytes memory returnResult) { // HACK(dorothy-zbornak): `abi.decode()` with the individual args // will cause a stack overflow. But we can prefix the call data with an - // offset to transform it into the encoding for the equivalent single struct arg. - // Decoding a single struct consumes far less stack space. + // offset to transform it into the encoding for the equivalent single struct arg, + // since decoding a single struct arg consumes far less stack space than + // decoding multiple struct args. + + // Where the encoding for multiple args (with the seleector ommitted) + // would typically look like: + // | argument | offset | + // |--------------------------|---------| + // | inputToken | 0 | + // | outputToken | 32 | + // | inputTokenAmount | 64 | + // | minOutputTokenAmount | 96 | + // | transformations (offset) | 128 | = 32 + // | transformations (data) | 160 | + + // We will ABI-decode a single struct arg copy with the layout: + // | argument | offset | + // |--------------------------|---------| + // | (arg 1 offset) | 0 | = 32 + // | inputToken | 32 | + // | outputToken | 64 | + // | inputTokenAmount | 96 | + // | minOutputTokenAmount | 128 | + // | transformations (offset) | 160 | = 32 + // | transformations (data) | 192 | + TransformERC20Args memory args; { bytes memory encodedStructArgs = new bytes(state.mtx.callData.length - 4 + 32); @@ -397,15 +421,15 @@ contract MetaTransactions is /// Warning: Do not let unadulerated `callData` into this function. function _callSelf(bytes32 hash, bytes memory callData, uint256 value) private - returns (bytes memory returnData) + returns (bytes memory returnResult) { bool success; - (success, returnData) = address(this).call{value: value}(callData); + (success, returnResult) = address(this).call{value: value}(callData); if (!success) { LibMetaTransactionsRichErrors.MetaTransactionCallFailedError( hash, callData, - returnData + returnResult ).rrevert(); } } diff --git a/contracts/zero-ex/contracts/src/features/SimpleFunctionRegistry.sol b/contracts/zero-ex/contracts/src/features/SimpleFunctionRegistry.sol index 2979a945d0..f9da65102f 100644 --- a/contracts/zero-ex/contracts/src/features/SimpleFunctionRegistry.sol +++ b/contracts/zero-ex/contracts/src/features/SimpleFunctionRegistry.sol @@ -35,7 +35,7 @@ contract SimpleFunctionRegistry is ISimpleFunctionRegistry, FixinCommon { - // solhint-disable + /// @dev Name of this feature. string public constant override FEATURE_NAME = "SimpleFunctionRegistry"; /// @dev Version of this feature. uint256 public immutable override FEATURE_VERSION = _encodeVersion(1, 0, 0); diff --git a/contracts/zero-ex/contracts/src/migrations/FullMigration.sol b/contracts/zero-ex/contracts/src/migrations/FullMigration.sol index 854a888f66..5cefc83643 100644 --- a/contracts/zero-ex/contracts/src/migrations/FullMigration.sol +++ b/contracts/zero-ex/contracts/src/migrations/FullMigration.sol @@ -49,20 +49,20 @@ contract FullMigration { address transformerDeployer; } - /// @dev The allowed caller of `deploy()`. - address public immutable deployer; + /// @dev The allowed caller of `initializeZeroEx()`. + address public immutable initializeCaller; /// @dev The initial migration contract. InitialMigration private _initialMigration; - /// @dev Instantiate this contract and set the allowed caller of `deploy()` - /// to `deployer`. - /// @param deployer_ The allowed caller of `deploy()`. - constructor(address payable deployer_) + /// @dev Instantiate this contract and set the allowed caller of `initializeZeroEx()` + /// to `initializeCaller`. + /// @param initializeCaller_ The allowed caller of `initializeZeroEx()`. + constructor(address payable initializeCaller_) public { - deployer = deployer_; + initializeCaller = initializeCaller_; // Create an initial migration contract with this contract set to the - // allowed deployer. + // allowed `initializeCaller`. _initialMigration = new InitialMigration(address(this)); } @@ -76,7 +76,7 @@ contract FullMigration { return address(_initialMigration); } - /// @dev Deploy the `ZeroEx` contract with the full feature set, + /// @dev Initialize the `ZeroEx` contract with the full feature set, /// transfer ownership to `owner`, then self-destruct. /// @param owner The owner of the contract. /// @param zeroEx The instance of the ZeroEx contract. ZeroEx should @@ -84,7 +84,7 @@ contract FullMigration { /// @param features Features to add to the proxy. /// @return _zeroEx The configured ZeroEx contract. Same as the `zeroEx` parameter. /// @param migrateOpts Parameters needed to initialize features. - function deploy( + function initializeZeroEx( address payable owner, ZeroEx zeroEx, Features memory features, @@ -93,10 +93,10 @@ contract FullMigration { public returns (ZeroEx _zeroEx) { - require(msg.sender == deployer, "FullMigration/INVALID_SENDER"); + require(msg.sender == initializeCaller, "FullMigration/INVALID_SENDER"); // Perform the initial migration with the owner set to this contract. - _initialMigration.deploy( + _initialMigration.initializeZeroEx( address(uint160(address(this))), zeroEx, InitialMigration.BootstrapFeatures({ @@ -117,7 +117,7 @@ contract FullMigration { return zeroEx; } - /// @dev Destroy this contract. Only callable from ourselves (from `deploy()`). + /// @dev Destroy this contract. Only callable from ourselves (from `initializeZeroEx()`). /// @param ethRecipient Receiver of any ETH in this contract. function die(address payable ethRecipient) external diff --git a/contracts/zero-ex/contracts/src/migrations/InitialMigration.sol b/contracts/zero-ex/contracts/src/migrations/InitialMigration.sol index 404ccfe765..8868a8a08c 100644 --- a/contracts/zero-ex/contracts/src/migrations/InitialMigration.sol +++ b/contracts/zero-ex/contracts/src/migrations/InitialMigration.sol @@ -35,29 +35,29 @@ contract InitialMigration { Ownable ownable; } - /// @dev The allowed caller of `deploy()`. In production, this would be + /// @dev The allowed caller of `initializeZeroEx()`. In production, this would be /// the governor. - address public immutable deployer; + address public immutable initializeCaller; /// @dev The real address of this contract. address private immutable _implementation; - /// @dev Instantiate this contract and set the allowed caller of `deploy()` - /// to `deployer_`. - /// @param deployer_ The allowed caller of `deploy()`. - constructor(address deployer_) public { - deployer = deployer_; + /// @dev Instantiate this contract and set the allowed caller of `initializeZeroEx()` + /// to `initializeCaller_`. + /// @param initializeCaller_ The allowed caller of `initializeZeroEx()`. + constructor(address initializeCaller_) public { + initializeCaller = initializeCaller_; _implementation = address(this); } - /// @dev Deploy the `ZeroEx` contract with the minimum feature set, + /// @dev Initialize the `ZeroEx` contract with the minimum feature set, /// transfers ownership to `owner`, then self-destructs. - /// Only callable by `deployer` set in the contstructor. + /// Only callable by `initializeCaller` set in the contstructor. /// @param owner The owner of the contract. /// @param zeroEx The instance of the ZeroEx contract. ZeroEx should /// been constructed with this contract as the bootstrapper. /// @param features Features to bootstrap into the proxy. /// @return _zeroEx The configured ZeroEx contract. Same as the `zeroEx` parameter. - function deploy( + function initializeZeroEx( address payable owner, ZeroEx zeroEx, BootstrapFeatures memory features @@ -66,8 +66,8 @@ contract InitialMigration { virtual returns (ZeroEx _zeroEx) { - // Must be called by the allowed deployer. - require(msg.sender == deployer, "InitialMigration/INVALID_SENDER"); + // Must be called by the allowed initializeCaller. + require(msg.sender == initializeCaller, "InitialMigration/INVALID_SENDER"); // Bootstrap the initial feature set. IBootstrap(address(zeroEx)).bootstrap( diff --git a/contracts/zero-ex/package.json b/contracts/zero-ex/package.json index f9a83055c9..19a77e7620 100644 --- a/contracts/zero-ex/package.json +++ b/contracts/zero-ex/package.json @@ -39,9 +39,9 @@ "publish:private": "yarn build && gitpkg publish" }, "config": { - "publicInterfaceContracts": "ZeroEx,FullMigration,InitialMigration,IFlashWallet,IAllowanceTarget,IERC20Transformer,IOwnable,ISimpleFunctionRegistry,ITokenSpender,ITransformERC20,FillQuoteTransformer,PayTakerTransformer,WethTransformer,Ownable,SimpleFunctionRegistry,TransformERC20,TokenSpender,AffiliateFeeTransformerSignatureValidator,MetaTransactions", + "publicInterfaceContracts": "ZeroEx,FullMigration,InitialMigration,IFlashWallet,IAllowanceTarget,IERC20Transformer,IOwnable,ISimpleFunctionRegistry,ITokenSpender,ITransformERC20,FillQuoteTransformer,PayTakerTransformer,WethTransformer,Ownable,SimpleFunctionRegistry,TransformERC20,TokenSpender,AffiliateFeeTransformer,SignatureValidator,MetaTransactions", "abis:comment": "This list is auto-generated by contracts-gen. Don't edit manually.", - "abis": "./test/generated-artifacts/@(AffiliateFeeTransformer|AllowanceTarget|Bootstrap|FillQuoteTransformer|FixinCommon|FixinEIP712|FlashWallet|FullMigration|IAllowanceTarget|IBootstrap|IERC20Transformer|IExchange|IFeature|IFlashWallet|IMetaTransactions|IOwnable|ISignatureValidator|ISimpleFunctionRegistry|ITestSimpleFunctionRegistryFeature|ITokenSpender|ITransformERC20|InitialMigration|LibBootstrap|LibCommonRichErrors|LibERC20Transformer|LibMetaTransactionsRichErrors|LibMetaTransactionsStorage|LibMigrate|LibOwnableRichErrors|LibOwnableStorage|LibProxyRichErrors|LibProxyStorage|LibSignatureRichErrors|LibSimpleFunctionRegistryRichErrors|LibSimpleFunctionRegistryStorage|LibSpenderRichErrors|LibStorage|LibTokenSpenderStorage|LibTransformERC20RichErrors|LibTransformERC20Storage|LibWalletRichErrors|MetaTransactions|Ownable|PayTakerTransformer|SignatureValidator|SimpleFunctionRegistry|TestCallTarget|TestDelegateCaller|TestFillQuoteTransformerExchange|TestFillQuoteTransformerHost|TestFullMigration|TestInitialMigration|TestMetaTransactionsTransformERC20Feature|TestMigrator|TestMintTokenERC20Transformer|TestMintableERC20Token|TestSimpleFunctionRegistryFeatureImpl1|TestSimpleFunctionRegistryFeatureImpl2|TestTokenSpender|TestTokenSpenderERC20Token|TestTransformERC20|TestTransformerBase|TestTransformerDeployerTransformer|TestTransformerHost|TestWeth|TestWethTransformerHost|TestZeroExFeature|TokenSpender|TransformERC20|Transformer|TransformerDeployer|WethTransformer|ZeroEx).json" + "abis": "./test/generated-artifacts/@(AffiliateFeeTransformer|AllowanceTarget|Bootstrap|FillQuoteTransformer|FixinCommon|FixinEIP712|FixinGasToken|FlashWallet|FullMigration|IAllowanceTarget|IBootstrap|IERC20Bridge|IERC20Transformer|IExchange|IFeature|IFlashWallet|IGasToken|IMetaTransactions|IOwnable|ISignatureValidator|ISimpleFunctionRegistry|ITestSimpleFunctionRegistryFeature|ITokenSpender|ITransformERC20|InitialMigration|LibBootstrap|LibCommonRichErrors|LibERC20Transformer|LibMetaTransactionsRichErrors|LibMetaTransactionsStorage|LibMigrate|LibOwnableRichErrors|LibOwnableStorage|LibProxyRichErrors|LibProxyStorage|LibSignatureRichErrors|LibSimpleFunctionRegistryRichErrors|LibSimpleFunctionRegistryStorage|LibSpenderRichErrors|LibStorage|LibTokenSpenderStorage|LibTransformERC20RichErrors|LibTransformERC20Storage|LibWalletRichErrors|MetaTransactions|Ownable|PayTakerTransformer|SignatureValidator|SimpleFunctionRegistry|TestCallTarget|TestDelegateCaller|TestFillQuoteTransformerBridge|TestFillQuoteTransformerExchange|TestFillQuoteTransformerHost|TestFullMigration|TestInitialMigration|TestMetaTransactionsTransformERC20Feature|TestMigrator|TestMintTokenERC20Transformer|TestMintableERC20Token|TestSimpleFunctionRegistryFeatureImpl1|TestSimpleFunctionRegistryFeatureImpl2|TestTokenSpender|TestTokenSpenderERC20Token|TestTransformERC20|TestTransformerBase|TestTransformerDeployerTransformer|TestTransformerHost|TestWeth|TestWethTransformerHost|TestZeroExFeature|TokenSpender|TransformERC20|Transformer|TransformerDeployer|WethTransformer|ZeroEx).json" }, "repository": { "type": "git", diff --git a/contracts/zero-ex/src/migration.ts b/contracts/zero-ex/src/migration.ts index 3ec41160fd..8d211800e0 100644 --- a/contracts/zero-ex/src/migration.ts +++ b/contracts/zero-ex/src/migration.ts @@ -73,7 +73,7 @@ export async function initialMigrateAsync( migrator.address, ); const _features = await deployBootstrapFeaturesAsync(provider, txDefaults, features); - await migrator.deploy(owner, zeroEx.address, _features).awaitTransactionSuccessAsync(); + await migrator.initializeZeroEx(owner, zeroEx.address, _features).awaitTransactionSuccessAsync(); return zeroEx; } @@ -170,6 +170,6 @@ export async function fullMigrateAsync( transformerDeployer: txDefaults.from as string, ...opts, }; - await migrator.deploy(owner, zeroEx.address, _features, _opts).awaitTransactionSuccessAsync(); + await migrator.initializeZeroEx(owner, zeroEx.address, _features, _opts).awaitTransactionSuccessAsync(); return zeroEx; } diff --git a/contracts/zero-ex/test/artifacts.ts b/contracts/zero-ex/test/artifacts.ts index 84d616b778..6750466ef4 100644 --- a/contracts/zero-ex/test/artifacts.ts +++ b/contracts/zero-ex/test/artifacts.ts @@ -10,8 +10,8 @@ import * as AllowanceTarget from '../test/generated-artifacts/AllowanceTarget.js import * as Bootstrap from '../test/generated-artifacts/Bootstrap.json'; import * as FillQuoteTransformer from '../test/generated-artifacts/FillQuoteTransformer.json'; import * as FixinCommon from '../test/generated-artifacts/FixinCommon.json'; -import * as FixinGasToken from '../test/generated-artifacts/FixinGasToken.json'; import * as FixinEIP712 from '../test/generated-artifacts/FixinEIP712.json'; +import * as FixinGasToken from '../test/generated-artifacts/FixinGasToken.json'; import * as FlashWallet from '../test/generated-artifacts/FlashWallet.json'; import * as FullMigration from '../test/generated-artifacts/FullMigration.json'; import * as IAllowanceTarget from '../test/generated-artifacts/IAllowanceTarget.json'; @@ -114,8 +114,8 @@ export const artifacts = { TokenSpender: TokenSpender as ContractArtifact, TransformERC20: TransformERC20 as ContractArtifact, FixinCommon: FixinCommon as ContractArtifact, - FixinGasToken: FixinGasToken as ContractArtifact, FixinEIP712: FixinEIP712 as ContractArtifact, + FixinGasToken: FixinGasToken as ContractArtifact, FullMigration: FullMigration as ContractArtifact, InitialMigration: InitialMigration as ContractArtifact, LibBootstrap: LibBootstrap as ContractArtifact, diff --git a/contracts/zero-ex/test/features/meta_transactions_test.ts b/contracts/zero-ex/test/features/meta_transactions_test.ts index 65bfb88ff6..3205822ea7 100644 --- a/contracts/zero-ex/test/features/meta_transactions_test.ts +++ b/contracts/zero-ex/test/features/meta_transactions_test.ts @@ -74,7 +74,7 @@ blockchainTests.resets('MetaTransactions feature', env => { sender, minGasPrice: getRandomInteger('2', '1e9'), maxGasPrice: getRandomInteger('1e9', '100e9'), - expirationTime: new BigNumber(Math.floor(_.now() / 1000) + 360), + expirationTimeSeconds: new BigNumber(Math.floor(_.now() / 1000) + 360), salt: new BigNumber(hexUtils.random()), callData: hexUtils.random(4), value: getRandomInteger(1, '1e18'), @@ -362,7 +362,7 @@ blockchainTests.resets('MetaTransactions feature', env => { it('fails if expired', async () => { const mtx = getRandomMetaTransaction({ - expirationTime: new BigNumber(Math.floor(_.now() / 1000 - 60)), + expirationTimeSeconds: new BigNumber(Math.floor(_.now() / 1000 - 60)), }); const mtxHash = getExchangeProxyMetaTransactionHash(mtx); const signature = await signMetaTransactionAsync(mtx); @@ -375,7 +375,7 @@ blockchainTests.resets('MetaTransactions feature', env => { new ZeroExRevertErrors.MetaTransactions.MetaTransactionExpiredError( mtxHash, undefined, - mtx.expirationTime, + mtx.expirationTimeSeconds, ), ); }); @@ -425,7 +425,7 @@ blockchainTests.resets('MetaTransactions feature', env => { }); }); - describe('executeMetaTransactions()', () => { + describe('batchExecuteMetaTransactions()', () => { it('can execute multiple transactions', async () => { const mtxs = _.times(2, i => { const args = getRandomTransformERC20Args(); @@ -447,9 +447,39 @@ blockchainTests.resets('MetaTransactions feature', env => { gasPrice: BigNumber.max(...mtxs.map(mtx => mtx.minGasPrice)), value: BigNumber.sum(...mtxs.map(mtx => mtx.value)), }; - const rawResults = await feature.executeMetaTransactions(mtxs, signatures).callAsync(callOpts); + const rawResults = await feature.batchExecuteMetaTransactions(mtxs, signatures).callAsync(callOpts); expect(rawResults).to.eql(mtxs.map(() => RAW_SUCCESS_RESULT)); }); + + it('cannot execute the same transaction twice', async () => { + const mtx = (() => { + const args = getRandomTransformERC20Args(); + return getRandomMetaTransaction({ + signer: _.sampleSize(signers, 1)[0], + callData: transformERC20Feature + .transformERC20( + args.inputToken, + args.outputToken, + args.inputTokenAmount, + args.minOutputTokenAmount, + args.transformations, + ) + .getABIEncodedTransactionData(), + }); + })(); + const mtxHash = getExchangeProxyMetaTransactionHash(mtx); + const mtxs = _.times(2, () => mtx); + const signatures = await Promise.all(mtxs.map(async mtx => signMetaTransactionAsync(mtx))); + const callOpts = { + gasPrice: BigNumber.max(...mtxs.map(mtx => mtx.minGasPrice)), + value: BigNumber.sum(...mtxs.map(mtx => mtx.value)), + }; + const block = await env.web3Wrapper.getBlockNumberAsync(); + const tx = feature.batchExecuteMetaTransactions(mtxs, signatures).callAsync(callOpts); + return expect(tx).to.revertWith( + new ZeroExRevertErrors.MetaTransactions.MetaTransactionAlreadyExecutedError(mtxHash, block), + ); + }); }); describe('getMetaTransactionExecutedBlock()', () => { diff --git a/contracts/zero-ex/test/features/signature_validator_test.ts b/contracts/zero-ex/test/features/signature_validator_test.ts index 5a2387bf43..99fdd79e9b 100644 --- a/contracts/zero-ex/test/features/signature_validator_test.ts +++ b/contracts/zero-ex/test/features/signature_validator_test.ts @@ -66,13 +66,12 @@ blockchainTests.resets('SignatureValidator feature', env => { const hash = hexUtils.random(); const signer = _.sampleSize(signers, 1)[0]; const signature = hexUtils.slice(await signatureUtils.ecSignHashAsync(env.provider, hash, signer), 1); - const notSigner = randomAddress(); - const tx = feature.validateHashSignature(hash, notSigner, signature).callAsync(); + const tx = feature.validateHashSignature(hash, signer, signature).callAsync(); return expect(tx).to.revertWith( new ZeroExRevertErrors.SignatureValidator.SignatureValidationError( ZeroExRevertErrors.SignatureValidator.SignatureValidationErrorCodes.InvalidLength, hash, - notSigner, + signer, signature, ), ); diff --git a/contracts/zero-ex/test/full_migration_test.ts b/contracts/zero-ex/test/full_migration_test.ts index ffb4dc8231..107e4f6b28 100644 --- a/contracts/zero-ex/test/full_migration_test.ts +++ b/contracts/zero-ex/test/full_migration_test.ts @@ -44,7 +44,9 @@ blockchainTests.resets('Full migration', env => { await migrator.getBootstrapper().callAsync(), ); features = await deployFullFeaturesAsync(env.provider, env.txDefaults, zeroEx.address); - await migrator.deploy(owner, zeroEx.address, features, { transformerDeployer }).awaitTransactionSuccessAsync(); + await migrator + .initializeZeroEx(owner, zeroEx.address, features, { transformerDeployer }) + .awaitTransactionSuccessAsync(); }); it('ZeroEx has the correct owner', async () => { @@ -58,10 +60,10 @@ blockchainTests.resets('Full migration', env => { expect(dieRecipient).to.eq(owner); }); - it('Non-deployer cannot call deploy()', async () => { + it('Non-deployer cannot call initializeZeroEx()', async () => { const notDeployer = randomAddress(); const tx = migrator - .deploy(owner, zeroEx.address, features, { transformerDeployer }) + .initializeZeroEx(owner, zeroEx.address, features, { transformerDeployer }) .callAsync({ from: notDeployer }); return expect(tx).to.revertWith('FullMigration/INVALID_SENDER'); }); @@ -89,7 +91,7 @@ blockchainTests.resets('Full migration', env => { contractType: IMetaTransactionsContract, fns: [ 'executeMetaTransaction', - 'executeMetaTransactions', + 'batchExecuteMetaTransactions', '_executeMetaTransaction', 'getMetaTransactionExecutedBlock', 'getMetaTransactionHashExecutedBlock', diff --git a/contracts/zero-ex/test/initial_migration_test.ts b/contracts/zero-ex/test/initial_migration_test.ts index 442a9bfbd9..f9985ea4c2 100644 --- a/contracts/zero-ex/test/initial_migration_test.ts +++ b/contracts/zero-ex/test/initial_migration_test.ts @@ -42,7 +42,7 @@ blockchainTests.resets('Initial migration', env => { artifacts, migrator.address, ); - await migrator.deploy(owner, zeroEx.address, features).awaitTransactionSuccessAsync(); + await migrator.initializeZeroEx(owner, zeroEx.address, features).awaitTransactionSuccessAsync(); }); it('Self-destructs after deployment', async () => { @@ -50,9 +50,9 @@ blockchainTests.resets('Initial migration', env => { expect(dieRecipient).to.eq(owner); }); - it('Non-deployer cannot call deploy()', async () => { + it('Non-deployer cannot call initializeZeroEx()', async () => { const notDeployer = randomAddress(); - const tx = migrator.deploy(owner, zeroEx.address, features).callAsync({ from: notDeployer }); + const tx = migrator.initializeZeroEx(owner, zeroEx.address, features).callAsync({ from: notDeployer }); return expect(tx).to.revertWith('InitialMigration/INVALID_SENDER'); }); diff --git a/contracts/zero-ex/test/wrappers.ts b/contracts/zero-ex/test/wrappers.ts index 96d6706e23..b5827399e8 100644 --- a/contracts/zero-ex/test/wrappers.ts +++ b/contracts/zero-ex/test/wrappers.ts @@ -8,8 +8,8 @@ export * from '../test/generated-wrappers/allowance_target'; export * from '../test/generated-wrappers/bootstrap'; export * from '../test/generated-wrappers/fill_quote_transformer'; export * from '../test/generated-wrappers/fixin_common'; -export * from '../test/generated-wrappers/fixin_gas_token'; export * from '../test/generated-wrappers/fixin_e_i_p712'; +export * from '../test/generated-wrappers/fixin_gas_token'; export * from '../test/generated-wrappers/flash_wallet'; export * from '../test/generated-wrappers/full_migration'; export * from '../test/generated-wrappers/i_allowance_target'; diff --git a/contracts/zero-ex/tsconfig.json b/contracts/zero-ex/tsconfig.json index 11585e644f..caa5d8fca0 100644 --- a/contracts/zero-ex/tsconfig.json +++ b/contracts/zero-ex/tsconfig.json @@ -28,8 +28,8 @@ "test/generated-artifacts/Bootstrap.json", "test/generated-artifacts/FillQuoteTransformer.json", "test/generated-artifacts/FixinCommon.json", - "test/generated-artifacts/FixinGasToken.json", "test/generated-artifacts/FixinEIP712.json", + "test/generated-artifacts/FixinGasToken.json", "test/generated-artifacts/FlashWallet.json", "test/generated-artifacts/FullMigration.json", "test/generated-artifacts/IAllowanceTarget.json",