-
Notifications
You must be signed in to change notification settings - Fork 3
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
[UI-666] Integrate lido warehouse packages #82
base: develop
Are you sure you want to change the base?
Conversation
From develop to main
From develop to main
From develop to main
[CI] Switch WFs to critical
from develop to main
from develop to main
Develop -> main
Develop to Main
develop to main
develop to main
…use-packages # Conflicts: # package.json # pages/_app.tsx # shared/api/withCsp.ts
@@ -18,7 +19,7 @@ export const ClaimFormDisconnected = () => { | |||
placeholder="0" | |||
/> | |||
</InputGroupStyled> | |||
<WalletConnect fullwidth /> | |||
<WalletConnectButton fullwidth /> |
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.
Sad to say, but this should be renamed. Sorry for not seeing this earlier.
We should avoid confusion with WalletConnect.
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.
Do you have suggests how to rename?
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.
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.
WalletConnectButton ---> ConnectWalletButton
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.
Looks good 👍 Just a few minor questions
config/rpc.ts
Outdated
@@ -1,4 +1,4 @@ | |||
import { CHAINS } from 'config/chains'; | |||
import { CHAINS } from '@lido-sdk/constants'; |
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.
Let's not used lido-sdk chains, it's buggy regarding types
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.
ok. but some code requires CHAINS from '@lido-sdk/constants', for example warehouse packages code.
How you suggest for avoid types problems (either satisfies operator or as operator or something else)
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.
done
@@ -1,14 +1,17 @@ | |||
import { useWeb3 } from 'reef-knot/web3-react'; | |||
|
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.
Do you mind removing spaces? All codebase is without them 🤔
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.
Yes, but wants some order in imports
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.
Lets make it in next PR (because will be many change files) via https://github.com/azat-io/eslint-plugin-perfectionist
features/claim/wallet.tsx
Outdated
</TokensAmount> | ||
| ||
<TokenToWallet address={token?.address} /> | ||
<TokenToWallet address={token?.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.
Why can't it be undefined?
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.
needs patch to TokenToWallet (warehouse)
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.
done
features/claim/wallet.tsx
Outdated
<FormatToken amount={unclaimed} symbol={token?.symbol} /> | ||
<FormatToken | ||
amount={unclaimed} | ||
symbol={token?.symbol || ''} |
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.
Why can't it be undefined?
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.
needs patch to FormatToken (warehouse)
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.
done
@@ -125,14 +128,14 @@ export const VestingDetailedSlide: FC<VestingDetailedSlideProps> = memo( | |||
<Column $primary> | |||
<DetailsHeader>Available</DetailsHeader> | |||
<DetailsValue> | |||
<FormatToken amount={unclaimed} symbol={token?.symbol} /> | |||
<FormatToken amount={unclaimed} symbol={token?.symbol || ''} /> |
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.
Same here, it should allow to pass undefined
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.
needs patch to FormatToken (warehouse)
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.
done
@@ -78,7 +81,7 @@ export const VestingSummarySlide: FC<VestingSummarySlideProps> = memo( | |||
<DetailsValue> | |||
<FormatToken | |||
amount={unclaimed?.add(locked ?? BigNumber.from(0))} | |||
symbol={token?.symbol} | |||
symbol={token?.symbol || ''} |
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.
same
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.
needs patch to FormatToken (warehouse)
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.
done
</AppWagmiConfig> | ||
|
||
const AppWrapper: FC<AppProps> = (props) => ( | ||
<NoSSRWrapper> |
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.
Why do we wrap the whole app into NoSSR?
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.
because several troubles places in widget and in warehouse (possibly reef knot related)
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.
Last time these errors were related to the connect button, but not the whole app. Can you double-check that the whole app isn't working, and we need to wrap it in NoSSRWrapper?
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.
after merge https://github.com/lidofinance/warehouse/pull/159 update :
|
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.
Looks good, let's check if it's possible to not wrap the whole app in nossrwrapper and we are good to go
Description
Use https://github.com/lidofinance/warehouse packages
Testing notes
Needs full testing
Design compared to https://www.figma.com/file/pmtFrBp0z1RvzdfcHaV2fn/Widget-UI-Guide?type=design&node-id=5-2728&mode=design&t=quBOmbXZML1wbwv7-0