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

Updated assignments commitments #141

Closed

Conversation

mooselumph
Copy link
Collaborator

Why are these changes needed?

  • Updates the blobHeaderHash to include the ChunkLength parameter for each quorum so that consensus on ChunkLength is enforced across nodes.
  • Implements corresponding updates to EigenDAServiceManager.sol and MockRollup.sol

Checks

  • I've made sure the lint is passing in this PR.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, in that case, please comment that they are not relevant.
  • Testing Strategy
    • Unit tests
    • Integration tests
    • This PR is not tested :(

Copy link
Contributor

@ian-shim ian-shim left a comment

Choose a reason for hiding this comment

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

@mooselumph
Copy link
Collaborator Author

I think this also needs to be updated: https://github.com/Layr-Labs/eigenda/blob/master/test/synthetic-test/synthetic_client_test.go#L497

Good catch, should be fixed now!

@mooselumph mooselumph requested a review from ian-shim January 6, 2024 01:18
@mooselumph mooselumph changed the base branch from master to m2-mainnet-contracts January 10, 2024 22:09
@mooselumph mooselumph changed the base branch from m2-mainnet-contracts to master January 10, 2024 22:10
@mooselumph mooselumph changed the base branch from master to m2-mainnet-contracts January 11, 2024 00:30
@mooselumph mooselumph changed the base branch from m2-mainnet-contracts to master January 11, 2024 00:30
quorums := make([]rollupbindings.IEigenDAServiceManagerQuorumBlobParam, len(blobHeader.GetBlobQuorumParams()))
for i, quorum := range blobHeader.GetBlobQuorumParams() {
quorums[i] = rollupbindings.IEigenDAServiceManagerQuorumBlobParam{
QuorumNumber: uint8(quorum.GetQuorumNumber()),
AdversaryThresholdPercentage: uint8(quorum.GetAdversaryThresholdPercentage()),
QuorumThresholdPercentage: uint8(quorum.GetQuorumThresholdPercentage()),
QuantizationParameter: uint8(quorum.GetQuantizationParam()),
ChunkLength: quorum.GetChunkLength(),
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: uint32?

@siddimore
Copy link
Contributor

Synthetic Test client changes look good. PR changes overall LGTM

@mooselumph mooselumph closed this Jan 18, 2024
Copy link
Contributor

@jianoaix jianoaix left a comment

Choose a reason for hiding this comment

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

look good

@@ -95,10 +95,17 @@ contract MockRollupTest is BLSMockAVSDeployer {
//get commitment with illegal value
(IEigenDAServiceManager.BlobHeader memory blobHeader, EigenDABlobUtils.BlobVerificationProof memory blobVerificationProof) = _getCommitment(pseudoRandomNumber);

IEigenDAServiceManager.QuorumBlobParam[] memory quorumBlobParamsCopy = new IEigenDAServiceManager.QuorumBlobParam[](2);
Copy link
Contributor

Choose a reason for hiding this comment

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

better not to hardcode it, i.e. create the array with blobHeader.quorumBlobParams.length instead of 2

EigenDABlobUtils.verifyBlob(blobHeader, eigenDAServiceManager, blobVerificationProof);

// zero out the chunkLengths (this a temporary hack)
for (uint256 i = 0; i < blobHeader.quorumBlobParams.length; i++)
Copy link
Contributor

Choose a reason for hiding this comment

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

why zero out?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants