Skip to content
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

Simplify reveal_to_target logic #890

Merged

Conversation

evanlinjin
Copy link
Member

@evanlinjin evanlinjin commented Mar 15, 2023

Description

  • Remove unnecessary check
  • Better comments
  • Better variable names
  • Added another test

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

New Features:

  • I've added tests for the new feature
  • I've added docs for the new feature

@evanlinjin evanlinjin force-pushed the simplify-reveal-to-target-logic branch from fdca1aa to 0caad68 Compare March 15, 2023 08:55
@evanlinjin evanlinjin marked this pull request as ready for review March 15, 2023 08:55
@evanlinjin evanlinjin added this to the 1.0.0-alpha.0 milestone Mar 15, 2023
Copy link
Contributor

@vladimirfomene vladimirfomene left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice one. This version of the code is easy to follow. I just have a nit.

);

// if we need to reveal new indexes, the latest revealed index goes here
let mut reveal_to_index = None;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe you should call this latest_reveal_index. It might remove the need to have a comment.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we probably need a comment anyway to inform that this variable is to be mutated "if we need to reveal new indices".

@evanlinjin evanlinjin self-assigned this Mar 16, 2023
- Remove unnecessary check
- Better comments
- Better variable names
- Add test for scanning txouts with lookahead
@evanlinjin evanlinjin force-pushed the simplify-reveal-to-target-logic branch from 0caad68 to 72b1e2a Compare March 18, 2023 01:18
@evanlinjin evanlinjin merged commit 919e74a into bitcoindevkit:master Mar 18, 2023
@notmandatory notmandatory mentioned this pull request Mar 20, 2023
29 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants