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): Add ClearResults and DeleteKeys gRPC methods #8237

Merged
merged 5 commits into from
Feb 6, 2024

Conversation

arya2
Copy link
Contributor

@arya2 arya2 commented Feb 6, 2024

Motivation

We want to clear results and delete keys in zebra-scan via gRPC requests.

Closes #8235

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

  • Adds the gRPC methods to the proto definition
  • Implements the methods on ScannerRPC to call the ScanService

Testing

These methods still need tests, but they're thin wrappers around scan service requests that do have tests.

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

  • Add tests to zebra-grpc

@arya2 arya2 added A-rpc Area: Remote Procedure Call interfaces C-feature Category: New features A-blockchain-scanner Area: Blockchain scanner of shielded transactions P-Medium ⚡ labels Feb 6, 2024
@arya2 arya2 self-assigned this Feb 6, 2024
@arya2 arya2 requested a review from a team as a code owner February 6, 2024 00:17
@arya2 arya2 requested review from oxarbitrage and removed request for a team February 6, 2024 00:17
zebra-grpc/src/server.rs Outdated Show resolved Hide resolved
zebra-grpc/src/server.rs Outdated Show resolved Hide resolved
oxarbitrage
oxarbitrage previously approved these changes Feb 6, 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 but i think we might want tests. We can do all the grpc testing (including snapshots) for all grpc in a separated ticket or add tests here so the other calls can follow.

Can you make a manual test and post the output ? A manual test should be to start the server and run these calls, make sure they reply as expected.

zebra-grpc/proto/scanner.proto Outdated Show resolved Hide resolved
zebra-grpc/src/server.rs Show resolved Hide resolved
zebra-grpc/src/server.rs Show resolved Hide resolved
@arya2
Copy link
Contributor Author

arya2 commented Feb 6, 2024

Looks good to me but i think we might want tests. We can do all the grpc testing (including snapshots) for all grpc in a separated ticket or add tests here so the other calls can follow.

I'll open an issue for adding tests for the gRPC methods, I think it's okay to add them later since the RPC methods are thin wrappers around the scan service calls.

Can you make a manual test and post the output ? A manual test should be to start the server and run these calls, make sure they reply as expected.

Yep, I'll start the server from zebrad in a follow-up PR targeting this branch for the manual test.

@arya2
Copy link
Contributor Author

arya2 commented Feb 6, 2024

Manual test of these methods:

~/github/zebra$ $HOME/go/bin/grpcurl -plaintext -import-path ./zebra-grpc/proto -proto zebra-grpc/proto/scanner.proto -d '{"keys": [""]}' '[::1]:50051' scanner.Scanner/ClearResults
{}
~/github/zebra$ $HOME/go/bin/grpcurl -plaintext -import-path ./zebra-grpc/proto -proto zebra-grpc/proto/scanner.proto -d '{"keys": [""]}' '[::1]:50051' scanner.Scanner/DeleteKeys
{}

It seems strange to have a successful response when the provided keys were not registered in the scanner, we may want to return an error in that case later.

@mergify mergify bot merged commit 8d1a1ac into main Feb 6, 2024
186 checks passed
@mergify mergify bot deleted the scan-grpc-clear-method branch February 6, 2024 21:08
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-rpc Area: Remote Procedure Call interfaces C-feature Category: New features P-Medium ⚡
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add ClearResults and DeleteKeys gRPC methods
2 participants