asset-swapper: RFQ-T firm quotes #2541

feuGeneA · 2020-04-07T18:28:18Z

Description

These changes support 0xProject/0x-api#162

Every commit is crafted to be completely independent and atomic. I suggest you review them one at a time, in order to give better context, rather than just reviewing all of the changes at once.

Testing instructions

While there aren't any tests here yet, this code is currently being exercised by a 0x-api test, which you can see in the RFQ-T commit at 0xProject/0x-api#162 , and which is using the asset-swapper, and contract-addresses packages from this branch.

The deployment of ERC20BridgeSampler was necessary not just for the RFQ-T test but also for the vanilla /swap/v0/quote endpoint test as well. Before that deployment, the market_operation_utils code was reverting when calling into the sampler, resulting in a HTTP 400 response from the 0x API.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)

Checklist:

All Comments addressed or cited below.
asset-swapper: RFQ-T firm quotes #2541 (comment)
asset-swapper: RFQ-T firm quotes #2541 (review)
Add new entries to the relevant CHANGELOG.jsons.

packages/asset-swapper/src/types.ts

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

feuGeneA · 2020-04-08T19:11:03Z

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

+
+        const ordersWithStringInts = responses.map(response => response.data); // not yet BigNumber
+
+        const orders: SignedOrder[] = ordersWithStringInts.map(orderWithStringInts => {


Find a helper function do this for us. @steveklebanoff ?

I have a feeliing they exist in @0x/connect

@dekz Are you open to exporting typeConverters or orderParsingUtils from the @0x/orderbook package?

https://github.com/0xProject/0x-monorepo/blob/master/packages/connect/src/utils/type_converters.ts#L24

Or is there somewhere else where this is already defined & exported?

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

packages/asset-swapper/src/swap_quoter.ts

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

packages/asset-swapper/src/swap_quoter.ts

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

Addresses review comment #2541 (comment)

coveralls · 2020-04-09T04:56:17Z

Coverage increased (+0.1%) to 79.613% when pulling 1da8f68 on rfq-t into 0e196a5 on development.

packages/asset-swapper/src/constants.ts

dekz · 2020-04-09T05:27:18Z

packages/asset-swapper/src/swap_quoter.ts

@@ -161,10 +165,13 @@ export class SwapQuoter {
        this.orderbook = orderbook;
        this.expiryBufferMs = expiryBufferMs;
        this.permittedOrderFeeTypes = permittedOrderFeeTypes;
+        this.rfqtTakerApiKeyWhitelist = options.rfqtTakerApiKeyWhitelist || [];
+        this.rfqtMakerEndpoints = options.rfqtMakerEndpoints || [];


rfqtMakerEndpoints seems to only be used to construct a QuoteRequestor, does it need to be set as a public accessor?

Are we asserting they are uris?

Addressed the rfqtMakerEndpoints bit in 3c795d3.

We are not directly asserting that they are URI's. We're just passing them through to the quote requestor, and letting it fail silently if they're not valid.

packages/asset-swapper/src/swap_quoter.ts

dekz · 2020-04-09T05:29:47Z

packages/asset-swapper/src/swap_quoter.ts

+        ];
+        if (options.intentOnFilling && options.apiKey) {
+            if (!this.rfqtTakerApiKeyWhitelist.includes(options.apiKey)) {
+                throw new Error('API key not permissioned for RFQ-T');


Is this a throwable validation at this layer? Or should it fail silently and give a normal quote?

If someone gets delisted they're going to start getting Errors rather than quotes, seems quite disruptive.

Yeah, agree that we should fallback to providing a regular quote in this case.

Addressed in 5f4778c

packages/asset-swapper/src/swap_quoter.ts

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

dekz · 2020-04-09T05:46:43Z

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

+
+        const ordersWithStringInts = responses.map(response => response.data); // not yet BigNumber
+
+        const orders: SignedOrder[] = ordersWithStringInts.map(orderWithStringInts => {


I have a feeliing they exist in @0x/connect

packages/asset-swapper/src/types.ts

Addresses review comment #2541 (comment)

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

* test for returning a 200 with invalid data, and additonal logging for that case * Ensure RFQ-T response has asset data we expect * validate signed order schema * give more descriptive variable names and test an unsigned order * takeout unused var

SwapQuoter._getSwapQuoteAsync() was merging defaults into the options sent into SwapQuoteCalculator.calculateMarket{Buy,Sell}SwapQuoteAsync(), but it was using the unmerged options function parameter for the RFQ-T options and also for the gas price option.

feuGeneA · 2020-04-15T06:58:06Z

@dekz

If possible I'd try and take one of the pre-existing Asset-swapper tests and test that nothing goes awry when the RFQT system is misconfigured. I.e test the user gets a swap when no MM quote endpoint can be reached.

Means we don't have to set up a golden path environment, but still get to exercise a large chunk of the logic around this.

We were able to get solid coverage of the new QuoteRequestor class (thanks @steveklebanoff!). What we did not cover in this repo is any of the changes to SwapQuoter, which do have some complex conditional logic, but are otherwise relatively trivial. However, we have integration tests in the 0x API repo that do exercise these SwapQuoter changes.

I feel this PR is ready for a final review. If you guys think any follow-up work is warranted, please be specific, and I'll be sure to track it.

dekz

LGTM, just some optional nits.

dekz · 2020-04-15T07:19:41Z

packages/asset-swapper/package.json

@@ -53,6 +54,8 @@
        "@0x/orderbook": "^2.2.5",
        "@0x/utils": "^5.4.1",
        "@0x/web3-wrapper": "^7.0.7",
+        "axios": "^0.19.2",
+        "axios-mock-adapter": "^1.18.1",


No because clients that use the exported mock need these deps.

dekz · 2020-04-15T08:13:26Z

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

@@ -0,0 +1,37 @@
+import axios from 'axios';


should this be under test/utils?

No because it's exported for external use. Specifically, we're using it from within the 0x API tests.

steveklebanoff · 2020-04-15T16:53:43Z

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

+        takerAddress: string,
+        options?: Partial<RfqtFirmQuoteRequestOpts>,
+    ): Promise<SignedOrder[]> {
+        const { makerEndpointMaxResponseTimeMs } = _.merge({}, constants.DEFAULT_RFQT_FIRM_QUOTE_REQUEST_OPTS, options);


So you like lodash now, huh? 😆 If you're using it here, would prefer to use flatten as well 🤓

I'm ok leaving as it though ❤️

steveklebanoff · 2020-04-15T16:55:15Z

packages/migrations/package.json

@@ -11,7 +11,7 @@
        "build": "tsc -b",
        "build:ci": "yarn build",
        "clean": "shx rm -rf lib ${npm_package_config_snapshot_name} ${npm_package_config_snapshot_name}-*.zip",
-        "lint": "tslint --format stylish --project .",
+        "lint": "tslint --format stylish --project . && prettier --check 'src/**/*.{ts,tsx,json,md}' --config ../../.prettierrc",


It seems like you agreed to keep prettier as a separate script in the asset-swapper package, can we do the same here?

Addressed in 1da8f68

steveklebanoff · 2020-04-15T17:00:00Z

LGTM pending last nit: https://github.com/0xProject/0x-monorepo/pull/2541/files#r408991756

steveklebanoff · 2020-04-15T19:23:27Z

Let's go 💪 💪 💪 !!!

feuGeneA added 5 commits April 7, 2020 00:48

prep asset-swapper: rename variable

3cdccb7

asset-swapper: support firm RFQ-T quote requests

403fb38

migrations: Deploy ERC20BridgeSampler

343caa1

migrations: add yarn publish:private

0e1572c

asset-swapper, migrations: incl. prettier in lint

4b5a02c

feuGeneA added the @0x/asset-swapper label Apr 7, 2020

feuGeneA self-assigned this Apr 7, 2020

feuGeneA changed the title ~~asset-swapper: RFQ-T~~ asset-swapper: RFQ-T firm quotes Apr 7, 2020

feuGeneA commented Apr 8, 2020

View reviewed changes

packages/asset-swapper/src/types.ts Outdated Show resolved Hide resolved

feuGeneA commented Apr 8, 2020

View reviewed changes

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

feuGeneA commented Apr 8, 2020

View reviewed changes

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

feuGeneA commented Apr 8, 2020

View reviewed changes

steveklebanoff reviewed Apr 8, 2020

View reviewed changes

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

feuGeneA mentioned this pull request Apr 8, 2020

RFQ-T Firm Quotes 0xProject/0x-api#162

Merged

2 tasks

feuGeneA requested review from dekz and dorothy-zbornak April 8, 2020 22:34

dorothy-zbornak reviewed Apr 9, 2020

View reviewed changes

feuGeneA added 4 commits April 8, 2020 22:48

mk SwapQuoterOpt rfqtTakerApiKeyWhitelist optional

5602aac

Addresses review comment #2541 (comment)

Use NULL_ADDRESS instead of literal

1670b21

Addresses review comment #2541 (comment)

Use map idiom instead of for loop

98fc780

Addresses review comment #2541 (comment)

Use Array.push instead of Array.concat

37390e0

Addresses review comment #2541 (comment)

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

Parallelize RFQ-T with orderbook query

5c39291

Addresses review comment #2541 (comment)

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

Pass QuoteRequestor via SwapQuoteOpts not ctor arg

7096a70

Addresses review comment #2541 (comment)

feuGeneA added 2 commits April 9, 2020 00:29

Parallelize RFQ-T with orderbook query

aadcf8f

Addresses review comment #2541 (comment)

Pass QuoteRequestor via SwapQuoteOpts not ctor arg

63bfd23

Addresses review comment #2541 (comment)

feuGeneA force-pushed the rfq-t branch from 7096a70 to 63bfd23 Compare April 9, 2020 04:30

feuGeneA added the @0x/migrations label Apr 9, 2020

dekz reviewed Apr 9, 2020

View reviewed changes

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

Promote a closure to a function

3f5ceb2

Addresses review comment #2541 (comment)

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

Validate maker endpoint responses with JSON Schema

8f94b52

Addresses review comment #2541 (comment)

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

Clarify parallelization of orderbook & RFQT

2967799

Addresses review comment #2541 (comment)

feuGeneA added 6 commits April 13, 2020 12:53

Eliminate unnecessary else

d55108a

Addresses review comment #2541 (comment)

Type Axios response with undefined, not void

ccc9e18

Addresses review comment #2541 (comment)

Validate maker endpoint responses with JSON Schema

bb15f78

Addresses review comment #2541 (comment)

Clarify parallelization of orderbook & RFQT

27ca75d

Addresses review comment #2541 (comment)

Remove an unnecessary type annotation

b854fcd

Bug fix: RFQ-T orders werent going through sorting

58d6256

steveklebanoff suggested changes Apr 13, 2020

View reviewed changes

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

packages/asset-swapper/src/utils/quote_requestor.ts Show resolved Hide resolved

steveklebanoff reviewed Apr 13, 2020

View reviewed changes

packages/asset-swapper/src/utils/quote_requestor.ts Show resolved Hide resolved

Steve Klebanoff and others added 3 commits April 15, 2020 00:11

Update {asset-s,migrat,contract-ad}* CHANGELOGs

3bdfcb8

feuGeneA requested a review from steveklebanoff April 15, 2020 07:04

dekz reviewed Apr 15, 2020

View reviewed changes

feuGeneA marked this pull request as ready for review April 15, 2020 14:37

feuGeneA requested review from abandeali1, BMillman19, dave4506 and fragosti as code owners April 15, 2020 14:37

Merge branch 'development' into rfq-t

513ddb4

steveklebanoff reviewed Apr 15, 2020

View reviewed changes

migrations: Add independent yarn prettier script

1da8f68

steveklebanoff approved these changes Apr 15, 2020

View reviewed changes

feuGeneA merged commit 110e1af into development Apr 15, 2020

feuGeneA deleted the rfq-t branch April 15, 2020 19:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

asset-swapper: RFQ-T firm quotes #2541

asset-swapper: RFQ-T firm quotes #2541

feuGeneA commented Apr 7, 2020 •

edited

Loading

feuGeneA Apr 8, 2020

dekz Apr 9, 2020

steveklebanoff Apr 10, 2020

coveralls commented Apr 9, 2020 •

edited

Loading

dekz Apr 9, 2020

dekz Apr 9, 2020

feuGeneA Apr 9, 2020

dekz Apr 9, 2020

fabioberger Apr 9, 2020

feuGeneA Apr 9, 2020

dekz Apr 9, 2020

feuGeneA commented Apr 15, 2020

dekz left a comment

dekz Apr 15, 2020

feuGeneA Apr 15, 2020

dekz Apr 15, 2020

feuGeneA Apr 15, 2020

steveklebanoff Apr 15, 2020

steveklebanoff Apr 15, 2020

feuGeneA Apr 15, 2020

steveklebanoff commented Apr 15, 2020

steveklebanoff commented Apr 15, 2020


		const ordersWithStringInts = responses.map(response => response.data); // not yet BigNumber

		const orders: SignedOrder[] = ordersWithStringInts.map(orderWithStringInts => {

asset-swapper: RFQ-T firm quotes #2541

asset-swapper: RFQ-T firm quotes #2541

Conversation

feuGeneA commented Apr 7, 2020 • edited Loading

Description

Testing instructions

Types of changes

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Apr 9, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feuGeneA commented Apr 15, 2020

dekz 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 commented Apr 15, 2020

steveklebanoff commented Apr 15, 2020

feuGeneA commented Apr 7, 2020 •

edited

Loading

coveralls commented Apr 9, 2020 •

edited

Loading