Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Rfqt included #293

Merged
merged 16 commits into from
Jul 27, 2020
5 changes: 0 additions & 5 deletions src/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ import {
DEFAULT_LOCAL_POSTGRES_URI,
DEFAULT_LOGGER_INCLUDE_TIMESTAMP,
DEFAULT_QUOTE_SLIPPAGE_PERCENTAGE,
DEFAULT_RFQT_SKIP_BUY_REQUESTS,
NULL_ADDRESS,
NULL_BYTES,
QUOTE_ORDER_EXPIRATION_BUFFER_MS,
Expand Down Expand Up @@ -187,10 +186,6 @@ export const RFQT_MAKER_ASSET_OFFERINGS: RfqtMakerAssetOfferings = _.isEmpty(pro
);

// tslint:disable-next-line:boolean-naming
export const RFQT_SKIP_BUY_REQUESTS: boolean = _.isEmpty(process.env.RFQT_SKIP_BUY_REQUESTS)
? DEFAULT_RFQT_SKIP_BUY_REQUESTS
: assertEnvVarType('RFQT_SKIP_BUY_REQUESTS', process.env.RFQT_SKIP_BUY_REQUESTS, EnvVarType.Boolean);

export const RFQT_REQUEST_MAX_RESPONSE_MS = 600;

// Whitelisted 0x API keys that can use the meta-txn /submit endpoint
Expand Down
1 change: 0 additions & 1 deletion src/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@ export const PROTOCOL_FEE_UTILS_POLLING_INTERVAL_IN_MS = 6000;
export const UNWRAP_QUOTE_GAS = new BigNumber(60000);
export const WRAP_QUOTE_GAS = UNWRAP_QUOTE_GAS;
export const ONE_GWEI = new BigNumber(1000000000);
export const DEFAULT_RFQT_SKIP_BUY_REQUESTS = false;

// API namespaces
export const SRA_PATH = '/sra/v3';
Expand Down
6 changes: 6 additions & 0 deletions src/errors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -131,10 +131,16 @@ export enum ValidationErrorCodes {
InvalidOrder = 1007,
InternalError = 1008,
TokenNotSupported = 1009,
FieldInvalid = 1010,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image
We should update the documentation here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

}

export enum ValidationErrorReasons {
PercentageOutOfRange = 'MUST_BE_LESS_THAN_OR_EQUAL_TO_ONE',
ConflictingFilteringArguments = 'CONFLICTING_FILTERING_ARGUMENTS',
ArgumentNotYetSupported = 'ARGUMENT_NOT_YET_SUPPORTED',
InvalidApiKey = 'INVALID_API_KEY',
TakerAddressInvalid = 'TAKER_ADDRESS_INVALID',
RequiresIntentOnFilling = 'REQUIRES_INTENT_ON_FILLING',
}
export abstract class AlertError {
public abstract message: string;
Expand Down
28 changes: 20 additions & 8 deletions src/handlers/swap_handlers.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import { SwapQuoterError } from '@0x/asset-swapper';
import { RfqtRequestOpts, SwapQuoterError } from '@0x/asset-swapper';
import { BigNumber, NULL_ADDRESS } from '@0x/utils';
import * as express from 'express';
import * as HttpStatus from 'http-status-codes';

import { CHAIN_ID } from '../config';
import { CHAIN_ID, RFQT_API_KEY_WHITELIST } from '../config';
import { DEFAULT_QUOTE_SLIPPAGE_PERCENTAGE, SWAP_DOCS_URL } from '../constants';
import {
InternalServerError,
Expand Down Expand Up @@ -194,6 +194,7 @@ export class SwapHandlers {
: {
intentOnFilling: rfqt.intentOnFilling,
isIndicative: rfqt.isIndicative,
nativeExclusivelyRFQT: rfqt.nativeExclusivelyRFQT,
},
skipValidation,
swapVersion,
Expand Down Expand Up @@ -269,17 +270,28 @@ const parseGetSwapQuoteRequestParams = (
},
]);
}
const excludedSources =
req.query.excludedSources === undefined
? undefined
: parseUtils.parseStringArrForERC20BridgeSources((req.query.excludedSources as string).split(','));
const affiliateAddress = req.query.affiliateAddress as string;

const apiKey = req.header('0x-api-key');
const rfqt =
// tslint:disable-next-line: boolean-naming
const { excludedSources, nativeExclusivelyRFQT } = parseUtils.parseRequestForExcludedSources(
{
excludedSources: req.query.excludedSources as string | undefined,
includedSources: req.query.includedSources as string | undefined,
intentOnFilling: req.query.intentOnFilling as string | undefined,
takerAddress,
apiKey,
},
RFQT_API_KEY_WHITELIST,
endpoint,
);

const affiliateAddress = req.query.affiliateAddress as string;
const rfqt: Pick<RfqtRequestOpts, 'intentOnFilling' | 'isIndicative' | 'nativeExclusivelyRFQT'> =
PirosB3 marked this conversation as resolved.
Show resolved Hide resolved
takerAddress && apiKey
? {
intentOnFilling: endpoint === 'quote' && req.query.intentOnFilling === 'true',
isIndicative: endpoint === 'price',
nativeExclusivelyRFQT,
}
: undefined;
// tslint:disable-next-line:boolean-naming
Expand Down
2 changes: 1 addition & 1 deletion src/schemas/swap_quote_request_schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
"excludedSources": {
"type": "string"
},
"apiKey": {
"includedSources": {
"type": "string"
},
"intentOnFilling": {
Expand Down
5 changes: 1 addition & 4 deletions src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -496,10 +496,7 @@ export interface GetSwapQuoteRequestParams {
gasPrice?: BigNumber;
excludedSources?: ERC20BridgeSource[];
affiliateAddress?: string;
rfqt?: {
intentOnFilling?: boolean;
isIndicative?: boolean;
};
rfqt?: Pick<RfqtRequestOpts, 'intentOnFilling' | 'isIndicative' | 'nativeExclusivelyRFQT'>;
skipValidation: boolean;
apiKey?: string;
}
Expand Down
109 changes: 109 additions & 0 deletions src/utils/parse_utils.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { assert } from '@0x/assert';
import { ERC20BridgeSource } from '@0x/asset-swapper';

import { ValidationError, ValidationErrorCodes, ValidationErrorReasons } from '../errors';
import {
MetaTransactionDailyLimiterConfig,
MetaTransactionRateLimitConfig,
Expand All @@ -10,7 +11,115 @@ import {

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

interface ParseRequestForExcludedSourcesParams {
PirosB3 marked this conversation as resolved.
Show resolved Hide resolved
takerAddress?: string;
excludedSources?: string;
includedSources?: string;
intentOnFilling?: string;
apiKey?: string;
}

/**
* This constant contains, as keys, all ERC20BridgeSource types except from `Native`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great documentation, thank you

* As we add more bridge sources to AssetSwapper, we want to keep ourselves accountable to add
* them to this constant. Since there isn't a good way to enumerate over enums, we use a obect type.
* The type has been defined in a way that the code won't compile if a new ERC20BridgeSource is added.
*/
const ALL_EXCEPT_NATIVE: { [key in Exclude<ERC20BridgeSource, ERC20BridgeSource.Native>]: boolean } = {
steveklebanoff marked this conversation as resolved.
Show resolved Hide resolved
Uniswap: true,
Balancer: true,
Curve: true,
Eth2Dai: true,
Kyber: true,
LiquidityProvider: true,
MultiBridge: true,
Uniswap_V2: true,
};

export const parseUtils = {
parseRequestForExcludedSources(
request: ParseRequestForExcludedSourcesParams,
validApiKeys: string[],
endpoint: 'price' | 'quote',
): { excludedSources: ERC20BridgeSource[]; nativeExclusivelyRFQT: boolean } {
// Ensure that both filtering arguments cannot be present.
if (request.excludedSources !== undefined && request.includedSources !== undefined) {
PirosB3 marked this conversation as resolved.
Show resolved Hide resolved
throw new ValidationError([
{
field: 'excludedSources',
code: ValidationErrorCodes.IncorrectFormat,
reason: ValidationErrorReasons.ConflictingFilteringArguments,
},
{
field: 'includedSources',
code: ValidationErrorCodes.IncorrectFormat,
reason: ValidationErrorReasons.ConflictingFilteringArguments,
},
]);
}

// If excludedSources is present, parse the string array and return
if (request.excludedSources !== undefined) {
return {
excludedSources: parseUtils.parseStringArrForERC20BridgeSources(request.excludedSources.split(',')),
nativeExclusivelyRFQT: false,
};
}

if (request.includedSources !== undefined) {
// Only RFQT is eligible as of now
if (request.includedSources === 'RFQT') {
// We assume that if a `takerAddress` key is present, it's value was already validated by the JSON
// schema.
if (request.takerAddress === undefined || request.takerAddress.length === 0) {
throw new ValidationError([
{
field: 'takerAddress',
code: ValidationErrorCodes.FieldInvalid,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this just be the RequiredField code?

reason: ValidationErrorReasons.TakerAddressInvalid,
Copy link
Contributor

@steveklebanoff steveklebanoff Jul 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

},
]);
}

// We enforce a valid API key - we don't want to fail silently.
if (!validApiKeys.includes(request.apiKey)) {
steveklebanoff marked this conversation as resolved.
Show resolved Hide resolved
throw new ValidationError([
{
field: '0x-api-key',
code: ValidationErrorCodes.FieldInvalid,
reason: ValidationErrorReasons.InvalidApiKey,
},
]);
}

// If the user is requesting a firm quote, we want to make sure that `intentOnFilling` is set to "true".
if (endpoint === 'quote' && request.intentOnFilling !== 'true') {
throw new ValidationError([
{
field: 'intentOnFilling',
code: ValidationErrorCodes.FieldInvalid,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again - should this just be a RequiredField code?

reason: ValidationErrorReasons.RequiresIntentOnFilling,
},
]);
}

return {
nativeExclusivelyRFQT: true,
excludedSources: Object.keys(ALL_EXCEPT_NATIVE) as ERC20BridgeSource[],
PirosB3 marked this conversation as resolved.
Show resolved Hide resolved
};
} else {
throw new ValidationError([
{
field: 'includedSources',
code: ValidationErrorCodes.IncorrectFormat,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code: FieldInvalid
reason: Invalid API key

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

reason: ValidationErrorReasons.ArgumentNotYetSupported,
},
]);
}
}

return { excludedSources: [], nativeExclusivelyRFQT: false };
},
parseStringArrForERC20BridgeSources(excludedSources: string[]): ERC20BridgeSource[] {
// Need to compare value of the enum instead of the key, as values are used by asset-swapper
// CurveUsdcDaiUsdt = 'Curve_USDC_DAI_USDT' is excludedSources=Curve_USDC_DAI_USDT
Expand Down
121 changes: 121 additions & 0 deletions test/parse_utils_test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,121 @@
import { ERC20BridgeSource } from '@0x/asset-swapper';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

import { expect } from '@0x/contracts-test-utils';
import { NULL_ADDRESS } from '@0x/utils';
import 'mocha';

import { parseUtils } from '../src/utils/parse_utils';

const SUITE_NAME = 'parseUtils';

describe(SUITE_NAME, () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice tests!

it('raises a ValidationError if includedSources is anything else than RFQT', async () => {
expect(() => {
parseUtils.parseRequestForExcludedSources(
{
includedSources: 'Uniswap',
},
[],
'price',
);
}).throws();
});

it('raises a ValidationError if includedSources is RFQT and a taker is not specified', async () => {
expect(() => {
parseUtils.parseRequestForExcludedSources(
{
includedSources: 'RFQT',
},
[],
'price',
);
}).throws();
});

it('raises a ValidationError if API keys are not present or valid', async () => {
expect(() => {
parseUtils.parseRequestForExcludedSources(
{
includedSources: 'RFQT',
takerAddress: NULL_ADDRESS,
apiKey: 'foo',
},
['lorem', 'ipsum'],
'price',
);
}).throws();
});

it('returns excludedSources correctly when excludedSources is present', async () => {
// tslint:disable-next-line: boolean-naming
const { excludedSources, nativeExclusivelyRFQT } = parseUtils.parseRequestForExcludedSources(
{
excludedSources: 'Uniswap,Kyber',
},
[],
'price',
);
expect(excludedSources[0]).to.eql(ERC20BridgeSource.Uniswap);
expect(excludedSources[1]).to.eql(ERC20BridgeSource.Kyber);
expect(nativeExclusivelyRFQT).to.eql(false);
});

it('returns empty array if no includedSources and excludedSources are present', async () => {
// tslint:disable-next-line: boolean-naming
const { excludedSources, nativeExclusivelyRFQT } = parseUtils.parseRequestForExcludedSources({}, [], 'price');
expect(excludedSources.length).to.eql(0);
expect(nativeExclusivelyRFQT).to.eql(false);
});

it('returns excludedSources correctly when includedSources=RFQT', async () => {
// tslint:disable-next-line: boolean-naming
const { excludedSources, nativeExclusivelyRFQT } = parseUtils.parseRequestForExcludedSources(
{
includedSources: 'RFQT',
takerAddress: NULL_ADDRESS,
apiKey: 'ipsum',
},
['lorem', 'ipsum'],
'price',
);
expect(nativeExclusivelyRFQT).to.eql(true);

// Ensure that all sources of liquidity are excluded aside from `Native`.
const allPossibleSources: Set<ERC20BridgeSource> = new Set(
Object.keys(ERC20BridgeSource).map(s => ERC20BridgeSource[s]),
);
for (const source of excludedSources) {
allPossibleSources.delete(source);
}
const allPossibleSourcesArray = Array.from(allPossibleSources);
expect(allPossibleSourcesArray.length).to.eql(1);
expect(allPossibleSourcesArray[0]).to.eql(ERC20BridgeSource.Native);
});

it('raises a ValidationError if includedSources and excludedSources are both present', async () => {
expect(() => {
parseUtils.parseRequestForExcludedSources(
{
excludedSources: 'Native',
includedSources: 'RFQT',
},
[],
'price',
);
}).throws();
});

it('raises a ValidationError if a firm quote is requested and "intentOnFilling" is not set to "true"', async () => {
expect(() => {
parseUtils.parseRequestForExcludedSources(
{
includedSources: 'RFQT',
takerAddress: NULL_ADDRESS,
apiKey: 'ipsum',
},
['lorem', 'ipsum'],
'quote',
);
}).throws();
});
});