-
Notifications
You must be signed in to change notification settings - Fork 43
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.
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@liamsi, One mid-level thing. We shouldn't pass the visitor the result of the append of nid+data and data only, or we should pass both as separate params as well.
Or just rework Visitor API completely, as we discussed 😅
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