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

Create a scan_block function to use across scanning tasks #7994

Merged
merged 8 commits into from
Nov 28, 2023

Conversation

upbqdn
Copy link
Member

@upbqdn upbqdn commented Nov 24, 2023

Motivation

Close #7947.

PR Author Checklist

  • Will the PR name make sense to users?
  • Does the PR have a priority label?
  • Have you added or updated tests?
  • Is the documentation up to date?
For significant changes:
  • Is there a summary in the CHANGELOG?
  • Can these changes be split into multiple PRs?

If a checkbox isn't relevant to the PR, mark it as done.

Solution

I wrapped the original scanning function so it:

  • takes:
    • Zebra's blocks;
    • Zebra's network type;
    • keys without accounts;
  • doesn't take:
    • Sapling nullifiers;
    • prior block metadata.

Testing

I updated two existing tests. Two more use the original scanning function, but they're better off with that one.

Review

Reviewer Checklist

Check before approving the PR:

  • Does the PR scope match the ticket?
  • Are there enough tests to make sure it works? Do the tests cover the PR motivation?
  • Are all the PR blockers dealt with?
    PR blockers can be dealt with in new tickets or PRs.

And check the PR Author checklist is complete.

@upbqdn upbqdn added A-rust Area: Updates to Rust code P-Medium ⚡ A-blockchain-scanner Area: Blockchain scanner of shielded transactions labels Nov 24, 2023
@upbqdn upbqdn self-assigned this Nov 24, 2023
@upbqdn upbqdn requested a review from a team as a code owner November 24, 2023 22:48
@upbqdn upbqdn requested review from oxarbitrage and removed request for a team November 24, 2023 22:48
oxarbitrage
oxarbitrage previously approved these changes Nov 26, 2023
Copy link
Contributor

@oxarbitrage oxarbitrage left a comment

Choose a reason for hiding this comment

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

Looks really good, it simplifies a lot of things and clarifies what is really needed and what can be left behind.

It is nice to see that you added the function into the test module first, i think we should move the "production code" from there after #7989 merges into the scan.rs file.

I made a few optional suggestions and comments, well done!

zebra-scan/src/tests.rs Outdated Show resolved Hide resolved
zebra-scan/src/tests.rs Outdated Show resolved Hide resolved
zebra-scan/src/tests.rs Outdated Show resolved Hide resolved
zebra-scan/src/tests.rs Show resolved Hide resolved
zebra-scan/src/tests.rs Show resolved Hide resolved
@upbqdn upbqdn requested a review from a team as a code owner November 28, 2023 11:43
@upbqdn upbqdn requested review from teor2345 and removed request for a team November 28, 2023 11:43
Co-authored-by: Alfredo Garcia <[email protected]>
teor2345
teor2345 previously approved these changes Nov 28, 2023
Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

This seems good enough to merge, we can do more tests and move code around later?

zebra-scan/src/tests.rs Outdated Show resolved Hide resolved
@mergify mergify bot merged commit e00a762 into main Nov 28, 2023
128 checks passed
@mergify mergify bot deleted the add-scan-block-fn branch November 28, 2023 21:58
@upbqdn upbqdn mentioned this pull request Dec 4, 2023
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-blockchain-scanner Area: Blockchain scanner of shielded transactions A-rust Area: Updates to Rust code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create a scan_block function to use across scanning tasks
4 participants