diff --git a/packages/asset-swapper/CHANGELOG.json b/packages/asset-swapper/CHANGELOG.json index 29a521bd58..cc4d84e97a 100644 --- a/packages/asset-swapper/CHANGELOG.json +++ b/packages/asset-swapper/CHANGELOG.json @@ -125,6 +125,14 @@ { "note": "Pass back fillData from quote reporter", "pr": 2702 + }, + { + "note": "Fix Balancer sampling", + "pr": 2711 + }, + { + "note": "Respect max slippage in EP consumer", + "pr": 2712 } ] }, diff --git a/packages/asset-swapper/contracts/src/BalancerSampler.sol b/packages/asset-swapper/contracts/src/BalancerSampler.sol index 0950cc8e15..8606935574 100644 --- a/packages/asset-swapper/contracts/src/BalancerSampler.sol +++ b/packages/asset-swapper/contracts/src/BalancerSampler.sol @@ -27,6 +27,12 @@ contract BalancerSampler { /// @dev Base gas limit for Balancer calls. uint256 constant private BALANCER_CALL_GAS = 300e3; // 300k + // Balancer math constants + // https://github.com/balancer-labs/balancer-core/blob/master/contracts/BConst.sol + uint256 constant private BONE = 10 ** 18; + uint256 constant private MAX_IN_RATIO = BONE / 2; + uint256 constant private MAX_OUT_RATIO = (BONE / 3) + 1 wei; + struct BalancerState { uint256 takerTokenBalance; uint256 makerTokenBalance; @@ -67,6 +73,11 @@ contract BalancerSampler { poolState.swapFee = pool.getSwapFee(); for (uint256 i = 0; i < numSamples; i++) { + // Handles this revert scenario: + // https://github.com/balancer-labs/balancer-core/blob/master/contracts/BPool.sol#L443 + if (takerTokenAmounts[i] > _bmul(poolState.takerTokenBalance, MAX_IN_RATIO)) { + break; + } (bool didSucceed, bytes memory resultData) = poolAddress.staticcall.gas(BALANCER_CALL_GAS)( abi.encodeWithSelector( @@ -120,6 +131,11 @@ contract BalancerSampler { poolState.swapFee = pool.getSwapFee(); for (uint256 i = 0; i < numSamples; i++) { + // Handles this revert scenario: + // https://github.com/balancer-labs/balancer-core/blob/master/contracts/BPool.sol#L505 + if (makerTokenAmounts[i] > _bmul(poolState.makerTokenBalance, MAX_OUT_RATIO)) { + break; + } (bool didSucceed, bytes memory resultData) = poolAddress.staticcall.gas(BALANCER_CALL_GAS)( abi.encodeWithSelector( @@ -140,4 +156,27 @@ contract BalancerSampler { takerTokenAmounts[i] = sellAmount; } } + + /// @dev Hacked version of Balancer's `bmul` function, returning 0 instead + /// of reverting. + /// https://github.com/balancer-labs/balancer-core/blob/master/contracts/BNum.sol#L63-L73 + /// @param a The first operand. + /// @param b The second operand. + /// @param c The result of the multiplication, or 0 if `bmul` would've reverted. + function _bmul(uint256 a, uint256 b) + private + pure + returns (uint256 c) + { + uint c0 = a * b; + if (a != 0 && c0 / a != b) { + return 0; + } + uint c1 = c0 + (BONE / 2); + if (c1 < c0) { + return 0; + } + uint c2 = c1 / BONE; + return c2; + } } diff --git a/packages/asset-swapper/src/index.ts b/packages/asset-swapper/src/index.ts index 96bf1d4073..f9029e256b 100644 --- a/packages/asset-swapper/src/index.ts +++ b/packages/asset-swapper/src/index.ts @@ -84,6 +84,7 @@ export { export { artifacts } from './artifacts'; export { InsufficientAssetLiquidityError } from './errors'; export { SwapQuoteConsumer } from './quote_consumers/swap_quote_consumer'; +export { getSwapMinBuyAmount } from './quote_consumers/utils'; export { SwapQuoter } from './swap_quoter'; export { AffiliateFee, diff --git a/packages/asset-swapper/src/quote_consumers/exchange_proxy_swap_quote_consumer.ts b/packages/asset-swapper/src/quote_consumers/exchange_proxy_swap_quote_consumer.ts index 3ff074ac04..14e0bc7d72 100644 --- a/packages/asset-swapper/src/quote_consumers/exchange_proxy_swap_quote_consumer.ts +++ b/packages/asset-swapper/src/quote_consumers/exchange_proxy_swap_quote_consumer.ts @@ -30,6 +30,8 @@ import { assert } from '../utils/assert'; import { ERC20BridgeSource, UniswapV2FillData } from '../utils/market_operation_utils/types'; import { getTokenFromAssetData } from '../utils/utils'; +import { getSwapMinBuyAmount } from './utils'; + // tslint:disable-next-line:custom-no-magic-numbers const MAX_UINT256 = new BigNumber(2).pow(256).minus(1); const { NULL_ADDRESS, ZERO_AMOUNT } = constants; @@ -93,6 +95,7 @@ export class ExchangeProxySwapQuoteConsumer implements SwapQuoteConsumerBase { const sellToken = getTokenFromAssetData(quote.takerAssetData); const buyToken = getTokenFromAssetData(quote.makerAssetData); const sellAmount = quote.worstCaseQuoteInfo.totalTakerAssetAmount; + let minBuyAmount = getSwapMinBuyAmount(quote); // VIP routes. if (isDirectUniswapCompatible(quote, optsWithDefaults)) { @@ -111,7 +114,7 @@ export class ExchangeProxySwapQuoteConsumer implements SwapQuoteConsumerBase { return a; }), sellAmount, - quote.worstCaseQuoteInfo.makerAssetAmount, + minBuyAmount, source === ERC20BridgeSource.SushiSwap, ) .getABIEncodedTransactionData(), @@ -210,6 +213,8 @@ export class ExchangeProxySwapQuoteConsumer implements SwapQuoteConsumerBase { ], }), }); + // Adjust the minimum buy amount by the fee. + minBuyAmount = BigNumber.max(0, minBuyAmount.minus(buyTokenFeeAmount)); } if (sellTokenFeeAmount.isGreaterThan(0) && feeRecipient !== NULL_ADDRESS) { @@ -225,7 +230,6 @@ export class ExchangeProxySwapQuoteConsumer implements SwapQuoteConsumerBase { }), }); - const minBuyAmount = BigNumber.max(0, quote.worstCaseQuoteInfo.makerAssetAmount.minus(buyTokenFeeAmount)); const calldataHexString = this._exchangeProxy .transformERC20( isFromETH ? ETH_TOKEN_ADDRESS : sellToken, diff --git a/packages/asset-swapper/src/quote_consumers/utils.ts b/packages/asset-swapper/src/quote_consumers/utils.ts new file mode 100644 index 0000000000..67b2677acf --- /dev/null +++ b/packages/asset-swapper/src/quote_consumers/utils.ts @@ -0,0 +1,33 @@ +import { BigNumber } from '@0x/utils'; +import * as _ from 'lodash'; + +import { MarketOperation, SwapQuote } from '../types'; +import { ERC20BridgeSource } from '../utils/market_operation_utils/types'; + +/** + * Compute the mminimum buy token amount for market operations by inferring + * the slippage from the orders in a quote. We cannot rely on + * `worstCaseQuoteInfo.makerAssetAmount` because that does not stop at + * maximum slippage. + */ +export function getSwapMinBuyAmount(quote: SwapQuote): BigNumber { + if (quote.type === MarketOperation.Buy || quote.isTwoHop) { + return quote.worstCaseQuoteInfo.makerAssetAmount; + } + let slipRatio = new BigNumber(1); + // Infer the allowed maker asset slippage from any non-native order. + for (const o of quote.orders) { + if (o.fills.length === 0 || o.fills[0].source === ERC20BridgeSource.Native) { + // No slippage on native orders. + continue; + } + const totalFillMakerAssetAmount = BigNumber.sum(...o.fills.map(f => f.output)); + slipRatio = o.fillableMakerAssetAmount.div(totalFillMakerAssetAmount); + break; + } + if (slipRatio.gte(1)) { + // No slippage allowed across all orders. + return quote.bestCaseQuoteInfo.makerAssetAmount; + } + return quote.bestCaseQuoteInfo.makerAssetAmount.times(slipRatio).integerValue(BigNumber.ROUND_DOWN); +} diff --git a/packages/asset-swapper/src/utils/market_operation_utils/balancer_utils.ts b/packages/asset-swapper/src/utils/market_operation_utils/balancer_utils.ts index 936cd07e37..6a76c29f03 100644 --- a/packages/asset-swapper/src/utils/market_operation_utils/balancer_utils.ts +++ b/packages/asset-swapper/src/utils/market_operation_utils/balancer_utils.ts @@ -2,6 +2,8 @@ import { BigNumber } from '@0x/utils'; import { bmath, getPoolsWithTokens, parsePoolData } from '@balancer-labs/sor'; import { Decimal } from 'decimal.js'; +import { ZERO_AMOUNT } from './constants'; + // tslint:disable:boolean-naming export interface BalancerPool { @@ -118,6 +120,9 @@ export class BalancerPoolsCache { // tslint:disable completed-docs export function computeBalancerSellQuote(pool: BalancerPool, takerFillAmount: BigNumber): BigNumber { + if (takerFillAmount.isGreaterThan(bmath.bmul(pool.balanceIn, bmath.MAX_IN_RATIO))) { + return ZERO_AMOUNT; + } const weightRatio = pool.weightIn.dividedBy(pool.weightOut); const adjustedIn = bmath.BONE.minus(pool.swapFee) .dividedBy(bmath.BONE) @@ -130,8 +135,8 @@ export function computeBalancerSellQuote(pool: BalancerPool, takerFillAmount: Bi } export function computeBalancerBuyQuote(pool: BalancerPool, makerFillAmount: BigNumber): BigNumber { - if (makerFillAmount.isGreaterThanOrEqualTo(pool.balanceOut)) { - return new BigNumber(0); + if (makerFillAmount.isGreaterThan(bmath.bmul(pool.balanceOut, bmath.MAX_OUT_RATIO))) { + return ZERO_AMOUNT; } const weightRatio = pool.weightOut.dividedBy(pool.weightIn); const diff = pool.balanceOut.minus(makerFillAmount); diff --git a/packages/asset-swapper/src/utils/market_operation_utils/sampler_operations.ts b/packages/asset-swapper/src/utils/market_operation_utils/sampler_operations.ts index 62bcaeba2b..9ef979049c 100644 --- a/packages/asset-swapper/src/utils/market_operation_utils/sampler_operations.ts +++ b/packages/asset-swapper/src/utils/market_operation_utils/sampler_operations.ts @@ -798,9 +798,8 @@ export class SamplerOperations { liquidityProviderRegistryAddress?: string, multiBridgeAddress?: string, ): BatchedOperation { - const _sources = BATCH_SOURCE_FILTERS.getAllowed(sources); const subOps = this._getSellQuoteOperations( - _sources, + sources, makerToken, takerToken, takerFillAmounts, @@ -839,9 +838,8 @@ export class SamplerOperations { wethAddress: string, liquidityProviderRegistryAddress?: string, ): BatchedOperation { - const _sources = BATCH_SOURCE_FILTERS.getAllowed(sources); const subOps = this._getBuyQuoteOperations( - _sources, + sources, makerToken, takerToken, makerFillAmounts, @@ -880,8 +878,13 @@ export class SamplerOperations { liquidityProviderRegistryAddress?: string, multiBridgeAddress?: string, ): SourceQuoteOperation[] { + const _sources = BATCH_SOURCE_FILTERS.exclude( + liquidityProviderRegistryAddress ? [] : [ERC20BridgeSource.LiquidityProvider], + ) + .exclude(multiBridgeAddress ? [] : [ERC20BridgeSource.MultiBridge]) + .getAllowed(sources); return _.flatten( - sources.map( + _sources.map( (source): SourceQuoteOperation | SourceQuoteOperation[] => { switch (source) { case ERC20BridgeSource.Eth2Dai: @@ -984,8 +987,11 @@ export class SamplerOperations { wethAddress: string, liquidityProviderRegistryAddress?: string, ): SourceQuoteOperation[] { + const _sources = BATCH_SOURCE_FILTERS.exclude( + liquidityProviderRegistryAddress ? [] : [ERC20BridgeSource.LiquidityProvider], + ).getAllowed(sources); return _.flatten( - sources.map( + _sources.map( (source): SourceQuoteOperation | SourceQuoteOperation[] => { switch (source) { case ERC20BridgeSource.Eth2Dai: diff --git a/packages/asset-swapper/src/utils/quote_simulation.ts b/packages/asset-swapper/src/utils/quote_simulation.ts index 2e0b2240bb..5e46046e79 100644 --- a/packages/asset-swapper/src/utils/quote_simulation.ts +++ b/packages/asset-swapper/src/utils/quote_simulation.ts @@ -3,7 +3,7 @@ import { BigNumber } from '@0x/utils'; import { constants } from '../constants'; import { MarketOperation } from '../types'; -import { CollapsedFill, FeeSchedule, OptimizedMarketOrder } from './market_operation_utils/types'; +import { CollapsedFill, ERC20BridgeSource, FeeSchedule, OptimizedMarketOrder } from './market_operation_utils/types'; import { isOrderTakerFeePayableWithMakerAsset, isOrderTakerFeePayableWithTakerAsset } from './utils'; const { PROTOCOL_FEE_MULTIPLIER, ZERO_AMOUNT } = constants; @@ -261,16 +261,27 @@ function createWorstCaseFillOrderCalls(quoteInfo: QuoteFillInfo): QuoteFillOrder // Apply order slippage to its fill paths. function getSlippedOrderFills(order: OptimizedMarketOrder, side: MarketOperation): CollapsedFill[] { - const totalInput = BigNumber.sum(...order.fills.map(f => f.input)); - const totalOutput = BigNumber.sum(...order.fills.map(f => f.output)); - const inputScaling = - side === MarketOperation.Sell - ? order.fillableTakerAssetAmount.div(totalInput) - : order.fillableMakerAssetAmount.div(totalInput); - const outputScaling = - side === MarketOperation.Sell - ? order.fillableMakerAssetAmount.div(totalOutput) - : order.fillableTakerAssetAmount.div(totalOutput); + // Infer the slippage from the order amounts vs fill amounts. + let inputScaling: BigNumber; + let outputScaling: BigNumber; + const source = order.fills[0].source; + if (source === ERC20BridgeSource.Native) { + // Native orders do not have slippage applied to them. + inputScaling = new BigNumber(1); + outputScaling = new BigNumber(1); + } else { + if (side === MarketOperation.Sell) { + const totalFillableTakerAssetAmount = BigNumber.sum(...order.fills.map(f => f.input)); + const totalFillableMakerAssetAmount = BigNumber.sum(...order.fills.map(f => f.output)); + inputScaling = order.fillableTakerAssetAmount.div(totalFillableTakerAssetAmount); + outputScaling = order.fillableMakerAssetAmount.div(totalFillableMakerAssetAmount); + } else { + const totalFillableTakerAssetAmount = BigNumber.sum(...order.fills.map(f => f.output)); + const totalFillableMakerAssetAmount = BigNumber.sum(...order.fills.map(f => f.input)); + inputScaling = order.fillableMakerAssetAmount.div(totalFillableMakerAssetAmount); + outputScaling = order.fillableTakerAssetAmount.div(totalFillableTakerAssetAmount); + } + } return order.fills.map(f => ({ ...f, input: f.input.times(inputScaling), diff --git a/packages/asset-swapper/test/exchange_proxy_swap_quote_consumer_test.ts b/packages/asset-swapper/test/exchange_proxy_swap_quote_consumer_test.ts index 5963c712f2..924d7782ce 100644 --- a/packages/asset-swapper/test/exchange_proxy_swap_quote_consumer_test.ts +++ b/packages/asset-swapper/test/exchange_proxy_swap_quote_consumer_test.ts @@ -18,6 +18,7 @@ import 'mocha'; import { constants } from '../src/constants'; import { ExchangeProxySwapQuoteConsumer } from '../src/quote_consumers/exchange_proxy_swap_quote_consumer'; +import { getSwapMinBuyAmount } from '../src/quote_consumers/utils'; import { MarketBuySwapQuote, MarketOperation, MarketSellSwapQuote } from '../src/types'; import { OptimizedMarketOrder } from '../src/utils/market_operation_utils/types'; @@ -191,7 +192,7 @@ describe('ExchangeProxySwapQuoteConsumer', () => { expect(callArgs.inputToken).to.eq(TAKER_TOKEN); expect(callArgs.outputToken).to.eq(MAKER_TOKEN); expect(callArgs.inputTokenAmount).to.bignumber.eq(quote.worstCaseQuoteInfo.totalTakerAssetAmount); - expect(callArgs.minOutputTokenAmount).to.bignumber.eq(quote.worstCaseQuoteInfo.makerAssetAmount); + expect(callArgs.minOutputTokenAmount).to.bignumber.eq(getSwapMinBuyAmount(quote)); expect(callArgs.transformations).to.be.length(2); expect( callArgs.transformations[0].deploymentNonce.toNumber() === @@ -220,7 +221,7 @@ describe('ExchangeProxySwapQuoteConsumer', () => { expect(callArgs.inputToken).to.eq(TAKER_TOKEN); expect(callArgs.outputToken).to.eq(MAKER_TOKEN); expect(callArgs.inputTokenAmount).to.bignumber.eq(quote.worstCaseQuoteInfo.totalTakerAssetAmount); - expect(callArgs.minOutputTokenAmount).to.bignumber.eq(quote.worstCaseQuoteInfo.makerAssetAmount); + expect(callArgs.minOutputTokenAmount).to.bignumber.eq(getSwapMinBuyAmount(quote)); expect(callArgs.transformations).to.be.length(2); expect( callArgs.transformations[0].deploymentNonce.toNumber() === @@ -318,7 +319,7 @@ describe('ExchangeProxySwapQuoteConsumer', () => { expect(callArgs.inputToken).to.eq(TAKER_TOKEN); expect(callArgs.outputToken).to.eq(MAKER_TOKEN); expect(callArgs.inputTokenAmount).to.bignumber.eq(quote.worstCaseQuoteInfo.totalTakerAssetAmount); - expect(callArgs.minOutputTokenAmount).to.bignumber.eq(quote.worstCaseQuoteInfo.makerAssetAmount); + expect(callArgs.minOutputTokenAmount).to.bignumber.eq(getSwapMinBuyAmount(quote)); expect(callArgs.transformations).to.be.length(3); expect( callArgs.transformations[0].deploymentNonce.toNumber() === diff --git a/packages/asset-swapper/test/quote_simulation_test.ts b/packages/asset-swapper/test/quote_simulation_test.ts index 07493b47d6..ae106b30df 100644 --- a/packages/asset-swapper/test/quote_simulation_test.ts +++ b/packages/asset-swapper/test/quote_simulation_test.ts @@ -22,7 +22,7 @@ describe('quote_simulation tests', async () => { const TAKER_TOKEN = randomAddress(); const DEFAULT_MAKER_ASSET_DATA = assetDataUtils.encodeERC20AssetData(MAKER_TOKEN); const DEFAULT_TAKER_ASSET_DATA = assetDataUtils.encodeERC20AssetData(TAKER_TOKEN); - const GAS_SCHEDULE = { [ERC20BridgeSource.Native]: _.constant(1) }; + const GAS_SCHEDULE = { [ERC20BridgeSource.Uniswap]: _.constant(1) }; // Check if two numbers are within `maxError` error rate within each other. function assertRoughlyEquals(n1: BigNumber, n2: BigNumber, maxError: BigNumber | number = 1e-10): void { @@ -164,7 +164,7 @@ describe('quote_simulation tests', async () => { const subFillOutputs = subdivideAmount(outputs[i], count); return { sourcePathId: nativeSourcePathId, - source: ERC20BridgeSource.Native, + source: ERC20BridgeSource.Uniswap, input: inputs[i], output: outputs[i], subFills: _.times(count, j => ({