From a46b13967a6a0a2b46fd951bbf81c2a6e5b7e6f4 Mon Sep 17 00:00:00 2001 From: James Towle Date: Mon, 17 Jun 2019 10:57:33 -0700 Subject: [PATCH] Refactored the SafeMath errors --- contracts/asset-proxy/test/erc1155_proxy.ts | 3 +- contracts/erc1155/test/erc1155_token.ts | 11 +++--- contracts/exchange/test/internal.ts | 31 +++++++-------- .../contracts/src/MixinSafeMathRichErrors.sol | 38 +++++++------------ contracts/utils/contracts/src/SafeMath.sol | 9 +++-- packages/utils/src/safe_math_revert_errors.ts | 29 ++++++-------- 6 files changed, 51 insertions(+), 70 deletions(-) diff --git a/contracts/asset-proxy/test/erc1155_proxy.ts b/contracts/asset-proxy/test/erc1155_proxy.ts index 67b188a3bd..56c4148c8b 100644 --- a/contracts/asset-proxy/test/erc1155_proxy.ts +++ b/contracts/asset-proxy/test/erc1155_proxy.ts @@ -1831,7 +1831,8 @@ describe('ERC1155Proxy', () => { // check balances before transfer const expectedInitialBalances = [spenderInitialFungibleBalance, receiverInitialFungibleBalance]; await erc1155Wrapper.assertBalancesAsync(tokenHolders, tokensToTransfer, expectedInitialBalances); - const expectedError = new SafeMathRevertErrors.Uint256UnderflowError( + const expectedError = new SafeMathRevertErrors.SafeMathError( + SafeMathRevertErrors.SafeMathErrorCodes.Uint256SubtractionUnderflow, spenderInitialFungibleBalance, valuesToTransfer[0].times(valueMultiplier), ); diff --git a/contracts/erc1155/test/erc1155_token.ts b/contracts/erc1155/test/erc1155_token.ts index 5d27578a4b..4e6e5b96cd 100644 --- a/contracts/erc1155/test/erc1155_token.ts +++ b/contracts/erc1155/test/erc1155_token.ts @@ -12,16 +12,13 @@ import { BigNumber, SafeMathRevertErrors } from '@0x/utils'; import * as chai from 'chai'; import { LogWithDecodedArgs } from 'ethereum-types'; import * as _ from 'lodash'; - import { - artifacts, DummyERC1155ReceiverBatchTokenReceivedEventArgs, DummyERC1155ReceiverContract, ERC1155MintableContract, + artifacts, } from '../src'; - import { Erc1155Wrapper } from './utils/erc1155_wrapper'; - chaiSetup.configure(); const expect = chai.expect; const blockchainLifecycle = new BlockchainLifecycle(web3Wrapper); @@ -172,7 +169,8 @@ describe('ERC1155Token', () => { const tokenToTransfer = fungibleToken; const valueToTransfer = spenderInitialFungibleBalance.plus(1); // create the expected error (a uint256 underflow) - const expectedError = new SafeMathRevertErrors.Uint256UnderflowError( + const expectedError = new SafeMathRevertErrors.SafeMathError( + SafeMathRevertErrors.SafeMathErrorCodes.Uint256SubtractionUnderflow, spenderInitialFungibleBalance, valueToTransfer, ); @@ -348,7 +346,8 @@ describe('ERC1155Token', () => { const valuesToTransfer = [spenderInitialFungibleBalance.plus(1)]; // create the expected error (a uint256 underflow) - const expectedError = new SafeMathRevertErrors.Uint256UnderflowError( + const expectedError = new SafeMathRevertErrors.SafeMathError( + SafeMathRevertErrors.SafeMathErrorCodes.Uint256SubtractionUnderflow, spenderInitialFungibleBalance, valuesToTransfer[0], ); diff --git a/contracts/exchange/test/internal.ts b/contracts/exchange/test/internal.ts index 12f77f1296..f5ff0605a3 100644 --- a/contracts/exchange/test/internal.ts +++ b/contracts/exchange/test/internal.ts @@ -50,15 +50,12 @@ const emptySignedOrder: SignedOrder = { signature: '', }; -const overflowErrorForCall = () => new SafeMathRevertErrors.Uint256OverflowError(); +const safeMathErrorForCall = () => new SafeMathRevertErrors.SafeMathError(); describe('Exchange core internal functions', () => { let chainId: number; let testExchange: TestExchangeInternalsContract; - let overflowErrorForSendTransaction: ( - a?: BigNumber | number | string, - b?: BigNumber | number | string, - ) => Error | undefined; + let safeMathErrorForSendTransaction: () => Error | undefined; let divisionByZeroErrorForCall: Error | undefined; let roundingErrorForCall: Error | undefined; @@ -79,7 +76,7 @@ describe('Exchange core internal functions', () => { txDefaults, new BigNumber(chainId), ); - overflowErrorForSendTransaction = overflowErrorForCall; + safeMathErrorForSendTransaction = safeMathErrorForCall; divisionByZeroErrorForCall = new Error(RevertReason.DivisionByZero); roundingErrorForCall = new Error(RevertReason.RoundingError); }); @@ -105,10 +102,10 @@ describe('Exchange core internal functions', () => { const remainderTimes1000 = remainder.multipliedBy('1000'); const isError = remainderTimes1000.gte(product); if (product.isGreaterThan(MAX_UINT256)) { - throw overflowErrorForCall(); + throw safeMathErrorForCall(); } if (remainderTimes1000.isGreaterThan(MAX_UINT256)) { - throw overflowErrorForCall(); + throw safeMathErrorForCall(); } return isError; } @@ -133,10 +130,10 @@ describe('Exchange core internal functions', () => { const errorTimes1000 = error.multipliedBy('1000'); const isError = errorTimes1000.gte(product); if (product.isGreaterThan(MAX_UINT256)) { - throw overflowErrorForCall(); + throw safeMathErrorForCall(); } if (errorTimes1000.isGreaterThan(MAX_UINT256)) { - throw overflowErrorForCall(); + throw safeMathErrorForCall(); } return isError; } @@ -155,7 +152,7 @@ describe('Exchange core internal functions', () => { } const product = numerator.multipliedBy(target); if (product.isGreaterThan(MAX_UINT256)) { - throw overflowErrorForCall(); + throw safeMathErrorForCall(); } return product.dividedToIntegerBy(denominator); } @@ -189,7 +186,7 @@ describe('Exchange core internal functions', () => { (totalVal: BigNumber, singleVal: BigNumber) => { const newTotal = totalVal.plus(singleVal); if (newTotal.isGreaterThan(MAX_UINT256)) { - throw overflowErrorForCall(); + throw safeMathErrorForCall(); } return newTotal; }, @@ -283,7 +280,7 @@ describe('Exchange core internal functions', () => { } const product = numerator.multipliedBy(target); if (product.isGreaterThan(MAX_UINT256)) { - throw overflowErrorForCall(); + throw safeMathErrorForCall(); } return product.dividedToIntegerBy(denominator); } @@ -314,7 +311,7 @@ describe('Exchange core internal functions', () => { const product = numerator.multipliedBy(target); const offset = product.plus(denominator.minus(1)); if (offset.isGreaterThan(MAX_UINT256)) { - throw overflowErrorForCall(); + throw safeMathErrorForCall(); } const result = offset.dividedToIntegerBy(denominator); if (product.mod(denominator).eq(0)) { @@ -371,7 +368,7 @@ describe('Exchange core internal functions', () => { const product = numerator.multipliedBy(target); const offset = product.plus(denominator.minus(1)); if (offset.isGreaterThan(MAX_UINT256)) { - throw overflowErrorForCall(); + throw safeMathErrorForCall(); } const result = offset.dividedToIntegerBy(denominator); if (product.mod(denominator).eq(0)) { @@ -445,8 +442,8 @@ describe('Exchange core internal functions', () => { ): Promise { const totalFilledAmount = takerAssetFilledAmount.plus(orderTakerAssetFilledAmount); if (totalFilledAmount.isGreaterThan(MAX_UINT256)) { - // FIXME throw overflowErrorForSendTransaction(takerAssetFilledAmount, orderTakerAssetFilledAmount); - throw overflowErrorForSendTransaction(); + // FIXME throw safeMathErrorForSendTransaction(takerAssetFilledAmount, orderTakerAssetFilledAmount); + throw safeMathErrorForSendTransaction(); } return totalFilledAmount; } diff --git a/contracts/utils/contracts/src/MixinSafeMathRichErrors.sol b/contracts/utils/contracts/src/MixinSafeMathRichErrors.sol index dc856e0da1..85a46320e0 100644 --- a/contracts/utils/contracts/src/MixinSafeMathRichErrors.sol +++ b/contracts/utils/contracts/src/MixinSafeMathRichErrors.sol @@ -6,40 +6,28 @@ import "./RichErrors.sol"; contract MixinSafeMathRichErrors is RichErrors { - // bytes4(keccak256("Uint256OverflowError(uint256,uint256)")) - bytes4 internal constant UINT256_OVERFLOW_ERROR = - 0x55101607; + // bytes4(keccak256("SafeMathError(uint8,uint256,uint256)")) + bytes4 internal constant SAFE_MATH_ERROR = + 0x35a51a70; - // bytes4(keccak256("Uint256UnderflowError(uint256,uint256)")) - bytes4 internal constant UINT256_UNDERFLOW_ERROR = - 0x60ee612f; - - // solhint-disable func-name-mixedcase - function Uint256OverflowError( - uint256 a, - uint256 b - ) - internal - pure - returns (bytes memory) - { - return abi.encodeWithSelector( - UINT256_OVERFLOW_ERROR, - a, - b - ); + enum SafeMathErrorCodes { + UINT256_ADDITION_OVERFLOW, + UINT256_MULTIPLICATION_OVERFLOW, + UINT256_SUBTRACTION_UNDERFLOW } - function Uint256UnderflowError( + // solhint-disable func-name-mixedcase + function SafeMathError( + SafeMathErrorCodes errorCode, uint256 a, uint256 b ) internal - pure - returns (bytes memory) + pure returns (bytes memory) { return abi.encodeWithSelector( - UINT256_UNDERFLOW_ERROR, + SAFE_MATH_ERROR, + errorCode, a, b ); diff --git a/contracts/utils/contracts/src/SafeMath.sol b/contracts/utils/contracts/src/SafeMath.sol index 623515ab86..f8c57fca31 100644 --- a/contracts/utils/contracts/src/SafeMath.sol +++ b/contracts/utils/contracts/src/SafeMath.sol @@ -17,7 +17,8 @@ contract SafeMath is } uint256 c = a * b; if (c / a != b) { - _rrevert(Uint256OverflowError( + _rrevert(SafeMathError( + SafeMathErrorCodes.UINT256_MULTIPLICATION_OVERFLOW, a, b )); @@ -40,7 +41,8 @@ contract SafeMath is returns (uint256) { if (b > a) { - _rrevert(Uint256UnderflowError( + _rrevert(SafeMathError( + SafeMathErrorCodes.UINT256_SUBTRACTION_UNDERFLOW, a, b )); @@ -55,7 +57,8 @@ contract SafeMath is { uint256 c = a + b; if (c < a) { - _rrevert(Uint256OverflowError( + _rrevert(SafeMathError( + SafeMathErrorCodes.UINT256_ADDITION_OVERFLOW, a, b )); diff --git a/packages/utils/src/safe_math_revert_errors.ts b/packages/utils/src/safe_math_revert_errors.ts index efd1387297..e91f90ef40 100644 --- a/packages/utils/src/safe_math_revert_errors.ts +++ b/packages/utils/src/safe_math_revert_errors.ts @@ -3,27 +3,20 @@ import { RevertError } from './revert_error'; // tslint:disable:max-classes-per-file -export class Uint256OverflowError extends RevertError { - constructor(a?: BigNumber | number | string, b?: BigNumber | number | string) { - super('Uint256OverflowError', 'Uint256OverflowError(uint256 a, uint256 b)', { +export enum SafeMathErrorCodes { + Uint256AdditionOverflow, + Uint256MultiplicationOverflow, + Uint256SubtractionUnderflow, +} + +export class SafeMathError extends RevertError { + constructor(error?: SafeMathErrorCodes, a?: BigNumber | number | string, b?: BigNumber | number | string) { + super('SafeMathError', 'SafeMathError(uint8 error, uint256 a, uint256 b)', { + error, a, b, }); } } -export class Uint256UnderflowError extends RevertError { - constructor(a?: BigNumber | number | string, b?: BigNumber | number | string) { - super('Uint256UnderflowError', 'Uint256UnderflowError(uint256 a, uint256 b)', { - a, - b, - }); - } -} - -const types = [Uint256OverflowError, Uint256UnderflowError]; - -// Register the types we've defined. -for (const type of types) { - RevertError.registerType(type); -} +RevertError.registerType(SafeMathError);