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 ClearResults ScanService request #8219

Merged
merged 3 commits into from
Feb 1, 2024

Conversation

arya2
Copy link
Contributor

@arya2 arya2 commented Jan 31, 2024

Motivation

This PR implements the ClearResults request for ScanService to delete the results for a key without removing the key itself.

Closes #8207.

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

  • Renames delete_sapling_results to delete_sapling_keys
  • Adds a delete_sapling_results that adds back an empty entry after deleting the results for a sapling key
  • Calls the new storage method from the ScanService::call() match arm for Request::ClearResults

Related cleanups:

  • Moves check for max number of keys to a request method

Testing

  • Tests that sapling_delete_results() deletes results but leaves an empty entry with the key
  • Tests that the scan service ClearResults request does the same

Review

Anyone can 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.

Follow Up Work

Test that the scan service responds with an error for requests with an invalid number of keys.

@arya2 arya2 added C-feature Category: New features A-blockchain-scanner Area: Blockchain scanner of shielded transactions P-Medium ⚡ labels Jan 31, 2024
@arya2 arya2 self-assigned this Jan 31, 2024
@arya2 arya2 requested review from a team as code owners January 31, 2024 20:18
@arya2 arya2 requested review from oxarbitrage and removed request for a team January 31, 2024 20:18
@upbqdn upbqdn self-requested a review February 1, 2024 11:01
upbqdn
upbqdn previously approved these changes Feb 1, 2024
zebra-scan/src/service.rs Outdated Show resolved Hide resolved
oxarbitrage
oxarbitrage previously approved these changes Feb 1, 2024
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 good to me, left a few minor comments.

zebra-scan/src/service.rs Show resolved Hide resolved
zebra-scan/src/service/tests.rs Show resolved Hide resolved
@arya2 arya2 dismissed stale reviews from oxarbitrage and upbqdn via 70e176e February 1, 2024 17:25
@arya2 arya2 force-pushed the scan-svc-clear-results branch from 59b6dad to ce95f17 Compare February 1, 2024 18:11
@mergify mergify bot merged commit 2bf16a3 into main Feb 1, 2024
132 checks passed
@mergify mergify bot deleted the scan-svc-clear-results branch February 1, 2024 20:07
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 C-feature Category: New features P-Medium ⚡
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement ClearResults scan service request to clear the results for a set of keys
3 participants