docs: ADR-066 Keyring plugin system #16637

bizk · 2023-06-21T15:43:57Z

Description

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

confirmed the correct type prefix in the PR title
confirmed ! in the type prefix if API or client breaking change
confirmed all author checklist items have been addressed
reviewed state machine logic
reviewed API design and naming
reviewed documentation is accurate
reviewed tests and test coverage
manually tested (if applicable)

docs/architecture/adr-066-keyring-plugin.md

Co-authored-by: Marko <marko@baricevic.me>

JulianToledano · 2023-06-26T15:06:27Z

@tac0turtle @alexanderbez updated with definitions!

facundomedica · 2023-07-13T08:34:24Z

Hello! I think we need (or I need lol) a bit more of context, what's the main reason for developing this? The ADR only mentions that 99designs/keyring is not maintained, but seems a bit orthogonal to a keyring plugin system. So I think it would be good to include in the context section what you've been talking in other discussions and issues.

That's my only comment, it looks great 💯

JulianToledano · 2023-07-15T15:58:16Z

Hello! I think we need (or I need lol) a bit more of context, what's the main reason for developing this? The ADR only mentions that 99designs/keyring is not maintained, but seems a bit orthogonal to a keyring plugin system. So I think it would be good to include in the context section what you've been talking in other discussions and issues.

That's my only comment, it looks great 💯

Hi! 👋
This work is part of the keyring refactor epic in #14940

The idea behind this is:

Provide teams with the possibility of develop new backends in any language. This open the doors to key types defined in other languages rather than go.
Remove 99designs/keyring dependency as it is not really under maintenance.

The plugin system will let anyone develop a keyring by fulfilling the gRPC service with all its perks associated and as mentioned in the ADR this is compatible with the current keyring as the main interface has not been modified.

tac0turtle · 2023-07-17T09:12:09Z

docs/architecture/adr-066-keyring-plugin.md

+  rpc ExportPubKeyArmor(ExportPubKeyArmorRequest) returns (ExportPubKeyArmorReturns);
+  rpc ExportPubKeyArmorByAddress(ExportPubKeyArmorByAddressRequest) returns (ExportPubKeyArmorByAddressReturns);
+  rpc ExportPrivKeyArmor(ExportPrivKeyArmorRequest) returns (ExportPrivKeyArmorReturns);
+  rpc ExportPrivKeyArmorByAddress(ExportPrivKeyArmorByAddressRequest) returns (ExportPrivKeyArmorByAddressReturns);


is there anyway to merge these four functions, it seems a bit odd to have the api be this big

The idea is to create an RPC endpoint for each existing keyring method. Alternatively, we can simplify this by having a single method that accepts a message containing all the necessary information to determine the user's intent. For example, we can define the following protobuf message:

message KeyringRequest { bool exportPubKey = 1; string uid = 2; bytes address = 3; }

Then, within the implementation, we can check if the uid field is empty. If it is, we can search for the record associated with the given address.

I think we should aim to come up with a simple api. The current api either good or bad is fairly large and increases the maintenance burden. Let's try to reduce it to a simplified api. If we need to create a larger struct that is passed that is okay as well.

tac0turtle · 2023-07-17T09:12:47Z

docs/architecture/adr-066-keyring-plugin.md

+  rpc Key(KeyRequest ) returns (KeyReturns);
+  rpc KeyByAddress(KeyByAddressRequest ) returns (KeyByAddressReturns);
+  rpc Delete(DeleteRequest ) returns (DeleteReturns);
+  rpc DeleteByAddress(DeleteByAddressRequest ) returns (DeleteByAddressReturns);


can these be deduplicated?

Key and Delete seem enough?

Same as the comment above.

tac0turtle · 2023-07-17T09:13:08Z

docs/architecture/adr-066-keyring-plugin.md

+#### Service Definition
+```protobuf
+service KeyringService {
+  rpc Backend(BackendRequest) returns (BackendResponse);


why does backend need to be part of this api?

This method is meant to return the plugins' name and version.

why does the node need to know what backend is being used?

julienrbrt · 2023-08-01T09:51:45Z

docs/architecture/adr-066-keyring-plugin.md

could you markdownlint this file?

julienrbrt · 2023-08-01T09:53:30Z

docs/architecture/adr-066-keyring-plugin.md

+Teams can easily develop plugins to meet their specific requirements,
+which opens the door to new functionalities in key management.
+
+### Negative


What is the overhead compared to the current implementation? In terms of latency.

The methods affected by this are those related to key creation and retrieval. Ultimately, it adds one more call to the codec. I'm not sure about the exact latency impact.

julienrbrt · 2023-08-01T09:55:28Z

docs/architecture/adr-066-keyring-plugin.md

+The idea is to define a service method for each method already present in the keyring interface.
+Similarly, a request/response message will be defined for each service method.
+
+#### Service Definition


Can you precise what is running the service (client or node) and if it is a node, what can is the increased load on the node?

The service will be instantiated whenever a keyring call is required and terminated afterward. I believe this can be managed either by the client or the node.

I think that the overall load won't be significantly impacted, considering that plugins are lightweight and are terminated when they're no longer needed.

hey @tac0turtle do you have any other comments for us or can we close this?

tac0turtle · 2023-08-28T12:55:00Z

should we close this and merge it into the other adr?

bizk · 2023-08-29T11:38:00Z

We had great discussions over this ADR and implemented everything that was requested. Since this is ready, merging it instead of incorporating it as part of the bigger ADR should keep the complexity and scope reduced and allow the rest of the team to focus on other topics that will require more attention.

In the other hand. It could be part of the Crypto ADR since it is aligned with the plans, but personally I don't see more benefits than having everything at one place.

tac0turtle

i think it makes sense to have a call about this ADR, i still think the API should be 70% less. The API proposed is the current API, for native it makes sense, but exposing so much will make it harder for users to integrate. What is the minimal API needed to funciton?

JulianToledano · 2023-09-14T13:32:03Z

What is the minimal API needed to funciton?

I think this would be the minimal API.

service KeyringService {
  rpc Backend(BackendRequest) returns (BackendResponse);
  rpc List(ListRequest) returns (stream ListResponse);
  rpc SupportedAlgorithms(SupportedAlgorithmsRequest) returns (SupportedAlgorithmsReturns);
  rpc Key(KeyRequest ) returns (KeyReturns);
  rpc Delete(DeleteRequest ) returns (DeleteReturns);
  rpc Rename(RenameRequest) returns (RenameReturns);
  rpc NewAccount(NewAccountRequest) returns (NewAccountReturns);
  rpc SaveLedgerKey(SaveLedgerKeyRequest) returns (SaveLedgerKeyReturns);
  rpc SaveOfflineKey(SaveOfflineKeyRequest)returns (SaveOfflineKeyReturns);
  rpc SaveMultisig(SaveMultisigRequest) returns (SaveMultisigReturns);
  rpc Sign(SignRequest) returns (SignReturns);
  rpc ImportKey(ImportKeyRequest) returns (ImportKeyReturns);
  rpc ExportKeyArmor(ExportKeyArmorRequest) returns (ExportKeyArmorReturns);
}

The messages will hold all the information needed to determine the user's intent. Like retrieving a key from an address or an id.

tac0turtle · 2023-09-27T10:18:11Z

I'm starting to think this is overkill for cli support. keyring is meant for cli, which cosmos is still one of the few frameworks that have native cli. I believe this is because of a lack of client support for anything else. Id propose we rewrite the current keyring to be better maintainable and easier to integrate with autocli. This way it keeps the design simple and pushes users to use a client based frontend which will have better support for signing.

JulianToledano · 2023-09-27T22:18:34Z

I agree, I think we can close this. The scope of the upcoming crypto ADR will make this one deprecated, as there will be the possibility of a gRPC keyring in the new one.

bizk and others added 3 commits June 20, 2023 14:58

Draft of ADR

67d7da6

update: draft

296c930

Update adr-066-keyring-plugin.md

99e6e2e

github-actions bot added the Type: ADR label Jun 21, 2023

Merge branch 'cosmos:main' into zondax/adr/keybase

1bc19e8

bizk marked this pull request as ready for review June 21, 2023 15:44

bizk requested a review from a team as a code owner June 21, 2023 15:44

github-prbot requested review from a team, kocubinski and likhita-809 and removed request for a team June 21, 2023 15:44

ainhoa-a mentioned this pull request Jun 22, 2023

EPIC: Zondax maintenance Q2/23 #16220

Closed

25 tasks

alexanderbez reviewed Jun 22, 2023

View reviewed changes

docs/architecture/adr-066-keyring-plugin.md Show resolved Hide resolved

tac0turtle reviewed Jun 23, 2023

View reviewed changes

docs/architecture/adr-066-keyring-plugin.md Outdated Show resolved Hide resolved

tac0turtle reviewed Jun 23, 2023

View reviewed changes

docs/architecture/adr-066-keyring-plugin.md Show resolved Hide resolved

JulianToledano and others added 2 commits June 23, 2023 14:32

Update docs/architecture/adr-066-keyring-plugin.md

6bb809e

Co-authored-by: Marko <marko@baricevic.me>

update: interfaces definitions

fd45f6a

fix

b4a2905

bizk requested review from alexanderbez and tac0turtle July 6, 2023 12:54

tac0turtle reviewed Jul 17, 2023

View reviewed changes

bizk requested a review from tac0turtle July 25, 2023 12:06

julienrbrt reviewed Aug 1, 2023

View reviewed changes

fix: markdownlint

3797520

bizk requested a review from julienrbrt August 7, 2023 11:52

JulianToledano mentioned this pull request Aug 15, 2023

EPIC: Keyring refactor #14940

Closed

11 tasks

Merge branch 'main' into zondax/adr/keybase

20c39cd

tac0turtle reviewed Sep 4, 2023

View reviewed changes

ainhoa-a mentioned this pull request Sep 6, 2023

[Epic]: Zondax Maintenance Q3/23 #17539

Closed

13 tasks

JulianToledano closed this Sep 27, 2023

bizk deleted the zondax/adr/keybase branch October 10, 2023 21:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

docs: ADR-066 Keyring plugin system #16637

docs: ADR-066 Keyring plugin system #16637

bizk commented Jun 21, 2023

JulianToledano commented Jun 26, 2023

facundomedica commented Jul 13, 2023 •

edited

Loading

JulianToledano commented Jul 15, 2023

tac0turtle Jul 17, 2023

JulianToledano Jul 17, 2023

tac0turtle Aug 17, 2023

tac0turtle Jul 17, 2023

JulianToledano Jul 17, 2023

tac0turtle Jul 17, 2023

JulianToledano Jul 17, 2023

tac0turtle Sep 4, 2023

julienrbrt Aug 1, 2023

julienrbrt Aug 1, 2023

JulianToledano Aug 2, 2023

julienrbrt Aug 1, 2023

JulianToledano Aug 2, 2023 •

edited

Loading

ainhoa-a Aug 10, 2023

tac0turtle commented Aug 28, 2023

bizk commented Aug 29, 2023

tac0turtle left a comment

JulianToledano commented Sep 14, 2023

tac0turtle commented Sep 27, 2023

JulianToledano commented Sep 27, 2023

docs: ADR-066 Keyring plugin system #16637

docs: ADR-066 Keyring plugin system #16637

Conversation

bizk commented Jun 21, 2023

Description

Author Checklist

Reviewers Checklist

JulianToledano commented Jun 26, 2023

facundomedica commented Jul 13, 2023 • edited Loading

JulianToledano commented Jul 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JulianToledano Aug 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tac0turtle commented Aug 28, 2023

bizk commented Aug 29, 2023

tac0turtle left a comment

Choose a reason for hiding this comment

JulianToledano commented Sep 14, 2023

tac0turtle commented Sep 27, 2023

JulianToledano commented Sep 27, 2023

facundomedica commented Jul 13, 2023 •

edited

Loading

JulianToledano Aug 2, 2023 •

edited

Loading