Refactoring: Push(namespace.PrefixedData)->Push(namespace.ID, []byte) #56
Conversation
Codecov Report
@@ Coverage Diff @@
## master #56 +/- ##
==========================================
- Coverage 95.56% 95.54% -0.03%
==========================================
Files 7 6 -1
Lines 361 359 -2
==========================================
- Hits 345 343 -2
Misses 11 11
Partials 5 5
Continue to review full report at Codecov.
|
|
Note that due to the copy: https://github.com/celestiaorg/nmt/pull/56/checks?check_run_id=6312183477 But also we do this copy in the wrapper now which can be removed. So it should be the same net alloc. |
There was a problem hiding this comment.
LGTM 👍
everytime I come to this repo, I'm always impressed with the test coverage and benchmarks etc. @liamsi 's work a long time ago is still paying dividends
also, one day we will stop changing the API for Push lolol
Yeah, that is what I thought as well but after we chatted about it it seemed not necessary and I didn't think about it further. Can you open an issue with the proposed method signature? I'll do a quick follow-up PR in the next few days. |
|
@Wondertan or should we hold off merging this as it is kinda useless without also changing the visitor? Line 22 in c1fbf91 and it's used here: Line 295 in c1fbf91 and here: Line 302 in c1fbf91 Am I understanding you correctly that you basically need it instead to be n.visit(leafHash, n.leaves[start][:NamespaceLen], n.leaves[start][NamespaceLen:])and n.visit(hash, left[:NamespaceLen], left[NamespaceLen:], right[:NamespaceLen], right[NamespaceLen:])(not exactly like the above but sth along these lines?) |
|
actually closing this until we figure out how the Visitor API should be changed or if we want to get rid of it etc |
|
I'll leave the branch around for a while though. |
for details see: #55 (particularly: #55 (comment))
closes: #55