feat(sdk-core): currency-safe CurrencyAmount comparisons - #697
Open
gomesalexandre wants to merge 1 commit into
Open
feat(sdk-core): currency-safe CurrencyAmount comparisons#697gomesalexandre wants to merge 1 commit into
gomesalexandre wants to merge 1 commit into
Conversation
greaterThan/equalTo/lessThan fell through to Fraction with no currency check, so comparing amounts of different currencies silently compared raw numerators (usdcAmount.greaterThan(daiAmount) returned a meaningless result). add/subtract/multiply already guard this with a CURRENCY invariant. Override the three comparisons to apply the same invariant when the other side is a CurrencyAmount, so a cross-currency comparison throws. The base Fraction | BigintIsh signature is kept so existing callers comparing against a raw amount (e.g. amount.equalTo(ZERO) with a JSBI zero) keep working - narrowing to CurrencyAmount<T> | 0 as the issue suggests breaks those legit call sites (verified against v2-sdk). closes Uniswap#53
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
closes #53
what
CurrencyAmount.greaterThan/equalTo/lessThanwere never overridden, so they fell through toFractionwith no currency check.usdcAmount.greaterThan(daiAmount)silently compared raw numerators across two different currencies. The sibling arithmetic methods (add/subtract/multiply) already guard this withinvariant(this.currency.equals(other.currency), 'CURRENCY').how
The three comparison methods now apply that same invariant when the other operand is itself a
CurrencyAmount, so a cross-currency comparison throwsCURRENCYinstead of returning a meaningless result.I kept the base
Fraction | BigintIshsignature rather than narrowing toCurrencyAmount<T> | 0as the issue suggests, on purpose: the narrowing breaks legitimate existing call sites that compare against a raw amount. Concretely,v2-sdk'spair.reserve0.equalTo(ZERO)(whereZEROis aJSBI, not the number literal0) fails to type-check underCurrencyAmount<T> | 0, and that raw-zero pattern is common. A runtime invariant delivers the currency safety the issue asks for without forcing a downstream migration.The
instanceof CurrencyAmountguard degrades gracefully: ifotheris aCurrencyAmountfrom a different copy of the package, it falls through to the base comparison (exactly the pre-fix behavior), never a wrong throw.Percent/PriceextendFraction(notCurrencyAmount), so they still compare as fractions.testing
sdk-core: 19 tests incurrencyAmount.test.tspass (3 new: same-currency comparisons, raw-amount comparisons including aJSBIzero, and a cross-currency throw).tsc+eslintclean.sdk-core:v2-sdk(103) andv3-sdk(338) pass with zero failures. (router-sdk/v4-sdk/universal-router-sdkfail only on pre-existing workspace module-resolution, unrelated to this change - their unbuilt sibling packages.)ETH.equals(WETH)is false, so those would newly throw): none exist.Changeset included (
minor), noting the behavior change.