Conversation
…l/cowswap into feat/migrate-from-uniswap-sdk
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis pull request introduces a comprehensive currency library layer built on arbitrary-precision arithmetic. It establishes abstract base classes for currency types (BaseCurrency, NativeCurrency, Token), implements core mathematical classes (Fraction, CurrencyAmount, Percent, Price), adds formatting utilities, and refactors imports throughout to use the new local abstractions instead of external dependencies. Several import paths are updated to reflect dependency changes. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| public tags: string[] = [], | ||
| ) { | ||
| super(chainId, address, decimals, symbol, name, bypassChecksum) | ||
| super(chainId, address, decimals, symbol, name) |
There was a problem hiding this comment.
we don't use bypassChecksum anywhere
| /** | ||
| * Represents an ERC20 token with a unique address and some metadata. | ||
| */ | ||
| export class Token extends BaseCurrency { |
There was a problem hiding this comment.
here I removed address check to integrate btc/sol support, will add it later. For our logic it's not necessary
| * number of decimal places, using the given rounding mode. | ||
| * Never returns scientific notation. | ||
| */ | ||
| export function toFixed(numerator: string, denominator: string, decimalPlaces: number, rounding: Rounding): string { |
There was a problem hiding this comment.
this is a replace for toFormat lib method from uniswap sdk
| * number of significant digits, using the given rounding mode. | ||
| * Never returns scientific notation. | ||
| */ | ||
| export function toSignificant( |
There was a problem hiding this comment.
its a replacement of Big and decimals js, this libs were used as a cascade for formatting, I've adjusted their implementation without these deps
There was a problem hiding this comment.
everything is covered by tests
| import { BigintIsh, Rounding } from '../constants' | ||
| import { Currency } from '../currency' | ||
|
|
||
| export class Price<TBase extends Currency, TQuote extends Currency> extends Fraction { |
There was a problem hiding this comment.
copypast from uniswap with our lint adjustments
| /** | ||
| * A currency is any fungible financial instrument, including Ether, all ERC20 tokens, and other chain-native currencies | ||
| */ | ||
| export abstract class BaseCurrency { |
There was a problem hiding this comment.
copypast with removing unnecessary changes
| import JSBI from 'jsbi' | ||
|
|
||
| // exports for external consumption | ||
| export type BigintIsh = JSBI | string | number |
There was a problem hiding this comment.
I'm going remove it and replace on bigint, but in the next iteration to not extend the scope
| /** | ||
| * applies number formatting to a numeric string that uses '.' as the decimal separator. | ||
| */ | ||
| export function applyFormat(value: string, format: FormatOptions = {}): string { |
There was a problem hiding this comment.
toFormat analog
| import { Rounding } from '../entities/constants' | ||
|
|
||
| // returns floor(log10(num/den)), assuming numerator and denominator are positive. | ||
| export function findExponent(numerator: bigint, denominator: bigint): number { |
There was a problem hiding this comment.
it's a common alghorithm to calculate exponent to make it faster, complexity is log10(num/den)
limitofzero
changed the title
|
|
||
| protected constructor(currency: T, numerator: BigintIsh, denominator?: BigintIsh) { | ||
| super(numerator, denominator) | ||
| if (!JSBI.lessThanOrEqual(this.quotient, MAX_UINT)) throw new Error('AMOUNT') |
There was a problem hiding this comment.
Are these the actual error messages or placeholders?
There was a problem hiding this comment.
it's the same error msg as it is uniswap sdk, I've replaced invariant lib only here: https://github.com/Uniswap/sdks/blob/9257f6b1cd86f120c1befaca644a2c62819c41a5/sdks/sdk-core/src/entities/fractions/currencyAmount.ts#L42
| * Represents the native currency of the chain on which it resides, e.g. | ||
| */ | ||
| export abstract class NativeCurrency extends BaseCurrency { | ||
| override readonly isNative: true = true |
There was a problem hiding this comment.
Noticed this pattern in a few different places, wondering whether this is needed, as you can always use instanceof.
libs/currency/package.json
Outdated
| }, | ||
| "dependencies": { | ||
| "@uniswap/sdk-core": "^3.0.1" | ||
| "@cowprotocol/cow-sdk": "7.4.1", |
There was a problem hiding this comment.
v8 has already been published and merged to develop.
There was a problem hiding this comment.
thank you, fixed
|
@limitofzero Added a few minor comments/questions, but approved. |
3 participants
Summary
Main goal of the pr: move entities from uniswap-sdk into cowsap. Remove extra deps like invariant, decimals.js, big.js and toFormat, and replaced them by utils in @cowprotocl/currencies (toSignificant etc). Added test coverage.
Unfortunately, there is no bundle size changed because uniswap sdk is using inside other packages, but its a good step to remove it in the future
Also I removed checkByAddress params and token fee data because we don't use it anywhere.
More detailed changes list:
To Test
Need to check rates and rounding of prices, they should work as before this pr. There is no functional changes, just need regress: check rates on swap/twap and in history(also in order progress). Rounding and decimals should be the same.
Summary by CodeRabbit
Release Notes
Refactor
Tests