RFQ-T Firm Quotes #162

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

These changes depend on the changes in 0xProject/0x-monorepo#2541 .

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.

The following commits should be removed before merging:

TEMPORARY: Update gitpkg monorepo references, 26d8527
- these will likely change once the monorepo PR is merged
TEMPORARY: use gene's ganache snapshot for tests, 0b09bab
- this will go away once the monorepo PR is merged and the ganache snapshot is deployed from it.

Checklist:

MORE TESTS
Create new "should validate?" boolean for firm quotes which is true or false, defaulting to true. Tell integrators to explicitly send in false if their taker address is a smart contract.

They're already deployed in the snapshot. No need to deploy them again.

To avoid trying to descend into docker-created folders, which are root-access-only when running in a Linux environment.

Because it's used (in the mocha invocation) and wasn't present after `yarn install`.

package.json

src/handlers/swap_handlers.ts

src/config.ts

src/types.ts

package.json

feuGeneA · 2020-04-08T22:18:42Z

@steveklebanoff , @PirosB3 and I had a call where we went over this code together. Aside from the inline comments I've already made, we also generated this feedback:

Should we have whitelists in BOTH the 0x API AND in RFQ-T providers themselves?
We should emit some log entries (a) just before hitting RFQ-T quote providers, and (b) just after hitting them, so we can tell how long that process is taking in the wild.
How is axios pooling HTTP requests? Is it re-using sockets or reinstantiating them? Utilizing an HTTP keep-alive would be ideal.
With the code as written now, an RFQ-T taker MUST hold some amount of the taker asset.
- This is because SwapService.calculateSwapQuoteAsync() says if (from) { ... this._estimateGasOrThrowRevertErrorAsync(...). Since an RFQ-T order always includes a taker address, it's falling into this bucket, and the fill simulation fails if the taker doesn't have any taker assets.
- The holding of taker assets is a problem for CFL integrators like 1inch, where the taker is a smart contract and therefore would not be holding any tokens.
- The only immediately available hacky fix would be to change it to say if (from && !intentOnFilling) { ... }. Ideally it would be if (from && !isRFQT) { ... }, but there's no notion of "isRFQT" in the code as written; intentOnFilling is the best we've got in this context.
- That hacky fix is tenable if and only if we are comfortable excluding the gas estimation logic for RFQ-T orders.
- We need to have TWO test cases, one that has taker holding assets, one that doesn't.

src/config.ts

src/services/swap_service.ts

dekz · 2020-04-09T05:15:08Z

src/services/swap_service.ts

        } = params;
        const assetSwapperOpts = {
            ...ASSET_SWAPPER_MARKET_ORDERS_OPTS,
            slippagePercentage,
            bridgeSlippage: slippagePercentage,
            gasPrice: providedGasPrice,
            excludedSources, // TODO(dave4506): overrides the excluded sources selected by chainId
+            apiKey,
+            intentOnFilling,
+            takerAddress: from,


Consider not conflating from and takerAddress. As you discovered, this is sometimes not often the case. I.e Via forwarder takerAddress is the Forwarder, from is the end user.

What happens when the Forwarder is used? the takerAddress in the order MUST be the Forwarder contract.

I tried to account for this here: 0255b25. Does that address the issue?

src/types.ts

src/config.ts

Addresses review comment #162 (comment)

Just pass the API key through to the maker. Addresses review comment #162 (comment)

Addresses review comment #162 (comment)

Just pass the API key through to the maker. Addresses review comment #162 (comment)

Addresses review comment #162 (comment)

.circleci/config.yml

src/config.ts

steveklebanoff · 2020-04-15T17:17:49Z

src/handlers/swap_handlers.ts

+        req.query.intentOnFilling === undefined
+            ? undefined
+            : {
+                  intentOnFilling: ['true', ''].includes(req.query.intentOnFilling) ? true : false,


Can we just do something like the following?

const rfqt = req.query.intentOnFilling === 'true' ? { intentOnFilling: true } : undefined;

I did some digging when I updated the schemas here. I had initially wanted to make them type: "boolean" (whatever exactly that would mean!) but the jsonschema package didn't handle that nicely. Later discovered this question which is a good analog to what we're doing here: OAI/OpenAPI-Specification#1782

I changed this here because I liked the idea of the mere presence of this flag indicating it should be true, eg /quote?intentOnFilling&....

How we resolve #162 (comment) will impact this

steveklebanoff · 2020-04-15T17:18:36Z

src/handlers/swap_handlers.ts

+              };
+    // tslint:disable-next-line:boolean-naming
+    const skipValidation =
+        req.query.skipValidation === undefined ? false : ['true', ''].includes(req.query.skipValidation) ? true : false;


Why do we want to consider req.query.skipValidation === '' as true?

Same logic behind my recent change to intentOnFilling. I think it makes logical sense to be able to say just /quote?skipValidation&..., and when that's the query string, req.query here contains "skipValidation": "".

steveklebanoff · 2020-04-15T17:21:17Z

src/schemas/swap_quote_request_schema.json

+            "type": "string"
+        },
+        "intentOnFilling": {
+            "type": "string",


Can we use a boolean type here? http://json-schema.org/understanding-json-schema/reference/boolean.html

I wanted to. I tried. jsonschema choked on it.

At test runtime i get this json schema error

{\"field\":\"instance.intentOnFilling\",\"code\":1001,\"reason\":\"is not of a type(s) boolean\"}

This happened no matter whether my client said quote?intentOnFilling or quote?intenOnFilling=true.

Got it.

I think this schema could be cleaner if we:

get rid of the flag functionality (i.e. setting ?intentOnFilling indicating that intentOnFilling is equal to true). I have not seen this used in query parameters before, and IMHO is not super important to support

Use an enum type to force the consumer of the API to specify the text true, false for our 'boolean' params (intentOnFilling, skipValidaiton). (See https://stackoverflow.com/a/16826238 and https://json-schema.org/understanding-json-schema/reference/generic.html#enumerated-values

Make sure to keep specifying these params optional though

This would allow ?skipValidation=truue to raise a schema error instead of silently being set to false on our backend.

How's this: bb2814a

Addresses review comments #162 (comment) and #162 (comment)

steveklebanoff · 2020-04-15T22:11:49Z

src/services/swap_service.ts

+                    ? undefined
+                    : {
+                          ...rfqt,
+                          takerAddress: isETHSell ? getContractAddressesForChainOrThrow(CHAIN_ID).forwarder : from,


Would be good to add a comment describing what's happening here

Addressed in 03f7673

steveklebanoff · 2020-04-15T22:11:57Z

src/services/swap_service.ts

@@ -120,7 +140,7 @@ export class SwapService {
        const affiliatedData = this._attributeCallData(data, affiliateAddress);

        let suggestedGasEstimate = new BigNumber(gas);
-        if (from) {
+        if (!skipValidation && from) {


steveklebanoff · 2020-04-15T22:13:50Z

LGTM pending:

Resolve of schema discussion RFQ-T Firm Quotes #162 (comment)
Merge of additional testing RFQT: More tests #170

* generate source maps * abstract tests out into nested contexts * test for skipping validation * dont include order for maker who does not have allowances set * bad api key test * fix test names and explicitly deposit so taker can fill

Addresses the following review comments: - #162 (comment) - #162 (comment) - #162 (comment)

Addresses review comment #162 (comment)

Addresses the following review comments: - #162 (comment) - #162 (comment) - #162 (comment)

Addresses review comment #162 (comment)

This reverts commit 08f4d31.

This reverts commit 7c448d3.

Pin to a gitpkg publish of the monorepo dependencies changed to support this PR to a commit hash of the development branch, rather than that of my PR branch.

I published a custom ganache image, with a monorepo commit hash appended to the version number for reference.

feuGeneA · 2020-04-16T01:34:19Z

deploy kovan-staging

steveklebanoff

Nice!

steveklebanoff · 2020-04-16T16:11:31Z

src/handlers/swap_handlers.ts

+    const rfqt =
+        req.query.intentOnFilling === undefined
+            ? undefined
+            : { intentOnFilling: req.query.intentOnFilling === 'true' ? true : false };


Nit but can't this just be { intentOnFilling: req.query.intentOnFilling === 'true' }

Addressed in 735fe3e

steveklebanoff · 2020-04-16T16:12:10Z

src/schemas/swap_quote_request_schema.json

        },
        "skipValidation": {
            "type": "string",
-            "allowEmptyValue": true
+            "enum": ["true", "false"]


github-actions · 2020-04-30T22:28:22Z

🎉 This PR is included in version 1.0.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

feuGeneA added 7 commits April 7, 2020 01:20

test: Stop running contract migrations

d3cfe8e

They're already deployed in the snapshot. No need to deploy them again.

Add test of /swap/v0/quote endpoint

05ca1b6

Add and use RFQ-T configuration types

e026de2

yarn clean: separate docker from ts

0a0de81

Only run prettier on ./src

89ff735

To avoid trying to descend into docker-created folders, which are root-access-only when running in a Linux environment.

Include prettier in lint

61cbd5c

yarn add --dev make-promises-safe

268bff8

Because it's used (in the mocha invocation) and wasn't present after `yarn install`.