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

gRPC create #19

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

gRPC create #19

wants to merge 11 commits into from

Conversation

elffjs
Copy link
Member

@elffjs elffjs commented Aug 17, 2024

Add a VIN VC retrieval/generation gRPC call for internal use.

  1. The arguments are the same as in the REST API: vehicle token id and a force boolean.
  2. The response in the successful case currently only contains the raw VC.

@@ -5,21 +5,15 @@ option go_package = "github.com/DIMO-Network/attestation-api/pkg/grpc";
package grpc;

service AttestationService {
rpc BatchCreateVINVC(BatchCreateVINVCRequest) returns (BatchCreateVINVCResponse);
Copy link
Member Author

Choose a reason for hiding this comment

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

I guess Yev wants to start with non-batch. Casing looks a bit odd but follows the official style guide.

message BatchCreateVINVCResponse {
repeated VINVCResult results = 1;
message CreateVinVcResponse {
string raw_vc = 1;
Copy link
Member Author

Choose a reason for hiding this comment

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

Just returning the raw string. I don't really want to build another RPC call into Telemetry. We'll get rid of all this soon.

@elffjs elffjs marked this pull request as ready for review October 15, 2024 18:42
@KevinJoiner
Copy link
Collaborator

Up to you but I don't mind updating the identity client error handling in this PR. Since this repo is still newer all the updates are welcome

Copy link
Collaborator

@KevinJoiner KevinJoiner left a comment

Choose a reason for hiding this comment

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

Let's merge this in I since I am about to have some larger interface signature changes to get this to work with our new indexing.

@elffjs
Copy link
Member Author

elffjs commented Oct 23, 2024

I need to add the actual wiring. This is getting pushed back so I’ll probably just merge in whatever you’re doing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants