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

feat(grpc): Add initial Getinfo grpc #8178

Merged
merged 14 commits into from
Jan 26, 2024
Merged

feat(grpc): Add initial Getinfo grpc #8178

merged 14 commits into from
Jan 26, 2024

Conversation

oxarbitrage
Copy link
Contributor

@oxarbitrage oxarbitrage commented Jan 22, 2024

Motivation

We want to get started with the grpc methods by adding a first dummy call that will work as the framework to add other more interesting ones.

Part of #8162

Depend on #8167

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

I followed the hello world example of tonic (https://github.com/hyperium/tonic/blob/master/examples/helloworld-tutorial.md) and adapted to our needs.

Access to the scanner database will be done by a service at #8185 . We want to add some real info to this method after that.

This is the most basic and smaller piece i was able to build.

Testing

No testing was done here, only manual testing.

In the server side the grpc endpoint can be started with:

cargo run --bin scanner-grpc-server

Then the client can make a call with:


$grpcurl -plaintext -import-path ./proto -proto scanner.proto '[::1]:50051' scanner.Scanner/GetInfo
{
  "minSaplingBirthdayHeight": 419200
}
$ 

I think we should make tests by using client code at https://github.com/hyperium/tonic/blob/master/examples/helloworld-tutorial.md#writing-our-client

Review

I think anyone that will be working in the grpc methods we are about to develop 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

  • Create test
  • Run with zebra instead of separated binary
  • Add configuration (endpoint socket/addr, port, etc).
  • Decide what to add that is actually useful in the getinfo response.

@oxarbitrage oxarbitrage added A-rpc Area: Remote Procedure Call interfaces A-blockchain-scanner Area: Blockchain scanner of shielded transactions P-Medium ⚡ labels Jan 22, 2024
@oxarbitrage oxarbitrage self-assigned this Jan 22, 2024
@oxarbitrage oxarbitrage requested a review from a team as a code owner January 22, 2024 16:53
@oxarbitrage oxarbitrage requested review from arya2 and removed request for a team January 22, 2024 16:53
@github-actions github-actions bot added the C-feature Category: New features label Jan 22, 2024
zebra-grpc/Cargo.toml Outdated Show resolved Hide resolved
zebra-grpc/src/server.rs Outdated Show resolved Hide resolved
@oxarbitrage oxarbitrage added do-not-merge Tells Mergify not to merge this PR and removed do-not-merge Tells Mergify not to merge this PR labels Jan 23, 2024
Base automatically changed from issue8161 to main January 23, 2024 23:07
@mpguerra mpguerra linked an issue Jan 24, 2024 that may be closed by this pull request
@oxarbitrage oxarbitrage requested a review from a team as a code owner January 25, 2024 20:08
Copy link
Contributor Author

@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, i am not really sure if the failing test is related to this PR. https://github.com/ZcashFoundation/zebra/actions/runs/7661775200/job/20881794125?pr=8178

@oxarbitrage oxarbitrage requested a review from a team as a code owner January 26, 2024 00:06
@github-actions github-actions bot added the C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG label Jan 26, 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.

Looks good!

@mergify mergify bot merged commit 78d33f3 into main Jan 26, 2024
186 checks passed
@mergify mergify bot deleted the issue8162 branch January 26, 2024 01: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 A-rpc Area: Remote Procedure Call interfaces C-feature Category: New features C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG P-Medium ⚡
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feature: Implement getinfo method for new gRPC interface
2 participants