feat: Cert encoding byte #370

ethenotethan · 2025-04-11T20:58:49Z

Fixes Issue

Fixes #363

Changes proposed

Added EncodingType enum with RLPEncoding (0) and ABIVerifyV2CertEncoding (1)
Introduced CertV2 format that includes an encoding byte in DA commitment
Added encoding/decoding logic to extract and set encoding type
Updated standard client to support encoding type specification
Modified server handlers to process encoding type parameters. Targeting the routing regex is unviable given necessity to support backwards compatibility with older DA commitment versions that don't use an encoding byte prefix. Instead the prefix processing had to happen in handlers.
Extended commitment metadata to track encoding information
Passes encoding commitment context through golang context to avoid breaking existing interfaces. Not the prettiest but neither is refactoring all storage interfaces in the short-term!

Screenshots (Optional)

Note to reviewers

- Remove commented out parseEncodingByte function - Remove commented out routingVarNameEncodingByteHex references - Fix and reenable test for unsupported version byte - Fix and reenable get_data_edge_cases test

…igenda-proxy into epociask--feat-encoding-byte

clients/standard_client/client.go

litt3 · 2025-04-15T13:44:19Z

commitments/eigenda.go

+	}
+
+	// For V0/V1 certificates (backward compatibility)
 	return append([]byte{byte(c.prefix)}, c.b...)


Perhaps we should return an error if encoding is set, but the cert version doesn't support specifying an encoding? Otherwise this will just quietly ignore the specified user preference for a different encoding.

litt3 · 2025-04-15T13:45:32Z

commitments/mode.go

 )

+// EncodingType represents the serialization format for the certificate
+type EncodingType byte


Rather than having two enums for encoding type (one with a string, and one with a byte), living in different locations, I think there should be a single enum type, with supporting methods to return either a byte or a string representation.

the issue with having an imported definition in the standard client package is that proxy's core dependencies would also be imported which isn't possible given how we've provisioned the go workspace

don't think that's true, see #376

commitments/mode.go

litt3 · 2025-04-15T14:36:40Z

server/handlers.go

 }

 // handlePostOPGenericCommitment handles the POST request for optimism generic commitments.
 func (svr *Server) handlePostOPGenericCommitment(w http.ResponseWriter, r *http.Request) error {


this method is identical to the standard commitment version, apart from the contained mode. I think a common utility method should be extracted, which accepts a mode input parameter

This comment wasn't addressed in this commit. handlePostOPGenericCommitment and handlePostStdCommitment are still identical, apart from CommitmentMeta.Mode, which could be passed in as a parameter

wdym - any common logic here has been extracted into parseEncodingQueryParamType. Outside of this there's only redundancy or code overlap when setting a V2 cert commitment version

server/handlers.go

litt3 · 2025-04-15T14:41:49Z

server/server.go

 	}
 	switch versionByte[0] {
-	case byte(commitments.CertV0), byte(commitments.CertV1):
+	case byte(commitments.CertV0), byte(commitments.CertV1), byte(commitments.CertV2):


A switch is a bit of an awkward way to accomplish this, particularly as more cert types are added.

Consider creating a new method isValidCertVersion. Or alternatively, perhaps a method byteToCertVersion, which includes an error return value, would be useful here and elsewhere.

store/generated_key/memstore/v2/memstore.go

litt3 · 2025-04-15T14:46:27Z

store/generated_key/memstore/v2/memstore.go

 // this is done to verify that a rollup must be able to provide
 // the same certificate used in dispersal for retrieval
-func (e *MemStore) Put(_ context.Context, value []byte) ([]byte, error) {
+func (e *MemStore) Put(ctx context.Context, value []byte) ([]byte, error) {


I'm not a fan of how context injection is being used for this PR.

IMO we should bite the bullet and just modify the interface methods to accept an encoding parameter. It's a more fundamental change, but the current strategy doesn't seem very maintainable to me.

would prefer that this be a follow up change or live as a separate PR given the large code overhead required

Let's continue this conversation on this thread, since they are getting at the same thing

samlaf

Approach LGTM. Left a bunch of nits on the specifics

README.md

samlaf · 2025-04-15T15:07:38Z

README.md

+
+For certificates with version byte 0x00 or 0x01, no encoding byte is included and RLP encoding is assumed. For version 0x02 and higher, an encoding byte is included that specifies the serialization format:
+- 0x00: RLP encoding (default)
+* `0x01`: `EigenDACertVerifier` *verifyDACertV2* ABI encoding (for smart contract verification)


do we want to wait until we've done the struct refactoring for verifyDACertV2? Then we can just say that its the ABI encoding of the solidity cert struct, which feels more natural than pointing to a function's arguments.

yes! and fully agree

samlaf · 2025-04-15T15:09:33Z

clients/standard_client/client.go

 )

+// EncodingType represents the serialization format for the certificate
+type EncodingType string


maybe CertEncoding or CertSerialization instead?

Also not a fan of defining fundamental types in the client. Could we define these in some proxy package and import it in the client instead?

how would you do that in a way that doesn't introduce proxy's dependency graph to the client?

You import a package that doesn't have many transitive dependencies. For example I tried adding a main.go in clients that only imports common/consts:

package clients import ( "fmt" "github.com/Layr-Labs/eigenda-proxy/common/consts" ) func main() { fmt.Println(consts.EthHappyPathFinalizationDepthBlocks) }

I don't fully understand it but I think golang does lazy module loading, and only based on PACKAGES that a build requires. The difference between package and module is important here. See https://go.dev/ref/mod#go-mod-file-go:

The go.mod file includes an explicit require directive for each module that provides any package transitively imported by a package or test in the main module.

gotcha but wouldn't there by be a cyclic issue here? https://github.com/Layr-Labs/eigenda-proxy/blob/main/go.mod#L30-L31

server module only imports clients for testing right? I think that's allowed...

see #376 for an example

samlaf · 2025-04-15T15:10:28Z

clients/standard_client/client.go

 type Config struct {
-	URL string // EigenDA proxy REST API URL
+	URL         string       // EigenDA proxy REST API URL
+	EncodingType EncodingType // Certificate encoding type (optional)


if optional, then describe what the zero (empty string) value defaults to

commitments/eigenda.go

samlaf · 2025-04-15T15:28:54Z

store/manager.go

 			m.log.Warn("Failed to read from cache targets", "err", err)
 		}

+		ctx = context.WithValue(ctx, common.EncodingCtxKey, cm.Encoding)


why use context key/value dict everywhere instead of passing as a proper (typed!) argument?

this is a workaround to conform with existing interface constraints

Why do we need to conform to existing interfaces?

The current context-based solution is orthogonal to the correct parameter-based solution, IMO.

I don't think we should introduce the tech debt of context based parameters for the sake of maintaining these internal interfaces.

common/serialization.go

server/handlers.go

samlaf · 2025-04-15T15:37:53Z

server/routing.go

 	subrouterGET.HandleFunc("/"+
 		"{optional_prefix:(?:0x)?}"+ // commitments can be prefixed with 0x
-		"{"+routingVarNameVersionByteHex+":[0-9a-fA-F]{2}}"+ // should always be 0x00 for now but we let others through to return a 404
+		"{"+routingVarNameVersionByteHex+":[0-9a-fA-F]{2}}"+


shouldn't we add the encodingByte regexp here as well?

how would you add that regex where you wouldn't also be unnecessarily truncating commitment bytes from an older version commitment that doesn't have the encoding prefix?

Oh fuck right... now we're stuck with some context-dependent encoding. :(
Maybe one way is to have 2 separate routes? Something like:

// Route for version bytes that require encoding byte router.HandleFunc("/"+ "{optional_prefix:(?:0x)?}"+ "{version:[0-9a-fA-F]{2}}"+ "{encoding:[0-9a-fA-F]{2}}"+ // Only for versions that need encoding "{commitment:[0-9a-fA-F]+}", handler).Methods("GET") // Route for version bytes that don't require encoding byte router.HandleFunc("/"+ "{optional_prefix:(?:0x)?}"+ "{version:[0-9a-fA-F]{2}}"+ // For older versions "{commitment:[0-9a-fA-F]+}", handler).Methods("GET")

Idk if this is any simpler overall, but I really don't like having implicit header parsing inside the handlers. If we do go this route though please add a comment in the router regexps explaining that we will further parse the routingVarNamePayloadHex to extract the encoding byte when versionByte >= 2 inside the handler.

anupsv · 2025-04-16T20:59:27Z

server/handlers.go

 		return fmt.Errorf("failed to decode commitment %s: %w", rawCommitmentHex, err)
 	}

+	if versionByte >= byte(commitments.CertV2) {


wondering why >= instead of of just == ?

He;s just saying that every future cert version is required to use the encoding byte. We should have enforced the encoding byte header from the start for everything, but now we're stuck with cert versions v0 and v1 not using an encoding byte.

I see. I guess when there is V3, we'll coalesce the common functions.

…igenda-proxy into epociask--feat-encoding-byte

samlaf · 2025-04-23T05:43:30Z

commitments/eigenda.go

 	prefix   EigenDACommitmentType
 	b        []byte
-	encoding *EncodingType // Optional encoding type, used only for V2+
+	encoding EncodingType // Optional encoding type, used only for V2+


Suggested change

encoding EncodingType // Optional encoding type, used only for V2+

encoding EncodingType // encoding type, used only for V2+

not optional anymore right?

also is it really only used for v2? doesnt ssem for v2 only in meta struct

// encoding type for the certificate (defaults to RLPEncoding)
Encoding EncodingType

samlaf · 2025-04-23T05:52:55Z

server/handlers.go

+
+	// if encoding param provided but historical V1 backend used then error
+	if svr.sm.GetDispersalBackend() == common.V1EigenDABackend {
+		http.Error(w, "Encoding type query parameter cannot be specified for V1 backend", http.StatusBadRequest)


dont you need to return here?

samlaf · 2025-04-23T05:54:06Z

server/handlers.go

-	}
-
+// handleGetEigenDA helper function used for processing gets for an eigenda backend type
+func (svr *Server) handleGetEigenDA(w http.ResponseWriter, r *http.Request,


dont think this should be called handle because its not a routing table entrypoint

or maybe prefix name with a _?

This is just an example for Ethen regarding discussions in his PR #370

chore: type alias proxy encoding enum in client This is just an example for Ethen regarding discussions in his PR #370

ethenotethan added 7 commits April 11, 2025 15:56

feat: Cert encoding byte

72832f7

Merge branch 'main' into epociask--feat-encoding-byte

487a64c

chore: Remove dead code and fix tests

0dfd293

- Remove commented out parseEncodingByte function - Remove commented out routingVarNameEncodingByteHex references - Fix and reenable test for unsupported version byte - Fix and reenable get_data_edge_cases test

Merge branch 'epociask--feat-encoding-byte' of github.com:Layr-Labs/e…

073e7ef

…igenda-proxy into epociask--feat-encoding-byte

feat: Cert encoding byte - fix lint

dd42630

feat: Cert encoding byte - update readme

9be5527

feat: Cert encoding byte - rm printlns

f09eb36

ethenotethan requested review from anupsv, litt3 and samlaf April 11, 2025 21:29

feat: Cert encoding byte - reference cert

4a081dd

litt3 reviewed Apr 15, 2025

View reviewed changes

samlaf reviewed Apr 15, 2025

View reviewed changes

anupsv reviewed Apr 16, 2025

View reviewed changes

ethenotethan added 6 commits April 18, 2025 21:17

feat: Cert encoding byte - address PR feedback

04dba8c

Merge branch 'main' into epociask--feat-encoding-byte

2dee986

feat: Cert encoding byte - fix lint

cff9a96

feat: Cert encoding byte - fix lint

8fa7fc9

Merge branch 'epociask--feat-encoding-byte' of github.com:Layr-Labs/e…

95dcbfc

…igenda-proxy into epociask--feat-encoding-byte

feat: Cert encoding byte - ignore context in memstore get function

03b6402

samlaf reviewed Apr 23, 2025

View reviewed changes

samlaf added a commit that referenced this pull request Apr 23, 2025

chore: type alias proxy encoding enum in client

2d5d9a7

This is just an example for Ethen regarding discussions in his PR #370

samlaf mentioned this pull request Apr 23, 2025

example: encoding byte type alias proxy client #376

Merged

example: encoding byte type alias proxy client (#376)

1ca439f

chore: type alias proxy encoding enum in client This is just an example for Ethen regarding discussions in his PR #370

	encoding EncodingType // Optional encoding type, used only for V2+
	encoding EncodingType // encoding type, used only for V2+

feat: Cert encoding byte #370

Are you sure you want to change the base?

feat: Cert encoding byte #370

Uh oh!

Conversation

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

Uh oh!

Fixes Issue

Changes proposed

Screenshots (Optional)

Note to reviewers

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

litt3 Apr 23, 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samlaf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

ethenotethan commented Apr 11, 2025 •

edited

Loading

litt3 Apr 23, 2025 •

edited

Loading