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.

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

Add minimal withdrawal size #11658

Merged
merged 9 commits into from
Nov 22, 2022
Merged

Add minimal withdrawal size #11658

merged 9 commits into from
Nov 22, 2022

Conversation

terencechain
Copy link
Member

Add minimal withdrawal size for ssz. This is needed to pass minimal ssz vector test

@terencechain terencechain self-assigned this Nov 13, 2022
@terencechain terencechain requested review from prestonvanloon and a team as code owners November 13, 2022 16:24
@@ -20,6 +20,7 @@ mainnet = {
"sync_committee_bits.type": "github.com/prysmaticlabs/go-bitfield.Bitvector512",
"sync_committee_aggregate_bytes.size": "16",
"sync_committee_aggregate_bits.type": "github.com/prysmaticlabs/go-bitfield.Bitvector128",
"withdrawal.size": "16",
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be linked to the minimal preset somehow? We define MAX_WITHDRAWALS_PER_PAYLOAD there, it'd be good if we could automatically change one if we change the other

Copy link
Member Author

Choose a reason for hiding this comment

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

Out of my knowledge zone. I'm not sure how to do that. Looking at other variables here, I don't think we've done it before. cc @prestonvanloon for input

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we can do this with the existing tooling. However, we rarely change this file and all the values are contained in the same file so it may not be worth the trouble of deduplicating. I would approve as is

Copy link
Contributor

Choose a reason for hiding this comment

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

The clear separation of minimal and mainnet in the same file makes this deduplication less of an issue as well

Copy link
Member

Choose a reason for hiding this comment

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

Probably. Someone has to go figure out though

@terencechain terencechain added OK to merge Ready For Review A pull request ready for code review labels Nov 21, 2022
@rauljordan
Copy link
Contributor

ptal @prestonvanloon

Copy link
Member

@prestonvanloon prestonvanloon left a comment

Choose a reason for hiding this comment

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

Templating looks good. I didn't check that the values are correct.

@prylabs-bulldozer prylabs-bulldozer bot merged commit e2ffaf9 into develop Nov 22, 2022
@delete-merged-branch delete-merged-branch bot deleted the withdrawal-minimal branch November 22, 2022 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready For Review A pull request ready for code review Spec
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants