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

extract some grandpa types to Primitives crate #12208

Merged

Conversation

yjhmelody
Copy link
Contributor

@yjhmelody yjhmelody commented Sep 8, 2022

  • I improve type params since many types only want to use Header rather than the Block type.
  • These types are used by me in both no_std/std. And I need grandpa justification field be pub.

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.

Two nitpicks, otherwise it looks good.

@@ -42,9 +43,25 @@ use crate::{AuthorityList, Commit, Error};
/// nodes, and are used by syncing nodes to prove authority set handoffs.
#[derive(Clone, Encode, Decode, PartialEq, Eq, Debug)]
pub struct GrandpaJustification<Block: BlockT> {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
pub struct GrandpaJustification<Block: BlockT> {
pub struct GrandpaJustification<Header: HeaderT> {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also want to use it in this way. But its impls must use Block type.

Comment on lines +47 to +48
pub justification: sp_finality_grandpa::GrandpaJustification<Block::Header>,
_block: PhantomData<Block>,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
pub justification: sp_finality_grandpa::GrandpaJustification<Block::Header>,
_block: PhantomData<Block>,
pub justification: sp_finality_grandpa::GrandpaJustification<Header>,

@bkchr bkchr added 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. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Sep 8, 2022
@davxy
Copy link
Member

davxy commented Sep 9, 2022

LGTM.

If we want to go even deeper I think that almost everything concerning communication
can be bounded over Header trait instead of Block.

For example starting from the CurrentRounds

https://github.com/yjhmelody/substrate/blob/6e4f14775ab528d5650ad0e6ed8220bb99601331/client/finality-grandpa/src/environment.rs#L173

The various messages

https://github.com/yjhmelody/substrate/blob/6e4f14775ab528d5650ad0e6ed8220bb99601331/client/finality-grandpa/src/communication/gossip.rs#L347

All the way up to the GossipMessage.

https://github.com/yjhmelody/substrate/blob/6e4f14775ab528d5650ad0e6ed8220bb99601331/client/finality-grandpa/src/communication/gossip.rs#L326

So in short I think that Grandpa mostly requires to be generic over the Header (as the external crate it is using).

Should also be said that in practice it also doesn't make much difference.
Indeed in a lot of places (in Substrate) generics are bound to Block where they can just be bound to Header.

@yjhmelody
Copy link
Contributor Author

yjhmelody commented Sep 9, 2022

So in short I think that Grandpa mostly requires to be generic over the Header (as the external crate it is using).

Should also be said that in practice it also doesn't make much difference. Indeed in a lot of places (in Substrate) generics are bound to Block where they can just be bound to Header.

Yeah, I know. When grandpa types and logic are used for a specific instance chain, Block trait constraint is enough. But current I need to verifiy grandpa in multi-chains(though all Block types maybe are same). So Header trait is better.

@davxy
Copy link
Member

davxy commented Sep 9, 2022

So in short I think that Grandpa mostly requires to be generic over the Header (as the external crate it is using).
Should also be said that in practice it also doesn't make much difference. Indeed in a lot of places (in Substrate) generics are bound to Block where they can just be bound to Header.

Yeah, I know. When grandpa types and logic are used for a specific instance chain, Block trait constraint is enough. But current I need to verifiy grandpa in multi-chains(though all Block types maybe are same). So Header trait is better.

Indeed I was silently pushing for bouding everything to Header :-D
But that is a lot of work and maybe a subject for another PR

@bkchr
Copy link
Member

bkchr commented Sep 12, 2022

bot merge

@paritytech-processbot
Copy link

Waiting for commit status.

@paritytech-processbot paritytech-processbot bot merged commit f1c60e5 into paritytech:master Sep 12, 2022
@yjhmelody yjhmelody deleted the primitives-grandpa-types branch September 13, 2022 02:35
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
* extract some grandpa types to primitives

* fmt

* fmt
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. 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. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants