-
Notifications
You must be signed in to change notification settings - Fork 101
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
[2] refactor(tokens): clean up token consts and use TokenWithLogo #3190
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
…cowswap into refactor/tokens-consts
…cowswap into refactor/tokens-consts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More nitpicks
@@ -2,7 +2,7 @@ | |||
// + logic for chainId switch to/from xDAI | |||
import { SupportedChainId } from '@cowprotocol/cow-sdk' | |||
|
|||
import { XDAI_SYMBOL } from './constants' | |||
import { XDAI_SYMBOL } from '../tokens' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found strange that getChainCurrencySymbols
is inside a folder called gnosis_chain
but accepts any chain as a parameter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed! Considering the comment in the file beginning it must be an artifact from Uniswap era.
To avoid the scope blowing I prefer to fix it in the another iteration
return null | ||
} | ||
|
||
return currency?.address || (currency as { tokenInfo?: { address?: string } })?.tokenInfo?.address || null | ||
return currency.address |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
heheh, looks simpler
typeof params !== 'number' && 'chainId' in params && 'symbol' in params | ||
|
||
/** | ||
* TODO: Eventually, we should get rid of uniswap entities and use our own |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯
…cowswap into refactor/tokens-consts # Conflicts: # libs/common-const/src/tokens.ts # libs/common-utils/src/index.ts
* refactor(tokens): remove excessive types and consts (#3196) * refactor(tokens): integrate TokenLogo component (#3197) * refactor(tokens): wire up components to new hooks (#3198) * refactor(tokens): remove Uniswap currency entities usage (#3199) * refactor(tokens): use new tokens UI and logic by default (#3200) * fix(tokens): fix e2e tests for tokens updates (#3193) * refactor(tokens): remove legacy code (#3194) * fix(tokens): fix tokens list loading state (#3201)
Summary
One of our strategic goals is independence from Uniswap legacy.
Currently, we widely use
Currency
,NativeCurrency
,Token
, andCurrencyAmount
from@uniswap/sdk-core
.In the previous PR, I added a new entity
TokenWithLogo
. It should replace Currency, NativeCurrency, Token from Uniswap.Currently the migration in the intermediate state, we have to work on it more in the future.
What is changed:
GpEther
andGnosisChainNativeCurrency
are replaced byNATIVE_CURRENCY_BUY_TOKEN
currency.isNative
/currency.isToken
is replaced bygetIsNativeToken()
currency.wrapped
is replaced bygetWrappedToken()
libs/common-const/src/tokens.ts
was split into several filesPlease, use the new helpers in your next changes.
To Test
Please, test everything in #3201