Uniswap / smart-order-router

GNU General Public License v3.0
413 stars 420 forks source link

calculateRatioAmountIn() throws an Error when the optimalRatio is zero. #64

Open eliotstock opened 2 years ago

eliotstock commented 2 years ago

https://github.com/Uniswap/smart-order-router/blob/main/src/routers/alpha-router/functions/calculate-ratio-amount-in.ts#L12

will throw an Error when the optimalRatio passed in is zero, ie. a Fraction with a denominator of 1 but no numerator.

Such a Fraction is easily produced by AlphaRouter.routeToRatio() here:

https://github.com/Uniswap/smart-order-router/blob/main/src/routers/alpha-router/alpha-router.ts#L559

Here's the stack trace from my project code:

/home/e/r/dro/smart-order-router/node_modules/@uniswap/sdk-core/src/entities/fractions/fraction.ts:38
    throw new Error('Could not parse fraction')
          ^
Error: Could not parse fraction
    at Function.tryParseFraction (/home/e/r/dro/smart-order-router/node_modules/@uniswap/sdk-core/src/entities/fractions/fraction.ts:38:11)
    at Fraction.multiply (/home/e/r/dro/smart-order-router/node_modules/@uniswap/sdk-core/src/entities/fractions/fraction.ts:108:34)
    at Object.calculateRatioAmountIn (/home/e/r/dro/smart-order-router/src/routers/alpha-router/functions/calculate-ratio-amount-in.ts:12:28)
    at AlphaRouter.routeToRatio (/home/e/r/dro/smart-order-router/src/routers/alpha-router/alpha-router.ts:606:26)
    at DRO.<anonymous> (/home/e/r/dro/dro/src/dro.ts:670:70)
    at step (/home/e/r/dro/dro/src/dro.ts:33:23)
    at Object.next (/home/e/r/dro/dro/src/dro.ts:14:53)
    at fulfilled (/home/e/r/dro/dro/src/dro.ts:5:58)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)

In case it matters, the version of JSBI in my codebase is:

"jsbi": "^3.2.4",

whereas in sdk-core v3.0.1 it's:

"jsbi": "^3.1.4",

hensha256 commented 2 years ago

65 seems to be a duplicate of this. This issue has more details so I closed the other, but it is there if we want to reference another issue.

hiroshitashir commented 1 year ago

I believe this bug is fixed in the current version 3.2.6.

In Fraction.constructor, numerator and denominator are always initialized.

  public constructor(numerator: BigintIsh, denominator: BigintIsh = JSBI.BigInt(1)) {
    this.numerator = JSBI.BigInt(numerator)
    this.denominator = JSBI.BigInt(denominator)
  }

In Fraction.tryParseFraction, new Fraction(fractionish) is created when type of fractionish is BigintIsh (or instance of JSBI, number or string).

  private static tryParseFraction(fractionish: BigintIsh | Fraction): Fraction {
    if (fractionish instanceof JSBI || typeof fractionish === 'number' || typeof fractionish === 'string')
      return new Fraction(fractionish)

    if ('numerator' in fractionish && 'denominator' in fractionish) return fractionish
    throw new Error('Could not parse fraction')
  }

Finally, when fractionish is CurrencyAmount, CurrencyAmount.constructor always calls Fraction.constructor with super(numerator, denominator), which means numerator and denominator are always initialized.

  protected constructor(currency: T, numerator: BigintIsh, denominator?: BigintIsh) {
    super(numerator, denominator)
    invariant(JSBI.lessThanOrEqual(this.quotient, MaxUint256), 'AMOUNT')
    this.currency = currency
    this.decimalScale = JSBI.exponentiate(JSBI.BigInt(10), JSBI.BigInt(currency.decimals))
  }