refactor!: version contract types #1516

pakim249CAL · 2025-04-30T21:53:36Z

Why are these changes needed?

Moves around the types to a versioned format.
Moves version specific hashing from EigenDAHasher to the respective verification libraries
does away with the combined CertVerifier to prep the groundwork for a new CertVerifierRouter to take its place.

Checks

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.
I've checked the new test coverage and the coverage percentage didn't drop.
Testing Strategy
- Unit tests
- Integration tests
- This PR is not tested :(

fix: remove operator state retriever change fix: readd v1 test

contracts/src/libraries/V1/EigenDATypesV1.sol

contracts/src/libraries/V2/EigenDATypesV2.sol

feat: remove old files refactor: file layout

…ntract-types

api/clients/v2/coretypes/conversion_utils.go

api/clients/v2/coretypes/eigenda_cert.go

litt3 · 2025-05-02T15:17:25Z

contracts/src/core/libraries/v1/EigenDATypesV1.sol

+        uint8 codingRate;
+    }
+
+    struct SecurityThresholds {


Should types used by both v1 and v2 live in this v1 specific file?

Maybe it would make sense to have a common package?

My idea was simply to separate out WHEN types are introduced. And future code can reference the same types from old versions, but anytime new types are introduced there should be a new version of types.

I don't like common because common needs to be regularly maintained/culled, and it would be hard to name things if we ever needed to modify a struct, but keep the old one for backwards compatibility. I think it is better to just namespace the types as we add them by eigenDA version so that we don't have to worry about this scenario.

contracts/src/core/EigenDAServiceManager.sol

contracts/src/periphery/cert/interfaces/IEigenDACertVerifier.sol

contracts/src/periphery/cert/v2/EigenDACertVerificationV2Lib.sol

samlaf · 2025-05-02T15:30:31Z

contracts/src/periphery/cert/v2/EigenDACertVerificationV2Lib.sol

@@ -64,7 +63,7 @@ library EigenDACertVerificationV2Lib {
        IEigenDASignatureVerifier signatureVerifier,
        DATypesV2.BatchHeaderV2 memory batchHeader,
        DATypesV2.BlobInclusionInfo memory blobInclusionInfo,
-        NonSignerStakesAndSignature memory nonSignerStakesAndSignature,
+        DATypesV1.NonSignerStakesAndSignature memory nonSignerStakesAndSignature,


feels weird that we're using v1 types in a v2 lib no? Perhaps the main core types should be in a DATypesShared lib or something? Especially since these types are eigenlayer related afaiu and not eigenda v1/v2 related right?

I am trying to move AWAY from "shared" files because it is very confusing and it is hard to figure out which structs are needed for V1, and which are needed for V2! I understand that this might seem a little strange, but I would argue that piquing your curiosity here is good. Because otherwise you might not ever have understood that that particular type has been used since EigenDA V1.

But it shouldn't matter that that type has been introduced since EigenDA V1. The plan is to anyways get rid of EigenDA V1 soon, and many have proposed we will literally delete everything related to V1 from the repo once we do that. So what will you do with these structs at that point?

I think if anything, I would prefer we duplicate all the structs in V2. I think a little copying is not that bad. I prefer to completely segregate v1 from v2 personally.

The problem with copying is that they end up being separate types in solidity though. We already have some duplicate structs that cause some problems in this repo, like NonSignerStakesAndSignatures which is duplicated in this repo from BLSSignatureChecker.

samlaf · 2025-05-02T15:31:42Z

contracts/src/core/libraries/v1/EigenDATypesV1.sol

+    struct NonSignerStakesAndSignature {
+        uint32[] nonSignerQuorumBitmapIndices;
+        BN254.G1Point[] nonSignerPubkeys;
+        BN254.G1Point[] quorumApks;
+        BN254.G2Point apkG2;
+        BN254.G1Point sigma;
+        uint32[] quorumApkIndices;
+        uint32[] totalStakeIndices;
+        uint32[][] nonSignerStakeIndices;
+    }
+
+    struct QuorumStakeTotals {
+        uint96[] signedStakeForQuorum;
+        uint96[] totalStakeForQuorum;
+    }
+
+    struct CheckSignaturesIndices {
+        uint32[] nonSignerQuorumBitmapIndices;
+        uint32[] quorumApkIndices;
+        uint32[] totalStakeIndices;
+        uint32[][] nonSignerStakeIndices;
+    }


are these really v1 types? See my other comment

My intention in this organizational structure is not to specify that certain types only belong to V1 and which belong to V2. It is to create a logical history of when types are introduced, and make it easier in future PRs to add namespaced types in case we ever need to modify structs, since the current situation is very confusing.

This problem is evident when you look at the fact that previously, in IEigenDAStructs, you had structs named "BlobHeader" and "BlobHeaderV2" all in a common file. I want to avoid that situation.

That's an interesting, but idiosyncratic way of versioning things. I personally prefer namespaces to be self-contained. See #1516 (comment)

samlaf

Sorry forgot to get back to this PR! I'll approve because we need to get things unblocked and PR stalemates are bad. But I still find the namespacing based on historical introduction to be weird. I've never seen this used, so it would require lots of documentation in many places to get people used to this architecture.

Also as I've noted, once we delete V1 then there will be a big impact and artificialy-induced refactor of V2 needed which shouldn't be the case. If you disagree I'm happy to merge now and keep discussing though. Refactors can always be done later.

pakim249CAL · 2025-05-06T11:56:42Z

Sorry forgot to get back to this PR! I'll approve because we need to get things unblocked and PR stalemates are bad. But I still find the namespacing based on historical introduction to be weird. I've never seen this used, so it would require lots of documentation in many places to get people used to this architecture.

Also as I've noted, once we delete V1 then there will be a big impact and artificialy-induced refactor of V2 needed which shouldn't be the case. If you disagree I'm happy to merge now and keep discussing though. Refactors can always be done later.

I will concede that it's very weird. The design arises from my personal need to separate them somehow though, because having them all in one file ended up creating a tangled mess in other files, where you could very easily be confused by type names that are introduced over time.

I think I need to come up with a better name or something, but I maintain that this kind of separation is useful when writing the actual contracts so that developers can understand more easily what is only needed for V1 interactions vs V2 interactions, and I think duplicating types leads to very confusing scenarios

Ultimately these types are schematics for many upgradeable contracts all at once, so I think it is important to structure them in a way that they can be kept around forever. In a sense, I am basically adding "epochs" that divide up when certain features were introduced. So versioning is probably a pretty bad name for this. But I think it is the least worst name.

* Refactor: Consolidate struct types into versioned libraries fix: remove operator state retriever change fix: readd v1 test * fix: fmt * fix: ci * fix: ci * fix: remove certverifier binding * fix: ci * fix: ci * feat: new file structure feat: remove old files refactor: file layout * fix: ci * fix: ci * fix: bindings * chore: address comments

pakim249CAL force-pushed the refactor/version-contract-types branch from ef44028 to 74387b2 Compare April 30, 2025 22:03

pakim249CAL changed the base branch from refactor/remove-operator-state-retriever to master April 30, 2025 22:03

pakim249CAL changed the title ~~Refactor/version contract types~~ refactor: version contract types Apr 30, 2025

pakim249CAL marked this pull request as ready for review April 30, 2025 22:27

pakim249CAL marked this pull request as draft April 30, 2025 22:28

Refactor: Consolidate struct types into versioned libraries

d93e21e

fix: remove operator state retriever change fix: readd v1 test

pakim249CAL force-pushed the refactor/version-contract-types branch from 9b31539 to d93e21e Compare April 30, 2025 22:56

pakim249CAL added 6 commits April 30, 2025 23:07

fix: fmt

7ce68c3

fix: ci

7757b99

fix: ci

b3fe8c1

fix: remove certverifier binding

52d12dc

fix: ci

0985b47

fix: ci

5be5313

samlaf reviewed May 1, 2025

View reviewed changes

contracts/src/libraries/V1/EigenDATypesV1.sol Outdated Show resolved Hide resolved

contracts/src/libraries/V2/EigenDATypesV2.sol Outdated Show resolved Hide resolved

samlaf mentioned this pull request May 1, 2025

feat: cert verifier v3 + router #1518

Merged

5 tasks

pakim249CAL added 3 commits May 1, 2025 20:50

feat: new file structure

2957935

feat: remove old files refactor: file layout

fix: ci

f451cb7

fix: ci

0dd14a2

pakim249CAL marked this pull request as ready for review May 2, 2025 02:19

pakim249CAL requested review from samlaf, mooselumph, ethenotethan, hopeyen, bxue-l2, ian-shim, litt3, anupsv and a team May 2, 2025 02:19

pakim249CAL changed the title ~~refactor: version contract types~~ refactor!: version contract types May 2, 2025

Merge remote-tracking branch 'origin/master' into refactor/version-co…

b964301

…ntract-types

fix: bindings

f597d07

litt3 reviewed May 2, 2025

View reviewed changes

samlaf reviewed May 2, 2025

View reviewed changes

chore: address comments

c1b9fdb

pakim249CAL requested review from samlaf and litt3 May 5, 2025 11:53

litt3 approved these changes May 5, 2025

View reviewed changes

samlaf approved these changes May 6, 2025

View reviewed changes

pakim249CAL merged commit 2a68458 into master May 6, 2025
15 checks passed

pakim249CAL deleted the refactor/version-contract-types branch May 6, 2025 19:36

refactor!: version contract types #1516

refactor!: version contract types #1516

Uh oh!

Conversation

pakim249CAL commented Apr 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why are these changes needed?

Checks

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

litt3 May 2, 2025

Choose a reason for hiding this comment

Uh oh!

pakim249CAL May 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

samlaf May 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pakim249CAL May 2, 2025

Choose a reason for hiding this comment

Uh oh!

samlaf May 6, 2025

Choose a reason for hiding this comment

Uh oh!

pakim249CAL May 6, 2025

Choose a reason for hiding this comment

Uh oh!

samlaf May 2, 2025

Choose a reason for hiding this comment

Uh oh!

pakim249CAL May 2, 2025

Choose a reason for hiding this comment

Uh oh!

samlaf May 6, 2025

Choose a reason for hiding this comment

Uh oh!

samlaf left a comment

Choose a reason for hiding this comment

Uh oh!

pakim249CAL commented May 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pakim249CAL commented Apr 30, 2025 •

edited

Loading

pakim249CAL May 2, 2025 •

edited

Loading

samlaf May 2, 2025 •

edited

Loading

pakim249CAL commented May 6, 2025 •

edited

Loading