celestiaorg / nmt

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

Add HexString method for namespace id #201

Closed walldiss closed 1 year ago

walldiss commented 1 year ago

Overview

Implements https://github.com/celestiaorg/nmt/issues/200

codecov[bot] commented 1 year ago

Codecov Report

Merging #201 (54ce402) into master (ba0ebb8) will increase coverage by 0.00%. The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #201   +/-   ##
=======================================
  Coverage   95.39%   95.40%           
=======================================
  Files           5        5           
  Lines         565      566    +1     
=======================================
+ Hits          539      540    +1     
  Misses         15       15           
  Partials       11       11           
Impacted Files Coverage Δ
namespace/id.go 100.00% <100.00%> (ø)
rootulp commented 1 year ago

This namespace does include the namespace version so I think we can include this change here.

Wondertan commented 1 year ago

Any reason not to change the default behavior of String to be always hex encoded? The current String is sorta useless while having default human-readable support for Stringer everywhere is not, AFAICS. The current String implementation can still be done with string(ID) if anyone needs raw nid in string format.

@evan-forbes, @rootulp, thoughts?

rootulp commented 1 year ago

My rationale for recommending a new HexString() method is to avoid breaking compatability

changing existing String() method (would break compatibility)

If we can verify that no consumers of this library rely on the existing string representation then I think we can make the breaking change and modify the existing String() method

staheri14 commented 1 year ago

@walldiss Could you please revise the title of the PR to capitalize HexString? otherwise, it might be misleading, suggesting that the introduced method is private.

Wondertan commented 1 year ago

If we can verify that no consumers of this library rely on the existing string representation then I think we can make the breaking change and modify the existing String() method

I verified the usage in the node and it's used only in places where human-readable string is expected.

evan-forbes commented 1 year ago

I'm fine with mergin either, but I tend to agree that returning hex makes a lot of sense. if we need a string() for some reason, we can just call string(id)

rootulp commented 1 year ago

Merging b/c @walldiss doesn't have permission to.