Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

Make weights compatible with u64 #4192

Merged
bkchr merged 5 commits into from
Nov 29, 2019
Merged

Make weights compatible with u64 #4192

bkchr merged 5 commits into from
Nov 29, 2019

Conversation

@stanislav-tkach
Copy link
Contributor

@stanislav-tkach stanislav-tkach commented Nov 23, 2019

Closes #4141.

@stanislav-tkach stanislav-tkach added the A3-in_progress Pull request is in progress. No review needed at this stage. label Nov 23, 2019
@kianenigma
Copy link
Contributor

kianenigma commented Nov 25, 2019

need to check if this change is breaking in anyway for Kusama

@stanislav-tkach stanislav-tkach removed the A3-in_progress Pull request is in progress. No review needed at this stage. label Nov 25, 2019
@stanislav-tkach stanislav-tkach changed the title [In Progress] Make weights compatible with u64 Make weights compatible with u64 Nov 25, 2019
bkchr
bkchr reviewed Nov 25, 2019
View reviewed changes
bin/node/runtime/src/impls.rs Outdated
10 * mb,
Weight::max_value() / 2,
Weight::max_value()
(u32::max_value() / 2).into(),
Copy link
Member

@bkchr bkchr Nov 25, 2019

Choose a reason for hiding this comment

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

This is against the whole purpose of this pr.

@gavofyork gavofyork added the A0-please_review Pull request needs code review. label Nov 26, 2019
@stanislav-tkach
Copy link
Contributor Author

stanislav-tkach commented Nov 27, 2019

Current state: Weight is used everywhere instead of u32, so changing the underlying Weight type no longer requires additional refactoring. There is an issue with u64, though: the weight_to_fee_should_not_overflow_on_large_weights test fails in that case. I have digged into that a little, but I need more time.

I think that we can merge this pull request as it is and create a separate issue for dealing with the overflow.

bkchr
bkchr approved these changes Nov 28, 2019
View reviewed changes
Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

Needs resolving.

@gavofyork
Copy link
Member

gavofyork commented Nov 28, 2019

CI fails.

@gavofyork gavofyork removed the A0-please_review Pull request needs code review. label Nov 28, 2019
@stanislav-tkach
Copy link
Contributor Author

stanislav-tkach commented Nov 29, 2019

I have fixed the benchmarks, though gitlab-check_polkadot is failing now. As far as I can see, it is unrelated to my changes.

@bkchr bkchr merged commit d5e189e into master Nov 29, 2019
@bkchr bkchr deleted the stas-4141 branch November 29, 2019 19:20
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@kianenigma kianenigma Awaiting requested review from kianenigma

@pepyakin pepyakin Awaiting requested review from pepyakin

1 more reviewer

@bkchr bkchr bkchr approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Make weights compatible with u64

5 participants

@stanislav-tkach @kianenigma @gavofyork @bkchr