EP: misc fixes (#38)

* `@0x/contracts-zero-ex`: Fix NativeOrdersFeature order hash cancellation not emitting proper maker.
`@0x/contracts-zero-ex`: Revert to original (deployed) ZeroEx/EP proxy implementation. Optimized one is now at `ZeroExOptimized.sol`.
`@0x/contracts-zero-ex`: Add gas limits to first `transferFrom()` call in `LibTokenSpender` and `UniswapFeature`.

* `@0x/contracts-zero-ex`: Update changelog

* disable `no-empty-blocks` solidity linter rule

* `@0x/contracts-zero-ex`: Use bloom filters of greedy tokens in token transfer logic
`@0x/contracts-zero-ex`: Turn `LibTokenSpender` into `FixinTokenSpender`.
`@0x/contracts-zero-ex`: Misc renames for consistency.

* `@0x/contracts-zero-ex`: Export `GREEDY_TOKENS` list

* rebase and update changelog

* `@0x/contracts-zero-ex`: Change bloom filter hash algo based on discussions

* `@0x/contracts-zero-ex`: Fix changelog

* update orders docs

* `@0x/contracts-zero-ex`: revert if allowance call fails in uniswap feature

Co-authored-by: Lawrence Forman <me@merklejerk.com>
This commit is contained in:
Lawrence Forman
2020-11-23 12:59:02 -05:00
committed by GitHub
parent b463a39bfa
commit ab698cec14
36 changed files with 673 additions and 240 deletions

View File

@@ -15,6 +15,7 @@ import * as FixinCommon from '../test/generated-artifacts/FixinCommon.json';
import * as FixinEIP712 from '../test/generated-artifacts/FixinEIP712.json';
import * as FixinProtocolFees from '../test/generated-artifacts/FixinProtocolFees.json';
import * as FixinReentrancyGuard from '../test/generated-artifacts/FixinReentrancyGuard.json';
import * as FixinTokenSpender from '../test/generated-artifacts/FixinTokenSpender.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';
@@ -64,7 +65,6 @@ import * as LibSimpleFunctionRegistryRichErrors from '../test/generated-artifact
import * as LibSimpleFunctionRegistryStorage from '../test/generated-artifacts/LibSimpleFunctionRegistryStorage.json';
import * as LibSpenderRichErrors from '../test/generated-artifacts/LibSpenderRichErrors.json';
import * as LibStorage from '../test/generated-artifacts/LibStorage.json';
import * as LibTokenSpender from '../test/generated-artifacts/LibTokenSpender.json';
import * as LibTokenSpenderStorage from '../test/generated-artifacts/LibTokenSpenderStorage.json';
import * as LibTransformERC20RichErrors from '../test/generated-artifacts/LibTransformERC20RichErrors.json';
import * as LibTransformERC20Storage from '../test/generated-artifacts/LibTransformERC20Storage.json';
@@ -98,11 +98,11 @@ import * as TestFillQuoteTransformerBridge from '../test/generated-artifacts/Tes
import * as TestFillQuoteTransformerExchange from '../test/generated-artifacts/TestFillQuoteTransformerExchange.json';
import * as TestFillQuoteTransformerHost from '../test/generated-artifacts/TestFillQuoteTransformerHost.json';
import * as TestFixinProtocolFees from '../test/generated-artifacts/TestFixinProtocolFees.json';
import * as TestFixinTokenSpender from '../test/generated-artifacts/TestFixinTokenSpender.json';
import * as TestFullMigration from '../test/generated-artifacts/TestFullMigration.json';
import * as TestInitialMigration from '../test/generated-artifacts/TestInitialMigration.json';
import * as TestLibNativeOrder from '../test/generated-artifacts/TestLibNativeOrder.json';
import * as TestLibSignature from '../test/generated-artifacts/TestLibSignature.json';
import * as TestLibTokenSpender from '../test/generated-artifacts/TestLibTokenSpender.json';
import * as TestLiquidityProvider from '../test/generated-artifacts/TestLiquidityProvider.json';
import * as TestMetaTransactionsTransformERC20Feature from '../test/generated-artifacts/TestMetaTransactionsTransformERC20Feature.json';
import * as TestMigrator from '../test/generated-artifacts/TestMigrator.json';
@@ -128,9 +128,11 @@ import * as TransformerDeployer from '../test/generated-artifacts/TransformerDep
import * as UniswapFeature from '../test/generated-artifacts/UniswapFeature.json';
import * as WethTransformer from '../test/generated-artifacts/WethTransformer.json';
import * as ZeroEx from '../test/generated-artifacts/ZeroEx.json';
import * as ZeroExOptimized from '../test/generated-artifacts/ZeroExOptimized.json';
export const artifacts = {
IZeroEx: IZeroEx as ContractArtifact,
ZeroEx: ZeroEx as ContractArtifact,
ZeroExOptimized: ZeroExOptimized as ContractArtifact,
LibCommonRichErrors: LibCommonRichErrors as ContractArtifact,
LibLiquidityProviderRichErrors: LibLiquidityProviderRichErrors as ContractArtifact,
LibMetaTransactionsRichErrors: LibMetaTransactionsRichErrors as ContractArtifact,
@@ -174,11 +176,11 @@ export const artifacts = {
LibNativeOrder: LibNativeOrder as ContractArtifact,
LibSignature: LibSignature as ContractArtifact,
LibSignedCallData: LibSignedCallData as ContractArtifact,
LibTokenSpender: LibTokenSpender as ContractArtifact,
FixinCommon: FixinCommon as ContractArtifact,
FixinEIP712: FixinEIP712 as ContractArtifact,
FixinProtocolFees: FixinProtocolFees as ContractArtifact,
FixinReentrancyGuard: FixinReentrancyGuard as ContractArtifact,
FixinTokenSpender: FixinTokenSpender as ContractArtifact,
FullMigration: FullMigration as ContractArtifact,
InitialMigration: InitialMigration as ContractArtifact,
LibBootstrap: LibBootstrap as ContractArtifact,
@@ -229,11 +231,11 @@ export const artifacts = {
TestFillQuoteTransformerExchange: TestFillQuoteTransformerExchange as ContractArtifact,
TestFillQuoteTransformerHost: TestFillQuoteTransformerHost as ContractArtifact,
TestFixinProtocolFees: TestFixinProtocolFees as ContractArtifact,
TestFixinTokenSpender: TestFixinTokenSpender as ContractArtifact,
TestFullMigration: TestFullMigration as ContractArtifact,
TestInitialMigration: TestInitialMigration as ContractArtifact,
TestLibNativeOrder: TestLibNativeOrder as ContractArtifact,
TestLibSignature: TestLibSignature as ContractArtifact,
TestLibTokenSpender: TestLibTokenSpender as ContractArtifact,
TestLiquidityProvider: TestLiquidityProvider as ContractArtifact,
TestMetaTransactionsTransformERC20Feature: TestMetaTransactionsTransformERC20Feature as ContractArtifact,
TestMigrator: TestMigrator as ContractArtifact,

View File

@@ -2,6 +2,7 @@ import { artifacts as erc20Artifacts, DummyERC20TokenContract } from '@0x/contra
import { blockchainTests, constants, expect, verifyEventsFromLogs } from '@0x/contracts-test-utils';
import { BigNumber, OwnableRevertErrors, ZeroExRevertErrors } from '@0x/utils';
import { ZERO_BYTES32 } from '../../src/constants';
import { IOwnableFeatureContract, IZeroExContract, LiquidityProviderFeatureContract } from '../../src/wrappers';
import { artifacts } from '../artifacts';
import { abis } from '../utils/abis';
@@ -64,6 +65,7 @@ blockchainTests('LiquidityProvider feature', env => {
env.txDefaults,
artifacts,
sandbox.address,
ZERO_BYTES32,
);
await new IOwnableFeatureContract(zeroEx.address, env.provider, env.txDefaults, abis)
.migrate(featureImpl.address, featureImpl.migrate().getABIEncodedTransactionData(), owner)

View File

@@ -10,7 +10,7 @@ import { fullMigrateAsync } from '../utils/migration';
import { getRandomLimitOrder, getRandomRfqOrder } from '../utils/orders';
import { TestMintableERC20TokenContract } from '../wrappers';
blockchainTests.resets('LimitOrdersFeature', env => {
blockchainTests.resets('NativeOrdersFeature', env => {
const { NULL_ADDRESS, MAX_UINT256, ZERO_AMOUNT } = constants;
const GAS_PRICE = new BigNumber('123e9');
const PROTOCOL_FEE_MULTIPLIER = 1337e3;

View File

@@ -66,7 +66,7 @@ blockchainTests.resets('SimpleFunctionRegistry feature', env => {
it('`rollback()` to zero impl succeeds for unregistered function', async () => {
await registry.rollback(testFnSelector, NULL_ADDRESS).awaitTransactionSuccessAsync();
const impl = await registry.getFunctionImplementation(testFnSelector).callAsync();
const impl = await zeroEx.getFunctionImplementation(testFnSelector).callAsync();
expect(impl).to.eq(NULL_ADDRESS);
});

View File

@@ -24,6 +24,7 @@ import {
TestMintTokenERC20TransformerContract,
TestMintTokenERC20TransformerEvents,
TestMintTokenERC20TransformerMintTransformEventArgs,
TestTransformERC20Contract,
TransformERC20FeatureEvents,
} from '../wrappers';
@@ -49,7 +50,7 @@ blockchainTests.resets('TransformERC20 feature', env => {
env.provider,
env.txDefaults,
{
transformERC20: (await TransformERC20FeatureContract.deployFrom0xArtifactAsync(
transformERC20: (await TestTransformERC20Contract.deployFrom0xArtifactAsync(
artifacts.TestTransformERC20,
env.provider,
env.txDefaults,

View File

@@ -7,34 +7,49 @@ import {
} from '@0x/contracts-test-utils';
import { BigNumber, hexUtils, StringRevertError, ZeroExRevertErrors } from '@0x/utils';
import { getTokenListBloomFilter } from '../src/bloom_filter_utils';
import { artifacts } from './artifacts';
import {
TestLibTokenSpenderContract,
TestLibTokenSpenderEvents,
TestFixinTokenSpenderContract,
TestFixinTokenSpenderEvents,
TestTokenSpenderERC20TokenContract,
TestTokenSpenderERC20TokenEvents,
} from './wrappers';
blockchainTests.resets('LibTokenSpender library', env => {
let tokenSpender: TestLibTokenSpenderContract;
blockchainTests.resets('FixinTokenSpender', env => {
let tokenSpender: TestFixinTokenSpenderContract;
let token: TestTokenSpenderERC20TokenContract;
let greedyToken: TestTokenSpenderERC20TokenContract;
let greedyTokensBloomFilter: string;
before(async () => {
tokenSpender = await TestLibTokenSpenderContract.deployFrom0xArtifactAsync(
artifacts.TestLibTokenSpender,
env.provider,
env.txDefaults,
artifacts,
);
token = await TestTokenSpenderERC20TokenContract.deployFrom0xArtifactAsync(
artifacts.TestTokenSpenderERC20Token,
env.provider,
env.txDefaults,
artifacts,
);
greedyToken = await TestTokenSpenderERC20TokenContract.deployFrom0xArtifactAsync(
artifacts.TestTokenSpenderERC20Token,
env.provider,
env.txDefaults,
artifacts,
);
await greedyToken.setGreedyRevert(true).awaitTransactionSuccessAsync();
greedyTokensBloomFilter = getTokenListBloomFilter([greedyToken.address]);
tokenSpender = await TestFixinTokenSpenderContract.deployFrom0xArtifactAsync(
artifacts.TestFixinTokenSpender,
env.provider,
env.txDefaults,
artifacts,
greedyTokensBloomFilter,
);
});
describe('spendERC20Tokens()', () => {
describe('transferERC20Tokens()', () => {
const EMPTY_RETURN_AMOUNT = 1337;
const FALSE_RETURN_AMOUNT = 1338;
const REVERT_RETURN_AMOUNT = 1339;
@@ -42,12 +57,12 @@ blockchainTests.resets('LibTokenSpender library', env => {
const EXTRA_RETURN_TRUE_AMOUNT = 1341;
const EXTRA_RETURN_FALSE_AMOUNT = 1342;
it('spendERC20Tokens() successfully calls compliant ERC20 token', async () => {
it('transferERC20Tokens() successfully calls compliant ERC20 token', async () => {
const tokenFrom = randomAddress();
const tokenTo = randomAddress();
const tokenAmount = new BigNumber(123456);
const receipt = await tokenSpender
.spendERC20Tokens(token.address, tokenFrom, tokenTo, tokenAmount)
.transferERC20Tokens(token.address, tokenFrom, tokenTo, tokenAmount)
.awaitTransactionSuccessAsync();
verifyEventsFromLogs(
receipt.logs,
@@ -63,12 +78,12 @@ blockchainTests.resets('LibTokenSpender library', env => {
);
});
it('spendERC20Tokens() successfully calls non-compliant ERC20 token', async () => {
it('transferERC20Tokens() successfully calls non-compliant ERC20 token', async () => {
const tokenFrom = randomAddress();
const tokenTo = randomAddress();
const tokenAmount = new BigNumber(EMPTY_RETURN_AMOUNT);
const receipt = await tokenSpender
.spendERC20Tokens(token.address, tokenFrom, tokenTo, tokenAmount)
.transferERC20Tokens(token.address, tokenFrom, tokenTo, tokenAmount)
.awaitTransactionSuccessAsync();
verifyEventsFromLogs(
receipt.logs,
@@ -84,12 +99,12 @@ blockchainTests.resets('LibTokenSpender library', env => {
);
});
it('spendERC20Tokens() reverts if ERC20 token reverts', async () => {
it('transferERC20Tokens() reverts if ERC20 token reverts', async () => {
const tokenFrom = randomAddress();
const tokenTo = randomAddress();
const tokenAmount = new BigNumber(REVERT_RETURN_AMOUNT);
const tx = tokenSpender
.spendERC20Tokens(token.address, tokenFrom, tokenTo, tokenAmount)
.transferERC20Tokens(token.address, tokenFrom, tokenTo, tokenAmount)
.awaitTransactionSuccessAsync();
const expectedError = new ZeroExRevertErrors.Spender.SpenderERC20TransferFromFailedError(
token.address,
@@ -101,12 +116,12 @@ blockchainTests.resets('LibTokenSpender library', env => {
return expect(tx).to.revertWith(expectedError);
});
it('spendERC20Tokens() reverts if ERC20 token returns false', async () => {
it('transferERC20Tokens() reverts if ERC20 token returns false', async () => {
const tokenFrom = randomAddress();
const tokenTo = randomAddress();
const tokenAmount = new BigNumber(FALSE_RETURN_AMOUNT);
const tx = tokenSpender
.spendERC20Tokens(token.address, tokenFrom, tokenTo, tokenAmount)
.transferERC20Tokens(token.address, tokenFrom, tokenTo, tokenAmount)
.awaitTransactionSuccessAsync();
return expect(tx).to.revertWith(
new ZeroExRevertErrors.Spender.SpenderERC20TransferFromFailedError(
@@ -119,12 +134,12 @@ blockchainTests.resets('LibTokenSpender library', env => {
);
});
it('spendERC20Tokens() falls back successfully to TokenSpender._spendERC20Tokens()', async () => {
it('transferERC20Tokens() falls back successfully to TokenSpender._spendERC20Tokens()', async () => {
const tokenFrom = randomAddress();
const tokenTo = randomAddress();
const tokenAmount = new BigNumber(TRIGGER_FALLBACK_SUCCESS_AMOUNT);
const receipt = await tokenSpender
.spendERC20Tokens(token.address, tokenFrom, tokenTo, tokenAmount)
.transferERC20Tokens(token.address, tokenFrom, tokenTo, tokenAmount)
.awaitTransactionSuccessAsync();
verifyEventsFromLogs(
receipt.logs,
@@ -136,17 +151,17 @@ blockchainTests.resets('LibTokenSpender library', env => {
amount: tokenAmount,
},
],
TestLibTokenSpenderEvents.FallbackCalled,
TestFixinTokenSpenderEvents.FallbackCalled,
);
});
it('spendERC20Tokens() allows extra data after true', async () => {
it('transferERC20Tokens() allows extra data after true', async () => {
const tokenFrom = randomAddress();
const tokenTo = randomAddress();
const tokenAmount = new BigNumber(EXTRA_RETURN_TRUE_AMOUNT);
const receipt = await tokenSpender
.spendERC20Tokens(token.address, tokenFrom, tokenTo, tokenAmount)
.transferERC20Tokens(token.address, tokenFrom, tokenTo, tokenAmount)
.awaitTransactionSuccessAsync();
verifyEventsFromLogs(
receipt.logs,
@@ -162,13 +177,13 @@ blockchainTests.resets('LibTokenSpender library', env => {
);
});
it("spendERC20Tokens() reverts when there's extra data after false", async () => {
it("transferERC20Tokens() reverts when there's extra data after false", async () => {
const tokenFrom = randomAddress();
const tokenTo = randomAddress();
const tokenAmount = new BigNumber(EXTRA_RETURN_FALSE_AMOUNT);
const tx = tokenSpender
.spendERC20Tokens(token.address, tokenFrom, tokenTo, tokenAmount)
.transferERC20Tokens(token.address, tokenFrom, tokenTo, tokenAmount)
.awaitTransactionSuccessAsync();
return expect(tx).to.revertWith(
new ZeroExRevertErrors.Spender.SpenderERC20TransferFromFailedError(
@@ -181,15 +196,82 @@ blockchainTests.resets('LibTokenSpender library', env => {
);
});
it('spendERC20Tokens() cannot call self', async () => {
it('transferERC20Tokens() cannot call self', async () => {
const tokenFrom = randomAddress();
const tokenTo = randomAddress();
const tokenAmount = new BigNumber(123456);
const tx = tokenSpender
.spendERC20Tokens(tokenSpender.address, tokenFrom, tokenTo, tokenAmount)
.transferERC20Tokens(tokenSpender.address, tokenFrom, tokenTo, tokenAmount)
.awaitTransactionSuccessAsync();
return expect(tx).to.revertWith('LibTokenSpender/CANNOT_INVOKE_SELF');
return expect(tx).to.revertWith('FixinTokenSpender/CANNOT_INVOKE_SELF');
});
it('calls transferFrom() directly for a greedy token with allowance', async () => {
const tokenFrom = randomAddress();
const tokenTo = randomAddress();
const tokenAmount = new BigNumber(123456);
await greedyToken.approveAs(tokenFrom, tokenSpender.address, tokenAmount).awaitTransactionSuccessAsync();
const receipt = await tokenSpender
.transferERC20Tokens(greedyToken.address, tokenFrom, tokenTo, tokenAmount)
.awaitTransactionSuccessAsync();
// Because there is an allowance set, we will call transferFrom()
// directly, which succeds and emits an event.
verifyEventsFromLogs(
receipt.logs,
[
{
sender: tokenSpender.address,
from: tokenFrom,
to: tokenTo,
amount: tokenAmount,
},
], // No events.
TestTokenSpenderERC20TokenEvents.TransferFromCalled,
);
});
it('calls only the fallback for a greedy token with no allowance', async () => {
const tokenFrom = randomAddress();
const tokenTo = randomAddress();
const tokenAmount = new BigNumber(TRIGGER_FALLBACK_SUCCESS_AMOUNT);
const receipt = await tokenSpender
.transferERC20Tokens(greedyToken.address, tokenFrom, tokenTo, tokenAmount)
.awaitTransactionSuccessAsync();
// Because this is a greedy token and there is no allowance set, transferFrom()
// will not be called before hitting the fallback, which only emits an event
// in the test contract.
verifyEventsFromLogs(
receipt.logs,
[], // No events.
TestTokenSpenderERC20TokenEvents.TransferFromCalled,
);
verifyEventsFromLogs(
receipt.logs,
[
{
token: greedyToken.address,
owner: tokenFrom,
to: tokenTo,
amount: tokenAmount,
},
],
TestFixinTokenSpenderEvents.FallbackCalled,
);
});
});
describe('isTokenPossiblyGreedy()', () => {
it('returns true for greedy token', async () => {
const isGreedy = await tokenSpender.isTokenPossiblyGreedy(greedyToken.address).callAsync();
expect(isGreedy).to.eq(true);
});
it('returns false for non-greedy token', async () => {
const isGreedy = await tokenSpender.isTokenPossiblyGreedy(token.address).callAsync();
expect(isGreedy).to.eq(false);
});
});

View File

@@ -13,7 +13,6 @@ import {
INativeOrdersFeatureContract,
IOwnableFeatureContract,
ISignatureValidatorFeatureContract,
ISimpleFunctionRegistryFeatureContract,
ITokenSpenderFeatureContract,
ITransformERC20FeatureContract,
TestFullMigrationContract,
@@ -27,7 +26,6 @@ blockchainTests.resets('Full migration', env => {
let zeroEx: ZeroExContract;
let features: FullFeatures;
let migrator: TestFullMigrationContract;
let registry: ISimpleFunctionRegistryFeatureContract;
const transformerDeployer = randomAddress();
before(async () => {
@@ -50,7 +48,6 @@ blockchainTests.resets('Full migration', env => {
await migrator
.migrateZeroEx(owner, zeroEx.address, features, { transformerDeployer })
.awaitTransactionSuccessAsync();
registry = new ISimpleFunctionRegistryFeatureContract(zeroEx.address, env.provider, env.txDefaults);
});
it('ZeroEx has the correct owner', async () => {
@@ -191,7 +188,7 @@ blockchainTests.resets('Full migration', env => {
for (const fn of featureInfo.fns) {
it(`${fn} is registered`, async () => {
const selector = contract.getSelector(fn);
const impl = await registry.getFunctionImplementation(selector).callAsync();
const impl = await zeroEx.getFunctionImplementation(selector).callAsync();
expect(impl).to.not.eq(NULL_ADDRESS);
});

View File

@@ -13,6 +13,7 @@ export * from '../test/generated-wrappers/fixin_common';
export * from '../test/generated-wrappers/fixin_e_i_p712';
export * from '../test/generated-wrappers/fixin_protocol_fees';
export * from '../test/generated-wrappers/fixin_reentrancy_guard';
export * from '../test/generated-wrappers/fixin_token_spender';
export * from '../test/generated-wrappers/flash_wallet';
export * from '../test/generated-wrappers/full_migration';
export * from '../test/generated-wrappers/i_allowance_target';
@@ -62,7 +63,6 @@ export * from '../test/generated-wrappers/lib_simple_function_registry_rich_erro
export * from '../test/generated-wrappers/lib_simple_function_registry_storage';
export * from '../test/generated-wrappers/lib_spender_rich_errors';
export * from '../test/generated-wrappers/lib_storage';
export * from '../test/generated-wrappers/lib_token_spender';
export * from '../test/generated-wrappers/lib_token_spender_storage';
export * from '../test/generated-wrappers/lib_transform_erc20_rich_errors';
export * from '../test/generated-wrappers/lib_transform_erc20_storage';
@@ -96,11 +96,11 @@ export * from '../test/generated-wrappers/test_fill_quote_transformer_bridge';
export * from '../test/generated-wrappers/test_fill_quote_transformer_exchange';
export * from '../test/generated-wrappers/test_fill_quote_transformer_host';
export * from '../test/generated-wrappers/test_fixin_protocol_fees';
export * from '../test/generated-wrappers/test_fixin_token_spender';
export * from '../test/generated-wrappers/test_full_migration';
export * from '../test/generated-wrappers/test_initial_migration';
export * from '../test/generated-wrappers/test_lib_native_order';
export * from '../test/generated-wrappers/test_lib_signature';
export * from '../test/generated-wrappers/test_lib_token_spender';
export * from '../test/generated-wrappers/test_liquidity_provider';
export * from '../test/generated-wrappers/test_meta_transactions_transform_erc20_feature';
export * from '../test/generated-wrappers/test_migrator';
@@ -126,3 +126,4 @@ export * from '../test/generated-wrappers/transformer_deployer';
export * from '../test/generated-wrappers/uniswap_feature';
export * from '../test/generated-wrappers/weth_transformer';
export * from '../test/generated-wrappers/zero_ex';
export * from '../test/generated-wrappers/zero_ex_optimized';

View File

@@ -83,7 +83,7 @@ blockchainTests.resets('ZeroEx contract', env => {
// registry.getSelector('extendSelf'),
];
const selectors = [...ownableSelectors, ...registrySelectors];
const impls = await Promise.all(selectors.map(s => registry.getFunctionImplementation(s).callAsync()));
const impls = await Promise.all(selectors.map(s => zeroEx.getFunctionImplementation(s).callAsync()));
for (let i = 0; i < impls.length; ++i) {
const selector = selectors[i];
const impl = impls[i];