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

EIP-7594: PeerDAS explicit csc integer size #3897

Merged
merged 14 commits into from
Aug 28, 2024
Merged

Conversation

barnabasbusa
Copy link
Member

No description provided.

Copy link
Contributor

@g11tech g11tech left a comment

Choose a reason for hiding this comment

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

lgtm

@agnxsh
Copy link
Contributor

agnxsh commented Aug 24, 2024

lgtm from nimbus

specs/_features/eip7594/p2p-interface.md Outdated Show resolved Hide resolved
Comment on lines 60 to 61
def test_custody_subnet_count_int_bitlength(spec, custody_subnet_count):
assert uint8(custody_subnet_count) == custody_subnet_count
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we want to set DATA_COLUMN_SIDECAR_SUBNET_COUNT to uint8 too, and then assert spec.config.DATA_COLUMN_SIDECAR_SUBNET_COUNT == uint8(spec.config.DATA_COLUMN_SIDECAR_SUBNET_COUNT)?

Copy link
Member Author

Choose a reason for hiding this comment

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

In this case we are only checking whether a client able read in the config.yaml value from the spec. Would it make more sense to assert that the csc value is uint8 in the enr value instead? This test case on its own probably makes little to no sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

These unit tests only cover the helper functions. Pyspec tests don't cover the actual process where the client sets the ENR values.

We have an assert custody_subnet_count <= DATA_COLUMN_SIDECAR_SUBNET_COUNT in get_custody_columns that capped the maximum value of custody_subnet_count.

Copy link
Member

@jtraglia jtraglia left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Copy link
Contributor

@hwwhww hwwhww left a comment

Choose a reason for hiding this comment

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

I pushed the commits to set DATA_COLUMN_SIDECAR_SUBNET_COUNT as uint8.

LGTM. Thank you @barnabasbusa!

@hwwhww hwwhww added the EIP-7594 PeerDAS label Aug 28, 2024
@hwwhww hwwhww merged commit a5990f9 into ethereum:dev Aug 28, 2024
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants