Refactor encoder (2 of N) #269
Conversation
| // Encode takes in a blob and returns the commitments and encoded chunks. The encoding will satisfy the property that | ||
| // for any number M such that M*params.ChunkLength > BlobCommitments.Length, then any set of M chunks will be sufficient to | ||
| // reconstruct the blob. | ||
| EncodeAndProve(data []byte, params EncodingParams) (BlobCommitments, []*Frame, error) |
There was a problem hiding this comment.
What do you think having an Encoder type (interface), and then Prover is a subtype of Encoder (is-a relationship, which is struct embedding in go)? The "XAndY()" method looks like it could be decomposed in general.
There was a problem hiding this comment.
I prefer both Prover and RSEncoder at the same level and under a new object called "Framer". Regardless of the name, the reason behind is both proving and encoding are separate, and totally parallelizable. For the time being, I think it is fine for now
There was a problem hiding this comment.
That sounds good to me as well. Then to be consistent, Decoder could also be a parallel concept and used under Framer?
There was a problem hiding this comment.
RSEncoder and Decoder are at the same level. "Framer" choose to include RSEncoder and Prover. The way that the current PR handles it is right to me. rs and kzgrs are at the same lelvel.
|
|
||
| // } | ||
|
|
||
| func roundUpDivide[T constraints.Integer](a, b T) T { |
There was a problem hiding this comment.
I saw multiple round-up division around the codebase, it can probably be moved to common and shared by all (sometime separately).
There was a problem hiding this comment.
Agreed. I'll try to get to this.
| @@ -156,6 +158,33 @@ func genRhsG1(samples []Sample, randomsFr []bls.Fr, m int, params rs.EncodingPar | |||
| return &rhsG1, nil | |||
| } | |||
|
|
|||
| // TODO(mooselumph): Cleanup this function | |||
There was a problem hiding this comment.
can you explain here, I can do it
|
|
||
| func encode(obj any) ([]byte, error) { | ||
| var buf bytes.Buffer | ||
| enc := gob.NewEncoder(&buf) |
There was a problem hiding this comment.
Does it have to be gob, does binary work? I can make a note for improvement in the future
| @@ -1,5 +1,5 @@ | |||
| definitions: | |||
| core.BlobCommitments: | |||
| encoding.BlobCommitments: | |||
| properties: | |||
| commitment: | |||
| $ref: '#/definitions/core.Commitment' | |||
There was a problem hiding this comment.
not familiar with swagger, but should the ref 3 lines below be changed too?
There was a problem hiding this comment.
It can be generated after fix the code:
eigenda/disperser/dataapi/server.go
Line 42 in 3c66c99
There was a problem hiding this comment.
@jianoaix, does anything need to be fixed here before this is merged?
There was a problem hiding this comment.
The core.Commitment is obsolete but it's since a previous PR. So sounds fine to fix that separately to get this merged now to avoid conflicts (give its size).
| @@ -117,9 +118,9 @@ func (c client) GetChunks( | |||
| return | |||
| } | |||
|
|
|||
| chunks := make([]*core.Chunk, len(reply.GetChunks())) | |||
| chunks := make([]*encoding.Frame, len(reply.GetChunks())) | |||
There was a problem hiding this comment.
since there are so many places where chunks are assigned to frames. Does it make sense to use encoding.Chunk? My only concerns against it is that chunk usually means data only. However, the data here contain both data and proof
There was a problem hiding this comment.
It might be a future cleanup action to make the variable names consistent.
| @@ -1,5 +1,5 @@ | |||
| definitions: | |||
| core.BlobCommitments: | |||
| encoding.BlobCommitments: | |||
| properties: | |||
| commitment: | |||
| $ref: '#/definitions/core.Commitment' | |||
Why are these changes needed?
Eliminates duplication in encoding types (removes core/encoding)
https://docs.google.com/document/d/1Z-XWWBxi_vCREpV1kbZj_PCb3ToM9_f0Ja3GMR0rNdU/edit
Checks