-
Notifications
You must be signed in to change notification settings - Fork 465
asset-swapper: RFQ-T firm quotes #2541
Changes from 5 commits
3cdccb7
403fb38
343caa1
0e1572c
4b5a02c
5602aac
1670b21
98fc780
37390e0
aadcf8f
63bfd23
121d51b
5f23833
227676c
93872ad
3c795d3
fa617d2
70add44
5f4778c
8cdc05f
39c2a75
eb5ec58
264407b
0cb5e45
84adbcb
d55108a
ccc9e18
bb15f78
27ca75d
b854fcd
58d6256
47ef7ff
aee758e
3bdfcb8
513ddb4
1da8f68
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 |
---|---|---|
|
@@ -27,6 +27,7 @@ import { DexOrderSampler } from './utils/market_operation_utils/sampler'; | |
import { orderPrunerUtils } from './utils/order_prune_utils'; | ||
import { OrderStateUtils } from './utils/order_state_utils'; | ||
import { ProtocolFeeUtils } from './utils/protocol_fee_utils'; | ||
import { QuoteRequestor } from './utils/quote_requestor'; | ||
import { sortingUtils } from './utils/sorting_utils'; | ||
import { SwapQuoteCalculator } from './utils/swap_quote_calculator'; | ||
|
||
|
@@ -36,12 +37,15 @@ export class SwapQuoter { | |
public readonly expiryBufferMs: number; | ||
public readonly chainId: number; | ||
public readonly permittedOrderFeeTypes: Set<OrderPrunerPermittedFeeTypes>; | ||
public readonly rfqtTakerApiKeyWhitelist: string[]; | ||
feuGeneA marked this conversation as resolved.
Show resolved
Hide resolved
|
||
public readonly rfqtMakerEndpoints: string[]; | ||
private readonly _contractAddresses: ContractAddresses; | ||
private readonly _protocolFeeUtils: ProtocolFeeUtils; | ||
private readonly _swapQuoteCalculator: SwapQuoteCalculator; | ||
private readonly _devUtilsContract: DevUtilsContract; | ||
private readonly _marketOperationUtils: MarketOperationUtils; | ||
private readonly _orderStateUtils: OrderStateUtils; | ||
private readonly _quoteRequestor: QuoteRequestor; | ||
|
||
/** | ||
* Instantiates a new SwapQuoter instance given existing liquidity in the form of orders and feeOrders. | ||
|
@@ -144,7 +148,12 @@ export class SwapQuoter { | |
* | ||
* @return An instance of SwapQuoter | ||
*/ | ||
constructor(supportedProvider: SupportedProvider, orderbook: Orderbook, options: Partial<SwapQuoterOpts> = {}) { | ||
constructor( | ||
supportedProvider: SupportedProvider, | ||
orderbook: Orderbook, | ||
options: Partial<SwapQuoterOpts> = {}, | ||
quoteRequestor?: QuoteRequestor, | ||
feuGeneA marked this conversation as resolved.
Show resolved
Hide resolved
|
||
) { | ||
const { | ||
chainId, | ||
expiryBufferMs, | ||
|
@@ -161,10 +170,13 @@ export class SwapQuoter { | |
this.orderbook = orderbook; | ||
this.expiryBufferMs = expiryBufferMs; | ||
this.permittedOrderFeeTypes = permittedOrderFeeTypes; | ||
this.rfqtTakerApiKeyWhitelist = options.rfqtTakerApiKeyWhitelist || []; | ||
this.rfqtMakerEndpoints = options.rfqtMakerEndpoints || []; | ||
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.
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. Are we asserting they are 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. Addressed the We are not directly asserting that they are URI's. We're just passing them through to the quote requestor, and letting it fail silently if they're not valid. |
||
this._contractAddresses = options.contractAddresses || getContractAddressesForChainOrThrow(chainId); | ||
this._devUtilsContract = new DevUtilsContract(this._contractAddresses.devUtils, provider); | ||
this._protocolFeeUtils = new ProtocolFeeUtils(constants.PROTOCOL_FEE_UTILS_POLLING_INTERVAL_IN_MS); | ||
this._orderStateUtils = new OrderStateUtils(this._devUtilsContract); | ||
this._quoteRequestor = quoteRequestor || new QuoteRequestor(this.rfqtMakerEndpoints); | ||
const sampler = new DexOrderSampler( | ||
new IERC20BridgeSamplerContract(this._contractAddresses.erc20BridgeSampler, this.provider, { | ||
gas: samplerGasLimit, | ||
|
@@ -523,10 +535,29 @@ export class SwapQuoter { | |
gasPrice = await this._protocolFeeUtils.getGasPriceEstimationOrThrowAsync(); | ||
} | ||
// get the relevant orders for the makerAsset | ||
let prunedOrders = await this._getSignedOrdersAsync(makerAssetData, takerAssetData); | ||
let orders = await this._getSignedOrdersAsync(makerAssetData, takerAssetData); | ||
feuGeneA marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if (options.intentOnFilling && options.apiKey) { | ||
if (!this.rfqtTakerApiKeyWhitelist.includes(options.apiKey)) { | ||
throw new Error('API key not permissioned for RFQ-T'); | ||
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. Is this a throwable validation at this layer? Or should it fail silently and give a normal quote? If someone gets delisted they're going to start getting Errors rather than quotes, seems quite disruptive. 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. Yeah, agree that we should fallback to providing a regular quote in this case. 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. Addressed in 5f4778c |
||
} | ||
if (!options.takerAddress || options.takerAddress === '0x0000000000000000000000000000000000000000') { | ||
feuGeneA marked this conversation as resolved.
Show resolved
Hide resolved
|
||
throw new Error('RFQ-T requests must specify a taker address'); | ||
} | ||
orders = orders.concat( | ||
feuGeneA marked this conversation as resolved.
Show resolved
Hide resolved
|
||
await this._quoteRequestor.requestRfqtFirmQuotesAsync( | ||
makerAssetData, | ||
takerAssetData, | ||
assetFillAmount, | ||
marketOperation, | ||
options.intentOnFilling, | ||
feuGeneA marked this conversation as resolved.
Show resolved
Hide resolved
|
||
options.apiKey, | ||
options.takerAddress, | ||
), | ||
); | ||
} | ||
// if no native orders, pass in a dummy order for the sampler to have required metadata for sampling | ||
if (prunedOrders.length === 0) { | ||
prunedOrders = [ | ||
if (orders.length === 0) { | ||
orders = [ | ||
createDummyOrderForSampler(makerAssetData, takerAssetData, this._contractAddresses.uniswapBridge), | ||
]; | ||
} | ||
|
@@ -535,14 +566,14 @@ export class SwapQuoter { | |
|
||
if (marketOperation === MarketOperation.Buy) { | ||
swapQuote = await this._swapQuoteCalculator.calculateMarketBuySwapQuoteAsync( | ||
prunedOrders, | ||
orders, | ||
assetFillAmount, | ||
gasPrice, | ||
calculateSwapQuoteOpts, | ||
); | ||
} else { | ||
swapQuote = await this._swapQuoteCalculator.calculateMarketSellSwapQuoteAsync( | ||
prunedOrders, | ||
orders, | ||
assetFillAmount, | ||
gasPrice, | ||
calculateSwapQuoteOpts, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,99 @@ | ||
import { assetDataUtils, SignedOrder } from '@0x/order-utils'; | ||
import { ERC20AssetData } from '@0x/types'; | ||
import { BigNumber, logUtils } from '@0x/utils'; | ||
import Axios, { AxiosResponse } from 'axios'; | ||
|
||
import { MarketOperation } from '../types'; | ||
|
||
/** | ||
* Request quotes from RFQ-T providers | ||
*/ | ||
export class QuoteRequestor { | ||
private readonly _rfqtMakerEndpoints: string[]; | ||
|
||
constructor(rfqtMakerEndpoints: string[]) { | ||
this._rfqtMakerEndpoints = rfqtMakerEndpoints; | ||
} | ||
|
||
public async requestRfqtFirmQuotesAsync( | ||
makerAssetData: string, | ||
takerAssetData: string, | ||
assetFillAmount: BigNumber, | ||
marketOperation: MarketOperation, | ||
intentOnFilling: boolean, | ||
feuGeneA marked this conversation as resolved.
Show resolved
Hide resolved
|
||
takerApiKey: string, | ||
takerAddress: string, | ||
): Promise<SignedOrder[]> { | ||
const makerEndpointMaxResponseTimeMs = 1000; | ||
feuGeneA marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
const getTokenAddressOrThrow = (assetData: string): string => { | ||
feuGeneA marked this conversation as resolved.
Show resolved
Hide resolved
|
||
const decodedAssetData = assetDataUtils.decodeAssetDataOrThrow(assetData); | ||
if (decodedAssetData.hasOwnProperty('tokenAddress')) { | ||
feuGeneA marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// type cast necessary here as decodeAssetDataOrThrow returns | ||
// an AssetData object, which doesn't necessarily contain a | ||
// token address. (it could possibly be a StaticCallAssetData, | ||
// which lacks an address.) so we'll just assume it's a token | ||
// here. should be safe, with the enclosing guard condition | ||
// and subsequent error. | ||
// tslint:disable-next-line:no-unnecessary-type-assertion | ||
return (decodedAssetData as ERC20AssetData).tokenAddress; | ||
} else { | ||
feuGeneA marked this conversation as resolved.
Show resolved
Hide resolved
|
||
throw new Error( | ||
`Decoded asset data (${JSON.stringify(decodedAssetData)}) does not contain a token address`, | ||
); | ||
} | ||
}; | ||
|
||
const buyToken = getTokenAddressOrThrow(makerAssetData); | ||
const sellToken = getTokenAddressOrThrow(takerAssetData); | ||
|
||
// create an array of promises for quote responses, using "undefined" | ||
// as a placeholder for failed requests. | ||
|
||
const responsePromises: Array<Promise<AxiosResponse<SignedOrder> | undefined>> = []; | ||
for (const rfqtMakerEndpoint of this._rfqtMakerEndpoints) { | ||
responsePromises.push( | ||
feuGeneA marked this conversation as resolved.
Show resolved
Hide resolved
|
||
Axios.get(`${rfqtMakerEndpoint}/quote`, { | ||
feuGeneA marked this conversation as resolved.
Show resolved
Hide resolved
|
||
headers: { '0x-api-key': takerApiKey }, | ||
params: { | ||
sellToken, | ||
buyToken, | ||
buyAmount: marketOperation === MarketOperation.Buy ? assetFillAmount.toString() : undefined, | ||
feuGeneA marked this conversation as resolved.
Show resolved
Hide resolved
|
||
sellAmount: marketOperation === MarketOperation.Sell ? assetFillAmount.toString() : undefined, | ||
takerAddress, | ||
feuGeneA marked this conversation as resolved.
Show resolved
Hide resolved
|
||
}, | ||
timeout: makerEndpointMaxResponseTimeMs, | ||
}).catch(err => { | ||
logUtils.warn( | ||
`Failed to get RFQ-T quote from market maker endpoint ${rfqtMakerEndpoint} for API key ${takerApiKey} for taker address ${takerAddress}: ${JSON.stringify( | ||
feuGeneA marked this conversation as resolved.
Show resolved
Hide resolved
|
||
err, | ||
)}`, | ||
); | ||
return undefined; | ||
}), | ||
); | ||
} | ||
|
||
const responsesIfDefined: Array<AxiosResponse<SignedOrder> | undefined> = await Promise.all(responsePromises); | ||
|
||
const responses: Array<AxiosResponse<SignedOrder>> = responsesIfDefined.filter( | ||
feuGeneA marked this conversation as resolved.
Show resolved
Hide resolved
|
||
(respIfDefd): respIfDefd is AxiosResponse<SignedOrder> => respIfDefd !== undefined, | ||
); | ||
|
||
const ordersWithStringInts = responses.map(response => response.data); // not yet BigNumber | ||
steveklebanoff marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
const orders: SignedOrder[] = ordersWithStringInts.map(orderWithStringInts => { | ||
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. Find a helper function do this for us. @steveklebanoff ? 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 have a feeliing they exist in 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. @dekz Are you open to exporting Or is there somewhere else where this is already defined & exported? |
||
return { | ||
...orderWithStringInts, | ||
makerAssetAmount: new BigNumber(orderWithStringInts.makerAssetAmount), | ||
takerAssetAmount: new BigNumber(orderWithStringInts.takerAssetAmount), | ||
makerFee: new BigNumber(orderWithStringInts.makerFee), | ||
takerFee: new BigNumber(orderWithStringInts.takerFee), | ||
expirationTimeSeconds: new BigNumber(orderWithStringInts.expirationTimeSeconds), | ||
salt: new BigNumber(orderWithStringInts.salt), | ||
}; | ||
}); | ||
|
||
return orders; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,7 +11,7 @@ | |
"build": "tsc -b", | ||
"build:ci": "yarn build", | ||
"clean": "shx rm -rf lib ${npm_package_config_snapshot_name} ${npm_package_config_snapshot_name}-*.zip", | ||
"lint": "tslint --format stylish --project .", | ||
"lint": "tslint --format stylish --project . && prettier --check 'src/**/*.{ts,tsx,json,md}' --config ../../.prettierrc", | ||
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 seems like you agreed to keep 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. Addressed in 1da8f68 |
||
"fix": "tslint --fix --format stylish --project .", | ||
"test": "yarn run_mocha", | ||
"test:circleci": "yarn test", | ||
|
@@ -28,7 +28,8 @@ | |
"build:snapshot:docker": "docker build --tag ${npm_package_config_docker_snapshot_name}:${npm_package_version} --tag ${npm_package_config_docker_snapshot_name}:latest .", | ||
"publish:snapshot": "aws s3 cp ${npm_package_config_snapshot_name}-${npm_package_version}.zip ${npm_package_config_s3_snapshot_bucket} && aws s3 cp ${npm_package_config_s3_snapshot_bucket}/${npm_package_config_snapshot_name}-${npm_package_version}.zip ${npm_package_config_s3_snapshot_bucket}/${npm_package_config_snapshot_name}-latest.zip", | ||
"publish:snapshot:docker": "docker push ${npm_package_config_docker_snapshot_name}:latest && docker push ${npm_package_config_docker_snapshot_name}:${npm_package_version}", | ||
"test_contract_configs": "node ./lib/test_contract_configs.js" | ||
"test_contract_configs": "node ./lib/test_contract_configs.js", | ||
"publish:private": "yarn build && gitpkg publish" | ||
}, | ||
"config": { | ||
"s3_snapshot_bucket": "s3://ganache-snapshots.0x.org", | ||
|
@@ -42,6 +43,9 @@ | |
"0x-migrate": "bin/0x-migrate.js" | ||
}, | ||
"license": "Apache-2.0", | ||
"gitpkg": { | ||
"registry": "git@github.com:0xProject/gitpkg-registry.git" | ||
}, | ||
"devDependencies": { | ||
"@0x/dev-utils": "^3.2.1", | ||
"@0x/ts-doc-gen": "^0.0.22", | ||
|
@@ -50,6 +54,7 @@ | |
"@types/yargs": "^11.0.0", | ||
"chai": "^4.0.1", | ||
"dirty-chai": "^2.0.1", | ||
"gitpkg": "https://github.com/0xProject/gitpkg.git", | ||
"make-promises-safe": "^1.1.0", | ||
"mocha": "^6.2.0", | ||
"npm-run-all": "^4.1.2", | ||
|
@@ -69,6 +74,7 @@ | |
"@0x/contracts-dev-utils": "^1.3.3", | ||
"@0x/contracts-erc1155": "^2.1.5", | ||
"@0x/contracts-erc20": "^3.1.5", | ||
"@0x/contracts-erc20-bridge-sampler": "^1.5.1", | ||
"@0x/contracts-erc721": "^3.1.5", | ||
"@0x/contracts-exchange": "^3.2.5", | ||
"@0x/contracts-exchange-forwarder": "^4.2.5", | ||
|
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 have the established pattern of having
prettier
as it's own script: https://github.com/0xProject/0x-monorepo/blob/development/packages/orderbook/package.json#L25There 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.
I introduced a
prettier
script in eb5ec58, but I also kept its invocation in thelint
script.I feel pretty strongly that
yarn lint
should run everything that CI'sstatic-tests
runs. I'm open to having my mind changed if anyone else feels strongly about it, but that's why I did it this way.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.
I think it's pretty weird to have different
lint
behavior forasset-swapper
compared to all other packages. IMHO it's important to be consistent here.Unless we want to make this change wholesale across all repos (a can of worms I don't think we should open right now), I think we should make this consistent.