-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Add proof size summary #10639
base: master
Are you sure you want to change the base?
Add proof size summary #10639
Conversation
@TarikGul any feedback? 👀 |
Yes ofcourse, sorry the extension has really been absorbing most of my time lately. I promise to have this reviewed today :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few comments.
@@ -59,7 +60,7 @@ function BlockByHash ({ className = '', error, value }: Props): React.ReactEleme | |||
const [isVersionCurrent, maxBlockWeight] = useMemo( | |||
() => [ | |||
!!runtimeVersion && api.runtimeVersion.specName.eq(runtimeVersion.specName) && api.runtimeVersion.specVersion.eq(runtimeVersion.specVersion), | |||
api.consts.system.blockWeights && api.consts.system.blockWeights.maxBlock && convertWeight(api.consts.system.blockWeights.maxBlock).v1Weight | |||
api.consts.system.blockWeights && api.consts.system.blockWeights.maxBlock && convertWeight(api.consts.system.blockWeights.maxBlock).v2Weight |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't take into account chains that don't use V2Weight. I would make it part of a conditional that checks to see if v2Weight
exists use that, else use v1Weight
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That being said I would imagine the only chains that still use it are under the live networks
tab.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmmm the v2Weight
is always returned, and it is compatible with v1s. I modified the convertWeight
method conveniently for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But since you legitimately had this question, perhaps that method should be modified to always return v2 weights 🤔
What do you think?
@@ -123,7 +124,8 @@ function BlockByHash ({ className = '', error, value }: Props): React.ReactEleme | |||
<div className={className}> | |||
<Summary | |||
events={events} | |||
maxBlockWeight={maxBlockWeight} | |||
maxBlockWeight={(maxBlockWeight as V2Weight).refTime.toBn()} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to above we can add a ternary here 2 different Summary
cards depending on the type of weight.
Description
This PR includes the proof size as a summary along with the consumed weight.
Rationale
When checking the weights, there could be occasions when one would expect a block to be fully utilized, while weight consumption is low. This is due to the fact that the PoV is quite large, since weights in V2 are two-dimensional. The main idea is to display both dimensions of the weight, and not just one.
Screenshots