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

docs: ADR 15 Namespace ID Size #1405

Merged
merged 47 commits into from
Mar 6, 2023
Merged

docs: ADR 15 Namespace ID Size #1405

merged 47 commits into from
Mar 6, 2023

Conversation

rootulp
Copy link
Collaborator

@rootulp rootulp commented Feb 17, 2023

ADR for #1308

@rootulp rootulp added the ADR item is directly relevant to writing or modifying an ADR label Feb 17, 2023
@rootulp rootulp self-assigned this Feb 17, 2023
@rootulp rootulp changed the title docs: ADR 14 Namespace ID Size docs: ADR 15 Namespace ID Size Feb 20, 2023
@rootulp rootulp mentioned this pull request Feb 20, 2023
Closed
3 tasks
@rootulp rootulp added this to the Mainnet milestone Feb 20, 2023
@rootulp rootulp marked this pull request as ready for review February 20, 2023 21:29
evan-forbes
evan-forbes previously approved these changes Feb 21, 2023
View reviewed changes
Copy link
Member

@evan-forbes evan-forbes left a comment

Choose a reason for hiding this comment

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

nice, makes sense to me! afaiu, nmt, celestia-app, and celestia-node should be able to have larger namespaces with only minor changes here and celestia-node, but I could be wrong.

docs/architecture/adr-015-namespace-id-size.md Outdated Show resolved Hide resolved
@rootulp @evan-forbes
Update docs/architecture/adr-015-namespace-id-size.md
4bae17b 
Co-authored-by: Evan Forbes <42654277+evan-forbes@users.noreply.github.com>
@MSevey MSevey requested a review from a team February 21, 2023 20:21
@musalbas
Copy link
Member

Should we not agree on the actual new namespace size before merging this or marking as ready for review? The decision section is empty.

@rootulp
Copy link
Collaborator Author

rootulp commented Feb 21, 2023

Should we not agree on the actual new namespace size before merging this or marking as ready for review?

I was hoping we could reach a decision via feedback on this PR. If you think we need a meeting to decide, I can set one up.

staheri14
staheri14 reviewed Feb 22, 2023
View reviewed changes
docs/architecture/adr-015-namespace-id-size.md Outdated Show resolved Hide resolved
docs/architecture/adr-015-namespace-id-size.md Outdated Show resolved Hide resolved
docs/architecture/adr-015-namespace-id-size.md Outdated Show resolved Hide resolved
docs/architecture/adr-015-namespace-id-size.md Outdated Show resolved Hide resolved
@MSevey MSevey requested a review from a team February 22, 2023 14:11
@MSevey MSevey requested a review from a team February 22, 2023 15:15
@rootulp rootulp requested a review from evan-forbes March 2, 2023 20:58
@MSevey MSevey requested a review from a team March 3, 2023 14:46
@rootulp rootulp requested a review from musalbas March 3, 2023 14:46
@rootulp rootulp mentioned this pull request Mar 3, 2023
Closed
evan-forbes
evan-forbes previously approved these changes Mar 3, 2023
View reviewed changes
@rootulp rootulp requested a review from liamsi March 3, 2023 20:30
@rootulp
Copy link
Collaborator Author

rootulp commented Mar 6, 2023

Will merge by EOD if there is no more feedback

staheri14
staheri14 previously approved these changes Mar 6, 2023
View reviewed changes
Copy link
Contributor

@staheri14 staheri14 left a comment

Choose a reason for hiding this comment

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

Nice! left some non-blocking comments.

docs/architecture/adr-015-namespace-id-size.md Outdated Show resolved Hide resolved
docs/architecture/adr-015-namespace-id-size.md Show resolved Hide resolved
docs/architecture/adr-015-namespace-id-size.md Outdated Show resolved Hide resolved
docs/architecture/adr-015-namespace-id-size.md Outdated Show resolved Hide resolved
docs/architecture/adr-015-namespace-id-size.md Outdated Show resolved Hide resolved
docs/architecture/adr-015-namespace-id-size.md Show resolved Hide resolved
docs/architecture/adr-015-namespace-id-size.md Show resolved Hide resolved
docs/architecture/adr-015-namespace-id-size.md Show resolved Hide resolved
docs/architecture/adr-015-namespace-id-size.md Show resolved Hide resolved
docs/architecture/adr-015-namespace-id-size.md Show resolved Hide resolved
@rootulp rootulp dismissed stale reviews from staheri14 and evan-forbes via cc14b43 March 6, 2023 19:33
@MSevey MSevey requested a review from a team March 6, 2023 19:33
rootulp and others added 10 commits March 6, 2023 14:37
@rootulp
namespaceA vs namespaceB
3cbd49e
@rootulp @staheri14
Update docs/architecture/adr-015-namespace-id-size.md
cdabb23 
Co-authored-by: Sanaz Taheri <35961250+staheri14@users.noreply.github.com>
@rootulp @staheri14
Update docs/architecture/adr-015-namespace-id-size.md
0cdb3ad 
Co-authored-by: Sanaz Taheri <35961250+staheri14@users.noreply.github.com>
@rootulp @staheri14
Update docs/architecture/adr-015-namespace-id-size.md
55047f5 
Co-authored-by: Sanaz Taheri <35961250+staheri14@users.noreply.github.com>
@rootulp @staheri14
Update docs/architecture/adr-015-namespace-id-size.md
e85427d 
Co-authored-by: Sanaz Taheri <35961250+staheri14@users.noreply.github.com>
@rootulp
state NMT num leaves
5e136ba
@rootulp
note on absence proof
f94c7f2
@rootulp @staheri14
Update docs/architecture/adr-015-namespace-id-size.md
5f58dd0 
Co-authored-by: Sanaz Taheri <35961250+staheri14@users.noreply.github.com>
@rootulp
new section for Maximum Blob Size
70fb130
@rootulp
revert ordered list suggestion
cc120d4
staheri14
staheri14 approved these changes Mar 6, 2023
View reviewed changes
Copy link
Contributor

@staheri14 staheri14 left a comment

Choose a reason for hiding this comment

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

Thanks for addressing the comments, LGTM!

@rootulp rootulp merged commit 514c8dd into celestiaorg:main Mar 6, 2023
@rootulp rootulp deleted the rp/adr-increase-namespace-id-size branch March 7, 2023 02:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rach-id rach-id rach-id left review comments

@staheri14 staheri14 staheri14 approved these changes

@evan-forbes evan-forbes evan-forbes approved these changes

@adlerjohn adlerjohn Awaiting requested review from adlerjohn

@MSevey MSevey Awaiting requested review from MSevey

@mojtaba-esk mojtaba-esk Awaiting requested review from mojtaba-esk

@musalbas musalbas Awaiting requested review from musalbas

@liamsi liamsi Awaiting requested review from liamsi liamsi is a code owner

Assignees

@rootulp rootulp

Labels
ADR item is directly relevant to writing or modifying an ADR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@rootulp @musalbas @liamsi @staheri14 @rach-id @evan-forbes