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

go/keymanager/api: Fix key manager client #6078

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

peternose
Copy link
Contributor

@peternose peternose commented Feb 22, 2025

Moving key manager backend from ServicesBackend to ClientBackend.

I also refactored clients, trying to follow these guidelines:

  • First define struct, then implement constructor, and finally implement all methods.
  • Accept interfaces, return structs.

Copy link

netlify bot commented Feb 22, 2025

Deploy Preview for oasisprotocol-oasis-core canceled.

Name Link
🔨 Latest commit 0cc9f88
🔍 Latest deploy log https://app.netlify.com/sites/oasisprotocol-oasis-core/deploys/67bbbeab3747ab0008406ffe

@@ -12,8 +12,10 @@ import (

var (
// serviceName is the gRPC service name.
serviceName = cmnGrpc.NewServiceName("KeyManager")
serviceName = cmnGrpc.NewServiceName("KeyManager.Secrets")
Copy link
Contributor Author

@peternose peternose Feb 22, 2025

Choose a reason for hiding this comment

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

Is this too breaking? Not sure who even calls this endpoint, since all nodes run consensus 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Should be fine. Maybe expose both for a few versions and use the new one in clients?

@peternose peternose force-pushed the peternose/trivial/fix-km-client branch from 5210628 to c354245 Compare February 22, 2025 14:39
@peternose peternose force-pushed the peternose/trivial/fix-km-client branch from c354245 to e22cea4 Compare February 22, 2025 14:48
Copy link

codecov bot commented Feb 22, 2025

Codecov Report

Attention: Patch coverage is 60.23739% with 134 lines in your changes missing coverage. Please review.

Project coverage is 65.26%. Comparing base (90ff4c1) to head (0cc9f88).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
go/keymanager/secrets/grpc.go 20.68% 20 Missing and 3 partials ⚠️
go/keymanager/churp/grpc.go 12.00% 21 Missing and 1 partial ⚠️
go/worker/keymanager/secrets.go 16.66% 12 Missing and 3 partials ⚠️
go/beacon/api/grpc.go 50.00% 7 Missing ⚠️
go/consensus/api/grpc.go 82.50% 7 Missing ⚠️
go/vault/api/grpc.go 41.66% 7 Missing ⚠️
go/storage/api/grpc.go 45.45% 6 Missing ⚠️
go/runtime/registry/notifier.go 16.66% 4 Missing and 1 partial ⚠️
go/worker/common/committee/keymanager.go 16.66% 4 Missing and 1 partial ⚠️
go/worker/keymanager/churp.go 16.66% 4 Missing and 1 partial ⚠️
... and 16 more
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6078      +/-   ##
==========================================
+ Coverage   64.86%   65.26%   +0.40%     
==========================================
  Files         636      636              
  Lines       64641    64729      +88     
==========================================
+ Hits        41929    42248     +319     
+ Misses      17823    17586     -237     
- Partials     4889     4895       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@peternose peternose marked this pull request as ready for review February 22, 2025 15:15
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