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

feat(parachain): Create struct for Approval Distribution Message #3326

Merged
merged 5 commits into from
Jun 21, 2023
Merged

feat(parachain): Create struct for Approval Distribution Message #3326

merged 5 commits into from
Jun 21, 2023

Conversation

edwardmack
Copy link
Contributor

Changes

Tests

go test github.com/ChainSafe/gossamer/lib/parachain -v

Issues

Closes: #3288

Primary Reviewer

@kishansagathiya

@kanishkatn
Copy link
Contributor

Do we wanna merge it to development or feat/parachain?

@edwardmack
Copy link
Contributor Author

Do we wanna merge it to development or feat/parachain?

It should be merged to feat/parachain but it depends on PR #3315 which was merged to development. Should we rebase feat/parachain onto development first? Or should I change this PR to merge to feat/parachain now knowing that we'll rebase later?

@kanishkatn
Copy link
Contributor

Yes, let's rebase it. I can do it!

@edwardmack edwardmack changed the base branch from development to feat/parachain June 15, 2023 15:05
@kanishkatn
Copy link
Contributor

It's protected, can't touch it. @timwu20 help!

kishansagathiya
kishansagathiya reviewed Jun 16, 2023
View reviewed changes
Copy link
Contributor

@kishansagathiya kishansagathiya left a comment

Choose a reason for hiding this comment

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

Looks good. Most of the of review-comments are minor.

I think there is a scope to improve code-comments since many of them are missing verbs. I have pointed out some of them, but not all.

lib/parachain/networkprotocol.go Outdated Show resolved Hide resolved
lib/parachain/networkprotocol_test.go Outdated Show resolved Hide resolved
lib/parachain/networkprotocol.go Outdated Show resolved Hide resolved
lib/parachain/networkprotocol.go Outdated Show resolved Hide resolved
lib/parachain/networkprotocol_test.go Outdated Show resolved Hide resolved
lib/parachain/networkprotocol.go Outdated Show resolved Hide resolved
lib/parachain/networkprotocol.go Outdated Show resolved Hide resolved
lib/parachain/networkprotocol.go Outdated Show resolved Hide resolved
lib/parachain/networkprotocol.go Outdated Show resolved Hide resolved
lib/parachain/networkprotocol.go Outdated Show resolved Hide resolved
kanishkatn
kanishkatn approved these changes Jun 19, 2023
View reviewed changes
Copy link
Contributor

@kanishkatn kanishkatn left a comment

Choose a reason for hiding this comment

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

lgtm! We should prolly rename the file to approval_subsystem.go #3316 (comment)

lib/parachain/approval_distribution_message.go Outdated Show resolved Hide resolved
jimjbrettj
jimjbrettj approved these changes Jun 20, 2023
View reviewed changes
Copy link
Contributor

@jimjbrettj jimjbrettj left a comment

Choose a reason for hiding this comment

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

lgtm

@edwardmack edwardmack force-pushed the ed/approvalDistributionMessageStruct branch from 76b946a to 4612a70 Compare June 21, 2023 14:41
@edwardmack
Copy link
Contributor Author

lgtm! We should prolly rename the file to approval_subsystem.go #3316 (comment)

I'm open to renaming as necessary, note that I renamed this in response to this comment #3326 (comment) @kishansagathiya What do you think of approval_subsystem.go?

@kanishkatn
Copy link
Contributor

I'm open to renaming as necessary, note that I renamed this in response to this comment #3326 (comment) @kishansagathiya What do you think of approval_subsystem.go?

No, it's okay for now.
I found out how complex these types can get today. I've also started creating separate files for them!

@edwardmack edwardmack merged commit 6a6480e into feat/parachain Jun 21, 2023
@edwardmack edwardmack deleted the ed/approvalDistributionMessageStruct branch June 21, 2023 15:13
kishansagathiya pushed a commit that referenced this pull request Jul 12, 2023
@edwardmack @kishansagathiya
feat(parachain): Create struct for Approval Distribution Message (#3326)
5e99a1f
kishansagathiya pushed a commit that referenced this pull request Jul 12, 2023
@edwardmack @kishansagathiya
feat(parachain): Create struct for Approval Distribution Message (#3326)
c284a70
kishansagathiya pushed a commit that referenced this pull request Jul 14, 2023
@edwardmack @kishansagathiya
feat(parachain): Create struct for Approval Distribution Message (#3326)
1c04418
kishansagathiya pushed a commit that referenced this pull request Jul 27, 2023
@edwardmack @kishansagathiya
feat(parachain): Create struct for Approval Distribution Message (#3326)
11eaf7c
@kishansagathiya kishansagathiya mentioned this pull request Jul 31, 2023
Closed
kishansagathiya pushed a commit that referenced this pull request Aug 4, 2023
@edwardmack @kishansagathiya
feat(parachain): Create struct for Approval Distribution Message (#3326)
a8e0d10
kishansagathiya pushed a commit that referenced this pull request Aug 22, 2023
@edwardmack @kishansagathiya
feat(parachain): Create struct for Approval Distribution Message (#3326)
30dd4ab
kishansagathiya pushed a commit that referenced this pull request Aug 22, 2023
@edwardmack @kishansagathiya
feat(parachain): Create struct for Approval Distribution Message (#3326)
8a47fd5
edwardmack added a commit that referenced this pull request Aug 30, 2023
@edwardmack
feat(parachain): Create struct for Approval Distribution Message (#3326)
665018a
edwardmack added a commit that referenced this pull request Aug 31, 2023
@edwardmack
feat(parachain): Create struct for Approval Distribution Message (#3326)
eb7d7a6
kishansagathiya pushed a commit that referenced this pull request Sep 14, 2023
@edwardmack @kishansagathiya
feat(parachain): Create struct for Approval Distribution Message (#3326)
4a103f4
kishansagathiya pushed a commit that referenced this pull request Jan 23, 2024
@edwardmack @kishansagathiya
feat(parachain): Create struct for Approval Distribution Message (#3326)
c94b5f7
kishansagathiya pushed a commit that referenced this pull request Jan 24, 2024
@edwardmack @kishansagathiya
feat(parachain): Create struct for Approval Distribution Message (#3326)
118753d
timwu20 pushed a commit that referenced this pull request Jun 13, 2024
@edwardmack @timwu20
feat(parachain): Create struct for Approval Distribution Message (#3326)
55c23c0
timwu20 pushed a commit that referenced this pull request Jun 17, 2024
@edwardmack @timwu20
feat(parachain): Create struct for Approval Distribution Message (#3326)
b8bf16a
timwu20 pushed a commit that referenced this pull request Jun 20, 2024
@edwardmack @timwu20
feat(parachain): Create struct for Approval Distribution Message (#3326)
178663e
edwardmack added a commit that referenced this pull request Jun 26, 2024
@edwardmack
feat(parachain): Create struct for Approval Distribution Message (#3326)
c763403
edwardmack added a commit that referenced this pull request Jul 12, 2024
@edwardmack
feat(parachain): Create struct for Approval Distribution Message (#3326)
65aea63
kishansagathiya pushed a commit that referenced this pull request Jul 15, 2024
@edwardmack @kishansagathiya
feat(parachain): Create struct for Approval Distribution Message (#3326)
e8a93a1
kishansagathiya pushed a commit that referenced this pull request Jul 15, 2024
@edwardmack @kishansagathiya
feat(parachain): Create struct for Approval Distribution Message (#3326)
8dc6bd9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jimjbrettj jimjbrettj jimjbrettj approved these changes

@kanishkatn kanishkatn kanishkatn approved these changes

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

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

@dimartiro dimartiro Awaiting requested review from dimartiro

@kishansagathiya kishansagathiya Awaiting requested review from kishansagathiya

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

Successfully merging this pull request may close these issues.

Create a go struct for Approval Distribution Message
4 participants
@edwardmack @kanishkatn @kishansagathiya @jimjbrettj