RFQ-T Indicative Quotes #172

feuGeneA · 2020-04-17T07:43:13Z

Adds a /swap/v0/price endpoint, delegating almost entirely to the handler for the /swap/v0/quote endpoint.

This depends on 0xProject/0x-monorepo#2555 and is pinned to a revision there in the TEMPORARY commit here, which will be updated when that PR lands.

Expand test cases. There's only a single one right now.

A handful of RFQ-T options were restructured in 0xProject/0x-monorepo@957e3eb This adapts to those changes.

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.

feuGeneA · 2020-04-17T16:03:01Z

deploy staging

feuGeneA · 2020-04-17T16:12:45Z

deploy kovan-staging

src/config.ts

steveklebanoff

Two minor comments but LGTM besides that 💪

src/handlers/swap_handlers.ts

src/services/swap_service.ts

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.

Addresses review comment #172 (comment)

Addresses (at least part of) review comment #172 (comment)

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

feuGeneA · 2020-04-21T16:25:41Z

deploy staging

feuGeneA · 2020-04-21T22:46:48Z

deploy staging

This reverts commit b8641c0.

This reverts commit 03b3ac1.

steveklebanoff · 2020-04-22T00:33:11Z

package.json

@@ -66,7 +66,7 @@
    },
    "dependencies": {
        "@0x/assert": "^3.0.4",
-        "@0x/asset-swapper": "0xProject/gitpkg-registry#0x-asset-swapper-v4.4.0-a458e81f8",
+        "@0x/asset-swapper": "0xProject/gitpkg-registry#0x-asset-swapper-v4.4.0-a9f445173",


NOTE: this needs to be updated now, correct?

steveklebanoff

LGTM Pending

Update asset-swapper git hash
Getting rid of CORS change

steveklebanoff · 2020-04-22T00:34:23Z

LGTM pending the two items mentioned. Would be good to get @dekz blessing here though.

dekz · 2020-04-22T04:45:19Z

src/handlers/swap_handlers.ts

+        params.skipValidation = true;
+        const quote = await this._calculateSwapQuoteAsync(params);
+        const { price, value, gasPrice, gas, protocolFee, buyAmount, sellAmount, sources, orders } = quote;
+        logger.info(`Serving indicative quote based on the following orders: ${JSON.stringify(orders)}`);


This would get a little verbose. Can we remove it?

Addressed in b2f6fe7

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.

Utilize the new `isIndicative` flag provided by asset-swapper in order to indicate when indicative quotes should NOT be solicited.

Addresses review comment #172 (comment)

feuGeneA · 2020-04-22T16:44:23Z

deploy staging

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

src/handlers/swap_handlers.ts

+    // tslint:disable-next-line:prefer-function-over-method
+    public async getSwapPriceAsync(req: express.Request, res: express.Response): Promise<void> {
+        const params = parseGetSwapQuoteRequestParams(req, 'price');
+        params.skipValidation = true;


nit but it'd be nice to have a comment explaining what's going on here

steveklebanoff · 2020-04-22T20:25:05Z

src/services/swap_service.ts

+                // If this is a forwarder transaction, then we want to request quotes with the taker as the
+                // forwarder contract. If it's not, then we want to request quotes with the taker set to the
+                // API's takerAddress query parameter, which in this context is known as `from`.
+                takerAddress: isETHSell ? this._forwarderAddress : from || '',


I expected this to be || undefined, does it have to be || ''?

steveklebanoff · 2020-04-22T20:25:33Z

src/types.ts

@@ -433,7 +435,8 @@ export interface CalculateSwapQuoteParams {
    affiliateAddress?: string;
    apiKey?: string;
    rfqt?: {
-        intentOnFilling: boolean;
+        intentOnFilling?: boolean;


nit but I'd prefer for both of these to be required

steveklebanoff

LGTM pending #184, or we could merge this as-is and merge #184 into master directly

steveklebanoff · 2020-04-22T22:31:38Z

deploy staging

feuGeneA · 2020-04-22T23:50:39Z

deploy staging

github-actions · 2020-04-30T22:27:40Z

🎉 This PR is included in version 1.0.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

feuGeneA added 2 commits April 17, 2020 03:12

Adapt to asset-swapper RFQ-T option restructuring

98ce9bd

A handful of RFQ-T options were restructured in 0xProject/0x-monorepo@957e3eb This adapts to those changes.

feuGeneA mentioned this pull request Apr 17, 2020

asset-swapper: RFQ-T indicative quotes 0xProject/0x-monorepo#2555

Merged

2 tasks

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

feuGeneA self-assigned this Apr 17, 2020

feuGeneA requested review from dekz, fragosti and steveklebanoff April 17, 2020 16:13

steveklebanoff reviewed Apr 17, 2020

View reviewed changes

src/config.ts Outdated Show resolved Hide resolved

steveklebanoff suggested changes Apr 18, 2020

View reviewed changes

src/handlers/swap_handlers.ts Show resolved Hide resolved

src/handlers/swap_handlers.ts Outdated Show resolved Hide resolved

dekz reviewed Apr 20, 2020

View reviewed changes

src/handlers/swap_handlers.ts Outdated Show resolved Hide resolved

src/services/swap_service.ts Outdated Show resolved Hide resolved

feuGeneA added 8 commits April 21, 2020 02:57

Remove Rfqt from gasSchedule

1d9ad66

Relay source breakdown to price endpoint consumer

cc3b686

Incorporate API key into GetSwapQuoteRequestParams

1b9b20c

Addresses review comment #172 (comment)

Get Forwarder address once, not every req

ee23cab

Addresses (at least part of) review comment #172 (comment)

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

e66055d

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

pin asset-swapper to monorepo commit

dd68439

Merge branch 'master' into rfqt-indicative

90d8dca

feuGeneA force-pushed the rfqt-indicative branch from c29677a to 90d8dca Compare April 21, 2020 07:23

Daniel Pyrathon and others added 2 commits April 21, 2020 13:20

Add the correct CORS headers

03b3ac1

Ran prettier

b8641c0

feuGeneA added 2 commits April 21, 2020 20:02

Revert "Ran prettier"

2932e1c

This reverts commit b8641c0.

Revert "Add the correct CORS headers"

b6632b0

This reverts commit 03b3ac1.

steveklebanoff reviewed Apr 22, 2020

View reviewed changes

steveklebanoff approved these changes Apr 22, 2020

View reviewed changes

dekz approved these changes Apr 22, 2020

View reviewed changes

feuGeneA added 2 commits April 22, 2020 11:19

Fix for weird zero orders. depends on new AS

1e867df

Utilize the new `isIndicative` flag provided by asset-swapper in order to indicate when indicative quotes should NOT be solicited.

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

Remove diagnostic logging of indicative quotes

3c6f4b9

Addresses review comment #172 (comment)

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

Remove diagnostic logging of indicative quotes

8221dae

Addresses review comment #172 (comment)

feuGeneA added 3 commits April 22, 2020 12:01

Remove diagnostic logging of indicative quotes

b2f6fe7

Addresses review comment #172 (comment)

update monorepo pin

4263b2a

Merge branch 'master' into rfqt-indicative

d00909a

feuGeneA added 2 commits April 22, 2020 14:25

Add test of indicative w/ intentOnFilling omitted

7fec8d9

Clarify RFQ-T enablement logic

a05246d

steveklebanoff reviewed Apr 22, 2020

View reviewed changes

steveklebanoff approved these changes Apr 22, 2020

View reviewed changes

Make axios retryable only when desired (#185)

c584917

update monorepo pin to development branch commit

dca2124

feuGeneA merged commit 5fa9ba3 into master Apr 23, 2020

feuGeneA deleted the rfqt-indicative branch April 23, 2020 00:43

github-actions bot added the released label Apr 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RFQ-T Indicative Quotes #172

RFQ-T Indicative Quotes #172

feuGeneA commented Apr 17, 2020 •

edited

Loading

feuGeneA commented Apr 17, 2020

feuGeneA commented Apr 17, 2020

steveklebanoff left a comment

feuGeneA commented Apr 21, 2020

feuGeneA commented Apr 21, 2020

steveklebanoff Apr 22, 2020

steveklebanoff left a comment •

edited by feuGeneA

Loading

steveklebanoff commented Apr 22, 2020

dekz Apr 22, 2020

feuGeneA Apr 22, 2020

feuGeneA commented Apr 22, 2020

steveklebanoff Apr 22, 2020

steveklebanoff Apr 22, 2020

steveklebanoff Apr 22, 2020

steveklebanoff left a comment

steveklebanoff commented Apr 22, 2020

feuGeneA commented Apr 22, 2020

github-actions bot commented Apr 30, 2020

RFQ-T Indicative Quotes #172

RFQ-T Indicative Quotes #172

Conversation

feuGeneA commented Apr 17, 2020 • edited Loading

feuGeneA commented Apr 17, 2020

feuGeneA commented Apr 17, 2020

steveklebanoff left a comment

Choose a reason for hiding this comment

feuGeneA commented Apr 21, 2020

feuGeneA commented Apr 21, 2020

steveklebanoff Apr 22, 2020

Choose a reason for hiding this comment

steveklebanoff left a comment • edited by feuGeneA Loading

Choose a reason for hiding this comment

steveklebanoff commented Apr 22, 2020

dekz Apr 22, 2020

Choose a reason for hiding this comment

feuGeneA Apr 22, 2020

Choose a reason for hiding this comment

feuGeneA commented Apr 22, 2020

steveklebanoff Apr 22, 2020

Choose a reason for hiding this comment

steveklebanoff Apr 22, 2020

Choose a reason for hiding this comment

steveklebanoff Apr 22, 2020

Choose a reason for hiding this comment

steveklebanoff left a comment

Choose a reason for hiding this comment

steveklebanoff commented Apr 22, 2020

feuGeneA commented Apr 22, 2020

github-actions bot commented Apr 30, 2020

feuGeneA commented Apr 17, 2020 •

edited

Loading

steveklebanoff left a comment •

edited by feuGeneA

Loading