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

Alter display info of Staking summary overview #7179

Open
wants to merge 39 commits into
base: master
Choose a base branch
from

Conversation

wirednkod
Copy link
Member

@wirednkod wirednkod commented Mar 17, 2022

Based on this there will be 2 PRs for altering the Overview staking summary area.

This PR addresses the minified version (can be seen below):

Untitled

cc @emostov @kianenigma (please some support on the outcome numbers)

@wirednkod wirednkod changed the title Nik fix staking numbers Display information with regard to bags list nomination limits (1st part) Mar 17, 2022
@wirednkod wirednkod marked this pull request as draft March 17, 2022 22:49
@wirednkod wirednkod changed the title Display information with regard to bags list nomination limits (1st part) Alter display info of Staking summary overview Mar 19, 2022
@wirednkod wirednkod marked this pull request as ready for review March 21, 2022 20:58
@wirednkod wirednkod requested a review from jacogr March 21, 2022 20:58
Copy link
Member

@jacogr jacogr left a comment

Choose a reason for hiding this comment

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

I'm not sure I "get" it. The numbers I would love to see on the overview page (number of actives) are now missing and nowhere to be found. (These numbers are on the targets page as well, with averages)

So while I really appreciate it, and certainly see a need to improve the displays, I'm not sure this is it since it (a) duplicates already available information & (b) removes other useful information.

Critically, there is a performance issue here, it burns the CPU, eating up one of my cores.

@kianenigma
Copy link

kianenigma commented Mar 23, 2022

I'm not sure I "get" it. The numbers I would love to see on the overview page (number of actives) are now missing and nowhere to be found. (These numbers are on the targets page as well, with averages)

So while I really appreciate it, and certainly see a need to improve the displays, I'm not sure this is it since it (a) duplicates already available information & (b) removes other useful information.

I disagree: The numbers that most users want to see is the answer to:

  1. How much stake I need to be an active nominator?
  2. How much stake I need to be an active validator?

The count of nominators, waiting etc. are metrics that you, I, other devs, or at most active community members are interested in, and while they should exist, they need not be in the main page.

Our plan is to:

  1. Remove the scattered information that was hard to find before (which is why you see some stuff removed here).
  2. Add these two in a smaller PR for ease of merging.
  3. Add the full matrix of information (all counters and all stakes, as illustrated here) as a drop-down (or a new tab, both fine for me) that users can optionally open. Fetching all of this information is rather expensive and should be optional.

Critically, there is a performance issue here, it burns the CPU, eating up one of my cores.

Both of the numbers that are displayed as a part of this PR can be 100% deduced from the list of validators and their backers (ErasStakers) which is already being fetched in this page. So while the code might need more care regarding re-using data, in principle, this has zero overhead.

@wirednkod
Copy link
Member Author

Critically, there is a performance issue here, it burns the CPU, eating up one of my cores.

I think I know where the performance issue may come from. I will fix this.

@jacogr
Copy link
Member

jacogr commented Mar 23, 2022

I'm sorry to disagree here - the info for min stake is there, explicitly with additional on-chain data around it. It may make sense to also display it on the "actions" page, but sadly as it stands we lost info and duplicated others.

I am not disagreeing that we can expand the info display, but certainly we should not drop stuff and replace with duplication of already available information (and what is dropped here gets fetched anyway since stuff loads in the background, expensive or not)

TL;DR This is a step backwards from what is there, with all the best intentions. Moving info is fine, just dropping without looking at the various areas is not. (As said there are certainly things that fit better elsewhere and vice versa)

@jacogr
Copy link
Member

jacogr commented Mar 23, 2022

Thinking out loud, maybe we are trying to fit a square peg into a round hole. What about the following approach -

  1. Add a "stats-only" landing, that has all the info that is useful for both power and normal users, basically a big dashboard and dashboard only. (Some info will be there immediately, since query some can only be filled when the background stuff is fully loaded). It basically includes anything that we can extract (and already do) from a stats perspective, e.g. stuff like min-comm and min set stakes are equally hidden, but important as a "what to do".
  2. Rename "Overview" to "Validators" and this new page to "Overview" (as the default landing)

This seems better than trying to shoehorn everything into the single top-bar?

@kianenigma
Copy link

TL;DR This is a step backwards from what is there, with all the best intentions. Moving info is fine, just dropping without looking at the various areas is not.

I understand your concern, but bare in mind that this PR is part of a 2 step PR group. If and when we deliver the second one as well, no information will be "dropped", and it will all be grouped together in a (perhaps opinionated) better way.

For example, where min-nominator/threshold is currently located is hard to find location (namely Targets page), and we get a lot of questions from users wanting to find this, having no idea it event exists. This is why I emphasize that this number should be in the front page.


This seems better than trying to shoehorn everything into the single top-bar?

I like this idea a lot, just worried about the duplicate data queries. For example, deducing min-active-nominator needs the full list of exposures. If we start fetching this for the new Overview page, can we reuse it in Validators page?

If this is fine, I am onboard your suggestions, with the minor tweak to call (the new) Validators tab Active Set to incorporate our new terminology (emphasizing Active).

Also, as a suggestion, in the new Overview page, you could show the validators from the Session::validators, without any of their information, which is a pretty small query, although this might be confusing.


All in all though, this seems like a lot more work to deliver. Given the fact that I personally find the information that we added in the front page now much more valuable than what was already there, I am still in favor of accepting this PR as an interim solution, until someone delivers the new overview page.

@jacogr
Copy link
Member

jacogr commented Mar 23, 2022

There is no duplicates -

  • the way staking is structured, for the bulk info we have a hook and pass it into each component - so they are not duplicated (single root hook, object with info gets passed in, e.g. Target/Overview/Summary atm use the same underlying data sources that retrieve from the actual root app)
  • on the API layer itself, all calls are memoized, so the there are no duplicates made, even when you try (worst that can happen is the processing of duplicate responses in different paths, however see the point above)

EIDIT: On the last point, not everything is memoized, there are some exceptions on the RPCs, e.g. the author_* endpoints, when it comes to anything state_* related, they all are.

@kianenigma
Copy link

@wirednkod has there been any further updates on this? WDYT?

@jacogr
Copy link
Member

jacogr commented Jun 14, 2022

The bagsList fallback won't be removed anytime reasonable, which is why we have the detection in-place, e.g. use detected & detect pallet.

The apps UI has to lag any removals by a very long time (in the order of 9+ months before even looking at deprecations), since teams do different things.

The same goes for new features, anything old cannot just be dropped, even if it is fully deployed on Kusama/Polkadot - even between these 2 there is also some lag. This just compounds with the number of different teams out there, hence we are not looking at delay between these 2, but rather across the ecosystem.

@wirednkod
Copy link
Member Author

Some small issues remaining -

  • http://localhost:3000/?rpc=wss%3A%2F%2Fws.azero.dev#/staking - fails (older version of staking, e.g. "Cannot read properties of undefined (reading 'maxElectingVoters')")
  • On Kusama we just need to do the (api.query.voterList || api.query.bagsList). dance (could be either pallet - this may be fixed by merging in master if the existing hooks are used)

Fixed

@jacogr
Copy link
Member

jacogr commented Jun 20, 2022

Something small, another network - http://localhost:3000/?rpc=wss%3A%2F%2Fautomata.api.onfinality.io%2Fpublic-ws#/staking (some parts just keep spinning, I'm guessing because there are no nominators?)

Dock has the same issue - http://localhost:3000/?rpc=wss%3A%2F%2Fmainnet-node.dock.io#/staking (although they do have nominators, so the above one may not be caused by that)

@wirednkod
Copy link
Member Author

Dock has the same issue - http://localhost:3000/?rpc=wss%3A%2F%2Fmainnet-node.dock.io#/staking (although they do have nominators, so the above one may not be caused by that)

Running this:

const [now, maxValidatorsCount, validators] = await Promise.all([
  api.query.timestamp.now(),
  api.query.staking.maxValidatorsCount,
  api.query.staking.maxNominatorsCount,
  api.query.staking.minNominatorBond,
  api.query.staking.minValidatorBond
]);

console.log('The current date is: ' + now);
console.log('The maximumNominators count: ' + maxNominatorsCount);
console.log('The maxValidatorsCount count: ' + maxValidatorsCount);
console.log('The minNominatorBond count: ' + minNominatorBond);
console.log('The minValidatorBond count: ' + minValidatorBond);

In the developer's screen of Dock PoS Mainnet and received for each one of them as undefined.
I think the case for Automata is similar - just haven't checked all Queries....

Im not sure on how to solve this. Any suggestions @jacogr @kianenigma ?

@jacogr
Copy link
Member

jacogr commented Jun 21, 2022

It is basically optional - if not available, just don't show that specific information. All the max/min stuff is very new, unless the chains follow Substrate master closely, which basically only a handful does, it won't be there. So it needs to be able to gracefully degrade. Teams generally deploy and keep it like that for a long time (only with new features on own pallets), since upgrading Substrate is seen as a PITA.

(And yes, I purposefully use "very" above, for Kusama/Polkadot it may feel old by now)

PS: Dock POS is actually a surprise to me, since their POS network is relatively new. (A restart/recreate of the old POA version)

@wirednkod
Copy link
Member Author

It is basically optional - if not available,

That was the approach I was thinking, but unfortunately at the moment when one value is undefined it actually means that its on a "pending" state. I have some idea for fixing that (e.g. using a type number | "N/A" but Im not sure how to identify e.g. that Dock PoS is not having/service maxValidators api, while another parachain does.

@jacogr
Copy link
Member

jacogr commented Jun 22, 2022

So to detect if not available, assuming api.query.staking.maxValidatorsCount (simplified) -

const maxValidatorsCount = useCall(api.query.staking.maxValidatorsCount);

...
return (
  {api.query.staking.maxValidatorsCount
    ? maxValidatorsCount
      ? formatNumber(maxValidatorsCount)
      : <Spinner />
    : '-'
  }
);

@kianenigma
Copy link

Automata is a bit of a pesky example here: they have pallet-staking in their runtime, but pretty much unused (probably by mistake). So almost no storage item is set in there. Might be a bit difficult to handle that case.

@wirednkod
Copy link
Member Author

wirednkod commented Jun 22, 2022

Automata is a bit of a pesky example here: they have pallet-staking in their runtime, but pretty much unused (probably by mistake). So almost no storage item is set in there. Might be a bit difficult to handle that case.

I have resolved the Dock PoS case, but the Automata one is really edgy. My intention is to push 1 last commit that will solve this edge case by introducing a timer (I know it sounds scary and unorthodox but let me explain).

This timer will exist only in the useSortedTargets, and once countdown is done (set to 10 seconds) then timerDone state will alter from false to true. This will lead to values of undefined to switch to null.
This way we will know to stop showing loaders in the new dashboard and show a - instead.

In case of - for a peculiar reason - an api call is delayed more than 10 seconds, then (after timer changes undefined to null and thus "-"), when the result is retrieved the "-" will change to the correct value.

This last commit can be reviewed separately and addresses the case of Automata as described above. In case there are objections we could possibly revert it and replace with some better solution.

@jacogr
Copy link
Member

jacogr commented Jun 27, 2022

Yes, I would revert the hack, it just feels incorrect and smelly :( Don't have a better idea atm, however need to look at the specific loading stuff (with the other fix in), to have some ideas. (Not sure why here, like in the current summaries, it enforces these to be shown, so would need to look with the other fix in, which simulated the same behavior here)

@wirednkod
Copy link
Member Author

Yes, I would revert the hack

Will do

it just feels incorrect and smelly :(

Well it is. The case though here is that it feels that they are not using the pallet correctly - I mean - I know the non-stop-turning loaders does not look good, but should UI care about the mis-configuration/usage that one does?

however need to look at the specific loading stuff (with the other fix in), to have some ideas.

Which fix you refer to?

@jacogr
Copy link
Member

jacogr commented Jun 28, 2022

The "other fix" I referred to is for Dock.

And yes, the UI should care about loading spinners - in the cases where it cannot deal with the info, it should not display anything. Graceful fallback aka feature detection should come into play. Automata + the current summaries is a good example - it just skips display of some values which is it cannot make sense of. (Completely skipping is always better than displaying "-", although it certainly is not consistently followed...)

As soon as we have loading spinners and broken displays, we get support requests since people do use this stuff. Support requests are not great to wake up to, so really don't want to create the opportunity for additional ones in the queue. (And anything that looks like a broken UI, no matter what the reason, will generate support requests in large numbers)

@wirednkod
Copy link
Member Author

Thank you for the elaboration @jacogr .
Just to clarify my sayings: Of course UI should care about loading spinners. What I was trying to say is that Automata+ are mis-using the way staking should be used - as far as I understand. Meaning that there are no Stakers (equals to 0 that at the moment is not expected from the UI -> translates to "dear Loader, please, keep spinning".

Having said that, and back to the "UI problem and solution", the issue I see here:

skips display of some values which is it cannot make sense of.

is that we cannot be aware if an API call is finished or undefined is returned - as undefined for current implementation means "keep spinning". The "hack" (that I will revert as promised) is more of a "wait until something happens and if not stop the loader".

As soon as we have loading spinners and broken displays, we get support requests since people do use this stuff. Support requests are not great to wake up to, so really don't want to create the opportunity for additional ones in the queue. (And anything that looks like a broken UI, no matter what the reason, will generate support requests in large numbers)

Could not agree more to that - Trying to figure out a solution that will not meddle WAY too much with the current code, but cannot think of one - any pointers that could help on the direction that would fit best?
I mean - a "API responded and there is nothing to do here" which would led to Completely skipping or non-visualize the components would be the best - but based on the current implementation - we do not have that (unless I am missing something)

@jacogr
Copy link
Member

jacogr commented Jun 30, 2022

undefined is only returned when the actual query is not there. So for instance, if I do -

const result = useCall(api.query.willNever.beThere)

It won't ever return a result for that specific one. In that case, UI-wise -

<div>
  {api.query.willNever?.beThere && formatNumber(result)}
</div>

The staking interfaces are really all-over the show when you look at the ecosystem, so in a wide variety of cases some queries just won't be there - generally since they are "relatively new". (And for those, it should just not display the specific info).

If the query is in the runtime, it will always return a result. (It could be the metadata default value if nothing in storage, but something does come back - unless there is a type decoding error, which luckily now happens very infrequently since most teams have moved to metadata v14)

@wirednkod
Copy link
Member Author

Hey @jacogr I tried to address the case with a simpler approach. WDYT?

cc @kianenigma

@kianenigma
Copy link

Looking forward to see this merged!

@wirednkod
Copy link
Member Author

@jacogr Kind reminder

@kianenigma
Copy link

Another one :)

@kianenigma
Copy link

If this is ever going to be merged, this quarter is the best time since we are releasing many new features for staking.

@kianenigma
Copy link

I think this is still relevant a good improvement :)

@IkerAlus
Copy link

@kianenigma is this PR still as relevant as it was at the time of your last comment?

@kianenigma
Copy link

I think we should either:

  1. improve the staking page of the apps, as it still gets a lot of traffic, in which case I would say yes we should revise this and its parent issue/PR
  2. put a banner in the staking tab warning users that this page is for power users and first time users should go to https://staking.polkadot.network (and optionally other platforms that provide 1 click staking)

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.

4 participants