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): Test the RegisterKeys scan service call #8281

Merged
merged 15 commits into from
Feb 19, 2024

Conversation

upbqdn
Copy link
Member

@upbqdn upbqdn commented Feb 16, 2024

Motivation

We are missing tests for the RegisterKeys service call.

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

This PR adds tests for RegisterKeys, and contains a bunch of additional clean-ups:

  • Don't wait for Sapling activation for tests.
  • Set the sleep interval for the scan service to ten secs to:
    • make tests faster, and
    • make the service respond faster to requests.
  • Enable mocking Sapling scanning keys for networks other than Mainnet.

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-blockchain-scanner Area: Blockchain scanner of shielded transactions P-Medium ⚡ labels Feb 16, 2024
@upbqdn upbqdn self-assigned this Feb 16, 2024
@upbqdn upbqdn requested review from a team as code owners February 16, 2024 22:38
@upbqdn upbqdn requested review from oxarbitrage and removed request for a team February 16, 2024 22:38
@github-actions github-actions bot added the C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG label Feb 16, 2024
@upbqdn upbqdn changed the title Test the RegisterKeys scan service call add(scan): Test the RegisterKeys scan service call Feb 16, 2024
@upbqdn upbqdn force-pushed the test-registerkeys-service branch from a9ff9a7 to 13c78f0 Compare February 16, 2024 22:44
@upbqdn upbqdn requested a review from a team as a code owner February 16, 2024 22:44
@upbqdn upbqdn force-pushed the test-registerkeys-service branch from a100faa to 69d1951 Compare February 16, 2024 23:22
@mpguerra mpguerra linked an issue Feb 19, 2024 that may be closed by this pull request
@upbqdn upbqdn removed the C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG label Feb 19, 2024
zebra-scan/src/service/scan_task/scan.rs Outdated Show resolved Hide resolved
zebra-scan/src/service/tests.rs Show resolved Hide resolved
zebra-scan/src/service/tests.rs Outdated Show resolved Hide resolved
zebra-scan/src/service/tests.rs Outdated Show resolved Hide resolved
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 great, thank you for also updating the CHECK_INTERVAL.

I'm not sure about adding the strum crate, I think we should add our own code where needed to avoid reviewing the added dependency, but I like the idea of iterating through Network variants in tests. We may want to cleanup other tests that check both networks to iterator through the default variants before adding fields to the Testnet variant so that they won't need to be updated later.

@github-actions github-actions bot added the C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG label Feb 19, 2024
@upbqdn upbqdn removed the C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG label Feb 19, 2024
@github-actions github-actions bot added the C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG label Feb 19, 2024
@upbqdn upbqdn removed the C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG label Feb 19, 2024
@mergify mergify bot merged commit 663c577 into main Feb 19, 2024
132 checks passed
@mergify mergify bot deleted the test-registerkeys-service branch February 19, 2024 23:45
idky137 pushed a commit to idky137/zebra that referenced this pull request Feb 28, 2024
…#8281)

* Impl generating continuous deserialized blocks

* Make `sapling_efvk_hrp` `pub`

* Don't wait for Sapling activation height in tests

* Set the sleep interval for scan service to 10 secs

* Simplify `sapling_key_to_scan_block_keys`

* Enable mocking Sapling scanning keys for Testnet

* Test the `RegisterKeys` scan service call

* Enable `shielded-scan` for `zebra-chain`

* Use an ephemeral database so results don't persist

* Don't generate blocks when mocking the state

* Improve error messages

* Simplify seeding mocked Sapling viewing keys

* Apply suggestions from code review

Co-authored-by: Arya <[email protected]>

* Use a manual iterator over `Network`

---------

Co-authored-by: Arya <[email protected]>
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.

add(scan): RegisterKeys gRPC
2 participants