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

Companion for 7155 (WeightInfo for Babe and Grandpa) #1736

Merged
4 commits merged into from
Sep 21, 2020

Conversation

shawntabrizi
Copy link
Member

@shawntabrizi shawntabrizi commented Sep 21, 2020

This simply adds the WeightInfo associated type to the Babe and Grandpa traits.

It uses the default values from Substrate which are still custom written.

paritytech/substrate#7155

@shawntabrizi shawntabrizi added A2-insubstantial Pull request requires no code review (e.g., a sub-repository hash update). A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Sep 21, 2020
@gui1117
Copy link
Contributor

gui1117 commented Sep 21, 2020

I feels slightly afraid of using type WeightInfo = (); in polkadot, on the substrate side nothing says that those default weight are used in production.
Maybe we could add a comment in substrate that default implementation is used in production or otherwise we can copy them here again. And when automatic weight implementation is done, manual weight implementation must be checked.

I'm not sure what is best, anyway I'm ok to use = () in polkadot

@andresilva
Copy link
Contributor

I agree with @thiolliere that this could be a bit dangerous. I think just copying the weights from substrate here would probably be a sensible thing to do, or maybe keep the same weights in substrate but implemented for some struct rather than (). Either way I think @shawntabrizi intends to deal with all custom weights in a follow-up PR so this can be addressed there.

Copy link
Contributor

@gui1117 gui1117 left a comment

Choose a reason for hiding this comment

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

yes I'm ok to address it in follow-up PR

@shawntabrizi
Copy link
Member Author

What is the difference between using = () and = SomeStruct?

In general, these weights were hardcoded into substrate before. The way it is written, nothing about the behavior of how these weights would update has changed.

@andresilva
Copy link
Contributor

andresilva commented Sep 21, 2020

There is no practical difference, but maybe = SomeStruct could communicate better that this is used in Polkadot and that these weights are critical 🤷 I don't really have a strict opinion on this, as you said these weights were already hardcoded in Substrate in the first place and could already be changed easily there without it getting noticed in Polkadot. But for example we added some tests for weights in Polkadot to make sure that some invariants were not broken (i.e. if some weight changed in Substrate we'd be aware of it in Polkadot).

@ghost
Copy link

ghost commented Sep 21, 2020

Waiting for commit status.

@ghost
Copy link

ghost commented Sep 21, 2020

Merge failed: "At least 3 approving reviews are required by reviewers with write access."

@shawntabrizi
Copy link
Member Author

bot merge

@ghost
Copy link

ghost commented Sep 21, 2020

Trying merge.

@ghost ghost merged commit 6c169ca into master Sep 21, 2020
@ghost ghost deleted the shawntabrizi-babe-weightinfo branch September 21, 2020 14:03
ordian added a commit that referenced this pull request Sep 22, 2020
* master:
  Companion for 7155 (WeightInfo for Babe and Grandpa) (#1736)
  Companion PR for #7136 (WeightInfo for Session / Offences) (#1735)
  Bump jsonrpc-core to v15 (#1737)
  Companion PR for #6215 (#1654)
  Companion PR for #7138 (WeightInfo for Scheduler) (#1734)
  Companion PR for Bounties #5715 (#1336)
ordian added a commit that referenced this pull request Sep 23, 2020
* master:
  Update to substrate 2.0 (#1744)
  Companion: Handle construct_runtime breaking change. (#1692)
  Companion for `ModuleToIndex` to `PalletInfo` rename (#1743)
  Companion for substrate/pull/7161 (#1739)
  Companion for 7155 (WeightInfo for Babe and Grandpa) (#1736)
  Companion PR for #7136 (WeightInfo for Session / Offences) (#1735)
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. A2-insubstantial Pull request requires no code review (e.g., a sub-repository hash update). B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants