Update panics by todo! and unimplemented! macros
#2471
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jvff
commented
Jul 8, 2021
teor2345
reviewed
Jul 9, 2021
zebra-chain/src/orchard/tree.rs
Outdated
Comment on lines
168
to
173
| impl From<Vec<NoteCommitment>> for NoteCommitmentTree { | ||
| fn from(_values: Vec<NoteCommitment>) -> Self { | ||
| unimplemented!(); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Thinking about merge conflicts here: we probably just want to let PR #2407 deal with this.
teor2345
reviewed
Jul 9, 2021
zebra-chain/src/sapling/tree.rs
Outdated
Comment on lines
118
to
122
| impl From<Vec<NoteCommitment>> for NoteCommitmentTree { | ||
| fn from(_values: Vec<NoteCommitment>) -> Self { | ||
| unimplemented!(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Thinking about merge conflicts here: we probably just want to let PR #2407 deal with this.
teor2345
reviewed
Jul 9, 2021
teor2345
reviewed
Jul 9, 2021
teor2345
suggested changes
Jul 9, 2021
jvff
force-pushed
the
update-panics-by-todo-and-unimplemented-macros
branch
from
e37e17e to
0d00293
Compare
So that it is clear why the panic happened upon initial inspection. Also include a reference to the mempool epic, so that it's easier to find the issue that tracks the implementation of the missing code.
Make it easy to find the relevant issue if the panic occurs.
The current implementation works, the commented out code was just a previous improvement idea, which is now tracked by issue ZcashFoundation#2473.
jvff
force-pushed
the
update-panics-by-todo-and-unimplemented-macros
branch
from
0d00293 to
7e6d8bb
Compare
teor2345
approved these changes
Jul 9, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Motivation
When NU5 activates, Zebra must not panic unexpectedly. This can happen for example if execution reaches a code that's not complete and has a
todo!orunimplemented!panicking placeholder.Solution
I executed the command:
and went through each occurrence.
unreachableif I figured that there was no code missing but the condition could be reached due a coding mistake;Closes #2369
Review
@teor2345.
Reviewer Checklist
Follow Up Work
Go through all
TODO,FIXMEandXXXcomments to see if there are any missing pieces that aren't tracked by issues.