Style: improve api clients comments #780

samlaf · 2024-10-02T19:22:14Z

Small improvements to comments/docs for api clients.
Had some fun in the plane while reading the code.
Added some TODOs in a few places which might be worth checking through, perhaps we can solve some of them right away @bxue-l2

api/clients/codecs/blob_codec.go

samlaf · 2024-10-02T19:23:07Z

api/clients/eigenda_client.go

+	// TODO: all of these should be private, to prevent users from using them directly,
+	// which breaks encapsulation and makes it hard for us to do refactors or changes


this I personally think is a refactor worth doing... but it might break some client code... :\

agree this should be private. From my experience, it is difficult to understand data semantics in each stage or encoding and decoding.

Don't understand your comment, why are you mentioning data semantics here?
And are you saying I should change the fields to private right now? Are we not scared of breaking user code that might be using these?

just from the developers' view, it tool me a while to understand the intermediate phases. If someone else would use it, I doubted it would be correct

Note: we decided to do all the breaking changes in a future PR.

api/clients/eigenda_client.go

jianoaix · 2024-10-16T22:03:16Z

api/proto/disperser/disperser.proto

 	// where the lower address has more significant bits. The integer must stay in the valid range to be interpreted
 	// as a field element on the bn254 curve. The valid range is 
 	// 0 <= x < 21888242871839275222246405745257275088548364400416034343698204186575808495617
-	// containing slightly less than 254 bits and more than 253 bits. If any one of the 32 bytes chunk is outside the range, 
-	// the whole request is deemed as invalid, and rejected. 
+	// which is a 254 bit number (meaning the first 2 bits of each chunk must always be 00).


This isn't accurate though, 21888242871839275222246405745257275088548364400416034343698204186575808495617 is smaller than 2^254-1 (i.e. the original comment is accurate that it doesn't have 254 bits).

Also shall we not use chunk here since it's easy to confuse with the encoding chunks (which will become more user visible in the future)?

Took me a while to figure this one out haha. Doesn't matter that it's < 2^254-1, what matters is that it's > 2^253-1, and so actually needs 254 bits.

The number is 254 bits according to https://hackmd.io/@jpw/bn254 (which also explains why its called bn254 and not bn253). Here's also a python script showing that it's a 254 bit number:

number = 21888242871839275222246405745257275088548364400416034343698204186575808495617 binary_representation = bin(number) # Count the number of bits (excluding the '0b' prefix) bit_count = len(binary_representation) - 2 print(f"The number is: {number}") print(f"Binary representation: {binary_representation}") print(f"Number of bits: {bit_count}")

What's a better word that you would recommend instead of chunk? @bxue-l2 maybe you have an idea since you initially wrote this comment I believe?

EDIT: based on bowen's comment below, I think I understand what you guys are saying now. Still think my comment is more accurate and the original one weird/wrong (a number cannot have more than 253 bits and less than 254 bits, it has a fixed number of bits, and could happen to have 254 bits).
Updated the comment order to reflect you guys' confusion, hopefully this helps: 6b78c52

(meaning the first 2 bits of each chunk must always be 00)

Pedantically, this is not true. Disperser will happily encode & disperse a blob that does not prefix chunks with 00 as long as all 32 byte chunks are in range. The 00 chunk prefix is just the easiest way to achieve lossless chunk encode/decode at the expense of 1 byte per chunk.

@pschork maybe wording is getting everyone confused? By "chunk" in here we are talking about 32byte field elements. If those are not prefixed by 00, then they are surely a number > the max allowed FE, and so therefore wouldn't be in range, right?

When encoding, if your encoding chunk size is 32 bytes and the current chunk is within range, you don't need to pad it, but if given chunk exceeds the range then you need to truncate the chunk and pad it with zero to fit within range. The problem is when you do this, you won't be able to decode the encoded blob because you wont know which chunks were or were not padded (aka lossy encoding). For lossless encoding you set the encoding chunk size to 31 bytes and reserve the first 1 byte with \x00 so that, when decoding, you can identify the start of a chunk.

https://github.com/Layr-Labs/eigenda/blob/master/encoding/utils/codec/codec.go

please avoid using chunk here, it'll quickly get overloaded and confusing.

I think we don't need to mention it's prefixed with 00 (it's not precise anyway), we just need to mention each 32 bytes should fall in that value range so it's a valid field element.

795868e
I removed mentions of chunk, and also removed the 00 part because it was confusing. (@pschork I just realized you interpreted my 00 as \x00, aka a full byte, but my message was using 00 meaning 2 bits)

api/clients/eigenda_client.go

api/clients/config.go

api/proto/disperser/disperser.proto

api/clients/codecs/blob_codec.go

api/clients/config.go

bxue-l2 · 2024-10-17T20:02:14Z

api/clients/config.go

-	// The total amount of time that the client will spend waiting for EigenDA to confirm a blob
+	// Timeout used when making dispersals to the EigenDA Disperser
+	// TODO: we should change this param as its name is quite confusing
+	ResponseTimeout time.Duration


agree, how about DispersalTimeout?

I'm fine with doing that, but as I mentioned in the other comment #780 (comment), do we have a policy for these breaking changes? Or are we fine with just doing it now?

Note: we decided to do all the breaking changes in a future PR.

api/clients/disperser_client.go

bxue-l2 · 2024-10-17T20:09:09Z

api/clients/eigenda_client.go

+	// TODO: all of these should be private, to prevent users from using them directly,
+	// which breaks encapsulation and makes it hard for us to do refactors or changes


agree this should be private. From my experience, it is difficult to understand data semantics in each stage or encoding and decoding.

api/clients/eigenda_client.go

dismissing because there's a lot of good comments that I need to address before we merge this.

jianoaix · 2024-10-18T16:10:34Z

api/proto/disperser/disperser.proto

@@ -78,12 +78,12 @@ message AuthenticationData {

 message DisperseBlobRequest {


The other copy: https://github.com/Layr-Labs/eigenda/blob/master/api/proto/disperser/v2/disperser_v2.proto#L25

api/clients/eigenda_client.go

method is now no longer used anymore, and could be deprecated

…stead of [0]

…orumID eventually

only need tracing-id key now since v2 will do idempotency using blobHeader Also realized using same key for idempotency and tracing doesn't make sense, since one might want different requests to be traced idependently but use the same idempotency key

bxue-l2

lg

samlaf requested a review from bxue-l2 October 2, 2024 19:22

samlaf commented Oct 2, 2024

View reviewed changes

api/clients/codecs/blob_codec.go Outdated Show resolved Hide resolved

samlaf commented Oct 2, 2024

View reviewed changes

api/clients/eigenda_client.go Outdated Show resolved Hide resolved

samlaf requested a review from jianoaix October 16, 2024 21:25

jianoaix reviewed Oct 16, 2024

View reviewed changes

bxue-l2 previously approved these changes Oct 17, 2024

View reviewed changes

samlaf requested review from jianoaix and bxue-l2 October 18, 2024 12:07

jianoaix reviewed Oct 18, 2024

View reviewed changes

samlaf added 10 commits October 19, 2024 18:47

style: improve api/clients comments

a4b7e40

refactor: remove eigendaClient GetCodec() dependency

005b4f1

method is now no longer used anymore, and could be deprecated

docs: add comment explaining why blobEncodingVersion stored in [1] in…

c3d77b8

…stead of [0]

style: updated TODO comment to change CustomQuorumIDs type to core.Qu…

6ad06b4

…orumID eventually

style: fix PutBlob's docstring comment wrt finalization status

908a1bf

style: made more accurate comment about field element value range

dc0f44c

chore: make protoc

b8ef081

docs: add comment explaining an assert statement

f3b0ea3

proto: make more precise DisperseBlobRequest.data comment

5b39fe6

samlaf force-pushed the style--improve-api-clients-comments branch from 795868e to 5b39fe6 Compare October 19, 2024 17:47

samlaf added 2 commits October 19, 2024 18:48

docs: update disperser_v2.proto comment to match disperser.proto

9d46d92

chore: change eigendaClient methods to pointer receiver

049235e

bxue-l2 approved these changes Oct 19, 2024

View reviewed changes

samlaf merged commit 78e4fc6 into master Oct 19, 2024
10 checks passed

samlaf deleted the style--improve-api-clients-comments branch October 19, 2024 18:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Style: improve api clients comments #780

Style: improve api clients comments #780

samlaf commented Oct 2, 2024

samlaf Oct 2, 2024

bxue-l2 Oct 17, 2024

samlaf Oct 18, 2024

bxue-l2 Oct 19, 2024

samlaf Oct 19, 2024

jianoaix Oct 16, 2024

samlaf Oct 18, 2024

samlaf Oct 18, 2024

samlaf Oct 18, 2024

pschork Oct 18, 2024 •

edited

Loading

samlaf Oct 18, 2024

pschork Oct 19, 2024 •

edited

Loading

pschork Oct 19, 2024

jianoaix Oct 19, 2024

samlaf Oct 19, 2024 •

edited

Loading

bxue-l2 Oct 17, 2024

samlaf Oct 18, 2024

samlaf Oct 19, 2024

bxue-l2 Oct 17, 2024

jianoaix Oct 18, 2024

samlaf Oct 19, 2024

bxue-l2 left a comment

		// TODO: all of these should be private, to prevent users from using them directly,
		// which breaks encapsulation and makes it hard for us to do refactors or changes

		@@ -78,12 +78,12 @@ message AuthenticationData {

		message DisperseBlobRequest {

Style: improve api clients comments #780

Style: improve api clients comments #780

Conversation

samlaf commented Oct 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pschork Oct 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pschork Oct 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samlaf Oct 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bxue-l2 left a comment

Choose a reason for hiding this comment

pschork Oct 18, 2024 •

edited

Loading

pschork Oct 19, 2024 •

edited

Loading

samlaf Oct 19, 2024 •

edited

Loading