-
Notifications
You must be signed in to change notification settings - Fork 20
Keystore capability service #1285
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds the keystore LOOPP service as a standard capability service and implements a new MultiAccountSigner for handling multiple keystore accounts. It also updates numerous service initialization interfaces to accept an additional keyStore parameter and modifies the corresponding protobuf definitions accordingly.
Reviewed Changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
pkg/types/core/keystore_test.go | Adds tests for the new MultiAccountSigner implementation. |
pkg/types/core/keystore.go | Introduces the multiAccountSigner implementation and its constructor. |
pkg/loop/standard_capabilities_test.go | Updates test cases to include the extra keyStore parameter in Initialise calls. |
pkg/loop/standard_capabilities.go | Updates the StandardCapabilities interface to include keyStore for service initialization. |
pkg/loop/internal/core/services/keystore/keystore.go | Introduces GRPC client/server implementations for keystore functionality. |
pkg/loop/internal/core/services/capability/standard/test/standard_capabilities.go | Updates Initialise signature to include keyStore. |
(Multiple files under pkg/capabilities/v2/...) | Updates Initialise signatures of various capabilities to include keyStore and related changes. |
pkg/capabilities/pb/capabilities.proto and pkg/capabilities/pb/capabilities.pb.go | Adds keyStore_id in protobuf definitions. |
Co-authored-by: Copilot <[email protected]>
// multiAccountSigner implements Keystore for multiple accounts. If a signer is not | ||
// found for a requested account, an error is returned. | ||
type multiAccountSigner struct { | ||
accounts []string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I said in the other thread; I'm curious as to why you need this -- do you mind explaining?
Aside from that -- you're making an implicit assumption that accounts and signers are in the same order. Can you add a comment making this explicit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice callout, will add a comment if this approach is maintained.
Requoting from the other PR:
"The existing Keystore interface I am using supports multiple accounts. While this PR only adds one account (P2P key), using an underlying struct that supports multiple accounts may easily permit future capability authors to use other keys.
That being said, the Keystore interface itself already allows this so really I don't need the implementation of that interface to overtly support multiple accounts at present."
Proceed in whichever thread you prefer. I am happy to define a "singleAccountSigner" implementation of the Keystore interface rather than a "multiAccountSigner" which would come at the benefit of no ambiguity around account/signer ordering. If we wanted more accounts for the capability to access later down the road, it would involve converting singleAccountSigner
to multiAccountSigner
, which has no backwards compatibility issues but is a tad bit more work.
MultiAccountSigner
that simply accepts a list of signer IDs and signers.