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.

Sign up for GitHub

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

Jump to bottom

Rename nID to namespace where applicable #2301

Closed
rootulp opened this issue Jun 1, 2023 · 5 comments · Fixed by #2388
Closed

Rename nID to namespace where applicable #2301

rootulp opened this issue Jun 1, 2023 · 5 comments · Fixed by #2388
Assignees
@Wondertan
Labels
enhancement New feature or request external Issues created by non node team members

Comments

@rootulp
Copy link
Contributor

rootulp commented Jun 1, 2023

Context

celestia-node/share/get.go

Line 52 in c77f6db

nID namespace.ID,

Problem

In a few locations in this repo, nID: namespace.ID is used when I think a namespace is being passed (i.e. version + ID).

Proposal

  1. Rename nID to namespace or ns where applicable.
  2. Replace the usage of the NMT type namespace.ID with celestia-app type Namespace
@rootulp rootulp added the enhancement New feature or request label Jun 1, 2023
@github-actions github-actions bot added the external Issues created by non node team members label Jun 1, 2023
This was referenced Jun 1, 2023
Closed
Merged
@Wondertan
Copy link
Member

Re 2. We want to move on Namespace from the app once it becomes a single slice. IIRC, with aligned on that in sync. When and how can we do it? Node team can help there

@rootulp
Copy link
Contributor Author

rootulp commented Jun 7, 2023 •
edited
Loading

once it becomes a single slice

I don't think celestia-app can make this change in the short-term b/c it's breaking and we've already cut celestia-app v1.0.0-rcs. IIRC celestia-node doesn't want to adopt the existing celestia-app Namespace type because of performance reasons. If that's the case, celestia-node can define a new type Namespace: []byte in this repo and convert back and forth using From and Bytes.

@Wondertan Wondertan self-assigned this Jun 7, 2023
@Wondertan
Copy link
Member

I don't think celestia-app can make this change in the short-term b/c it's breaking and we've already cut celestia-app v1.0.0-rcs.

That's fair. The good thing is the change is not consensus-breaking, so we can still do it. Adding it to my list of things to take during share pkg extraction.

Meanwhile, cleaning up and defining the type in celestia-node makes total sense. Assigning this on me.

@Wondertan Wondertan mentioned this issue Jun 14, 2023
Merged
@Wondertan
Copy link
Member

@rootulp, thinking about this more, the namespace in NMT should be called just the namespace then as well because NMT commits to the version as well thus, it operates over namespaces rather than IDs. I am not sure anymore it is worth renaming in celestia-node only without nmt

@rootulp
Copy link
Contributor Author

rootulp commented Jun 15, 2023

+1 to renaming in NMT. Created celestiaorg/nmt#206

@Wondertan Wondertan mentioned this issue Jun 16, 2023
Closed
3 tasks
github-merge-queue bot pushed a commit that referenced this issue Jun 21, 2023
@Wondertan
share: introduce Namespace type (#2367)
9adb61e 
Mostly copied from the app's Namespace type with multiple modifications.
In preparation for
#2301.

We want a more straightforward Namespace type from what the app
provides:
* Repsents 1:1 mapping of the serialized form of Namespace [version byte
+ namespace id]
* So that it does not allocate on the hot path when getting data by
namespace
* and had simpler json serialization
@Wondertan Wondertan mentioned this issue Jun 22, 2023
Merged
github-merge-queue bot pushed a commit that referenced this issue Jun 27, 2023
@Wondertan
refactor!(share): integrate new Namespace type (#2388)
ccae150 
## Notable Changes
- `namespace.ID` to  `share.Namespace`
- Changes every comment namespace ID mentioning to just namespace. I
renamed every such mention besides in ADRs. I don't want to touch ADRs
here, as they need a more holistic re-review and up-to-date catchup.
	- Uses all the utility methods on the type, where suitable
- Namespace constructor now only creates Blob Namespaces. For other
reserved namespaces, the predefined globals should be used.
- Uses namespace.ValidateDataNamespace everywhere data is requested.
This is guarantees we verify the namespace are 100% valid and forbids
requesting parity and padding namespaces.
- Restricts PFBs for reserved namespaces 
- Reversed the dependency from `share -> share/ipld` to `share/ipld ->
share`
- NewBlobV0 constructor. Similar to NewNamespaceV0
- `sharetest` pkg for share related testing utilities
- `edstest` pkg for eds related testing utilities

## Follow-ups
- `blobtest` pkg to generate node's blob type


## Refs
* Substitutes zombie PR
#2376. I push to the
branch, but GH does not see commits.
* Based on #2367
* Closes #2301
* Closes #2309 
* Blocked on #2256
0semter0 pushed a commit to 0semter0/celestia-node that referenced this issue Aug 12, 2024
@Wondertan
share: introduce Namespace type (#2367)
e077a22 
Mostly copied from the app's Namespace type with multiple modifications.
In preparation for
celestiaorg/celestia-node#2301.

We want a more straightforward Namespace type from what the app
provides:
* Repsents 1:1 mapping of the serialized form of Namespace [version byte
+ namespace id]
* So that it does not allocate on the hot path when getting data by
namespace
* and had simpler json serialization
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Wondertan Wondertan

Labels
enhancement New feature or request external Issues created by non node team members
Projects
None yet
Milestone
No milestone
2 participants
@rootulp @Wondertan