diff --git a/lib/entities/HardQuoteRequest.ts b/lib/entities/HardQuoteRequest.ts index 9404907e..bf6b7469 100644 --- a/lib/entities/HardQuoteRequest.ts +++ b/lib/entities/HardQuoteRequest.ts @@ -100,6 +100,36 @@ export class HardQuoteRequest { return utils.getAddress(this.order.info.outputs[0].token); } + /** + * Whether every output pays outputs[0].token. + * + * `totalOutputAmountStart` below sums the outputs as raw integers, and `toCleanJSON` + * sends quoters that one scalar alongside a single `tokenOut` taken from outputs[0]. + * Both are only meaningful when the outputs agree on a token: otherwise the amount + * mixes assets of different denominations, and the quoter prices something the order + * does not contain. Multi-output orders are the normal case, since fee outputs use the + * swapper output's token. + */ + public get hasUniformOutputTokens(): boolean { + const tokenOut = this.tokenOut; + for (const output of this.order.info.outputs) { + if (utils.getAddress(output.token) !== tokenOut) { + return false; + } + } + + return true; + } + + public get outputTokens(): string[] { + const tokens: string[] = []; + for (const output of this.order.info.outputs) { + tokens.push(utils.getAddress(output.token)); + } + + return tokens; + } + public get totalOutputAmountStart(): BigNumber { let amount = BigNumber.from(0); for (const output of this.order.info.outputs) { diff --git a/lib/entities/aws-metrics-logger.ts b/lib/entities/aws-metrics-logger.ts index 91a473e8..45b99392 100644 --- a/lib/entities/aws-metrics-logger.ts +++ b/lib/entities/aws-metrics-logger.ts @@ -49,6 +49,8 @@ export enum Metric { QUOTE_POST_ERROR = 'QUOTE_POST_ERROR', QUOTE_POST_ATTEMPT = 'QUOTE_POST_ATTEMPT', + // Hard quote rejected before the auction because its outputs span multiple tokens. + QUOTE_MIXED_OUTPUT_TOKENS = 'QUOTE_MIXED_OUTPUT_TOKENS', RFQ_REQUESTED = 'RFQ_REQUESTED', RFQ_SUCCESS = 'RFQ_SUCCESS', diff --git a/lib/handlers/hard-quote/handler.ts b/lib/handlers/hard-quote/handler.ts index bd8a391b..7b38aae3 100644 --- a/lib/handlers/hard-quote/handler.ts +++ b/lib/handlers/hard-quote/handler.ts @@ -22,7 +22,13 @@ import { V3HardQuoteResponse } from '../../entities/V3HardQuoteResponse'; import { checkDefined } from '../../preconditions/preconditions'; import { getBestQuote } from '../../quoters/best-quote'; import { ChainId } from '../../util/chains'; -import { NoQuotesAvailable, OrderDeadlineExpired, OrderPostError, UnknownOrderCosignerError } from '../../util/errors'; +import { + MixedOutputTokensError, + NoQuotesAvailable, + OrderDeadlineExpired, + OrderPostError, + UnknownOrderCosignerError, +} from '../../util/errors'; import { timestampInMstoSeconds } from '../../util/time'; import { APIGLambdaHandler } from '../base'; import { APIHandleRequestParams, ErrorResponse, Response } from '../base/api-handler'; @@ -68,6 +74,22 @@ export class QuoteHandler extends APIGLambdaHandler< requestBody.tokenInChainId ); const request = HardQuoteRequest.fromHardRequestBody(requestBody, orderType); + + // Reject before the auction rather than after. We quote this order as a single + // tokenOut plus one summed amount, so a quoter pricing an order whose outputs span + // multiple tokens is bidding on something the order does not contain. Winning that + // auction hands it exclusivity on an order it has to fade, which costs it fill rate + // and circuit-breaker standing for a request it was never able to price. The order + // service rejects the same shape at POST /order, so the fill could not happen. + if (!request.hasUniformOutputTokens) { + log.error( + { tokenOut: request.tokenOut, outputTokens: request.outputTokens, requestId: request.requestId }, + 'Order outputs span multiple tokens' + ); + metric.putMetric(Metric.QUOTE_MIXED_OUTPUT_TOKENS, 1, MetricLoggerUnit.Count); + throw new MixedOutputTokensError(); + } + // re-create KmsClient every call to avoid clock skew issue // https://github.com/aws/aws-sdk-js-v3/issues/6400 const kmsKeyId = checkDefined(process.env.KMS_KEY_ID, 'KMS_KEY_ID is not defined'); diff --git a/lib/util/errors.ts b/lib/util/errors.ts index b9453ccc..274e4c39 100644 --- a/lib/util/errors.ts +++ b/lib/util/errors.ts @@ -74,6 +74,28 @@ export class OrderDeadlineExpired extends CustomError { } } +export class MixedOutputTokensError extends CustomError { + private static MESSAGE = + 'All order outputs must pay the same token. A hard quote is priced as a single tokenOut and a single summed amount, so an order whose outputs span multiple tokens cannot be quoted.'; + + constructor(message?: string) { + super(message ?? MixedOutputTokensError.MESSAGE); + // Set the prototype explicitly. + Object.setPrototypeOf(this, MixedOutputTokensError.prototype); + } + + toJSON(id?: string): APIGatewayProxyResult { + return { + statusCode: 400, + body: JSON.stringify({ + errorCode: ErrorCode.ValidationError, + detail: this.message, + id, + }), + }; + } +} + export class UnknownOrderCosignerError extends CustomError { private static MESSAGE = 'Unknown cosigner'; diff --git a/test/entities/HardQuoteRequest.test.ts b/test/entities/HardQuoteRequest.test.ts index 8859420d..b6b3f061 100644 --- a/test/entities/HardQuoteRequest.test.ts +++ b/test/entities/HardQuoteRequest.test.ts @@ -98,6 +98,58 @@ describe('QuoteRequest', () => { expect(request.type).toEqual(TradeType.EXACT_INPUT); }); + describe('hasUniformOutputTokens', () => { + const requestWithOutputs = (outputs: { token: string; startAmount: BigNumber }[]) => { + const order = new UnsignedV2DutchOrder( + getOrderInfo({ + outputs: outputs.map((output) => ({ + token: output.token, + startAmount: output.startAmount, + endAmount: output.startAmount, + recipient: ethers.constants.AddressZero, + })), + }), + CHAIN_ID + ); + return makeRequest({ encodedInnerOrder: order.serialize(), innerSig: '0x' }); + }; + + it('is true for a single output', () => { + expect(requestWithOutputs([{ token: TOKEN_OUT, startAmount: RAW_AMOUNT }]).hasUniformOutputTokens).toBe(true); + }); + + it('is true for a fee output in the swapper output token', () => { + const request = requestWithOutputs([ + { token: TOKEN_OUT, startAmount: RAW_AMOUNT }, + { token: TOKEN_OUT, startAmount: RAW_AMOUNT.div(1000) }, + ]); + expect(request.hasUniformOutputTokens).toBe(true); + expect(request.outputTokens).toEqual([TOKEN_OUT, TOKEN_OUT]); + }); + + it('is true regardless of address casing', () => { + const request = requestWithOutputs([ + { token: TOKEN_OUT.toLowerCase(), startAmount: RAW_AMOUNT }, + { token: TOKEN_OUT, startAmount: RAW_AMOUNT.div(1000) }, + ]); + expect(request.hasUniformOutputTokens).toBe(true); + }); + + // The scalar the quoter is asked to beat would otherwise mix 1 WETH with a raw + // 1000000 of a 6 decimal token. + it('is false when a higher output pays a different token', () => { + const request = requestWithOutputs([ + { token: TOKEN_OUT, startAmount: RAW_AMOUNT }, + { token: TOKEN_IN, startAmount: BigNumber.from('1000000') }, + ]); + expect(request.hasUniformOutputTokens).toBe(false); + expect(request.outputTokens).toEqual([TOKEN_OUT, TOKEN_IN]); + // tokenOut and amount both stay silent about the second token + expect(request.tokenOut).toEqual(TOKEN_OUT); + expect(request.numOutputs).toEqual(2); + }); + }); + it('toCleanJSON', async () => { const order = new UnsignedV2DutchOrder( getOrderInfo({ diff --git a/test/handlers/hard-quote/handler.test.ts b/test/handlers/hard-quote/handler.test.ts index 9d829233..2ba7bf21 100644 --- a/test/handlers/hard-quote/handler.test.ts +++ b/test/handlers/hard-quote/handler.test.ts @@ -114,6 +114,90 @@ describe('Quote handler', () => { jest.clearAllMocks(); }); + describe('orders whose outputs span multiple tokens', () => { + const mixedOutputOrder = () => + getOrder({ + cosigner: cosignerWallet.address, + outputs: [ + { + token: TOKEN_OUT, + startAmount: RAW_AMOUNT, + endAmount: RAW_AMOUNT, + recipient: ethers.constants.AddressZero, + }, + { + token: TOKEN_IN, + startAmount: BigNumber.from('4800000000000'), + endAmount: BigNumber.from('4800000000000'), + recipient: ethers.constants.AddressZero, + }, + ], + }); + + it('are rejected with a 400', async () => { + const quoters = [new MockQuoter(logger, 1, 1)]; + const request = await getRequest(mixedOutputOrder()); + + const response: APIGatewayProxyResult = await getQuoteHandler(quoters).handler( + getEvent(request), + {} as unknown as Context + ); + + expect(response.statusCode).toEqual(400); + expect(JSON.parse(response.body).detail).toContain('All order outputs must pay the same token'); + }); + + // The point of rejecting here rather than after the auction: a quoter that wins on a + // price it could not have computed gets exclusivity on an order it has to fade. + it('never reach the quoters', async () => { + const quoter = new MockQuoter(logger, 1, 1); + const quoteSpy = jest.spyOn(quoter, 'quote'); + const request = await getRequest(mixedOutputOrder()); + + await getQuoteHandler([quoter]).handler(getEvent(request), {} as unknown as Context); + + expect(quoteSpy).not.toHaveBeenCalled(); + }); + + it('are rejected before the order is cosigned', async () => { + const request = await getRequest(mixedOutputOrder()); + + await getQuoteHandler([new MockQuoter(logger, 1, 1)]).handler(getEvent(request), {} as unknown as Context); + + expect(mockSignDigest).not.toHaveBeenCalled(); + }); + + it('still accepts a fee output in the swapper output token', async () => { + const quoters = [new MockQuoter(logger, 1, 1)]; + const request = await getRequest( + getOrder({ + cosigner: cosignerWallet.address, + outputs: [ + { + token: TOKEN_OUT, + startAmount: RAW_AMOUNT, + endAmount: RAW_AMOUNT, + recipient: ethers.constants.AddressZero, + }, + { + token: TOKEN_OUT, + startAmount: RAW_AMOUNT.div(1000), + endAmount: RAW_AMOUNT.div(1000), + recipient: ethers.constants.AddressZero, + }, + ], + }) + ); + + const response: APIGatewayProxyResult = await getQuoteHandler(quoters).handler( + getEvent(request), + {} as unknown as Context + ); + + expect(response.statusCode).toEqual(200); + }); + }); + it('Simple request and response', async () => { const quoters = [new MockQuoter(logger, 1, 1)]; const request = await getRequest(getOrder({ cosigner: cosignerWallet.address }));