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

4326-Validators list - Page and widgets #4375

Merged
merged 47 commits into from
Jul 25, 2023

Conversation

eshark9312
Copy link
Contributor

Resolves #4326
Initialized Validators Module with Validators List pages and widgets.

@vercel
Copy link

vercel bot commented May 3, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
dao ✅ Ready (Inspect) Visit Preview Jul 24, 2023 6:55pm
pioneer-2 ✅ Ready (Inspect) Visit Preview Jul 24, 2023 6:55pm
pioneer-2-storybook ✅ Ready (Inspect) Visit Preview Jul 24, 2023 6:55pm

@eshark9312
Copy link
Contributor Author

Do I have to list all the active validators for the net or the ones of the account who is signed in the page?

@traumschule
Copy link
Contributor

All according to the design

validators-design

const { api } = useApi()
// const [staking, setStaking] = useState<Staking | any>(null)
// const [validators, setValidators] = useState<ValidatorProps[]>([])
// const [nominators, setNominators] = useState<NominatorProps[]>([])
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove the commented lines

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I removed the comment lines

totalStake,
idealStaking,
eraDuration,
activeEra
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add return values for "totalRewards" and "lastRewards"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I fixed the code, so the "lastRewards" value was implemented.
But I can't find the exact api function to get the value for "total Rewards".

Copy link
Contributor

Choose a reason for hiding this comment

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

For validator rewards (allocated but unminted) we need to know the first and last era and sum them up from API staking.erasValidatorReward, unless there's a backend to query totals.

@traumschule
Copy link
Contributor

traumschule commented May 24, 2023

moved

@chrlschwb chrlschwb added the community-dev issue suitable for community-dev pipeline label Jun 7, 2023
Copy link
Member

@thesan thesan left a comment

Choose a reason for hiding this comment

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

@eshark9312 almost there ! I added a few change requests mostly about reducing the amount of rpc queries. Apart from I pushed a small change related to the mocked chain unwrap so you should probably pull the branch before adding changes.

packages/ui/src/validators/constants/constant.ts Outdated Show resolved Hide resolved
packages/ui/src/validators/hooks/useStakingStatistics.tsx Outdated Show resolved Hide resolved
packages/ui/src/validators/components/statistics/Era.tsx Outdated Show resolved Hide resolved
Copy link
Member

@thesan thesan left a comment

Choose a reason for hiding this comment

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

There might be a regression on the latest commits. Here's the current Storybook:
https://pioneer-2-storybook-git-fork-eshark9312-valida-b7c579-joystream.vercel.app/?path=/story/pages-validators-validatorlist--statistics
Screenshot from 2023-07-24 19-46-35

And here's was the one before the last push:
https://pioneer-2-storybook-iz2xsiyh3-joystream.vercel.app/?path=/story/pages-validators-validatorlist--statistics
Screenshot from 2023-07-24 19-46-54

Also the "100%" style is a bit broken because the 1 overflows on the border.

Comment on lines 30 to 32
if (activeValidators && api)
return activeEra ? combineLatest(activeValidators.map((address) => api.query.staking.erasStakers(activeEra.eraIndex, address))): undefined
return (
activeEra &&
Copy link
Member

Choose a reason for hiding this comment

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

Last really small detail but this code is mixing an if statement and a js type coercion/short-circuit evaluation (I'm not sure what to call it actually 😅). To keep the code clearer please chose one of these 2

Either the if statement:

const stakers = useObservable(() => {
  if (activeValidators && api && activeEra)
    return combineLatest(activeValidators.map((address) => api.query.staking.erasStakers(activeEra.eraIndex, address)))
...

Or:

const stakers = useObservable(() =>
  activeValidators && api && activeEra &&
    combineLatest(activeValidators.map((address) => api.query.staking.erasStakers(activeEra.eraIndex, address)))
...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have modified the code according to your review so that the storybook seems to be good now.
And in the Piechart, 100% is never displayed in any case.

Copy link
Member

@thesan thesan left a comment

Choose a reason for hiding this comment

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

Good job!

@thesan thesan merged commit 9f9514b into Joystream:dev Jul 25, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community-dev issue suitable for community-dev pipeline
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Validators list - Page and widgets
4 participants