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

add(scan): Implement Results request #8224

Merged
merged 3 commits into from
Feb 2, 2024
Merged

add(scan): Implement Results request #8224

merged 3 commits into from
Feb 2, 2024

Conversation

oxarbitrage
Copy link
Contributor

Motivation

We need to implement the Results request in order to later do the get_results grpc.

Close #8205

PR Author Checklist

Check before marking the PR as ready for review:

  • 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?

Solution

Instead of just returning ids, this PR returns all data we have in a nested Tree. This data includes the corresponding key and height for each transaction hash found. A nested Tree structure is used to avoid repeating any data.

This version just returns all the results we have for the given keys. Height ranges or other tweaks were not included.

This request is less complex than others as no command needs to be sent to be scanner.

Testing

Test for the service is added, test for the sapling_results database function was not added as it is already tested by deletes_keys_and_results_correctly.

Review

Anyone can review. I am open to suggestions to improve it.

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.

Follow Up Work

#8163

@oxarbitrage oxarbitrage added A-blockchain-scanner Area: Blockchain scanner of shielded transactions P-Medium ⚡ labels Feb 1, 2024
@oxarbitrage oxarbitrage self-assigned this Feb 1, 2024
zebra-scan/src/service.rs Outdated Show resolved Hide resolved
zebra-scan/src/service.rs Outdated Show resolved Hide resolved
zebra-scan/src/service.rs Outdated Show resolved Hide resolved
arya2
arya2 previously approved these changes Feb 1, 2024
Copy link
Contributor

@arya2 arya2 left a comment

Choose a reason for hiding this comment

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

This looks good, just needs a blocking thread for the disk reads.

@oxarbitrage oxarbitrage marked this pull request as ready for review February 1, 2024 21:32
@oxarbitrage oxarbitrage requested review from a team as code owners February 1, 2024 21:32
@oxarbitrage oxarbitrage requested review from arya2 and removed request for a team February 1, 2024 21:32
mergify bot added a commit that referenced this pull request Feb 2, 2024
@mergify mergify bot merged commit 052f235 into main Feb 2, 2024
132 checks passed
@mergify mergify bot deleted the issue8205 branch February 2, 2024 02:29
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 P-Medium ⚡
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement Results scan service request to read any cached results for a set of keys
2 participants