Fix expiration watcher comparator

6 files changed, 71 insertions, 11 deletions

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,5 +1,14 @@
 [
     {
+        "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);
+});