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

Update validator list columns #4643

Merged
merged 13 commits into from
Dec 7, 2023

Conversation

eshark9312
Copy link
Contributor

@eshark9312 eshark9312 commented Nov 23, 2023

Copy link

vercel bot commented Nov 23, 2023

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

Name Status Preview Updated (UTC)
dao ✅ Ready (Inspect) Visit Preview Dec 7, 2023 6:25pm
pioneer-2 ✅ Ready (Inspect) Visit Preview Dec 7, 2023 6:25pm
pioneer-2-storybook ✅ Ready (Inspect) Visit Preview Dec 7, 2023 6:25pm

Copy link
Collaborator

@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.

Nice job, please check my suggestions 👇

@@ -19,8 +19,10 @@ export const ValidatorsList = ({ validators }: ValidatorsListProps) => {
<ListHeader>Validator</ListHeader>
<ListHeader>Verification</ListHeader>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add a tooltip next to the "Verification" column header: image

The copy could be something like:

The profile of Verified validator has been entirely verified by the Membership working group.

But it would be better to have the link point at the "About" tab and add the Working group description to this tab. See #4654

WDYT @dmtrjsg ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes its a good idea, and we could just add the description to the "About" section which is exactly the same as the are on the working group cards on the WG landing page where they are shown as a list.

Comment on lines 24 to 25
<ListHeader>Expected Nom APR</ListHeader>
<ListHeader>Comission</ListHeader>
Copy link
Collaborator

Choose a reason for hiding this comment

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

The table should be sort-able by "Expected Nom APR" and "Commission"

image

<ListHeader>APR</ListHeader>
<ListHeader>Own Stake</ListHeader>
<ListHeader>Total Stake</ListHeader>
<ListHeader>Expected Nom APR</ListHeader>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know it's not in the design but personally I could really use a tooltip on "Expected Nom APR" !

WDYT @dmtrjsg ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Tooltip is 100% warranted.

Text: "This column shows the expected APR for nominators who are nominating funds for the chosen validator. The APR is subject to the amount staked and have a diminishing return for higher token amounts."

@@ -111,6 +111,15 @@ export const useValidatorsList = () => {
isActive: activeValidators.includes(address),
totalRewards: rewardHistory.reduce((total: BN, data) => total.add(data.eraReward), new BN(0)),
APR: apr,
commission: validatorInfo.commission.toNumber() / 10 ** 7,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add a function in @/common/utils.ts (next to clamp):

export const perbillToPercent = (perbill: BN) => perbill.toNumber() / 10 ** 7

and use it here to clarify the code a bit.

Copy link
Collaborator

@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.

Also please mix the mocked data a bit so sorting can be tested in the story.

packages/ui/src/validators/components/ValidatorsList.tsx Outdated Show resolved Hide resolved
isDescending={sortByApr === 'desc'}
>
Expected Nom APR
<Tooltip tooltipText="This column shows the expected APR for nominators who are nominating funds for the chosen validator. The APR is subject to the amount staked and have a diminishing return for higher token amounts.">
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it would be nice to clarify how this APR was calculated so I'm proposing to add:

This is calculated as follow: Last reward extrapolated over a year times The nominator commission divided by The total staked by the validator

@eshark9312 could you confirm this description is accurate.
@bwhm could you confirm this is actually how it should be calculated.

Comment on lines 27 to 39
useEffect(() => {
if (!sortByApr) return
setSortedValidators(sortedValidators.sort((a, b) => (sortByApr === 'desc' ? a.APR - b.APR : b.APR - a.APR)))
}, [sortByApr])

useEffect(() => {
if (!sortByCommission) return
setSortedValidators(
sortedValidators.sort((a, b) =>
sortByCommission === 'desc' ? a.commission - b.commission : b.commission - a.commission
)
)
}, [sortByCommission])
Copy link
Collaborator

Choose a reason for hiding this comment

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

From this code the validators can only be sorted by 1 col at a time but if APR is "desc" and the user click on commission: validators are now sorted by commission "desc" only but it looks like it sorted by both APR and commission. Another problem is that there's one useEffect per sorted columns.

Instead you could have something like:

  const [sortBy, setSortBy] = useState<'address' | 'apr' | 'commission'>('address')
  const [isDescending, setDescending] = useState(false)

.vscode/settings.json Outdated Show resolved Hide resolved
Copy link
Collaborator

@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.

Great work. Just one last detail

packages/ui/src/validators/components/ValidatorsList.tsx Outdated Show resolved Hide resolved
packages/ui/src/validators/components/ValidatorsList.tsx Outdated Show resolved Hide resolved
packages/ui/src/validators/components/ValidatorsList.tsx Outdated Show resolved Hide resolved
Copy link
Collaborator

@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.

Perfect ! Thank you 🙏

@thesan thesan merged commit 99fb088 into Joystream:dev Dec 7, 2023
4 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 scope:validators
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants