asset-swapper: RFQ-T indicative quotes #2555

feuGeneA · 2020-04-16T03:20:07Z

When a request specifies a whitelisted API key, and a taker address, and does not specify intentToFill=true, the SwapQuoter passes its QuoteRequestor into MarketOperationUtils, so that it can call QuoteRequestor.getRfqtIndicativeQuotesAsync(), which it transforms into weird pseudo-native-orders, which in turn get fed into order routing along with all of the other native orders.

This code is not yet covered with tests here, but is being exercised in the end-to-end test in 0xProject/0x-api#172, which is integrated with CI there, and which is living at 0xProject/0x-api@66c9d9d#diff-491d90fb8eb25e485b3d8c9429e6754cR326.

Add tests
Updated CHANGELOGs

coveralls · 2020-04-16T04:12:52Z

Coverage increased (+0.1%) to 79.688% when pulling 153533f on rfq-t-indicative into 7396bb5 on development.

packages/asset-swapper/src/utils/market_operation_utils/fills.ts

dekz · 2020-04-16T11:42:34Z

packages/asset-swapper/src/utils/market_operation_utils/index.ts

@@ -92,10 +88,25 @@ export class MarketOperationUtils {
                this._liquidityProviderRegistry,
            ),
        );
+        const rfqtPromise =
+            opts !== undefined && _opts.rfqt !== undefined && _opts.rfqt.quoteRequestor !== undefined


Since we merge defaults into _opts, and never access it further down, do we need the opts !== undefined check?

This was a bug, that I noticed too, and fixed here: d6d4d29#diff-bdb1465a97651026c09ef607e5de9e6fR97

dekz · 2020-04-16T11:49:06Z

packages/asset-swapper/src/utils/market_operation_utils/sampler_operations.ts

@@ -16,7 +16,9 @@ export const samplerOperations = {
                    .getABIEncodedTransactionData();
            },
            handleCallResultsAsync: async (contract, callResults) => {
-                return contract.getABIDecodedReturnData<BigNumber[]>('getOrderFillableTakerAssetAmounts', callResults);
+                return contract
+                    .getABIDecodedReturnData<BigNumber[]>('getOrderFillableTakerAssetAmounts', callResults)


I'm assuming they're not signing the orders where the intent to fill is false? Rather than use takerAddress as a conditional can we determine this off the signature, or lack there of?

I'm sorry, I wrote this commit while blindly following the spec I wrote. In retrospect, this idea, of making the sampler bypass fillability checks because these orders are unsigned, is moot now.

This is because we're getting indicative quotes in parallel with the on-chain call, not prior to it (like we do for firm quotes), and we're piping the quotes directly into the order router, and there's no further fillability check after routing*, so there's no need for such a hack.

It was only ever consideration when we were trying to decide whether to request quotes from SwapQuoter or from MarketOperationUtils, but ultimately we decided to do both (for latency reasons), alleviating the need for this.

*: Of course there's the gas estimation / transaction validation stage that happens in the 0x-api side, but you'll see there that I'm forcing skipValidation to true when handling /swap/price requests and delegating them to the handler for /swap/quote.

packages/asset-swapper/src/utils/market_operation_utils/fills.ts

steveklebanoff

Nice work! Added a few comments. I can't speak to the "paths" much though, will defer to Jacob & Lawrence on the feedback there.

packages/asset-swapper/src/utils/market_operation_utils/constants.ts

packages/asset-swapper/src/utils/market_operation_utils/fills.ts

packages/asset-swapper/src/utils/market_operation_utils/index.ts

steveklebanoff · 2020-04-16T22:38:48Z

packages/asset-swapper/src/utils/market_operation_utils/index.ts

@@ -165,11 +171,26 @@ export class MarketOperationUtils {
                this._liquidityProviderRegistry,
            ),
        );
+        const rfqtPromise =


At what point do we want to short-circuit considering RFQT MMers will not suppot buys to begin with?

seems ok to leave in, depends on the ETA for buyAmount support in RFQT?

I'd be down to leave it in if we can get it looking nicer.

This is my attempt at making it look nicer: 5effc6e

packages/asset-swapper/src/utils/market_operation_utils/sampler_operations.ts

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

steveklebanoff · 2020-04-16T22:54:46Z

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

+        return responses;
+    }
+
+    private _isValidRfqtIndicativeQuoteResponse(response: RfqtIndicativeQuoteResponse): boolean {


I think this is OK for now, but it may be nice in the future to introduce a proper JSON schema file for this.

There's one subtlety here: apiKey has been moved to be within the rfqt namespace, after talking to Fabio and discovering that he only needs to re-use the API key in 0x API, not in asset-swapper.

For use in upcoming implementation of indicative quotes.

These changes have been exercised via mocha tests in the 0x-api repo. Not sure why I had to add GetMarketOrdersRfqtOpts to the package exports. `yarn test:generate_docs:circleci` said: $ node ./packages/monorepo-scripts/lib/doc_generate.js --package @0x/asset-swapper GENERATE_DOCS: Generating Typedoc JSON for @0x/asset-swapper... GENERATE_DOCS: Generating Typedoc Markdown for @0x/asset-swapper... GENERATE_DOCS: Modifying Markdown To Exclude Unexported Items... Error: @0x/asset-swapper package needs to export: GetMarketOrdersRfqtOpts From it's index.ts. If any are from external dependencies, then add them to the EXTERNAL_TYPE_MAP. at DocGenerateUtils._lookForMissingReferenceExportsThrowIfExists (/root/repo/packages/monorepo-scripts/lib/utils/doc_generate_utils.js:288:19) at DocGenerateUtils.<anonymous> (/root/repo/packages/monorepo-scripts/lib/utils/doc_generate_utils.js:255:34) at step (/root/repo/packages/monorepo-scripts/lib/utils/doc_generate_utils.js:32:23) at Object.next (/root/repo/packages/monorepo-scripts/lib/utils/doc_generate_utils.js:13:53) at fulfilled (/root/repo/packages/monorepo-scripts/lib/utils/doc_generate_utils.js:4:58) at <anonymous> at process._tickCallback (internal/process/next_tick.js:189:7)

steveklebanoff · 2020-04-17T23:48:17Z

packages/asset-swapper/src/utils/market_operation_utils/types.ts

@@ -34,6 +35,7 @@ export enum ERC20BridgeSource {
    CurveUsdcDaiUsdtTusd = 'Curve_USDC_DAI_USDT_TUSD',
    LiquidityProvider = 'LiquidityProvider',
    CurveUsdcDaiUsdtBusd = 'Curve_USDC_DAI_USDT_BUSD',
+    Rfqt = 'Rfqt',


Is this used anywhere?

No, it's not. It's a remnant from the first draft. I've removed it here: 4cc9cea

steveklebanoff

Added a few comments. Overall looking quite good tho! Definitely would love a blessing from @dekz or @dorothy-zbornak so we can have a vote of confidence on all the sampling / path stuff though.

steveklebanoff · 2020-04-18T05:12:08Z

packages/asset-swapper/src/utils/market_operation_utils/orders.ts

+            makerAddress: NULL_ADDRESS,
+            senderAddress: NULL_ADDRESS,
+            feeRecipientAddress: NULL_ADDRESS,
+            salt: ZERO_AMOUNT, // generatePseudoRandomSalt(),


What's up with this comment? Can we leave as zero?

Oh good eye. This method was basically copied from

0x-monorepo/packages/asset-swapper/src/utils/market_operation_utils/orders.ts

Line 338 in 110e1af

function createCommonBridgeOrderFields(opts: CreateOrderFromPathOpts): CommonBridgeOrderFields {

, and I changed it to zero here, but preserved that call in the comments just in case it ended up not working for some reason. At this point I can say it's working, but I'd love some feedback from @dorothy-zbornak on whether we should be concerned about this.

I don't think it matters since it's not signed and not intended to be used in anyway, just a vessel for the amounts.

Yeah it shouldn't matter. Might even be better to make these orders as unfillable as possible.

I've cleaned up this function here: ad7868e

steveklebanoff · 2020-04-18T05:23:41Z

packages/asset-swapper/src/utils/market_operation_utils/index.ts

@@ -92,10 +93,25 @@ export class MarketOperationUtils {
                this._liquidityProviderRegistry,
            ),
        );
+        const rfqtPromise =


When would _opts ever be undefined?

Can make either make this ternary shorter, or break it out into something else? This feels a bit jarring to read

One option (still a pretty long statement for ternary tho)

const rfqtIndicativeQuotes = _opts.rfqt && _opts.rfqt.quoteRequestor ? _opts.rfqt.quoteRequestor.requestRfqtIndicativeQuotesAsync(...) : Promise.resolve<RfqtIndicativeQuoteResponse[]>([]);

Another option

let rfqtIndicativeQuotesPromise = Promise.resolve<RfqtIndicativeQuoteResponse[]>([]); if (_opts.rfqt && _opts.rfqt.quoteRequestor) { rfqtIndicativeQuotesPromise = _opts.rfqt.quoteRequestor.requestRfqtIndicativeQuotesAsync(...) }

Third option (I like this best)

const rfqtIndicativeQuotes = async (): Promise<RfqtIndicativeQuoteResponse[]> => { if (_opts.rfqt && _opts.rfqt.quoteRequestor) { return _opts.rfqt.quoteRequestor.requestRfqtIndicativeQuotesAsync(...); } return []; }

I cleaned this up here: 5effc6e

Please let me know what you think.

packages/asset-swapper/src/utils/market_operation_utils/index.ts

steveklebanoff · 2020-04-18T05:28:54Z

packages/asset-swapper/src/utils/market_operation_utils/orders.ts

+            senderAddress: NULL_ADDRESS,
+            feeRecipientAddress: NULL_ADDRESS,
+            salt: ZERO_AMOUNT, // generatePseudoRandomSalt(),
+            expirationTimeSeconds: new BigNumber(Math.floor(Date.now() / ONE_SECOND_MS) + ONE_HOUR_IN_SECONDS),


In a future revision I def think we should expect an expirationTimeSeconds from the MMer and use that here

Can probably just set this to zero as well.

I put it at zero for now: ad7868e

steveklebanoff · 2020-04-18T05:30:53Z

packages/asset-swapper/src/utils/market_operation_utils/orders.ts

+            takerFee: ZERO_AMOUNT,
+            fillableTakerFeeAmount: ZERO_AMOUNT,
+            signature: WALLET_SIGNATURE,
+            chainId: 0, // HACK !!!!!!!!! how can we get at this from this context?


I think you can pass it in from MarketOperationUtils's this._orderDomain but will let @dekz or @dorothy-zbornak chime in here if there's a better way

both chainId and exchangeAddress are in the order domain.

I just set them both to zero for now, per #2555 (comment)

packages/asset-swapper/src/utils/market_operation_utils/orders.ts

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

dekz · 2020-04-20T03:48:47Z

packages/asset-swapper/src/utils/market_operation_utils/orders.ts

+            takerFee: ZERO_AMOUNT,
+            fillableTakerFeeAmount: ZERO_AMOUNT,
+            signature: WALLET_SIGNATURE,
+            chainId: 0, // HACK !!!!!!!!! how can we get at this from this context?


both chainId and exchangeAddress are in the order domain.

dekz · 2020-04-20T04:17:43Z

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

+    makerAssetData: string,
+    takerAssetData: string,
+    assetFillAmount: BigNumber,
+): { buyToken: string; sellToken: string; buyAmount: string | undefined; sellAmount: string | undefined } {


Suggested change

): { buyToken: string; sellToken: string; buyAmount: string | undefined; sellAmount: string | undefined } {

): { buyToken: string; sellToken: string; buyAmount?: string; sellAmount?: string } {

I made this change here: 2c97208

dekz · 2020-04-20T04:20:04Z

packages/asset-swapper/src/utils/market_operation_utils/orders.ts

+            makerAddress: NULL_ADDRESS,
+            senderAddress: NULL_ADDRESS,
+            feeRecipientAddress: NULL_ADDRESS,
+            salt: ZERO_AMOUNT, // generatePseudoRandomSalt(),


I don't think it matters since it's not signed and not intended to be used in anyway, just a vessel for the amounts.

dekz · 2020-04-20T04:21:53Z

packages/asset-swapper/src/utils/market_operation_utils/index.ts

@@ -165,11 +171,26 @@ export class MarketOperationUtils {
                this._liquidityProviderRegistry,
            ),
        );
+        const rfqtPromise =


seems ok to leave in, depends on the ETA for buyAmount support in RFQT?

dekz · 2020-04-20T04:23:09Z

packages/asset-swapper/src/types.ts

+export interface RfqtRequestOpts {
+    takerAddress: string;
+    apiKey: string;
+    intentOnFilling?: boolean;


should we force the default on this early on if it's not set to false?

Suggested change

intentOnFilling?: boolean;

intentOnFilling: boolean;

I made this change here: 245b6da

dekz · 2020-04-20T04:23:43Z

packages/asset-swapper/src/utils/market_operation_utils/index.ts

@@ -165,11 +176,26 @@ export class MarketOperationUtils {
                this._liquidityProviderRegistry,
            ),
        );
+        const rfqtPromise =
+            opts !== undefined && _opts.rfqt !== undefined && _opts.rfqt.quoteRequestor !== undefined


Suggested change

opts !== undefined && _opts.rfqt !== undefined && _opts.rfqt.quoteRequestor !== undefined

_opts !== undefined && _opts.rfqt !== undefined && _opts.rfqt.quoteRequestor !== undefined

Fixed by consolidating this crazy conditional with the other instance of it here: 5effc6e

dorothy-zbornak

Some minor suggestions, but looks pretty good!

dorothy-zbornak · 2020-04-20T16:01:51Z

packages/asset-swapper/src/swap_quoter.ts

+            calcOpts.rfqt &&
+            !calcOpts.rfqt.intentOnFilling &&
+            calcOpts.rfqt.apiKey &&
+            this._rfqtTakerApiKeyWhitelist.includes(calcOpts.rfqt.apiKey)


Maybe these conditions (minus the intentOnFilling check) could be put into a function like isRfqtEnabled(opts.rfqt) since there are two places where this happens.

I simplified this here: ba2ac6a

dorothy-zbornak · 2020-04-20T16:09:31Z

packages/asset-swapper/src/utils/market_operation_utils/index.ts

@@ -165,11 +171,26 @@ export class MarketOperationUtils {
                this._liquidityProviderRegistry,
            ),
        );
+        const rfqtPromise =


I'd be down to leave it in if we can get it looking nicer.

dorothy-zbornak · 2020-04-20T16:23:07Z

packages/asset-swapper/src/utils/market_operation_utils/index.ts

+            _opts !== undefined && _opts.rfqt !== undefined && _opts.rfqt.quoteRequestor !== undefined
+                ? _opts.rfqt.quoteRequestor.requestRfqtIndicativeQuotesAsync(
+                      nativeOrders[0].makerAssetData,
+                      nativeOrders[0].takerAssetData,
+                      takerAmount,
+                      MarketOperation.Sell,
+                      _opts.rfqt,
+                  )
+                : Promise.resolve<RfqtIndicativeQuoteResponse[]>([]);


Can't we just put this all into a function like getRfqtIndicativeQuotesAsync(nativeOrders, takerAmount, MarketOpreration.Sell, _opts.rfqt)?

Great idea, thank you, did it here: 5effc6e

dorothy-zbornak · 2020-04-20T16:23:38Z

packages/asset-swapper/src/utils/market_operation_utils/index.ts

+            _opts !== undefined && _opts.rfqt !== undefined && _opts.rfqt.quoteRequestor !== undefined
+                ? _opts.rfqt.quoteRequestor.requestRfqtIndicativeQuotesAsync(
+                      nativeOrders[0].makerAssetData,
+                      nativeOrders[0].takerAssetData,
+                      takerAmount,
+                      MarketOperation.Sell,
+                      _opts.rfqt,
+                  )
+                : Promise.resolve<RfqtIndicativeQuoteResponse[]>([]);


Promise.resolve() is probably unnecessary here.

I tried to remove it and got errors.

dorothy-zbornak · 2020-04-20T16:40:07Z

packages/asset-swapper/src/utils/market_operation_utils/orders.ts

+            makerAddress: NULL_ADDRESS,
+            senderAddress: NULL_ADDRESS,
+            feeRecipientAddress: NULL_ADDRESS,
+            salt: ZERO_AMOUNT, // generatePseudoRandomSalt(),


Yeah it shouldn't matter. Might even be better to make these orders as unfillable as possible.

dorothy-zbornak · 2020-04-20T16:41:05Z

packages/asset-swapper/src/utils/market_operation_utils/orders.ts

+            senderAddress: NULL_ADDRESS,
+            feeRecipientAddress: NULL_ADDRESS,
+            salt: ZERO_AMOUNT, // generatePseudoRandomSalt(),
+            expirationTimeSeconds: new BigNumber(Math.floor(Date.now() / ONE_SECOND_MS) + ONE_HOUR_IN_SECONDS),


Can probably just set this to zero as well.

dorothy-zbornak · 2020-04-20T17:05:10Z

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

+        takerAddress === undefined ||
+        takerAddress === '' ||


Suggested change

takerAddress === undefined ||

takerAddress === '' ||

!takerAddress ||

I made this change here: 83289bc

dorothy-zbornak · 2020-04-20T17:08:11Z

packages/asset-swapper/src/utils/market_operation_utils/orders.ts

+            chainId: 0, // HACK !!!!!!!!! how can we get at this from this context?
+            exchangeAddress: NULL_ADDRESS, // HACK !!!!!!!!! how can we get at this from this context?


Neither of these really matter since these orders aren't valid anyway, so I'm fine with leaving it as-is and making the comments less frightening lol

I did just that: ad7868e

Thank you

Validate that the responses returned from maker endpoints both conform to expected JSON schema data types and also have the expected asset data, per the taker's request.

Addresses review comment #2555 (comment)

dekz

LGTM let's try this puppy out.

See 0xProject/0x-monorepo#2555 (comment)

steveklebanoff · 2020-04-22T00:29:00Z

I feel good about all the outstanding changes I requested -- nice work!

steveklebanoff · 2020-04-22T00:29:42Z

packages/asset-swapper/src/utils/market_operation_utils/index.ts

+            opts.rfqt,
+        );
+    } else {
+        return Promise.resolve<RfqtIndicativeQuoteResponse[]>([]);


Can't we just return []; here?

Note: this is a nit, don't need to change

steveklebanoff · 2020-04-22T00:31:16Z

packages/asset-swapper/src/utils/market_operation_utils/index.ts

+    assetFillAmount: BigNumber,
+    opts: Partial<GetMarketOrdersOpts>,
+): Promise<RfqtIndicativeQuoteResponse[]> {
+    if (opts.rfqt && opts.rfqt.quoteRequestor) {


NOTE: This is where we need to ensure intentOnFilling is to true

I tried to get fancy back in 5effc6e. I changed something more than the single refactor targetted by the commit, and it broke things! This reverts part of that commit, restoring clean runs of 0x API tests.

steveklebanoff · 2020-04-22T17:46:11Z

packages/asset-swapper/src/swap_quoter.ts

-        return opts !== undefined && !opts.intentOnFilling && this._rfqtTakerApiKeyWhitelist.includes(opts.apiKey);
+        return (
+            opts !== undefined &&
+            opts.isIndicative !== undefined &&


dont need this undefined check

steveklebanoff · 2020-04-22T20:15:23Z

packages/asset-swapper/src/types.ts

+    takerAddress: string;
+    apiKey: string;
+    intentOnFilling: boolean;
+    isIndicative?: boolean;


Nit but would prefer for this to be required

steveklebanoff

LGTM

* Adapt to asset-swapper RFQ-T option restructuring A handful of RFQ-T options were restructured in 0xProject/0x-monorepo@957e3eb This adapts to those changes. * Support for RFQ-T indicative quotes Renamed method getSwapQuoteAsync() to _getSwapQuoteAsync(), and created a new getSwapQuoteAsync() to delegeate to it, all for the purposes of also calling into _getSwapQuoteAsync() of the newly introduced method getSwapPriceAsync(). Unfortunately, the diff looks noisy because the old public method was changed to be private, forcing it to move down below publics in the method order. This resulted resulting in my diff viewer showing that two previously existing methods (getSwapPricesAsync() [notice that one's plural, not to be confused with the aforementioned singular version] and getTokenPricesAsync()) moved UP in the diff. Those two existing methods have not changed, and it's easier to review this commit if you understand the pattern they contribute to the overall diff. * Remove Rfqt from gasSchedule * Relay source breakdown to price endpoint consumer * Log orders underlying indicative quotes Log the orders that are used to construct indicative quotes. These are "dummy" orders, but for now we need to see them in the logs in order to verify that indicative quotes are being served, since there is no separate source for RFQ-T (they are lumped in with the '0x' source.) Long term, this log entry may prove useful if it isn't too noisy. If it is, we can remove it after manual testing. * Incorporate API key into GetSwapQuoteRequestParams Addresses review comment #172 (comment) * Get Forwarder address once, not every req Addresses (at least part of) review comment #172 (comment) * Accommodate asset-swpr's now-req'd intentOnFilling See 0xProject/0x-monorepo#2555 (comment) * pin asset-swapper to monorepo commit * Add the correct CORS headers * Ran prettier * Revert "Ran prettier" This reverts commit b8641c0. * Revert "Add the correct CORS headers" This reverts commit 03b3ac1. * Test case for weird zero order from indicative When the client gives an API key and a taker address, but not saying intentOnFilling=true, the process is falling into an indicative quote mode, and a weird "all zeroes" order (zero salt, NULL_ADDRESS exchange address, etc) was being included in the order salad. * Fix for weird zero orders. depends on new AS Utilize the new `isIndicative` flag provided by asset-swapper in order to indicate when indicative quotes should NOT be solicited. * Remove diagnostic logging of indicative quotes Addresses review comment #172 (comment) * update monorepo pin * Add test of indicative w/ intentOnFilling omitted * Clarify RFQ-T enablement logic * Make axios retryable only when desired (#185) * update monorepo pin to development branch commit Co-authored-by: Daniel Pyrathon <[email protected]> Co-authored-by: Steve Klebanoff <[email protected]>

feuGeneA self-assigned this Apr 16, 2020

feuGeneA requested review from dekz and dorothy-zbornak April 16, 2020 05:30

dekz reviewed Apr 16, 2020

View reviewed changes

dorothy-zbornak reviewed Apr 16, 2020

View reviewed changes

packages/asset-swapper/src/utils/market_operation_utils/fills.ts Outdated Show resolved Hide resolved

dorothy-zbornak reviewed Apr 16, 2020

View reviewed changes

packages/asset-swapper/src/utils/market_operation_utils/fills.ts Outdated Show resolved Hide resolved

steveklebanoff suggested changes Apr 16, 2020

View reviewed changes

steveklebanoff marked this pull request as ready for review April 16, 2020 22:58

steveklebanoff requested review from BMillman19, dave4506 and fragosti as code owners April 16, 2020 22:58

steveklebanoff marked this pull request as draft April 16, 2020 22:58

feuGeneA added 5 commits April 17, 2020 01:18

asset-swapper: Restructure RFQ-T request options

957e3eb

There's one subtlety here: apiKey has been moved to be within the rfqt namespace, after talking to Fabio and discovering that he only needs to re-use the API key in 0x API, not in asset-swapper.

asset-swapper: Rm unused field in SwapQuoterOpts

80c8712

asset-swapper: Extract method inferQueryParams

33fdfdc

For use in upcoming implementation of indicative quotes.

asset-swapper: Differentiate RFQT warning messages

45bc967

feuGeneA force-pushed the rfq-t-indicative branch from 6c62c33 to 45bc967 Compare April 17, 2020 07:18

feuGeneA marked this pull request as ready for review April 17, 2020 07:45

feuGeneA mentioned this pull request Apr 17, 2020

RFQ-T Indicative Quotes 0xProject/0x-api#172

Merged

1 task

steveklebanoff reviewed Apr 17, 2020

View reviewed changes

steveklebanoff suggested changes Apr 18, 2020

View reviewed changes

dekz reviewed Apr 20, 2020

View reviewed changes

dorothy-zbornak approved these changes Apr 20, 2020

View reviewed changes

feuGeneA added 5 commits April 20, 2020 22:56

asset-swapper: validate RFQT indicative quotes

2456adc

Validate that the responses returned from maker endpoints both conform to expected JSON schema data types and also have the expected asset data, per the taker's request.

asset-swapper: test RFQT indicative quote handling

464c918

asset-swapper: Rm unused ERC20BridgeSource.Rfqt

4cc9cea

Addresses review comment #2555 (comment)

asset-swapper: extract method

5effc6e

Addresses review comment #2555 (comment)

asset-s: clean up order faking for indicative RFQT

ad7868e

Addresses review comment #2555 (comment)

feuGeneA added 4 commits April 20, 2020 22:56

asset-s: test requestRfqtIndicativeQuotesAsync

8875f92

asset-s: Require RfqtRequestOpts.intentOnFilling

245b6da

Addresses review comment #2555 (comment)

asset-s: Clarify indicative quote enablement

ba2ac6a

Addresses review comment #2555 (comment)

asset-s: consider falsey takerAddress as absent

83289bc

Addresses review comment #2555 (comment)

feuGeneA requested review from abandeali1 and hysz as code owners April 21, 2020 04:27

feuGeneA added a commit that referenced this pull request Apr 21, 2020

asset-s: simplify type with ?, not |undefined

636d87a

Addresses review comment #2555 (comment)

feuGeneA added 2 commits April 21, 2020 00:49

asset-s: simplify type with ?, not |undefined

2c97208

Addresses review comment #2555 (comment)

Merge branch 'development' into rfq-t-indicative

03ed3ac

feuGeneA force-pushed the rfq-t-indicative branch from dc339d6 to 03ed3ac Compare April 21, 2020 05:30

asset-swapper: update CHANGELOG

e8ff5da

feuGeneA requested a review from dekz April 21, 2020 05:49

dekz approved these changes Apr 21, 2020

View reviewed changes

feuGeneA added a commit to 0xProject/0x-api that referenced this pull request Apr 21, 2020

Accommodate asset-swpr's now-req'd intentOnFilling

d3ee6bd

See 0xProject/0x-monorepo#2555 (comment)

feuGeneA added a commit to 0xProject/0x-api that referenced this pull request Apr 21, 2020

Accommodate asset-swpr's now-req'd intentOnFilling

e66055d

See 0xProject/0x-monorepo#2555 (comment)

steveklebanoff reviewed Apr 22, 2020

View reviewed changes

feuGeneA added 2 commits April 22, 2020 11:58

asset-s: Add RfqtRequestOpts.isIndicative

11622c5

Fix bug in prior revision: wrong asset data

153533f

I tried to get fancy back in 5effc6e. I changed something more than the single refactor targetted by the commit, and it broke things! This reverts part of that commit, restoring clean runs of 0x API tests.

feuGeneA force-pushed the rfq-t-indicative branch from f953647 to 153533f Compare April 22, 2020 16:43

steveklebanoff reviewed Apr 22, 2020

View reviewed changes

steveklebanoff approved these changes Apr 22, 2020

View reviewed changes

feuGeneA merged commit 07acc95 into development Apr 22, 2020

feuGeneA deleted the rfq-t-indicative branch April 22, 2020 20:34

	): { buyToken: string; sellToken: string; buyAmount: string \| undefined; sellAmount: string \| undefined } {
	): { buyToken: string; sellToken: string; buyAmount?: string; sellAmount?: string } {

	opts !== undefined && _opts.rfqt !== undefined && _opts.rfqt.quoteRequestor !== undefined
	_opts !== undefined && _opts.rfqt !== undefined && _opts.rfqt.quoteRequestor !== undefined

	takerAddress === undefined \|\|
	takerAddress === '' \|\|
	!takerAddress \|\|

		chainId: 0, // HACK !!!!!!!!! how can we get at this from this context?
		exchangeAddress: NULL_ADDRESS, // HACK !!!!!!!!! how can we get at this from this context?

asset-swapper: RFQ-T indicative quotes #2555

asset-swapper: RFQ-T indicative quotes #2555

Conversation

feuGeneA commented Apr 16, 2020 • edited Loading

coveralls commented Apr 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steveklebanoff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steveklebanoff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feuGeneA Apr 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dorothy-zbornak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dekz left a comment

Choose a reason for hiding this comment

steveklebanoff commented Apr 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steveklebanoff left a comment

Choose a reason for hiding this comment

feuGeneA commented Apr 16, 2020 •

edited

Loading

coveralls commented Apr 16, 2020 •

edited

Loading

feuGeneA Apr 21, 2020 •

edited

Loading