Display state of Grandpa #134
Conversation
Since the consensus view will be added users could mistake the settings as being applied there as well.
By using the NodeId instead of the Address in the first dimension of the consensus matrice we save quite some space in the payload which is sent to the browser. The commit also contains some minor refactoring.
This look a bit nicer, otherwise the UI will still adapt the box sizes once everything has already been loaded up.
So that frontend can immediately display the current state and doesn't have to aggregate first.
Otherwise the UI will update old blocks which are still visible to an empty shell.
If only one authority has already submitted consensus info for a new block then the cache of that one is offset by one from all other authorities.
When nodes lose their connection to telemetry or connect on first time they sent their current authority set for the UI to have something to display. These sets don't contain an explicit block number, because the set didn't change -- it just got resent. In this case the set is `undefined`.
This is necessary to cover the case where one node connects, submits its authority set containing another node which has not yet connected to Telemetry. In this case we still want to create a shell object and fill it with the address.
In the case of only one block having been produced, two authorities, and only one authority connected, the UI did not show up.
maciejhirsz
left a comment
maciejhirsz
left a comment
There was a problem hiding this comment.
This looks really impressive! The main issue I have is what I wrote in the comment about sending mappings between BE and FE.
I'd be also interested to look at how much data would be transmitted for large sets of validators. In worst case there might be some quadratic behavior here (all validators showing state for all other validators). At very least it might make sense to separate the feeds between node listing and consensus view, so that when you are observing consensus you don't get messages about node updates, and vice versa.
Didn't dig much deeper, as I reckon changing this will amount to enough code changes to warrant a new review. I'm happy to help implementing the changes, but it will have to wait till after Sub0 (need to finish my slides over Easter :P).
maciejhirsz
left a comment
maciejhirsz
left a comment
There was a problem hiding this comment.
Some grumbles, but they are all pretty minor. 👍
|
|
||
| // we manually parse the authorities message, because the array was formatted as a | ||
| // string by substrate before sending it. | ||
| const authorities = JSON.parse(String(message.authorities)) as Types.Authorities; |
There was a problem hiding this comment.
This is more FYI, but I think the telemetry allows nested JSON now without having to serialize things as strings.
There was a problem hiding this comment.
Do I understand you right that we should be able to remove the serde serialization happening here?
(I had tried it naively without looking any further into it and ran into the trait 'slog::Value' is not implemented for 'std::vec::Vec<std::string::String>'.)
There was a problem hiding this comment.
Yes, I think that was fixed recently.
This reverts commit bf4d9ea.
Change version number to `^1.1.45` and run `npm update polkadot-identicon`.
Changed version number, ran cd packages/frontend/ && npm update react-reasure && cd ../../ && yarn install
|
@maciejhirsz Thanks for the comments, they were all really informative! I updated the PR with your comments addressed. |
Addresses paritytech/substrate#422.
The PR on Substrate's side is paritytech/substrate#2198.
You can view the interactive visualization here (ping me if it is not running).
Should look roughly like this:
