From 25744056993878392b67796286b4b41fda3489f5 Mon Sep 17 00:00:00 2001 From: Leonid Logvinov Date: Mon, 16 Apr 2018 16:21:02 +0200 Subject: Fix expiration watcher comparator --- packages/0x.js/CHANGELOG.json | 9 +++++ packages/0x.js/package.json | 4 +-- .../0x.js/src/order_watcher/expiration_watcher.ts | 13 ++++++-- packages/0x.js/test/0x.js_test.ts | 7 ---- packages/0x.js/test/expiration_watcher_test.ts | 39 ++++++++++++++++++++++ packages/0x.js/test/global_hooks.ts | 10 ++++++ packages/migrations/src/migration.ts | 2 +- 7 files changed, 72 insertions(+), 12 deletions(-) create mode 100644 packages/0x.js/test/global_hooks.ts diff --git a/packages/0x.js/CHANGELOG.json b/packages/0x.js/CHANGELOG.json index 8413e7da8..12bfbe0a3 100644 --- a/packages/0x.js/CHANGELOG.json +++ b/packages/0x.js/CHANGELOG.json @@ -1,4 +1,13 @@ [ + { + "version": "0.36.2", + "changes": [ + { + "note": "Fixed expiration watcher comparator to handle orders with equal expiration times", + "pr": 526 + } + ] + }, { "version": "0.36.1", "changes": [ diff --git a/packages/0x.js/package.json b/packages/0x.js/package.json index f47c2af92..4715c1536 100644 --- a/packages/0x.js/package.json +++ b/packages/0x.js/package.json @@ -26,7 +26,7 @@ "build:umd:prod": "NODE_ENV=production webpack", "build:commonjs": "tsc && yarn update_artifacts && copyfiles -u 2 './src/compact_artifacts/**/*.json' ./lib/src/compact_artifacts && copyfiles -u 3 './lib/src/monorepo_scripts/**/*' ./scripts", "test:commonjs": "run-s build:commonjs run_mocha", - "run_mocha": "mocha lib/test/**/*_test.js --timeout 10000 --bail --exit", + "run_mocha": "mocha lib/test/**/*_test.js lib/test/global_hooks.js --timeout 10000 --bail --exit", "manual:postpublish": "yarn build; node ./scripts/postpublish.js", "docs:stage": "yarn build && node ./scripts/stage_docs.js", "docs:json": "typedoc --excludePrivate --excludeExternals --target ES5 --json $JSON_FILE_PATH $PROJECT_FILES", @@ -115,7 +115,7 @@ "web3": "^0.20.0" }, "optionalDependencies": { - "@0xproject/migrations": "^0.0.1" + "@0xproject/migrations": "^0.0.2" }, "publishConfig": { "access": "public" diff --git a/packages/0x.js/src/order_watcher/expiration_watcher.ts b/packages/0x.js/src/order_watcher/expiration_watcher.ts index 8b306bf3b..27ec7107d 100644 --- a/packages/0x.js/src/order_watcher/expiration_watcher.ts +++ b/packages/0x.js/src/order_watcher/expiration_watcher.ts @@ -22,8 +22,17 @@ export class ExpirationWatcher { this._expirationMarginMs = expirationMarginIfExistsMs || DEFAULT_EXPIRATION_MARGIN_MS; this._orderExpirationCheckingIntervalMs = expirationMarginIfExistsMs || DEFAULT_ORDER_EXPIRATION_CHECKING_INTERVAL_MS; - const scoreFunction = (orderHash: string) => this._expiration[orderHash].toNumber(); - const comparator = (lhs: string, rhs: string) => scoreFunction(lhs) - scoreFunction(rhs); + const comparator = (lhsOrderHash: string, rhsOrderHash: string) => { + const lhsExpiration = this._expiration[lhsOrderHash].toNumber(); + const rhsExpiration = this._expiration[rhsOrderHash].toNumber(); + if (lhsExpiration !== rhsExpiration) { + return lhsExpiration - rhsExpiration; + } else { + // HACK: If two orders have identical expirations, the order in which they are emitted by the + // ExpirationWatcher does not matter, so we emit them in alphabetical order by orderHash. + return lhsOrderHash.localeCompare(rhsOrderHash); + } + }; this._orderHashByExpirationRBTree = new RBTree(comparator); } public subscribe(callback: (orderHash: string) => void): void { diff --git a/packages/0x.js/test/0x.js_test.ts b/packages/0x.js/test/0x.js_test.ts index de5a6be58..838ee7080 100644 --- a/packages/0x.js/test/0x.js_test.ts +++ b/packages/0x.js/test/0x.js_test.ts @@ -1,9 +1,4 @@ -import { Deployer } from '@0xproject/deployer'; import { BlockchainLifecycle, devConstants, web3Factory } from '@0xproject/dev-utils'; -// HACK: This dependency is optional since it is only available when run from within -// the monorepo. tslint doesn't handle optional dependencies -// tslint:disable-next-line:no-implicit-dependencies -import { runMigrationsAsync } from '@0xproject/migrations'; import { BigNumber } from '@0xproject/utils'; import * as chai from 'chai'; import * as _ from 'lodash'; @@ -15,7 +10,6 @@ import { ApprovalContractEventArgs, LogWithDecodedArgs, Order, TokenEvents, Zero import { chaiSetup } from './utils/chai_setup'; import { constants } from './utils/constants'; -import { deployer } from './utils/deployer'; import { TokenUtils } from './utils/token_utils'; import { provider, web3Wrapper } from './utils/web3_wrapper'; @@ -28,7 +22,6 @@ const SHOULD_ADD_PERSONAL_MESSAGE_PREFIX = false; describe('ZeroEx library', () => { let zeroEx: ZeroEx; before(async () => { - await runMigrationsAsync(deployer); const config = { networkId: constants.TESTRPC_NETWORK_ID, }; diff --git a/packages/0x.js/test/expiration_watcher_test.ts b/packages/0x.js/test/expiration_watcher_test.ts index 29b111fa3..1b022539a 100644 --- a/packages/0x.js/test/expiration_watcher_test.ts +++ b/packages/0x.js/test/expiration_watcher_test.ts @@ -153,4 +153,43 @@ describe('ExpirationWatcher', () => { timer.tick(order2Lifetime * 1000); })().catch(done); }); + it('emits events in correct order when expirations are equal', (done: DoneCallback) => { + (async () => { + const order1Lifetime = 60; + const order2Lifetime = 60; + const order1ExpirationUnixTimestampSec = currentUnixTimestampSec.plus(order1Lifetime); + const order2ExpirationUnixTimestampSec = currentUnixTimestampSec.plus(order2Lifetime); + const signedOrder1 = await fillScenarios.createFillableSignedOrderAsync( + makerTokenAddress, + takerTokenAddress, + makerAddress, + takerAddress, + fillableAmount, + order1ExpirationUnixTimestampSec, + ); + const signedOrder2 = await fillScenarios.createFillableSignedOrderAsync( + makerTokenAddress, + takerTokenAddress, + makerAddress, + takerAddress, + fillableAmount, + order2ExpirationUnixTimestampSec, + ); + const orderHash1 = ZeroEx.getOrderHashHex(signedOrder1); + const orderHash2 = ZeroEx.getOrderHashHex(signedOrder2); + expirationWatcher.addOrder(orderHash1, signedOrder1.expirationUnixTimestampSec.times(1000)); + expirationWatcher.addOrder(orderHash2, signedOrder2.expirationUnixTimestampSec.times(1000)); + const expirationOrder = orderHash1 < orderHash2 ? [orderHash1, orderHash2] : [orderHash2, orderHash1]; + const expectToBeCalledOnce = false; + const callbackAsync = reportNoErrorCallbackErrors(done, expectToBeCalledOnce)((hash: string) => { + const orderHash = expirationOrder.shift(); + expect(hash).to.be.equal(orderHash); + if (_.isEmpty(expirationOrder)) { + done(); + } + }); + expirationWatcher.subscribe(callbackAsync); + timer.tick(order2Lifetime * 1000); + })().catch(done); + }); }); diff --git a/packages/0x.js/test/global_hooks.ts b/packages/0x.js/test/global_hooks.ts new file mode 100644 index 000000000..d5062a3a1 --- /dev/null +++ b/packages/0x.js/test/global_hooks.ts @@ -0,0 +1,10 @@ +// HACK: This dependency is optional since it is only available when run from within +// the monorepo. tslint doesn't handle optional dependencies +// tslint:disable-next-line:no-implicit-dependencies +import { runMigrationsAsync } from '@0xproject/migrations'; + +import { deployer } from './utils/deployer'; + +before('migrate contracts', async () => { + await runMigrationsAsync(deployer); +}); diff --git a/packages/migrations/src/migration.ts b/packages/migrations/src/migration.ts index 4827328fc..6313efcff 100644 --- a/packages/migrations/src/migration.ts +++ b/packages/migrations/src/migration.ts @@ -69,7 +69,7 @@ export const runMigrationsAsync = async (deployer: Deployer) => { }, ); for (const token of tokenInfo) { - const totalSupply = new BigNumber(0); + const totalSupply = new BigNumber(100000000000000000000); const args = [token.name, token.symbol, token.decimals, totalSupply]; const dummyToken = await deployer.deployAsync(ContractName.DummyToken, args); await tokenReg.addToken.sendTransactionAsync( -- cgit From 1ca86730fac5d264dcde3260dc2991b2695ad734 Mon Sep 17 00:00:00 2001 From: Fabio Berger Date: Thu, 19 Apr 2018 10:37:13 +0900 Subject: Remove hack above migrations package --- packages/0x.js/test/global_hooks.ts | 3 --- 1 file changed, 3 deletions(-) diff --git a/packages/0x.js/test/global_hooks.ts b/packages/0x.js/test/global_hooks.ts index d5062a3a1..e3c986524 100644 --- a/packages/0x.js/test/global_hooks.ts +++ b/packages/0x.js/test/global_hooks.ts @@ -1,6 +1,3 @@ -// HACK: This dependency is optional since it is only available when run from within -// the monorepo. tslint doesn't handle optional dependencies -// tslint:disable-next-line:no-implicit-dependencies import { runMigrationsAsync } from '@0xproject/migrations'; import { deployer } from './utils/deployer'; -- cgit