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

Add proof size summary #10639

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

Moliholy
Copy link
Contributor

@Moliholy Moliholy commented Jun 4, 2024

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

Screenshot 2024-06-04 at 15 03 59

@TarikGul TarikGul self-requested a review June 7, 2024 17:13
@Moliholy
Copy link
Contributor Author

@TarikGul any feedback? 👀

@TarikGul
Copy link
Member

@TarikGul any feedback? 👀

Yes ofcourse, sorry the extension has really been absorbing most of my time lately. I promise to have this reviewed today :)

Copy link
Member

@TarikGul TarikGul left a 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
Copy link
Member

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.

Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

@Moliholy Moliholy Jun 20, 2024

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()}
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants