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

[UI-666] Integrate lido warehouse packages #82

Open
wants to merge 24 commits into
base: develop
Choose a base branch
from

Conversation

solidovic
Copy link

@solidovic solidovic commented Jul 27, 2023

@solidovic solidovic marked this pull request as ready for review July 28, 2023 12:10
@solidovic solidovic requested a review from a team as a code owner July 28, 2023 12:10
…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 />
Copy link
Contributor

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.

Copy link
Author

@solidovic solidovic Aug 3, 2023

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WalletConnectButton ---> ConnectWalletButton

Copy link
Contributor

@hexnickk4997 hexnickk4997 left a 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';
Copy link
Contributor

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

Copy link
Author

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)

Copy link
Author

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';

Copy link
Contributor

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 🤔

Copy link
Author

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

Copy link
Author

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

</TokensAmount>
&nbsp;
<TokenToWallet address={token?.address} />
<TokenToWallet address={token?.address || ''} />
Copy link
Contributor

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?

Copy link
Author

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)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

<FormatToken amount={unclaimed} symbol={token?.symbol} />
<FormatToken
amount={unclaimed}
symbol={token?.symbol || ''}
Copy link
Contributor

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?

Copy link
Author

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)

Copy link
Author

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 || ''} />
Copy link
Contributor

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

Copy link
Author

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)

Copy link
Author

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same

Copy link
Author

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)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

pages/_app.tsx Show resolved Hide resolved
</AppWagmiConfig>

const AppWrapper: FC<AppProps> = (props) => (
<NoSSRWrapper>
Copy link
Contributor

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?

Copy link
Author

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)

Copy link
Contributor

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It wasn't much, but some info still was rendered

image

@solidovic
Copy link
Author

solidovic commented Aug 2, 2023

after merge https://github.com/lidofinance/warehouse/pull/159 update :

  • @lidofinance/next-widget-layout
  • @lidofinance/eth-ui-wallet-modal
  • @lidofinance/eth-next-widget-app-evm
  • @lidofinance/eth-ui-primitives

@solidovic solidovic changed the title Integrate lido warehouse packages [UI-666] Integrate lido warehouse packages Aug 14, 2023
Copy link
Contributor

@hexnickk4997 hexnickk4997 left a 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants