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

docs(cip-19): add NamespaceRangeID along with NamespaceRangeContainer #197

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

vgonkivs
Copy link
Member

Overview

Added NamespaceRangeID that forms a request to get a range of namespace shares along with their inclusion proofs. The shares MAY span across multiple rows in the DataSquare.

cips/cip-19.md Outdated
@@ -311,6 +311,49 @@ Namespace data may span over multiple rows, in which case all the data is encaps
containers. This enables parallelization of namespace data retrieval and certain [compositions](#protocol-compositions)
may get advantage of that by requesting containers of a single namespace from multiple servers simultaneously.

#### NamespaceRangeID
Copy link
Contributor

Choose a reason for hiding this comment

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

namespace range may suggest it's a range of namespaces, like 0x00000001 up to 0x00000004. Maybe just NamespaceData would be better? just dropping Row prefix from RowNamespaceData, also better showing correlation between the two?

Copy link
Member

@Wondertan Wondertan Aug 28, 2024

Choose a reason for hiding this comment

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

We have:
RowNamespaceData - namespace data within a row
NamespaceData - all the namespace data that can span over multiple rows

To avoid namespace confusion I propose to do:
NamespaceRange -> RangeNamespaceData - namespace data range that can span over multiple rows

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree with @Wondertan. The container itself and request namings should contain Range


```text
NamespaceRangeID {
SampleID;
Copy link
Contributor

Choose a reason for hiding this comment

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

what is the purpose of sample here? does it mean take Length shares from given SampleID? I think it needs better clarification below. Also why not just using EdsId + Namespace providing whole namespace from given block? Is there a usecase for a user to request his shares but eg. skipping the first and last one?

Copy link
Member Author

Choose a reason for hiding this comment

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

does it mean take Length shares from given SampleID?

Yes

Copy link
Member Author

Choose a reason for hiding this comment

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

Also why not just using EdsId + Namespace providing whole namespace from given block? Is there a usecase for a user to request his shares but eg. skipping the first and last one?

This means we will retrieve the whole eds from the requested namespace. For example, blob.GetProof needs a specific range of shares rather than the whole namespace.

Copy link
Contributor

Choose a reason for hiding this comment

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

should we then specify here what should happen if cid is pointing to shares outside of it's namespace? eg. what if the sample id is not in correct namespace or the length goes beyond the namespace?

Copy link
Contributor

Choose a reason for hiding this comment

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

also I'm not exactly sure what is the usecase here, user would need to already know the blob's position in eds and it's length in shares to even use that, however shwap itself doesn't give any of such abilities

cips/cip-19.md Outdated Show resolved Hide resolved
SampleID;
Namespace;
Length: u16;
OmitData: bool;
Copy link
Contributor

Choose a reason for hiding this comment

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

should OmitData also be present in RowNamespaceId?

Copy link
Member

Choose a reason for hiding this comment

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

It may also be part of SampleID, i.e., one can request just the proof for the share/sample without the data itself. However, we are unaware of any use cases for the Row and Sample containers that can be requested without data. We could add that to be consistent with OmitData usage across types, but our approach is to specify and implement only what's known to be helpful in practice.

@jcstein
Copy link
Member

jcstein commented Aug 21, 2024

Will let @vgonkivs address comments from @zvolin and then give review 🫡

cips/cip-19.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@rootulp rootulp left a comment

Choose a reason for hiding this comment

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

LGTM after one last rename

cips/cip-19.md Outdated Show resolved Hide resolved
Copy link
Member

@jcstein jcstein left a comment

Choose a reason for hiding this comment

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

LGTM after @rootulp 's comment is committed, and after review from previous reviewers

Co-authored-by: Rootul P <[email protected]>
@vgonkivs vgonkivs marked this pull request as draft August 29, 2024 15:40
@vgonkivs
Copy link
Member Author

Moving this PR to draft until we have a working prototype to avoid any changes in the future.

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.

5 participants