-
Notifications
You must be signed in to change notification settings - Fork 238
feat: Rfqt included #293
feat: Rfqt included #293
Changes from 12 commits
f0dd0c1
e470999
8336194
e1666dd
ae8e116
2073d0f
bc3b60d
7afdc84
bd5cf55
0bc3af3
cb332a8
def329a
8a20d60
3569285
464f4db
dab684c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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, | ||
|
@@ -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`. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this just be the |
||
reason: ValidationErrorReasons.TakerAddressInvalid, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looking at this again, I think we could get rid of
EDIT: looks like |
||
}, | ||
]); | ||
} | ||
|
||
// 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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again - should this just be a |
||
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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. code: FieldInvalid There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,121 @@ | ||
import { ERC20BridgeSource } from '@0x/asset-swapper'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
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, () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should update the documentation here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
0xProject/website#318