From f2e16dfb21058564df4399f746c7be52f270d57e Mon Sep 17 00:00:00 2001 From: Lawrence Forman Date: Wed, 29 Apr 2020 12:30:34 -0400 Subject: [PATCH] `@0x/contracts-zero-ex`: Switch rich revert style. `@0x/contracts-zero-ex`: Merge `FixinOwnable` -> `FixinCommon`. --- .../contracts/src/features/Bootstrap.sol | 13 ++--- .../contracts/src/features/Ownable.sol | 11 ++-- .../src/features/SimpleFunctionRegistry.sol | 17 ++++--- .../contracts/src/fixins/FixinCommon.sol | 28 +++++++++-- .../contracts/src/fixins/FixinOwnable.sol | 50 ------------------- .../contracts/src/migrations/LibBootstrap.sol | 6 +-- .../contracts/src/migrations/LibMigrate.sol | 6 +-- contracts/zero-ex/package.json | 2 +- contracts/zero-ex/test/artifacts.ts | 2 - contracts/zero-ex/test/wrappers.ts | 1 - contracts/zero-ex/tsconfig.json | 1 - 11 files changed, 53 insertions(+), 84 deletions(-) delete mode 100644 contracts/zero-ex/contracts/src/fixins/FixinOwnable.sol diff --git a/contracts/zero-ex/contracts/src/features/Bootstrap.sol b/contracts/zero-ex/contracts/src/features/Bootstrap.sol index 50e7dc54df..3a65797818 100644 --- a/contracts/zero-ex/contracts/src/features/Bootstrap.sol +++ b/contracts/zero-ex/contracts/src/features/Bootstrap.sol @@ -19,16 +19,15 @@ pragma solidity ^0.6.5; pragma experimental ABIEncoderV2; +import "@0x/contracts-utils/contracts/src/v06/errors/LibRichErrorsV06.sol"; import "../migrations/LibBootstrap.sol"; -import "../fixins/FixinCommon.sol"; import "../storage/LibProxyStorage.sol"; import "./IBootstrap.sol"; /// @dev Detachable `bootstrap()` feature. contract Bootstrap is - IBootstrap, - FixinCommon + IBootstrap { // solhint-disable state-visibility,indent /// @dev The ZeroEx contract. @@ -42,6 +41,8 @@ contract Bootstrap is address immutable private _bootstrapCaller; // solhint-enable state-visibility,indent + using LibRichErrorsV06 for bytes; + /// @dev Construct this contract and set the bootstrap migration contract. /// After constructing this contract, `bootstrap()` should be called /// to seed the initial feature set. @@ -60,10 +61,10 @@ contract Bootstrap is function bootstrap(address target, bytes calldata callData) external override { // Only the bootstrap caller can call this function. if (msg.sender != _bootstrapCaller) { - _rrevert(LibProxyRichErrors.InvalidBootstrapCallerError( + LibProxyRichErrors.InvalidBootstrapCallerError( msg.sender, _bootstrapCaller - )); + ).rrevert(); } // Deregister. LibProxyStorage.getStorage().impls[this.bootstrap.selector] = address(0); @@ -77,7 +78,7 @@ contract Bootstrap is /// Can only be called by the deployer. function die() external { if (msg.sender != _deployer) { - _rrevert(LibProxyRichErrors.InvalidDieCallerError(msg.sender, _deployer)); + LibProxyRichErrors.InvalidDieCallerError(msg.sender, _deployer).rrevert(); } selfdestruct(msg.sender); } diff --git a/contracts/zero-ex/contracts/src/features/Ownable.sol b/contracts/zero-ex/contracts/src/features/Ownable.sol index e4d95c097d..73730f1f28 100644 --- a/contracts/zero-ex/contracts/src/features/Ownable.sol +++ b/contracts/zero-ex/contracts/src/features/Ownable.sol @@ -19,7 +19,8 @@ pragma solidity ^0.6.5; pragma experimental ABIEncoderV2; -import "../fixins/FixinOwnable.sol"; +import "@0x/contracts-utils/contracts/src/v06/errors/LibRichErrorsV06.sol"; +import "../fixins/FixinCommon.sol"; import "../errors/LibOwnableRichErrors.sol"; import "../storage/LibOwnableStorage.sol"; import "../migrations/LibBootstrap.sol"; @@ -33,7 +34,7 @@ import "./ISimpleFunctionRegistry.sol"; contract Ownable is IFeature, IOwnable, - FixinOwnable + FixinCommon { // solhint-disable const-name-snakecase @@ -48,6 +49,8 @@ contract Ownable is address immutable private _implementation; // solhint-enable + using LibRichErrorsV06 for bytes; + constructor() public { _implementation = address(this); } @@ -79,7 +82,7 @@ contract Ownable is LibOwnableStorage.Storage storage proxyStor = LibOwnableStorage.getStorage(); if (newOwner == address(0)) { - _rrevert(LibOwnableRichErrors.TransferOwnerToZeroError()); + LibOwnableRichErrors.TransferOwnerToZeroError().rrevert(); } else { proxyStor.owner = newOwner; emit OwnershipTransferred(msg.sender, newOwner); @@ -103,7 +106,7 @@ contract Ownable is address prevOwner = stor.owner; if (prevOwner == address(this)) { // If the owner is already set to ourselves then we've reentered. - _rrevert(LibOwnableRichErrors.AlreadyMigratingError()); + LibOwnableRichErrors.AlreadyMigratingError().rrevert(); } // Temporarily set the owner to ourselves so we can perform admin functions. stor.owner = address(this); diff --git a/contracts/zero-ex/contracts/src/features/SimpleFunctionRegistry.sol b/contracts/zero-ex/contracts/src/features/SimpleFunctionRegistry.sol index 8e0c1cade9..2acbccad8e 100644 --- a/contracts/zero-ex/contracts/src/features/SimpleFunctionRegistry.sol +++ b/contracts/zero-ex/contracts/src/features/SimpleFunctionRegistry.sol @@ -19,7 +19,8 @@ pragma solidity ^0.6.5; pragma experimental ABIEncoderV2; -import "../fixins/FixinOwnable.sol"; +import "@0x/contracts-utils/contracts/src/v06/errors/LibRichErrorsV06.sol"; +import "../fixins/FixinCommon.sol"; import "../storage/LibProxyStorage.sol"; import "../storage/LibSimpleFunctionRegistryStorage.sol"; import "../errors/LibSimpleFunctionRegistryRichErrors.sol"; @@ -32,7 +33,7 @@ import "./ISimpleFunctionRegistry.sol"; contract SimpleFunctionRegistry is IFeature, ISimpleFunctionRegistry, - FixinOwnable + FixinCommon { // solhint-disable const-name-snakecase @@ -47,6 +48,8 @@ contract SimpleFunctionRegistry is address immutable private _implementation; // solhint-enable + using LibRichErrorsV06 for bytes; + constructor() public { _implementation = address(this); } @@ -95,12 +98,10 @@ contract SimpleFunctionRegistry is } } if (i == 0) { - _rrevert( - LibSimpleFunctionRegistryRichErrors.NotInRollbackHistoryError( - selector, - targetImpl - ) - ); + LibSimpleFunctionRegistryRichErrors.NotInRollbackHistoryError( + selector, + targetImpl + ).rrevert(); } proxyStor.impls[selector] = targetImpl; emit ProxyFunctionUpdated(selector, currentImpl, targetImpl); diff --git a/contracts/zero-ex/contracts/src/fixins/FixinCommon.sol b/contracts/zero-ex/contracts/src/fixins/FixinCommon.sol index c8181515bd..7a2cd44389 100644 --- a/contracts/zero-ex/contracts/src/fixins/FixinCommon.sol +++ b/contracts/zero-ex/contracts/src/fixins/FixinCommon.sol @@ -21,22 +21,40 @@ pragma experimental ABIEncoderV2; import "@0x/contracts-utils/contracts/src/v06/errors/LibRichErrorsV06.sol"; import "../errors/LibCommonRichErrors.sol"; +import "../errors/LibOwnableRichErrors.sol"; +import "../storage/LibOwnableStorage.sol"; /// @dev Common utilities. contract FixinCommon { + using LibRichErrorsV06 for bytes; + /// @dev The caller must be this contract. modifier onlySelf() { if (msg.sender != address(this)) { - _rrevert(LibCommonRichErrors.OnlyCallableBySelfError(msg.sender)); + LibCommonRichErrors.OnlyCallableBySelfError(msg.sender).rrevert(); } _; } - /// @dev Reverts with arbitrary data `errorData`. - /// @param errorData ABI encoded error data. - function _rrevert(bytes memory errorData) internal pure { - LibRichErrorsV06.rrevert(errorData); + /// @dev The caller of this function must be the owner. + modifier onlyOwner() { + { + address owner = _getOwner(); + if (msg.sender != owner) { + LibOwnableRichErrors.OnlyOwnerError( + msg.sender, + owner + ).rrevert(); + } + } + _; + } + + /// @dev Get the owner of this contract. + /// @return owner The owner of this contract. + function _getOwner() internal view returns (address owner) { + return LibOwnableStorage.getStorage().owner; } } diff --git a/contracts/zero-ex/contracts/src/fixins/FixinOwnable.sol b/contracts/zero-ex/contracts/src/fixins/FixinOwnable.sol deleted file mode 100644 index e784d7883f..0000000000 --- a/contracts/zero-ex/contracts/src/fixins/FixinOwnable.sol +++ /dev/null @@ -1,50 +0,0 @@ -/* - - Copyright 2020 ZeroEx Intl. - - Licensed under the Apache License, Version 2.0 (the "License"); - you may not use this file except in compliance with the License. - You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - - Unless required by applicable law or agreed to in writing, software - distributed under the License is distributed on an "AS IS" BASIS, - WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - See the License for the specific language governing permissions and - limitations under the License. - -*/ - -pragma solidity ^0.6.5; -pragma experimental ABIEncoderV2; - -import "../errors/LibOwnableRichErrors.sol"; -import "../storage/LibOwnableStorage.sol"; -import "./FixinCommon.sol"; - - -/// @dev A feature mixin for restricting callers to owners. -contract FixinOwnable is - FixinCommon -{ - /// @dev The caller of this function must be the owner. - modifier onlyOwner() { - { - address owner = _getOwner(); - if (msg.sender != owner) { - _rrevert(LibOwnableRichErrors.OnlyOwnerError( - msg.sender, - owner - )); - } - } - _; - } - - /// @dev Get the owner of this contract. - /// @return owner The owner of this contract. - function _getOwner() internal view returns (address owner) { - return LibOwnableStorage.getStorage().owner; - } -} diff --git a/contracts/zero-ex/contracts/src/migrations/LibBootstrap.sol b/contracts/zero-ex/contracts/src/migrations/LibBootstrap.sol index b5304441fc..970b8a480d 100644 --- a/contracts/zero-ex/contracts/src/migrations/LibBootstrap.sol +++ b/contracts/zero-ex/contracts/src/migrations/LibBootstrap.sol @@ -29,6 +29,8 @@ library LibBootstrap { /// This is `keccack('BOOTSTRAP_SUCCESS')`. bytes4 internal constant BOOTSTRAP_SUCCESS = 0xd150751b; + using LibRichErrorsV06 for bytes; + /// @dev Perform a delegatecall and ensure it returns the magic bytes. /// @param target The call target. /// @param data The call data. @@ -43,9 +45,7 @@ library LibBootstrap { resultData.length != 32 || abi.decode(resultData, (bytes4)) != BOOTSTRAP_SUCCESS) { - LibRichErrorsV06.rrevert( - LibProxyRichErrors.BootstrapCallFailedError(target, resultData) - ); + LibProxyRichErrors.BootstrapCallFailedError(target, resultData).rrevert(); } } } diff --git a/contracts/zero-ex/contracts/src/migrations/LibMigrate.sol b/contracts/zero-ex/contracts/src/migrations/LibMigrate.sol index 6581e539a1..0f5157a6dd 100644 --- a/contracts/zero-ex/contracts/src/migrations/LibMigrate.sol +++ b/contracts/zero-ex/contracts/src/migrations/LibMigrate.sol @@ -29,6 +29,8 @@ library LibMigrate { /// This is `keccack('MIGRATE_SUCCESS')`. bytes4 internal constant MIGRATE_SUCCESS = 0x2c64c5ef; + using LibRichErrorsV06 for bytes; + /// @dev Perform a delegatecall and ensure it returns the magic bytes. /// @param target The call target. /// @param data The call data. @@ -43,9 +45,7 @@ library LibMigrate { resultData.length != 32 || abi.decode(resultData, (bytes4)) != MIGRATE_SUCCESS) { - LibRichErrorsV06.rrevert( - LibOwnableRichErrors.MigrateCallFailedError(target, resultData) - ); + LibOwnableRichErrors.MigrateCallFailedError(target, resultData).rrevert(); } } } diff --git a/contracts/zero-ex/package.json b/contracts/zero-ex/package.json index 1397061b7e..132c48a16f 100644 --- a/contracts/zero-ex/package.json +++ b/contracts/zero-ex/package.json @@ -40,7 +40,7 @@ "config": { "publicInterfaceContracts": "ZeroEx,IOwnable,ISimpleFunctionRegistry", "abis:comment": "This list is auto-generated by contracts-gen. Don't edit manually.", - "abis": "./test/generated-artifacts/@(Bootstrap|FixinCommon|FixinOwnable|IBootstrap|IFeature|IOwnable|ISimpleFunctionRegistry|ITestSimpleFunctionRegistryFeature|InitialMigration|LibBootstrap|LibCommonRichErrors|LibMigrate|LibOwnableRichErrors|LibOwnableStorage|LibProxyRichErrors|LibProxyStorage|LibSimpleFunctionRegistryRichErrors|LibSimpleFunctionRegistryStorage|LibStorage|Ownable|SimpleFunctionRegistry|TestInitialMigration|TestMigrator|TestSimpleFunctionRegistryFeatureImpl1|TestSimpleFunctionRegistryFeatureImpl2|TestZeroExFeature|ZeroEx).json" + "abis": "./test/generated-artifacts/@(Bootstrap|FixinCommon|IBootstrap|IFeature|IOwnable|ISimpleFunctionRegistry|ITestSimpleFunctionRegistryFeature|InitialMigration|LibBootstrap|LibCommonRichErrors|LibMigrate|LibOwnableRichErrors|LibOwnableStorage|LibProxyRichErrors|LibProxyStorage|LibSimpleFunctionRegistryRichErrors|LibSimpleFunctionRegistryStorage|LibStorage|Ownable|SimpleFunctionRegistry|TestInitialMigration|TestMigrator|TestSimpleFunctionRegistryFeatureImpl1|TestSimpleFunctionRegistryFeatureImpl2|TestZeroExFeature|ZeroEx).json" }, "repository": { "type": "git", diff --git a/contracts/zero-ex/test/artifacts.ts b/contracts/zero-ex/test/artifacts.ts index 3d8de03402..19e3f584f0 100644 --- a/contracts/zero-ex/test/artifacts.ts +++ b/contracts/zero-ex/test/artifacts.ts @@ -7,7 +7,6 @@ import { ContractArtifact } from 'ethereum-types'; import * as Bootstrap from '../test/generated-artifacts/Bootstrap.json'; import * as FixinCommon from '../test/generated-artifacts/FixinCommon.json'; -import * as FixinOwnable from '../test/generated-artifacts/FixinOwnable.json'; import * as IBootstrap from '../test/generated-artifacts/IBootstrap.json'; import * as IFeature from '../test/generated-artifacts/IFeature.json'; import * as InitialMigration from '../test/generated-artifacts/InitialMigration.json'; @@ -46,7 +45,6 @@ export const artifacts = { Ownable: Ownable as ContractArtifact, SimpleFunctionRegistry: SimpleFunctionRegistry as ContractArtifact, FixinCommon: FixinCommon as ContractArtifact, - FixinOwnable: FixinOwnable as ContractArtifact, InitialMigration: InitialMigration as ContractArtifact, LibBootstrap: LibBootstrap as ContractArtifact, LibMigrate: LibMigrate as ContractArtifact, diff --git a/contracts/zero-ex/test/wrappers.ts b/contracts/zero-ex/test/wrappers.ts index 60d1125898..8a12827df6 100644 --- a/contracts/zero-ex/test/wrappers.ts +++ b/contracts/zero-ex/test/wrappers.ts @@ -5,7 +5,6 @@ */ export * from '../test/generated-wrappers/bootstrap'; export * from '../test/generated-wrappers/fixin_common'; -export * from '../test/generated-wrappers/fixin_ownable'; export * from '../test/generated-wrappers/i_bootstrap'; export * from '../test/generated-wrappers/i_feature'; export * from '../test/generated-wrappers/i_ownable'; diff --git a/contracts/zero-ex/tsconfig.json b/contracts/zero-ex/tsconfig.json index 628c352f79..f8aa02b184 100644 --- a/contracts/zero-ex/tsconfig.json +++ b/contracts/zero-ex/tsconfig.json @@ -8,7 +8,6 @@ "generated-artifacts/ZeroEx.json", "test/generated-artifacts/Bootstrap.json", "test/generated-artifacts/FixinCommon.json", - "test/generated-artifacts/FixinOwnable.json", "test/generated-artifacts/IBootstrap.json", "test/generated-artifacts/IFeature.json", "test/generated-artifacts/IOwnable.json",