@0x/contracts-zero-ex: Address review feedback.

`@0x/contracts-zero-ex`: Add target implementation address to `rollback()`.
`@0x/contracts-zero-ex`: Add storage ID uniqueness test.
`@0x/contracts-zero-ex`: Add rollback history querying functions to `SimpleFunctionRegistry`.
This commit is contained in:
Lawrence Forman
2020-04-15 13:11:57 -04:00
parent 223aa04424
commit 72908b02fe
9 changed files with 197 additions and 49 deletions

View File

@@ -1,5 +1,5 @@
import { blockchainTests, constants, expect, randomAddress, verifyEventsFromLogs } from '@0x/contracts-test-utils';
import { hexUtils, OwnableRevertErrors, ZeroExRevertErrors } from '@0x/utils';
import { BigNumber, hexUtils, OwnableRevertErrors, ZeroExRevertErrors } from '@0x/utils';
import { artifacts } from '../artifacts';
import { basicMigrateAsync } from '../utils/migration';
@@ -9,12 +9,14 @@ import {
ITestSimpleFunctionRegistryFeatureContract,
TestSimpleFunctionRegistryFeatureImpl1Contract,
TestSimpleFunctionRegistryFeatureImpl2Contract,
ZeroExContract,
} from '../wrappers';
blockchainTests.resets('SimpleFunctionRegistry feature', env => {
const { NULL_ADDRESS } = constants;
const notOwner = randomAddress();
let owner: string;
let zeroEx: ZeroExContract;
let registry: ISimpleFunctionRegistryContract;
let testFnSelector: string;
let testFeature: ITestSimpleFunctionRegistryFeatureContract;
@@ -23,7 +25,7 @@ blockchainTests.resets('SimpleFunctionRegistry feature', env => {
before(async () => {
[owner] = await env.getAccountAddressesAsync();
const zeroEx = await basicMigrateAsync(owner, env.provider, env.txDefaults);
zeroEx = await basicMigrateAsync(owner, env.provider, env.txDefaults);
registry = new ISimpleFunctionRegistryContract(zeroEx.address, env.provider, {
...env.txDefaults,
from: owner,
@@ -50,17 +52,24 @@ blockchainTests.resets('SimpleFunctionRegistry feature', env => {
});
it('`rollback()` cannot be called by a non-owner', async () => {
const tx = registry.rollback(hexUtils.random(4)).callAsync({ from: notOwner });
const tx = registry.rollback(hexUtils.random(4), NULL_ADDRESS).callAsync({ from: notOwner });
return expect(tx).to.revertWith(new OwnableRevertErrors.OnlyOwnerError(notOwner, owner));
});
it('`rollback()` reverts for unregistered function', async () => {
const tx = registry.rollback(testFnSelector).awaitTransactionSuccessAsync();
it('`rollback()` to non-zero impl reverts for unregistered function', async () => {
const rollbackAddress = randomAddress();
const tx = registry.rollback(testFnSelector, rollbackAddress).awaitTransactionSuccessAsync();
return expect(tx).to.revertWith(
new ZeroExRevertErrors.SimpleFunctionRegistry.NoRollbackHistoryError(testFnSelector),
new ZeroExRevertErrors.SimpleFunctionRegistry.NotInRollbackHistoryError(testFnSelector, rollbackAddress),
);
});
it('`rollback()` to zero impl succeeds for unregistered function', async () => {
await registry.rollback(testFnSelector, NULL_ADDRESS).awaitTransactionSuccessAsync();
const impl = await zeroEx.getFunctionImplementation(testFnSelector).callAsync();
expect(impl).to.eq(NULL_ADDRESS);
});
it('owner can add a new function with `extend()`', async () => {
const { logs } = await registry.extend(testFnSelector, testFeatureImpl1.address).awaitTransactionSuccessAsync();
verifyEventsFromLogs(
@@ -79,7 +88,7 @@ blockchainTests.resets('SimpleFunctionRegistry feature', env => {
expect(r).to.bignumber.eq(1338);
});
it('owner can unset a function with `extend()`', async () => {
it('owner can zero a function with `extend()`', async () => {
await registry.extend(testFnSelector, testFeatureImpl1.address).awaitTransactionSuccessAsync();
await registry.extend(testFnSelector, constants.NULL_ADDRESS).awaitTransactionSuccessAsync();
return expect(testFeature.testFn().callAsync()).to.revertWith(
@@ -87,32 +96,77 @@ blockchainTests.resets('SimpleFunctionRegistry feature', env => {
);
});
it('owner can rollback a new function to unset', async () => {
await registry.extend(testFnSelector, testFeatureImpl1.address).awaitTransactionSuccessAsync();
const { logs } = await registry.rollback(testFnSelector).awaitTransactionSuccessAsync();
verifyEventsFromLogs(
logs,
[{ selector: testFnSelector, oldImpl: testFeatureImpl1.address, newImpl: NULL_ADDRESS }],
ISimpleFunctionRegistryEvents.ProxyFunctionUpdated,
);
return expect(testFeature.testFn().callAsync()).to.revertWith(
new ZeroExRevertErrors.Proxy.NotImplementedError(testFnSelector),
);
});
it('owner can rollback a function to a previous version', async () => {
it('can query rollback history', async () => {
await registry.extend(testFnSelector, testFeatureImpl1.address).awaitTransactionSuccessAsync();
await registry.extend(testFnSelector, testFeatureImpl2.address).awaitTransactionSuccessAsync();
await registry.rollback(testFnSelector).awaitTransactionSuccessAsync();
const r = await testFeature.testFn().callAsync();
expect(r).to.bignumber.eq(1337);
await registry.extend(testFnSelector, NULL_ADDRESS).awaitTransactionSuccessAsync();
const rollbackLength = await registry.getRollbackLength(testFnSelector).callAsync();
expect(rollbackLength).to.bignumber.eq(3);
const entries = await Promise.all(
[...new Array(rollbackLength.toNumber())].map((v, i) =>
registry.getRollbackEntryAtIndex(testFnSelector, new BigNumber(i)).callAsync(),
),
);
expect(entries).to.deep.eq([NULL_ADDRESS, testFeatureImpl1.address, testFeatureImpl2.address]);
});
it('owner can rollback an unset function to a previous version', async () => {
it('owner can rollback a function to zero', async () => {
await registry.extend(testFnSelector, testFeatureImpl1.address).awaitTransactionSuccessAsync();
await registry.extend(testFnSelector, constants.NULL_ADDRESS).awaitTransactionSuccessAsync();
await registry.rollback(testFnSelector).awaitTransactionSuccessAsync();
await registry.extend(testFnSelector, testFeatureImpl2.address).awaitTransactionSuccessAsync();
const { logs } = await registry.rollback(testFnSelector, NULL_ADDRESS).awaitTransactionSuccessAsync();
verifyEventsFromLogs(
logs,
[{ selector: testFnSelector, oldImpl: testFeatureImpl2.address, newImpl: NULL_ADDRESS }],
ISimpleFunctionRegistryEvents.ProxyFunctionUpdated,
);
const rollbackLength = await registry.getRollbackLength(testFnSelector).callAsync();
expect(rollbackLength).to.bignumber.eq(0);
return expect(testFeature.testFn().callAsync()).to.revertWith(
new ZeroExRevertErrors.Proxy.NotImplementedError(testFnSelector),
);
});
it('owner can rollback a function to the prior version', async () => {
await registry.extend(testFnSelector, testFeatureImpl1.address).awaitTransactionSuccessAsync();
await registry.extend(testFnSelector, testFeatureImpl2.address).awaitTransactionSuccessAsync();
await registry.rollback(testFnSelector, testFeatureImpl1.address).awaitTransactionSuccessAsync();
const r = await testFeature.testFn().callAsync();
expect(r).to.bignumber.eq(1337);
const rollbackLength = await registry.getRollbackLength(testFnSelector).callAsync();
expect(rollbackLength).to.bignumber.eq(1);
});
it('owner can rollback a zero function to the prior version', async () => {
await registry.extend(testFnSelector, testFeatureImpl2.address).awaitTransactionSuccessAsync();
await registry.extend(testFnSelector, testFeatureImpl1.address).awaitTransactionSuccessAsync();
await registry.extend(testFnSelector, constants.NULL_ADDRESS).awaitTransactionSuccessAsync();
await registry.rollback(testFnSelector, testFeatureImpl1.address).awaitTransactionSuccessAsync();
const r = await testFeature.testFn().callAsync();
expect(r).to.bignumber.eq(1337);
const rollbackLength = await registry.getRollbackLength(testFnSelector).callAsync();
expect(rollbackLength).to.bignumber.eq(2);
});
it('owner can rollback a function to a much older version', async () => {
await registry.extend(testFnSelector, testFeatureImpl1.address).awaitTransactionSuccessAsync();
await registry.extend(testFnSelector, NULL_ADDRESS).awaitTransactionSuccessAsync();
await registry.extend(testFnSelector, testFeatureImpl2.address).awaitTransactionSuccessAsync();
await registry.rollback(testFnSelector, testFeatureImpl1.address).awaitTransactionSuccessAsync();
const r = await testFeature.testFn().callAsync();
expect(r).to.bignumber.eq(1337);
const rollbackLength = await registry.getRollbackLength(testFnSelector).callAsync();
expect(rollbackLength).to.bignumber.eq(1);
});
it('owner cannot rollback a function to a version not in history', async () => {
await registry.extend(testFnSelector, NULL_ADDRESS).awaitTransactionSuccessAsync();
await registry.extend(testFnSelector, testFeatureImpl2.address).awaitTransactionSuccessAsync();
const tx = registry.rollback(testFnSelector, testFeatureImpl1.address).awaitTransactionSuccessAsync();
return expect(tx).to.revertWith(
new ZeroExRevertErrors.SimpleFunctionRegistry.NotInRollbackHistoryError(
testFnSelector,
testFeatureImpl1.address,
),
);
});
});

View File

@@ -0,0 +1,33 @@
import { describe } from '@0x/contracts-test-utils';
import { readdir, readFile } from 'fs';
import { basename, resolve } from 'path';
import { promisify } from 'util';
describe('Storage ID uniqueness test', () => {
const STORAGE_SOURCES_DIR = resolve(__dirname, '../../contracts/src/storage');
async function findStorageIdFromSourceFileAsync(path: string): Promise<string | void> {
const contents = await promisify(readFile)(path, { encoding: 'utf-8' });
const m = /STORAGE_ID\s*=\s*(0x[a-fA-F0-9]{64})/m.exec(contents);
if (m) {
return m[1];
}
}
it('all STORAGE_IDs are unique in storage libraries', async () => {
const sourcePaths = (await promisify(readdir)(STORAGE_SOURCES_DIR))
.filter(p => p.endsWith('.sol'))
.map(p => resolve(STORAGE_SOURCES_DIR, p));
const storageIds = await Promise.all(sourcePaths.map(async p => findStorageIdFromSourceFileAsync(p)));
for (let i = 0; i < storageIds.length; ++i) {
const storageId = storageIds[i];
for (let j = 0; j < storageIds.length; ++j) {
if (i !== j && storageId === storageIds[j]) {
throw new Error(
`Found duplicate STORAGE_ID ${storageId} ` +
`in files ${basename(sourcePaths[i])}, ${basename(sourcePaths[j])}`,
);
}
}
}
});
});

View File

@@ -50,13 +50,13 @@ blockchainTests.resets('ZeroEx contract', env => {
});
it('can attach ether to a call', async () => {
const wei = Math.floor(Math.random() * 100);
const wei = Math.floor(Math.random() * 100 + 1);
const receipt = await testFeature.payableFn().awaitTransactionSuccessAsync({ value: wei });
verifyEventsFromLogs(receipt.logs, [{ value: new BigNumber(wei) }], TestZeroExFeatureEvents.PayableFnCalled);
});
it('reverts when attaching ether to a non-payable function', async () => {
const wei = Math.floor(Math.random() * 100);
const wei = Math.floor(Math.random() * 100 + 1);
const tx = testFeature.notPayableFn().awaitTransactionSuccessAsync({ value: wei });
// This will cause an empty revert.
return expect(tx).to.be.rejectedWith('revert');