Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
eliotstock opened this issue Feb 9, 2022 · 2 comments
Labels
bug Something isn't working p1

Comments

@eliotstock
Copy link

  • I'm submitting a ...
    [*] bug report
    [ ] feature request
    [ ] question about the decisions made in the repository
    [ ] question about how to use this project

  • Summary

  • Other information (e.g. detailed explanation, stack traces, related issues, suggestions how to fix, links for us to have context, eg. StackOverflow, personal fork, etc.)
    This line:

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
Copy link
Contributor

#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
Copy link

hiroshitashir commented Jul 11, 2023

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))
  }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working p1
Projects
Development

No branches or pull requests

4 participants