Refactor reveal_to_target and next_store_index
#1261
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
evanlinjin
requested changes
Jan 9, 2024
evanlinjin
force-pushed
the
keychain_txout_index_refactor
branch
from
6800d62
to
a28bbb2
Compare
|
@danielabrozzoni I added another |
Rename `v` to `index` for better clarity, and add a comment explaining the `range`
Simplify the `reveal_to_target` algorithm by exiting prematurely if the `target_index` is already revealed. Since the `reveal_to_index` variable was different from `Some(target_index)` only if the target was already revealed, we can getrid of the variable altogether.
danielabrozzoni
force-pushed
the
keychain_txout_index_refactor
branch
from
a28bbb2
to
5f1ee2c
Compare
danielabrozzoni
force-pushed
the
keychain_txout_index_refactor
branch
from
5f1ee2c
to
761189a
Compare
evanlinjin
approved these changes
Jan 11, 2024
Comment on lines
+209
to
+210
| // This range is filtering out the spks with a keychain different than | ||
| // `keychain`. We don't use filter here as range is more optimized. |
There was a problem hiding this comment.
I would prefer "Range through all spks of keychain.". However, I feel like this comment is not needed as the codebase already shows this.
Definitely not a blocker though.
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.
Description
As a part of #1203, I found myself having to modify
reveal_to_target, but had some troubles understanding exactly what the method was doing.This PR refactors
reveal_to_targetto be, in my opinion, a bit clearer. We now exist prematurely ifnext_reveal_index<target_index; this way, we don't need to keep track ofnext_reveal_index, as it would always be equal toSome(target_index).As a part of trying to understand
reveal_to_targetI had to read throughnext_store_index, I slightly improved it for clarity reasons as well.Changelog notice
Checklists
All Submissions:
cargo fmtandcargo clippybefore committing