Merge pull request #797 from 0xProject/refactor/contracts/registerAssetProxy

Make registerAssetProxy append only
This commit is contained in:
Amir Bandeali
2018-06-29 17:25:21 -07:00
committed by GitHub
12 changed files with 68 additions and 203 deletions

View File

@@ -32,43 +32,28 @@ contract MixinAssetProxyDispatcher is
// Mapping from Asset Proxy Id's to their respective Asset Proxy
mapping (bytes4 => IAssetProxy) public assetProxies;
/// @dev Registers an asset proxy to an asset proxy id.
/// An id can only be assigned to a single proxy at a given time.
/// @param assetProxyId Id to register`newAssetProxy` under.
/// @param newAssetProxy Address of new asset proxy to register, or 0x0 to unset assetProxyId.
/// @param oldAssetProxy Existing asset proxy to overwrite, or 0x0 if assetProxyId is currently unused.
function registerAssetProxy(
bytes4 assetProxyId,
address newAssetProxy,
address oldAssetProxy
)
/// @dev Registers an asset proxy to its asset proxy id.
/// Once an asset proxy is registered, it cannot be unregistered.
/// @param assetProxy Address of new asset proxy to register.
function registerAssetProxy(address assetProxy)
external
onlyOwner
{
// Ensure the existing asset proxy is not unintentionally overwritten
IAssetProxy assetProxyContract = IAssetProxy(assetProxy);
// Ensure that no asset proxy exists with current id.
bytes4 assetProxyId = assetProxyContract.getProxyId();
address currentAssetProxy = assetProxies[assetProxyId];
require(
oldAssetProxy == currentAssetProxy,
"ASSET_PROXY_MISMATCH"
currentAssetProxy == address(0),
"ASSET_PROXY_ALREADY_EXISTS"
);
IAssetProxy assetProxy = IAssetProxy(newAssetProxy);
// Ensure that the id of newAssetProxy matches the passed in assetProxyId, unless it is being reset to 0.
if (newAssetProxy != address(0)) {
bytes4 newAssetProxyId = assetProxy.getProxyId();
require(
newAssetProxyId == assetProxyId,
"ASSET_PROXY_ID_MISMATCH"
);
}
// Add asset proxy and log registration.
assetProxies[assetProxyId] = assetProxy;
emit AssetProxySet(
assetProxies[assetProxyId] = assetProxyContract;
emit AssetProxyRegistered(
assetProxyId,
newAssetProxy,
oldAssetProxy
assetProxy
);
}
@@ -112,7 +97,7 @@ contract MixinAssetProxyDispatcher is
0xFFFFFFFF00000000000000000000000000000000000000000000000000000000
)
}
IAssetProxy assetProxy = assetProxies[assetProxyId];
address assetProxy = assetProxies[assetProxyId];
// Ensure that assetProxy exists
require(

View File

@@ -20,16 +20,10 @@ pragma solidity ^0.4.24;
contract IAssetProxyDispatcher {
/// @dev Registers an asset proxy to an asset proxy id.
/// An id can only be assigned to a single proxy at a given time.
/// @param assetProxyId Id to register`newAssetProxy` under.
/// @param newAssetProxy Address of new asset proxy to register, or 0x0 to unset assetProxyId.
/// @param oldAssetProxy Existing asset proxy to overwrite, or 0x0 if assetProxyId is currently unused.
function registerAssetProxy(
bytes4 assetProxyId,
address newAssetProxy,
address oldAssetProxy
)
/// @dev Registers an asset proxy to its asset proxy id.
/// Once an asset proxy is registered, it cannot be unregistered.
/// @param assetProxy Address of new asset proxy to register.
function registerAssetProxy(address assetProxy)
external;
/// @dev Gets an asset proxy.

View File

@@ -54,8 +54,7 @@ contract LibExchangeErrors {
string constant FAILED_EXECUTION = "FAILED_EXECUTION"; // Transaction execution failed.
/// registerAssetProxy errors ///
string constant ASSET_PROXY_MISMATCH = "ASSET_PROXY_MISMATCH"; // oldAssetProxy proxy does not match currentAssetProxy.
string constant ASSET_PROXY_ID_MISMATCH = "ASSET_PROXY_ID_MISMATCH"; // newAssetProxyId does not match given assetProxyId.
string constant ASSET_PROXY_ALREADY_EXISTS = "ASSET_PROXY_ALREADY_EXISTS"; // AssetProxy with same id already exists.
/// dispatchTransferFrom errors ///
string constant ASSET_PROXY_DOES_NOT_EXIST = "ASSET_PROXY_DOES_NOT_EXIST"; // No assetProxy registered at given id.

View File

@@ -26,10 +26,9 @@ contract MAssetProxyDispatcher is
{
// Logs registration of new asset proxy
event AssetProxySet(
event AssetProxyRegistered(
bytes4 id, // Id of new registered AssetProxy.
address newAssetProxy, // Address of new registered AssetProxy.
address oldAssetProxy // Address of AssetProxy that was overwritten at given id (or null address).
address assetProxy // Address of new registered AssetProxy.
);
/// @dev Forwards arguments to assetProxy and calls `transferFrom`. Either succeeds or throws.

View File

@@ -96,8 +96,8 @@ export async function coreCombinatorialUtilsFactoryAsync(
zrxAssetData,
);
const exchangeWrapper = new ExchangeWrapper(exchangeContract, provider);
await exchangeWrapper.registerAssetProxyAsync(AssetProxyId.ERC20, erc20Proxy.address, ownerAddress);
await exchangeWrapper.registerAssetProxyAsync(AssetProxyId.ERC721, erc721Proxy.address, ownerAddress);
await exchangeWrapper.registerAssetProxyAsync(erc20Proxy.address, ownerAddress);
await exchangeWrapper.registerAssetProxyAsync(erc721Proxy.address, ownerAddress);
await web3Wrapper.awaitTransactionSuccessAsync(
await erc20Proxy.addAuthorizedAddress.sendTransactionAsync(exchangeContract.address, {

View File

@@ -1,12 +1,10 @@
import { AssetProxyId, SignedOrder } from '@0xproject/types';
import { SignedOrder } from '@0xproject/types';
import { BigNumber } from '@0xproject/utils';
import { Web3Wrapper } from '@0xproject/web3-wrapper';
import { Provider, TransactionReceiptWithDecodedLogs } from 'ethereum-types';
import * as _ from 'lodash';
import { ExchangeContract } from '../generated_contract_wrappers/exchange';
import { constants } from './constants';
import { formatters } from './formatters';
import { LogDecoder } from './log_decoder';
import { orderUtils } from './order_utils';
@@ -192,20 +190,10 @@ export class ExchangeWrapper {
return tx;
}
public async registerAssetProxyAsync(
assetProxyId: AssetProxyId,
assetProxyAddress: string,
from: string,
opts: { oldAssetProxyAddressIfExists?: string } = {},
): Promise<TransactionReceiptWithDecodedLogs> {
const oldAssetProxyAddress = _.isUndefined(opts.oldAssetProxyAddressIfExists)
? constants.NULL_ADDRESS
: opts.oldAssetProxyAddressIfExists;
const txHash = await this._exchange.registerAssetProxy.sendTransactionAsync(
assetProxyId,
assetProxyAddress,
oldAssetProxyAddress,
{ from },
);
const txHash = await this._exchange.registerAssetProxy.sendTransactionAsync(assetProxyAddress, { from });
const tx = await this._logDecoder.getTxWithDecodedLogsAsync(txHash);
return tx;
}

View File

@@ -1,6 +1,6 @@
import { BlockchainLifecycle } from '@0xproject/dev-utils';
import { assetProxyUtils, orderHashUtils } from '@0xproject/order-utils';
import { AssetProxyId, RevertReason, SignedOrder } from '@0xproject/types';
import { RevertReason, SignedOrder } from '@0xproject/types';
import { BigNumber } from '@0xproject/utils';
import { Web3Wrapper } from '@0xproject/web3-wrapper';
import * as chai from 'chai';
@@ -90,8 +90,8 @@ describe('Exchange core', () => {
assetProxyUtils.encodeERC20AssetData(zrxToken.address),
);
exchangeWrapper = new ExchangeWrapper(exchange, provider);
await exchangeWrapper.registerAssetProxyAsync(AssetProxyId.ERC20, erc20Proxy.address, owner);
await exchangeWrapper.registerAssetProxyAsync(AssetProxyId.ERC721, erc721Proxy.address, owner);
await exchangeWrapper.registerAssetProxyAsync(erc20Proxy.address, owner);
await exchangeWrapper.registerAssetProxyAsync(erc721Proxy.address, owner);
await web3Wrapper.awaitTransactionSuccessAsync(
await erc20Proxy.addAuthorizedAddress.sendTransactionAsync(exchange.address, {

View File

@@ -3,24 +3,29 @@ import { assetProxyUtils } from '@0xproject/order-utils';
import { AssetProxyId, RevertReason } from '@0xproject/types';
import { BigNumber } from '@0xproject/utils';
import * as chai from 'chai';
import { LogWithDecodedArgs } from 'ethereum-types';
import * as _ from 'lodash';
import { DummyERC20TokenContract } from '../../src/generated_contract_wrappers/dummy_e_r_c20_token';
import { ERC20ProxyContract } from '../../src/generated_contract_wrappers/e_r_c20_proxy';
import { ERC721ProxyContract } from '../../src/generated_contract_wrappers/e_r_c721_proxy';
import { TestAssetProxyDispatcherContract } from '../../src/generated_contract_wrappers/test_asset_proxy_dispatcher';
import {
AssetProxyRegisteredContractEventArgs,
TestAssetProxyDispatcherContract,
} from '../../src/generated_contract_wrappers/test_asset_proxy_dispatcher';
import { artifacts } from '../../src/utils/artifacts';
import { expectRevertReasonOrAlwaysFailingTransactionAsync } from '../../src/utils/assertions';
import { chaiSetup } from '../../src/utils/chai_setup';
import { constants } from '../../src/utils/constants';
import { ERC20Wrapper } from '../../src/utils/erc20_wrapper';
import { ERC721Wrapper } from '../../src/utils/erc721_wrapper';
import { LogDecoder } from '../../src/utils/log_decoder';
import { provider, txDefaults, web3Wrapper } from '../../src/utils/web3_wrapper';
chaiSetup.configure();
const expect = chai.expect;
const blockchainLifecycle = new BlockchainLifecycle(web3Wrapper);
// tslint:disable:no-unnecessary-type-assertion
describe('AssetProxyDispatcher', () => {
let owner: string;
let notOwner: string;
@@ -82,14 +87,8 @@ describe('AssetProxyDispatcher', () => {
});
describe('registerAssetProxy', () => {
it('should record proxy upon registration', async () => {
const prevProxyAddress = constants.NULL_ADDRESS;
await web3Wrapper.awaitTransactionSuccessAsync(
await assetProxyDispatcher.registerAssetProxy.sendTransactionAsync(
AssetProxyId.ERC20,
erc20Proxy.address,
prevProxyAddress,
{ from: owner },
),
await assetProxyDispatcher.registerAssetProxy.sendTransactionAsync(erc20Proxy.address, { from: owner }),
constants.AWAIT_TRANSACTION_MINED_MS,
);
const proxyAddress = await assetProxyDispatcher.getAssetProxy.callAsync(AssetProxyId.ERC20);
@@ -98,46 +97,30 @@ describe('AssetProxyDispatcher', () => {
it('should be able to record multiple proxies', async () => {
// Record first proxy
const prevERC20ProxyAddress = constants.NULL_ADDRESS;
await web3Wrapper.awaitTransactionSuccessAsync(
await assetProxyDispatcher.registerAssetProxy.sendTransactionAsync(
AssetProxyId.ERC20,
erc20Proxy.address,
prevERC20ProxyAddress,
{ from: owner },
),
await assetProxyDispatcher.registerAssetProxy.sendTransactionAsync(erc20Proxy.address, { from: owner }),
constants.AWAIT_TRANSACTION_MINED_MS,
);
let proxyAddress = await assetProxyDispatcher.getAssetProxy.callAsync(AssetProxyId.ERC20);
expect(proxyAddress).to.be.equal(erc20Proxy.address);
// Record another proxy
const prevERC721ProxyAddress = constants.NULL_ADDRESS;
await web3Wrapper.awaitTransactionSuccessAsync(
await assetProxyDispatcher.registerAssetProxy.sendTransactionAsync(
AssetProxyId.ERC721,
erc721Proxy.address,
prevERC721ProxyAddress,
{ from: owner },
),
await assetProxyDispatcher.registerAssetProxy.sendTransactionAsync(erc721Proxy.address, {
from: owner,
}),
constants.AWAIT_TRANSACTION_MINED_MS,
);
proxyAddress = await assetProxyDispatcher.getAssetProxy.callAsync(AssetProxyId.ERC721);
expect(proxyAddress).to.be.equal(erc721Proxy.address);
});
it('should replace proxy address upon re-registration', async () => {
it('should throw if a proxy with the same id is already registered', async () => {
// Initial registration
const prevProxyAddress = constants.NULL_ADDRESS;
await web3Wrapper.awaitTransactionSuccessAsync(
await assetProxyDispatcher.registerAssetProxy.sendTransactionAsync(
AssetProxyId.ERC20,
erc20Proxy.address,
prevProxyAddress,
{ from: owner },
),
await assetProxyDispatcher.registerAssetProxy.sendTransactionAsync(erc20Proxy.address, { from: owner }),
constants.AWAIT_TRANSACTION_MINED_MS,
);
let proxyAddress = await assetProxyDispatcher.getAssetProxy.callAsync(AssetProxyId.ERC20);
const proxyAddress = await assetProxyDispatcher.getAssetProxy.callAsync(AssetProxyId.ERC20);
expect(proxyAddress).to.be.equal(erc20Proxy.address);
// Deploy a new version of the ERC20 Transfer Proxy contract
const newErc20TransferProxy = await ERC20ProxyContract.deployFrom0xArtifactAsync(
@@ -146,114 +129,37 @@ describe('AssetProxyDispatcher', () => {
txDefaults,
);
// Register new ERC20 Transfer Proxy contract
const newAddress = newErc20TransferProxy.address;
const currentAddress = erc20Proxy.address;
await web3Wrapper.awaitTransactionSuccessAsync(
await assetProxyDispatcher.registerAssetProxy.sendTransactionAsync(
AssetProxyId.ERC20,
newAddress,
currentAddress,
{ from: owner },
),
constants.AWAIT_TRANSACTION_MINED_MS,
);
// Verify new asset proxy has replaced initial version
proxyAddress = await assetProxyDispatcher.getAssetProxy.callAsync(AssetProxyId.ERC20);
expect(proxyAddress).to.be.equal(newAddress);
});
it('should throw if registering with incorrect "currentAssetProxyAddress" field', async () => {
// Initial registration
const prevProxyAddress = constants.NULL_ADDRESS;
await web3Wrapper.awaitTransactionSuccessAsync(
await assetProxyDispatcher.registerAssetProxy.sendTransactionAsync(
AssetProxyId.ERC20,
erc20Proxy.address,
prevProxyAddress,
{ from: owner },
),
constants.AWAIT_TRANSACTION_MINED_MS,
);
const proxyAddress = await assetProxyDispatcher.getAssetProxy.callAsync(AssetProxyId.ERC20);
expect(proxyAddress).to.be.equal(erc20Proxy.address);
// The following transaction will throw because the currentAddress is no longer constants.NULL_ADDRESS
return expectRevertReasonOrAlwaysFailingTransactionAsync(
assetProxyDispatcher.registerAssetProxy.sendTransactionAsync(
AssetProxyId.ERC20,
erc20Proxy.address,
constants.NULL_ADDRESS,
{ from: owner },
),
RevertReason.AssetProxyMismatch,
assetProxyDispatcher.registerAssetProxy.sendTransactionAsync(newErc20TransferProxy.address, {
from: owner,
}),
RevertReason.AssetProxyAlreadyExists,
);
});
it('should be able to reset proxy address to NULL', async () => {
// Initial registration
const prevProxyAddress = constants.NULL_ADDRESS;
await web3Wrapper.awaitTransactionSuccessAsync(
await assetProxyDispatcher.registerAssetProxy.sendTransactionAsync(
AssetProxyId.ERC20,
erc20Proxy.address,
prevProxyAddress,
{ from: owner },
),
constants.AWAIT_TRANSACTION_MINED_MS,
);
const proxyAddress = await assetProxyDispatcher.getAssetProxy.callAsync(AssetProxyId.ERC20);
expect(proxyAddress).to.be.equal(erc20Proxy.address);
// The following transaction will reset the proxy address
const newProxyAddress = constants.NULL_ADDRESS;
await web3Wrapper.awaitTransactionSuccessAsync(
await assetProxyDispatcher.registerAssetProxy.sendTransactionAsync(
AssetProxyId.ERC20,
newProxyAddress,
erc20Proxy.address,
{ from: owner },
),
constants.AWAIT_TRANSACTION_MINED_MS,
);
const finalProxyAddress = await assetProxyDispatcher.getAssetProxy.callAsync(AssetProxyId.ERC20);
expect(finalProxyAddress).to.be.equal(newProxyAddress);
});
it('should throw if requesting address is not owner', async () => {
const prevProxyAddress = constants.NULL_ADDRESS;
return expectRevertReasonOrAlwaysFailingTransactionAsync(
assetProxyDispatcher.registerAssetProxy.sendTransactionAsync(
AssetProxyId.ERC20,
erc20Proxy.address,
prevProxyAddress,
{ from: notOwner },
),
assetProxyDispatcher.registerAssetProxy.sendTransactionAsync(erc20Proxy.address, { from: notOwner }),
RevertReason.OnlyContractOwner,
);
});
it('should throw if attempting to register a proxy to the incorrect id', async () => {
const prevProxyAddress = constants.NULL_ADDRESS;
return expectRevertReasonOrAlwaysFailingTransactionAsync(
assetProxyDispatcher.registerAssetProxy.sendTransactionAsync(
AssetProxyId.ERC721,
erc20Proxy.address,
prevProxyAddress,
{ from: owner },
),
RevertReason.AssetProxyIdMismatch,
it('should log an event with correct arguments when an asset proxy is registered', async () => {
const logDecoder = new LogDecoder(web3Wrapper, assetProxyDispatcher.address);
const txReceipt = await logDecoder.getTxWithDecodedLogsAsync(
await assetProxyDispatcher.registerAssetProxy.sendTransactionAsync(erc20Proxy.address, { from: owner }),
);
const logs = txReceipt.logs;
const log = logs[0] as LogWithDecodedArgs<AssetProxyRegisteredContractEventArgs>;
expect(log.args.id).to.equal(AssetProxyId.ERC20);
expect(log.args.assetProxy).to.equal(erc20Proxy.address);
});
});
describe('getAssetProxy', () => {
it('should return correct address of registered proxy', async () => {
const prevProxyAddress = constants.NULL_ADDRESS;
await web3Wrapper.awaitTransactionSuccessAsync(
await assetProxyDispatcher.registerAssetProxy.sendTransactionAsync(
AssetProxyId.ERC20,
erc20Proxy.address,
prevProxyAddress,
{ from: owner },
),
await assetProxyDispatcher.registerAssetProxy.sendTransactionAsync(erc20Proxy.address, { from: owner }),
constants.AWAIT_TRANSACTION_MINED_MS,
);
const proxyAddress = await assetProxyDispatcher.getAssetProxy.callAsync(AssetProxyId.ERC20);
@@ -269,14 +175,8 @@ describe('AssetProxyDispatcher', () => {
describe('dispatchTransferFrom', () => {
it('should dispatch transfer to registered proxy', async () => {
// Register ERC20 proxy
const prevProxyAddress = constants.NULL_ADDRESS;
await web3Wrapper.awaitTransactionSuccessAsync(
await assetProxyDispatcher.registerAssetProxy.sendTransactionAsync(
AssetProxyId.ERC20,
erc20Proxy.address,
prevProxyAddress,
{ from: owner },
),
await assetProxyDispatcher.registerAssetProxy.sendTransactionAsync(erc20Proxy.address, { from: owner }),
constants.AWAIT_TRANSACTION_MINED_MS,
);
// Construct metadata for ERC20 proxy
@@ -323,3 +223,4 @@ describe('AssetProxyDispatcher', () => {
});
});
});
// tslint:enable:no-unnecessary-type-assertion

View File

@@ -1,6 +1,6 @@
import { BlockchainLifecycle } from '@0xproject/dev-utils';
import { assetProxyUtils } from '@0xproject/order-utils';
import { AssetProxyId, RevertReason } from '@0xproject/types';
import { RevertReason } from '@0xproject/types';
import { BigNumber } from '@0xproject/utils';
import { Web3Wrapper } from '@0xproject/web3-wrapper';
import * as chai from 'chai';
@@ -103,8 +103,8 @@ describe('matchOrders', () => {
assetProxyUtils.encodeERC20AssetData(zrxToken.address),
);
exchangeWrapper = new ExchangeWrapper(exchange, provider);
await exchangeWrapper.registerAssetProxyAsync(AssetProxyId.ERC20, erc20Proxy.address, owner);
await exchangeWrapper.registerAssetProxyAsync(AssetProxyId.ERC721, erc721Proxy.address, owner);
await exchangeWrapper.registerAssetProxyAsync(erc20Proxy.address, owner);
await exchangeWrapper.registerAssetProxyAsync(erc721Proxy.address, owner);
// Authorize ERC20 and ERC721 trades by exchange
await web3Wrapper.awaitTransactionSuccessAsync(
await erc20Proxy.addAuthorizedAddress.sendTransactionAsync(exchange.address, {

View File

@@ -1,6 +1,6 @@
import { BlockchainLifecycle } from '@0xproject/dev-utils';
import { assetProxyUtils, generatePseudoRandomSalt } from '@0xproject/order-utils';
import { AssetProxyId, OrderWithoutExchangeAddress, RevertReason, SignedOrder } from '@0xproject/types';
import { OrderWithoutExchangeAddress, RevertReason, SignedOrder } from '@0xproject/types';
import { BigNumber } from '@0xproject/utils';
import * as chai from 'chai';
import * as _ from 'lodash';
@@ -91,7 +91,7 @@ describe('Exchange transactions', () => {
assetProxyUtils.encodeERC20AssetData(zrxToken.address),
);
exchangeWrapper = new ExchangeWrapper(exchange, provider);
await exchangeWrapper.registerAssetProxyAsync(AssetProxyId.ERC20, erc20Proxy.address, owner);
await exchangeWrapper.registerAssetProxyAsync(erc20Proxy.address, owner);
await web3Wrapper.awaitTransactionSuccessAsync(
await erc20Proxy.addAuthorizedAddress.sendTransactionAsync(exchange.address, { from: owner }),

View File

@@ -1,6 +1,6 @@
import { BlockchainLifecycle } from '@0xproject/dev-utils';
import { assetProxyUtils } from '@0xproject/order-utils';
import { AssetProxyId, RevertReason, SignedOrder } from '@0xproject/types';
import { RevertReason, SignedOrder } from '@0xproject/types';
import { BigNumber } from '@0xproject/utils';
import { Web3Wrapper } from '@0xproject/web3-wrapper';
import * as chai from 'chai';
@@ -87,8 +87,8 @@ describe('Exchange wrappers', () => {
assetProxyUtils.encodeERC20AssetData(zrxToken.address),
);
exchangeWrapper = new ExchangeWrapper(exchange, provider);
await exchangeWrapper.registerAssetProxyAsync(AssetProxyId.ERC20, erc20Proxy.address, owner);
await exchangeWrapper.registerAssetProxyAsync(AssetProxyId.ERC721, erc721Proxy.address, owner);
await exchangeWrapper.registerAssetProxyAsync(erc20Proxy.address, owner);
await exchangeWrapper.registerAssetProxyAsync(erc721Proxy.address, owner);
await web3Wrapper.awaitTransactionSuccessAsync(
await erc20Proxy.addAuthorizedAddress.sendTransactionAsync(exchange.address, {

View File

@@ -187,8 +187,7 @@ export enum RevertReason {
InvalidTxHash = 'INVALID_TX_HASH',
InvalidTxSignature = 'INVALID_TX_SIGNATURE',
FailedExecution = 'FAILED_EXECUTION',
AssetProxyMismatch = 'ASSET_PROXY_MISMATCH',
AssetProxyIdMismatch = 'ASSET_PROXY_ID_MISMATCH',
AssetProxyAlreadyExists = 'ASSET_PROXY_ALREADY_EXISTS',
LengthGreaterThan0Required = 'LENGTH_GREATER_THAN_0_REQUIRED',
LengthGreaterThan131Required = 'LENGTH_GREATER_THAN_131_REQUIRED',
Length0Required = 'LENGTH_0_REQUIRED',