feat: Rfqt included by PirosB3 · Pull Request #293 · 0xProject/0x-api

PirosB3 · 2020-07-23T23:34:43Z

Description

Adds a new parameter includedSources=RFQT which allows a requestor to solely request RFQT liquidity from the order salad. As of now, the implementation of includedSources exclusively permits RFQT but it can be extended in the future will full backwards-compatibility for our integrators.

Testing Instructions

Added unit tests
Tested in staging

Checklist

Update documentation as needed. Website Documentation PR:
Prefix PR title with [WIP] if necessary.
Add tests to cover changes as needed.

PirosB3 · 2020-07-24T04:26:08Z

deploy staging

PirosB3 · 2020-07-24T15:44:17Z

src/utils/parse_utils.ts

+                    },
+                ]);
+            }
+            return {


just to confirm here @steveklebanoff we shouldn't be making any check for intentOnFilling right? I partially remember our discussion and wanted to confirm :)

i think it'd be nice to confirm intentOnFilling if they are hitting /quote but we'd have to make sure this doesn't trigger if they hit /price (switch on endpoint) -- that being said, this is just a 'nice to have' feature

Per convo, let's do it

PirosB3 · 2020-07-24T15:51:12Z

src/schemas/swap_quote_request_schema.json

+        "includedSources": {
+            "type": "string"
+        },
        "apiKey": {


curious, why is apiKey a GET parameter here?

good call -- perhaps this is a mistake?

delete this

steveklebanoff

Awesome work! I'm impressed you did this so quickly

steveklebanoff · 2020-07-24T16:33:30Z

src/handlers/swap_handlers.ts

+    );
+
+    const affiliateAddress = req.query.affiliateAddress as string;
+    const rfqt: Partial<RfqtRequestOpts> =


thanks for adding this typing!

steveklebanoff · 2020-07-24T16:33:53Z

src/schemas/swap_quote_request_schema.json

+        "includedSources": {
+            "type": "string"
+        },
        "apiKey": {


good call -- perhaps this is a mistake?

steveklebanoff · 2020-07-24T16:41:45Z

src/handlers/swap_handlers.ts

+    // tslint:disable-next-line: boolean-naming
+    const { excludedSources, nativeExclusivelyRFQT } = parseUtils.parseRequestForExcludedSources(
+        {
+            excludedSources: req.query.excludedSources as string,


I think it's the case that excludedSources can be undefined? If so :as string | undefined

steveklebanoff · 2020-07-24T16:43:08Z

src/utils/parse_utils.ts

+}
+
+/**
+ * This constant contains, as keys, all ERC20BridgeSource types except from `Native`.


Great documentation, thank you

steveklebanoff · 2020-07-24T16:45:45Z

src/utils/parse_utils.ts


 import { AvailableRateLimiter, DatabaseKeysUsedForRateLimiter, RollingLimiterIntervalUnit } from './rate-limiters';

+interface SwapRequestParams {


IMHO this name is too generic and could be confusing in the future. That being said, I can't think of a great name for this.

This is leading me to believe that it may be better to just send in these as regular parameters instead of introducing an object to hold them. What do you think?

ParseRequestForExcludedSourcesParams

steveklebanoff · 2020-07-24T16:55:53Z

src/utils/parse_utils.ts

+                throw new ValidationError([
+                    {
+                        field: '0x-api-key',
+                        code: ValidationErrorCodes.IncorrectFormat,


I feel like these are not quite 'IncorrectFormat's -- as the format is correct but the value is invalid.

What do you think about introducing FieldValueInvalid as a ValidationErrorCode instead of a reason? Then we could add InvalidApiKey as a reason

code: FieldInvalid
reason: Invalid API key

steveklebanoff · 2020-07-24T16:56:57Z

src/errors.ts

    PercentageOutOfRange = 'MUST_BE_LESS_THAN_OR_EQUAL_TO_ONE',
+    ConflictingFilteringArguments = 'CONFLICTING_FILTERING_ARGUMENTS',
+    ArgumentNotYetSupported = 'ARGUMENT_NOT_YET_SUPPORTED',
+    FieldInvalid = 'FieldInvalid',


FieldInvalid seems more like a ValidationErrorCode to me than a ValidationErrorReason

steveklebanoff · 2020-07-24T17:00:14Z

src/types.ts

-        intentOnFilling?: boolean;
-        isIndicative?: boolean;
-    };
+    rfqt?: Partial<RfqtRequestOpts>;


I think we only want this to be intentOnFilling, isIndicative and nativeExclusivelyRFQT, not the other RfqtRequestOpts -- is that correct?

If so, use Pick instead of Partial

Pick< RfqtRequestOpts , 'intentOnFilling' | ....>

steveklebanoff · 2020-07-24T17:00:50Z

test/parse_utils_test.ts

+
+const SUITE_NAME = 'parseUtils';
+
+describe(SUITE_NAME, () => {


nice tests!

steveklebanoff · 2020-07-24T17:01:26Z

test/parse_utils_test.ts

@@ -0,0 +1,101 @@
+import { ERC20BridgeSource } from '@0x/asset-swapper';


it'd be nice to add to the RFQT integration tests, to show that indicative & firm quotes return as expect when specifying includedSources=RFQT

steveklebanoff · 2020-07-27T16:26:13Z

src/errors.ts

    InvalidOrder = 1007,
    InternalError = 1008,
    TokenNotSupported = 1009,
+    FieldInvalid = 1010,


We should update the documentation here

0xProject/website#318

src/handlers/swap_handlers.ts

src/utils/parse_utils.ts

steveklebanoff · 2020-07-27T16:40:40Z

src/utils/parse_utils.ts

+                    throw new ValidationError([
+                        {
+                            field: 'takerAddress',
+                            code: ValidationErrorCodes.FieldInvalid,


Should this just be the RequiredField code?

src/utils/parse_utils.ts

steveklebanoff · 2020-07-27T16:42:26Z

src/utils/parse_utils.ts

+                    throw new ValidationError([
+                        {
+                            field: 'intentOnFilling',
+                            code: ValidationErrorCodes.FieldInvalid,


Again - should this just be a RequiredField code?

src/utils/parse_utils.ts

# Conflicts: # src/handlers/swap_handlers.ts

PirosB3 · 2020-07-27T21:53:35Z

deploy staging

steveklebanoff · 2020-07-27T22:48:58Z

src/utils/parse_utils.ts

+                }
+
+                // We enforce a valid API key - we don't want to fail silently.
+                if (request.apiKey === undefined) {


Thank you for distinguishing these!

steveklebanoff · 2020-07-27T22:54:28Z

src/utils/parse_utils.ts

+                        {
+                            field: 'takerAddress',
+                            code: ValidationErrorCodes.RequiredField,
+                            reason: ValidationErrorReasons.TakerAddressInvalid,


Looking at this again, I think we could get rid of ValidationErrorReasons.TakerAddressInvalid

field: takerAddress, code: ValidationErrorCodes.RequiredField is descriptive enough.

EDIT: looks like reason is required, so perhaps we leave as-is

# [1.13.0](v1.12.0...v1.13.0) (2020-08-03) ### Bug Fixes * Kovan deploy UniswapV2 ([#304](#304)) ([f4fb99b](f4fb99b)) * Kovan ERC20BridgeSampler ([#299](#299)) ([56f7a90](56f7a90)) ### Features * added a unique identifier to the quote within the timestamp metadata … ([#281](#281)) ([d992563](d992563)) * Rfqt included ([#293](#293)) ([965dedb](965dedb))

github-actions · 2020-08-03T17:22:25Z

🎉 This PR is included in version 1.13.0 🎉

The release is available on:

GitHub release
v1.13.0

Your semantic-release bot 📦🚀

Daniel Pyrathon added 4 commits July 23, 2020 14:30

added initial implementation and unit tests

f0dd0c1

completed implementation

e470999

Upgrades Asset Swapper

8336194

Merge branch 'master' of github.com:0xProject/0x-api into rfqt-included

e1666dd

PirosB3 changed the title ~~Rfqt included~~ feat: Rfqt included Jul 23, 2020

Daniel Pyrathon added 3 commits July 23, 2020 16:55

fixed typos

ae8e116

remove test filter

2073d0f

Merge branch 'master' of github.com:0xProject/0x-api into rfqt-included

bc3b60d

linting and prettifying

7afdc84

PirosB3 requested review from dekz and steveklebanoff and removed request for steveklebanoff July 24, 2020 05:40

PirosB3 marked this pull request as ready for review July 24, 2020 05:41

PirosB3 commented Jul 24, 2020

View reviewed changes

steveklebanoff suggested changes Jul 24, 2020

View reviewed changes

Daniel Pyrathon added 4 commits July 24, 2020 13:14

started addressing feedback

bd5cf55

refactor if-else

0bc3af3

type changes

cb332a8

inting fixing

def329a

steveklebanoff reviewed Jul 27, 2020

View reviewed changes

src/handlers/swap_handlers.ts Show resolved Hide resolved

steveklebanoff reviewed Jul 27, 2020

View reviewed changes

src/utils/parse_utils.ts Show resolved Hide resolved

steveklebanoff reviewed Jul 27, 2020

View reviewed changes

src/utils/parse_utils.ts Show resolved Hide resolved

steveklebanoff reviewed Jul 27, 2020

View reviewed changes

src/utils/parse_utils.ts Show resolved Hide resolved

steveklebanoff reviewed Jul 27, 2020

View reviewed changes

src/utils/parse_utils.ts Show resolved Hide resolved

steveklebanoff reviewed Jul 27, 2020

View reviewed changes

src/utils/parse_utils.ts Show resolved Hide resolved

Daniel Pyrathon added 4 commits July 27, 2020 13:03

Added some RFQ tests

8a20d60

fixes

3569285

trailing comma

464f4db

Merge branch 'master' of github.com:0xProject/0x-api into rfqt-included

dab684c

# Conflicts: # src/handlers/swap_handlers.ts

steveklebanoff reviewed Jul 27, 2020

View reviewed changes

steveklebanoff approved these changes Jul 27, 2020

View reviewed changes

PirosB3 merged commit 965dedb into master Jul 27, 2020

PirosB3 deleted the rfqt-included branch July 27, 2020 23:01

github-actions bot added the released label Aug 3, 2020


		import { AvailableRateLimiter, DatabaseKeysUsedForRateLimiter, RollingLimiterIntervalUnit } from './rate-limiters';

		interface SwapRequestParams {


		const SUITE_NAME = 'parseUtils';

		describe(SUITE_NAME, () => {

		@@ -0,0 +1,101 @@
		import { ERC20BridgeSource } from '@0x/asset-swapper';

Conversation

PirosB3 commented Jul 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Testing Instructions

Checklist

Uh oh!

PirosB3 commented Jul 24, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

steveklebanoff left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

steveklebanoff Jul 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

PirosB3 commented Jul 27, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PirosB3 commented Jul 23, 2020 •

edited

Loading

steveklebanoff Jul 24, 2020 •

edited

Loading

steveklebanoff Jul 27, 2020 •

edited

Loading