Merge pull request #2696 from 0xProject/bugfix/quote-report-generator-signature

Quote report generator: use signature instead of order hash for lookup

packages/asset-swapper/src/utils/quote_report_generator.ts

@@ -1,4 +1,3 @@
-import { orderHashUtils } from '@0x/order-utils';
 import { SignedOrder } from '@0x/types';
 import { BigNumber } from '@0x/utils';
 import * as _ from 'lodash';
@@ -31,7 +30,6 @@ interface NativeReportSourceBase {
     liquiditySource: ERC20BridgeSource.Native;
     makerAmount: BigNumber;
     takerAmount: BigNumber;
-    orderHash: string;
     nativeOrder: SignedOrder;
     fillableTakerAmount: BigNumber;
 }
@@ -53,17 +51,6 @@ export interface QuoteReport {
     sourcesDelivered: QuoteReportSource[];
 }
 
-const nativeOrderFromCollapsedFill = (cf: CollapsedFill): SignedOrder | undefined => {
-    // Cast as NativeCollapsedFill and then check
-    // if it really is a NativeCollapsedFill
-    const possibleNativeCollapsedFill = cf as NativeCollapsedFill;
-    if (possibleNativeCollapsedFill.fillData && possibleNativeCollapsedFill.fillData.order) {
-        return possibleNativeCollapsedFill.fillData.order;
-    } else {
-        return undefined;
-    }
-};
-
 /**
  * Generates a report of sources considered while computing the optimized
  * swap quote, and the sources ultimately included in the computed quote.
@@ -77,23 +64,9 @@ export function generateQuoteReport(
     liquidityDelivered: CollapsedFill[] | DexSample<MultiHopFillData>,
     quoteRequestor?: QuoteRequestor,
 ): QuoteReport {
-    // convert order fillable amount array to easy to look up hash
-    if (orderFillableAmounts.length !== nativeOrders.length) {
-        // length mismatch, abort
-        throw new Error('orderFillableAmounts must be the same length as nativeOrders');
-    }
-    const orderHashesToFillableAmounts: { [orderHash: string]: BigNumber } = {};
-    nativeOrders.forEach((nativeOrder, idx) => {
-        orderHashesToFillableAmounts[orderHashUtils.getOrderHash(nativeOrder)] = orderFillableAmounts[idx];
-    });
-
     const dexReportSourcesConsidered = dexQuotes.map(quote => _dexSampleToReportSource(quote, marketOperation));
-    const nativeOrderSourcesConsidered = nativeOrders.map(order =>
+    const nativeOrderSourcesConsidered = nativeOrders.map((order, idx) =>
-        _nativeOrderToReportSource(
+        _nativeOrderToReportSource(order, orderFillableAmounts[idx], quoteRequestor),
-            order,
-            orderHashesToFillableAmounts[orderHashUtils.getOrderHash(order)],
-            quoteRequestor,
-        ),
     );
     const multiHopSourcesConsidered = multiHopQuotes.map(quote =>
         _multiHopSampleToReportSource(quote, marketOperation),
@@ -106,12 +79,18 @@ export function generateQuoteReport(
 
     let sourcesDelivered;
     if (Array.isArray(liquidityDelivered)) {
+        // create easy way to look up fillable amounts
+        const nativeOrderSignaturesToFillableAmounts = _nativeOrderSignaturesToFillableAmounts(
+            nativeOrders,
+            orderFillableAmounts,
+        );
+        // map sources delivered
         sourcesDelivered = liquidityDelivered.map(collapsedFill => {
-            const foundNativeOrder = nativeOrderFromCollapsedFill(collapsedFill);
+            const foundNativeOrder = _nativeOrderFromCollapsedFill(collapsedFill);
             if (foundNativeOrder) {
                 return _nativeOrderToReportSource(
                     foundNativeOrder,
-                    orderHashesToFillableAmounts[orderHashUtils.getOrderHash(foundNativeOrder)],
+                    nativeOrderSignaturesToFillableAmounts[foundNativeOrder.signature],
                     quoteRequestor,
                 );
             } else {
@@ -179,24 +158,48 @@ function _multiHopSampleToReportSource(
     }
 }
 
+function _nativeOrderSignaturesToFillableAmounts(
+    nativeOrders: SignedOrder[],
+    fillableAmounts: BigNumber[],
+): { [orderSignature: string]: BigNumber } {
+    // create easy way to look up fillable amounts based on native order signatures
+    if (fillableAmounts.length !== nativeOrders.length) {
+        // length mismatch, abort
+        throw new Error('orderFillableAmounts must be the same length as nativeOrders');
+    }
+    const nativeOrderSignaturesToFillableAmounts: { [orderSignature: string]: BigNumber } = {};
+    nativeOrders.forEach((nativeOrder, idx) => {
+        nativeOrderSignaturesToFillableAmounts[nativeOrder.signature] = fillableAmounts[idx];
+    });
+    return nativeOrderSignaturesToFillableAmounts;
+}
+
+function _nativeOrderFromCollapsedFill(cf: CollapsedFill): SignedOrder | undefined {
+    // Cast as NativeCollapsedFill and then check
+    // if it really is a NativeCollapsedFill
+    const possibleNativeCollapsedFill = cf as NativeCollapsedFill;
+    if (possibleNativeCollapsedFill.fillData && possibleNativeCollapsedFill.fillData.order) {
+        return possibleNativeCollapsedFill.fillData.order;
+    } else {
+        return undefined;
+    }
+}
+
 function _nativeOrderToReportSource(
     nativeOrder: SignedOrder,
     fillableAmount: BigNumber,
     quoteRequestor?: QuoteRequestor,
 ): NativeRFQTReportSource | NativeOrderbookReportSource {
-    const orderHash = orderHashUtils.getOrderHash(nativeOrder);
-
     const nativeOrderBase: NativeReportSourceBase = {
         liquiditySource: ERC20BridgeSource.Native,
         makerAmount: nativeOrder.makerAssetAmount,
         takerAmount: nativeOrder.takerAssetAmount,
         fillableTakerAmount: fillableAmount,
         nativeOrder,
-        orderHash,
     };
 
     // if we find this is an rfqt order, label it as such and associate makerUri
-    const foundRfqtMakerUri = quoteRequestor && quoteRequestor.getMakerUriForOrderHash(orderHash);
+    const foundRfqtMakerUri = quoteRequestor && quoteRequestor.getMakerUriForOrderSignature(nativeOrder.signature);
     if (foundRfqtMakerUri) {
         const rfqtSource: NativeRFQTReportSource = {
             ...nativeOrderBase,

packages/asset-swapper/src/utils/quote_requestor.ts

@@ -1,5 +1,5 @@
 import { schemas, SchemaValidator } from '@0x/json-schemas';
-import { assetDataUtils, orderCalculationUtils, orderHashUtils, SignedOrder } from '@0x/order-utils';
+import { assetDataUtils, orderCalculationUtils, SignedOrder } from '@0x/order-utils';
 import { RFQTFirmQuote, RFQTIndicativeQuote, TakerRequest } from '@0x/quote-server';
 import { ERC20AssetData } from '@0x/types';
 import { BigNumber, logUtils } from '@0x/utils';
@@ -106,7 +106,7 @@ export type LogFunction = (obj: object, msg?: string, ...args: any[]) => void;
 
 export class QuoteRequestor {
     private readonly _schemaValidator: SchemaValidator = new SchemaValidator();
-    private readonly _orderHashToMakerUri: { [orderHash: string]: string } = {};
+    private readonly _orderSignatureToMakerUri: { [orderSignature: string]: string } = {};
 
     constructor(
         private readonly _rfqtAssetOfferings: RfqtMakerAssetOfferings,
@@ -196,7 +196,7 @@ export class QuoteRequestor {
             }
 
             // Store makerUri for looking up later
-            this._orderHashToMakerUri[orderHashUtils.getOrderHash(orderWithBigNumberInts)] = firmQuoteResponse.makerUri;
+            this._orderSignatureToMakerUri[orderWithBigNumberInts.signature] = firmQuoteResponse.makerUri;
 
             // Passed all validation, add it to result
             result.push({ signedOrder: orderWithBigNumberInts });
@@ -269,10 +269,10 @@ export class QuoteRequestor {
     }
 
     /**
-     * Given an order hash, returns the makerUri that the order originated from
+     * Given an order signature, returns the makerUri that the order originated from
      */
-    public getMakerUriForOrderHash(orderHash: string): string | undefined {
+    public getMakerUriForOrderSignature(orderSignature: string): string | undefined {
-        return this._orderHashToMakerUri[orderHash];
+        return this._orderSignatureToMakerUri[orderSignature];
     }
 
     private _isValidRfqtIndicativeQuoteResponse(response: RFQTIndicativeQuote): boolean {

packages/asset-swapper/test/quote_report_generator_test.ts

@@ -1,5 +1,4 @@
 // tslint:disable:custom-no-magic-numbers
-import { orderHashUtils } from '@0x/order-utils';
 import { SignedOrder } from '@0x/types';
 import { BigNumber, hexUtils } from '@0x/utils';
 import * as chai from 'chai';
@@ -113,19 +112,19 @@ describe('generateQuoteReport', async () => {
         // quote generator mock
         const quoteRequestor = TypeMoq.Mock.ofType<QuoteRequestor>();
         quoteRequestor
-            .setup(qr => qr.getMakerUriForOrderHash(orderHashUtils.getOrderHash(orderbookOrder2)))
+            .setup(qr => qr.getMakerUriForOrderSignature(orderbookOrder2.signature))
             .returns(() => {
                 return undefined;
             })
             .verifiable(TypeMoq.Times.atLeastOnce());
         quoteRequestor
-            .setup(qr => qr.getMakerUriForOrderHash(orderHashUtils.getOrderHash(rfqtOrder1)))
+            .setup(qr => qr.getMakerUriForOrderSignature(rfqtOrder1.signature))
             .returns(() => {
                 return 'https://rfqt1.provider.club';
             })
             .verifiable(TypeMoq.Times.atLeastOnce());
         quoteRequestor
-            .setup(qr => qr.getMakerUriForOrderHash(orderHashUtils.getOrderHash(rfqtOrder2)))
+            .setup(qr => qr.getMakerUriForOrderSignature(rfqtOrder2.signature))
             .returns(() => {
                 return 'https://rfqt2.provider.club';
             })
@@ -145,7 +144,6 @@ describe('generateQuoteReport', async () => {
             liquiditySource: ERC20BridgeSource.Native,
             makerAmount: rfqtOrder1.makerAssetAmount,
             takerAmount: rfqtOrder1.takerAssetAmount,
-            orderHash: orderHashUtils.getOrderHash(rfqtOrder1),
             nativeOrder: rfqtOrder1,
             fillableTakerAmount: rfqtOrder1FillableAmount,
             isRfqt: true,
@@ -155,7 +153,6 @@ describe('generateQuoteReport', async () => {
             liquiditySource: ERC20BridgeSource.Native,
             makerAmount: rfqtOrder2.makerAssetAmount,
             takerAmount: rfqtOrder2.takerAssetAmount,
-            orderHash: orderHashUtils.getOrderHash(rfqtOrder2),
             nativeOrder: rfqtOrder2,
             fillableTakerAmount: rfqtOrder2FillableAmount,
             isRfqt: true,
@@ -165,7 +162,6 @@ describe('generateQuoteReport', async () => {
             liquiditySource: ERC20BridgeSource.Native,
             makerAmount: orderbookOrder1.makerAssetAmount,
             takerAmount: orderbookOrder1.takerAssetAmount,
-            orderHash: orderHashUtils.getOrderHash(orderbookOrder1),
             nativeOrder: orderbookOrder1,
             fillableTakerAmount: orderbookOrder1FillableAmount,
             isRfqt: false,
@@ -174,7 +170,6 @@ describe('generateQuoteReport', async () => {
             liquiditySource: ERC20BridgeSource.Native,
             makerAmount: orderbookOrder2.makerAssetAmount,
             takerAmount: orderbookOrder2.takerAssetAmount,
-            orderHash: orderHashUtils.getOrderHash(orderbookOrder2),
             nativeOrder: orderbookOrder2,
             fillableTakerAmount: orderbookOrder2FillableAmount,
             isRfqt: false,
@@ -291,7 +286,6 @@ describe('generateQuoteReport', async () => {
             liquiditySource: ERC20BridgeSource.Native,
             makerAmount: orderbookOrder1.makerAssetAmount,
             takerAmount: orderbookOrder1.takerAssetAmount,
-            orderHash: orderHashUtils.getOrderHash(orderbookOrder1),
             nativeOrder: orderbookOrder1,
             fillableTakerAmount: orderbookOrder1FillableAmount,
             isRfqt: false,
@@ -300,7 +294,6 @@ describe('generateQuoteReport', async () => {
             liquiditySource: ERC20BridgeSource.Native,
             makerAmount: orderbookOrder2.makerAssetAmount,
             takerAmount: orderbookOrder2.takerAssetAmount,
-            orderHash: orderHashUtils.getOrderHash(orderbookOrder2),
             nativeOrder: orderbookOrder2,
             fillableTakerAmount: orderbookOrder2FillableAmount,
             isRfqt: false,
@@ -393,7 +386,6 @@ describe('generateQuoteReport', async () => {
             liquiditySource: ERC20BridgeSource.Native,
             makerAmount: orderbookOrder1.makerAssetAmount,
             takerAmount: orderbookOrder1.takerAssetAmount,
-            orderHash: orderHashUtils.getOrderHash(orderbookOrder1),
             nativeOrder: orderbookOrder1,
             fillableTakerAmount: orderbookOrder1FillableAmount,
             isRfqt: false,