apps icon indicating copy to clipboard operation
apps copied to clipboard

Alter display info of Staking summary overview

Open wirednkod opened this issue 2 years ago • 38 comments

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 avatar Mar 17 '22 22:03 wirednkod

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.

kianenigma avatar Mar 23 '22 10:03 kianenigma

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.

wirednkod avatar Mar 23 '22 11:03 wirednkod

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 avatar Mar 23 '22 12:03 jacogr

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?

jacogr avatar Mar 23 '22 13:03 jacogr

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.

kianenigma avatar Mar 23 '22 14:03 kianenigma

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.

jacogr avatar Mar 23 '22 14:03 jacogr

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

kianenigma avatar Apr 13 '22 15:04 kianenigma

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

(yeap :) - Just delay due to COVID)

I have 3 approaches for the Overview page and I would like some opinion design-wise @jacogr @kianenigma a) to follow same design as Summary - just bring everything in front page and organise them based on "group" (e.g. Validators, Nominators, Targets etc) as can be seen below:

image

Or b) create a "table" view for Nominators/Validators as can be seen below:

image

Or c) which I find cleaner and personally prefer:

image

Opinions?

wirednkod avatar Apr 19 '22 19:04 wirednkod

not a fan of b, but both a and c look fine to me.

Looking at all the stats that we have, and combining them with the new stuff that I want, I can list the following as these sections in design a and/or c:

  • Overview
    • Epoch and Era Progress
    • Stake rates (ideal, current, inflation)
    • Returns
  • Validators
    • Count
      • Total
      • Active
      • Waiting
    • Stake
      • Intention threshold
      • Min active threshold
      • Average active
  • Nominators
    • Count
      • Total
      • Electing
      • Active
    • Stake
      • Intention threshold
      • Min active threshold
      • Average active
  • Bags
    • Total bags
    • Total nodes

Of the above, I think only the bags stats can stay in its own tab as well as in the overview page. Everything else can only be in the overview page.

kianenigma avatar Apr 20 '22 08:04 kianenigma

Based on @kianenigma 's input, the Overview Screen is built and looks like the following:

image

This is ready for review - just I would like someone to: a) Cross check that calculations on numbers are correct; and b) Verify if there exists/should be average stake for active nominators (as in validators)

wirednkod avatar Apr 21 '22 14:04 wirednkod

@jacogr any feedback would be highly appreciated 🙏🏽

wirednkod avatar May 06 '22 15:05 wirednkod

Opening it, I went "wow, nice". Certainly really cool.

I will delve into the code (hopefully) today. A couple of play-through comments -

  • we have lost compatibility, i.e. networks such as Dock never finishes loading everything - http://localhost:3000/?rpc=wss%3A%2F%2Fmainnet-node.dock.io#/staking (this means we assume some params that are on the current staking is everywhere, without fallbacks - this is problematic since networks always evolve at their own pace, even between Polkadot and Kusama which generally tracks latest stuff)

  • not 100% sure I quite like the hover-highlight, it is probably my old eyes, I just find myself squinting

Will go through the code.

jacogr avatar May 17 '22 09:05 jacogr

Will got through (hopefully final) today. Looking forward to have it in :)

jacogr avatar May 24 '22 07:05 jacogr

Can't wait to see this merged.

kianenigma avatar Jun 11 '22 13:06 kianenigma

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)

jacogr avatar Jun 14 '22 04:06 jacogr

Yeah, both are because of things being re-named on the substrate/polkadot side. Relevant PRs

substrate commit: 8cbda809d3495f38ce04eec46e90330db213af8a polkadot commit: 731979d91922211033a3e8d1d3b4d7c8017b25d5

@wirednkod you should probably make an issue to remove codes like api.query.bagsList once everything is migrated.

kianenigma avatar Jun 14 '22 09:06 kianenigma

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.

jacogr avatar Jun 14 '22 10:06 jacogr

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

wirednkod avatar Jun 20 '22 08:06 wirednkod

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)

jacogr avatar Jun 20 '22 09:06 jacogr

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 ?

wirednkod avatar Jun 21 '22 18:06 wirednkod

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)

jacogr avatar Jun 21 '22 18:06 jacogr

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.

wirednkod avatar Jun 21 '22 19:06 wirednkod

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 />
    : '-'
  }
);

jacogr avatar Jun 22 '22 03:06 jacogr

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.

kianenigma avatar Jun 22 '22 08:06 kianenigma

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.

wirednkod avatar Jun 22 '22 10:06 wirednkod

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)

jacogr avatar Jun 27 '22 15:06 jacogr

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?

wirednkod avatar Jun 27 '22 16:06 wirednkod

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)

jacogr avatar Jun 28 '22 08:06 jacogr

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)

wirednkod avatar Jun 28 '22 08:06 wirednkod

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)

jacogr avatar Jun 30 '22 10:06 jacogr