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.

Sign up for GitHub

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

Jump to bottom

Implement parachain inherents #2566

Merged
merged 125 commits into from
Oct 7, 2022
Merged

Implement parachain inherents #2566

merged 125 commits into from
Oct 7, 2022

Conversation

kishansagathiya
Copy link
Contributor

@kishansagathiya kishansagathiya commented May 23, 2022 •
edited
Loading

Changes

  • Implements parachn0 inherent

Tests

go test -tags integration github.com/ChainSafe/gossamer

Issues

Fixes #2426

Primary Reviewer

@timwu20

@kishansagathiya kishansagathiya marked this pull request as ready for review May 25, 2022 15:26
qdm12
qdm12 reviewed May 25, 2022
View reviewed changes
Copy link
Contributor

@qdm12 qdm12 left a comment

Choose a reason for hiding this comment

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

It looks like many exported types got added to lib/babe.
Are you sure we really do need those types? I would suggest just relying on variable names to reduce complexity. At the very least, these types should be unexported since they are not used outside the babe package.

dot/types/inherents.go Outdated Show resolved Hide resolved
dot/types/inherents.go Outdated Show resolved Hide resolved
dot/types/inherents.go Outdated Show resolved Hide resolved
lib/babe/build.go Outdated Show resolved Hide resolved
lib/babe/build.go Outdated Show resolved Hide resolved
lib/runtime/wasmer/imports.go Outdated Show resolved Hide resolved
lib/runtime/wasmer/imports.go Outdated Show resolved Hide resolved
lib/runtime/wasmer/imports.go Outdated Show resolved Hide resolved
lib/babe/build.go Outdated Show resolved Hide resolved
lib/babe/build.go Outdated Show resolved Hide resolved
timwu20
timwu20 requested changes Jun 3, 2022
View reviewed changes
dot/types/inherents.go Outdated Show resolved Hide resolved
lib/babe/parachain_inherents.go Outdated Show resolved Hide resolved
lib/babe/parachain_inherents.go Outdated Show resolved Hide resolved
lib/babe/parachain_inherents.go Outdated Show resolved Hide resolved
lib/babe/parachain_inherents.go Outdated Show resolved Hide resolved
lib/babe/parachain_inherents.go Outdated Show resolved Hide resolved
lib/babe/parachain_inherents.go Outdated Show resolved Hide resolved
lib/babe/parachain_inherents.go Outdated Show resolved Hide resolved
lib/babe/parachain_inherents.go Outdated Show resolved Hide resolved
kishansagathiya and others added 12 commits September 28, 2022 19:25
@kishansagathiya @qdm12
Update lib/babe/parachain_inherents_test.go
8d82ec5 
Co-authored-by: Quentin McGaw <[email protected]>
@kishansagathiya @qdm12
Update lib/babe/parachain_inherents.go
3815c0e 
Co-authored-by: Quentin McGaw <[email protected]>
@kishansagathiya
use MapRange to preserve order of keys in map while encoding
5e927ec
@kishansagathiya
Merge branch 'kishan/feat/parachain-inherents' of github.com:ChainSaf…
ba45f8f 
…e/gossamer into kishan/feat/parachain-inherents
@kishansagathiya
Merge branch 'development' into kishan/feat/parachain-inherents
f9f2740
@kishansagathiya
fix test
62ca950
@kishansagathiya @qdm12
Update lib/babe/parachain_inherents_test.go
942429e 
Co-authored-by: Quentin McGaw <[email protected]>
@kishansagathiya @qdm12
Update lib/babe/parachain_inherents_test.go
f4f5716 
Co-authored-by: Quentin McGaw <[email protected]>
@kishansagathiya @qdm12
Update lib/babe/parachain_inherents_test.go
4a697d0 
Co-authored-by: Quentin McGaw <[email protected]>
@kishansagathiya
added some values in parachain inherents for tests
b658c9e
@kishansagathiya
Merge branch 'kishan/feat/parachain-inherents' of github.com:ChainSaf…
e414ecf 
…e/gossamer into kishan/feat/parachain-inherents
@kishansagathiya
more non-zero values
d5affe4
@timwu20
Copy link
Contributor

timwu20 commented Sep 29, 2022

@timwu20 When you try to scale encode a struct, its private (not exported) field can be read by our scale package. I observed this with InherentsData struct which has data unexported. With that in mind, does it make sense to keep things exported in parachain inherents struct?
@qdm12

That's interesting. Most Go codecs usually don't have access to unexported fields, should we do the same thing @timwu20 ? Or is there a use case where we need access to unexported fields?

That's how they implemented InherentData in substrate. They manually encode it, and the trait ensures that the type that you are using can be scale encoded.

timwu20
timwu20 requested changes Sep 29, 2022
View reviewed changes
dot/types/inherents.go Outdated Show resolved Hide resolved
dot/types/inherents.go Show resolved Hide resolved
pkg/scale/encode.go Outdated Show resolved Hide resolved
lib/babe/build.go Show resolved Hide resolved
@kishansagathiya kishansagathiya mentioned this pull request Sep 30, 2022
Merged
@kishansagathiya kishansagathiya merged commit 14dda74 into development Oct 7, 2022
@kishansagathiya kishansagathiya deleted the kishan/feat/parachain-inherents branch October 7, 2022 15:32
EclesioMeloJunior pushed a commit that referenced this pull request Oct 12, 2022
@kishansagathiya @EclesioMeloJunior
Implement parachain inherents (#2566)
e522a25 
- Added support for parachain inherent and newheads
- Improved inherentsData's encode function
- Made inherent identifers enums
- Common setInherent function
@github-actions
Copy link

🎉 This PR is included in version 0.7.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@qdm12 qdm12 qdm12 approved these changes

@timwu20 timwu20 timwu20 approved these changes

@edwardmack edwardmack Awaiting requested review from edwardmack

@EclesioMeloJunior EclesioMeloJunior Awaiting requested review from EclesioMeloJunior EclesioMeloJunior is a code owner

@jimjbrettj jimjbrettj Awaiting requested review from jimjbrettj jimjbrettj is a code owner

@noot noot Awaiting requested review from noot

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement Parachain Inherents
4 participants
@kishansagathiya @qdm12 @timwu20 @noot