celestiaorg / nmt

Namespaced Merkle Tree
Apache License 2.0
112 stars 39 forks source link

doc: introduces short namespace absence proof in the NMT specs #209

Closed staheri14 closed 1 year ago

staheri14 commented 1 year ago

Overview

Closes #205 Additionally, in line with this, to ensure consistency with how namespace ID and namespace (i.e., a combination of namespace ID and version) are utilized in the core-app, I have replaced all instances of words that refer to or imply namespace ID with the more accurate term "namespace".

Checklist

codecov[bot] commented 1 year ago

Codecov Report

Merging #209 (5946b91) into master (cdc88e8) will not change coverage. The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #209   +/-   ##
=======================================
  Coverage   94.03%   94.03%           
=======================================
  Files           5        5           
  Lines         570      570           
=======================================
  Hits          536      536           
  Misses         19       19           
  Partials       15       15           
rootulp commented 1 year ago

Closes https://github.com/celestiaorg/nmt/issues/206

staheri14 commented 1 year ago

Overall LGTM, thanks for addressing #206

I think we may want to also rename

https://github.com/celestiaorg/nmt/blob/cdc88e891d11878dc8b63447e01706d746791b14/namespace/id.go#L8

but that could be a follow up

Sure, thanks for mentioning it, I will take care of the code change as well, however, as this will constitute a breaking change, it deserves a separate PR.

cmwaters commented 1 year ago

Overall LGTM, thanks for addressing #206

I think we may want to also rename

https://github.com/celestiaorg/nmt/blob/cdc88e891d11878dc8b63447e01706d746791b14/namespace/id.go#L8

but that could be a follow up

If we do, I don't think it needs to remain in its own package. It should just be nmt.Namespace

staheri14 commented 1 year ago

Overall LGTM, thanks for addressing #206 I think we may want to also rename https://github.com/celestiaorg/nmt/blob/cdc88e891d11878dc8b63447e01706d746791b14/namespace/id.go#L8

but that could be a follow up

If we do, I don't think it needs to remain in its own package. It should just be nmt.Namespace

Thanks for your comment, I noted this in its respective issue.

staheri14 commented 1 year ago

Going to merge this PR, but will be monitoring this PR for addressing your questions and comments.