Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: add path generation for nmt subroots #621
feat: add path generation for nmt subroots #621
Changes from 3 commits
70babe7
41991de
adedcae
240cb03
fb0787f
5b24277
fb65861
65928ed
c5b4b90
b2db386
b607473
d2d645b
9cf9f54
32918de
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Shouldn't the path to the root be the empty set?
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.
good catch! the implementation changed as this was one of the bugs
9cf9f54
there's a test specifically for this now too
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.
Probably would be better to add to the docs that we expect
canClimbRight()
as a precondition.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.
I don't think being able to climb right is a technically a precondition? or perhaps I'm just not clear on what you mean by this? the implementation that uses this code only climbs if it can, but is that different?
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.
I don't understand this part. You can only climb in one direction, right? It is when you're going down that you have
right
orleft
path, no?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.
Like you mentioned, with climb terminology, one could think a child node climbs up to a parent node (i.e. only one direction). But another way of thinking about this method:
isLeftChild()
.Maybe this comment helps? https://github.com/celestiaorg/celestia-app/pull/621/files/d2d645b024464bcbcfd7114b42e9e154f77729cf#r948000052
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.
What
canClimbRight()
does is, for (depth=3, position=5), check if we can climb from 5 to 2. In this case,canClimbRight()
will return false.For (depth=3, position=6), it can climb right to 3.
I guess we need to explain it with an example in the docs.
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.
Shouldn't we also check if
depth != 0
?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.
you're right, we probably should just to avoid potential leaky abstractions (although I hope others do not use coord for anything 😅) as we're relying on the implementation to do this for us atm
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.
32918de