refactor!: cryptosuite-style signers #801

rflechtner · 2023-10-18T14:51:47Z

fixes KILTProtocol/ticket#2962

Apparently nobody likes our present signer interfaces; they seem to be hard to wrap your head around.
Upcoming presentation signing with data integrity proofs means that a different sort of signer interface is required to produce those signatures, as the key identifier and algorithm is signed into the proof and thus must be known prior to signing, which does not work with our current callbacks (which return the key id together with the signature).

In order to avoid a proliferation of different signing interfaces in our sdk, this PR aims to migrate all signer functions to the interfaces introduced by our data integrity proof cryptosuites. This may actually also increase ergonomics, as it is implemented in a way that users can generate signers for their keys and DIDs once, and simply throw them all at a given function, which will select and call the appropriate handler.

TBD (possibly in a later PR)

Helper to create a signature using a DID and its signers
Signing did creation with the signers
Signing submittable extrinsic with a signer
Reusable logic for selecting a signer to be used with a did + error reporting
Use signers from external library after adjusting them to accept private keys

How to test:

This is WIP; ergonomics is important here, so writing some example signing flows would help a lot.

Checklist:

I have verified that the code works
I have verified that the code is easy to understand
- If not, I have left a well-balanced amount of inline comments
I have left the code in a better state
I have documented the changes (where applicable)
- Either PR or Ticket to update the Docs
- Link the PR/Ticket here

ntn-x2

In general I like it! I see what you did there. I left some comments around naming and typing. It looks like there's mostly raw strings everywhere, and maybe we can change that to help TS developers a bit more? Or am I missing something?

packages/utils/src/Signers.ts

packages/did/src/DidDetails/FullDidDetails.spec.ts

packages/did/src/Did.chain.ts

packages/types/src/Signers.ts

ntn-x2 · 2023-10-24T08:56:10Z

packages/types/src/Signers.ts

+
+export type SignerInterface = {
+  algorithm: string
+  id: string


Also, should the id be optional? For instance when signing an extrinsic, we don't really care about the ID of the key. We don't care about the algorithm either, but I guess we have it to rule out signatures that will definitely fail when submitted to the chain.

We do care about the id even when signing extrinsics, because we need it to figure out which verification relationship a key is in. Even as an account signer we need to know for which account this signs. So I'd say the id is crucial, always

Ok, I see. I guess I was referring to the case where the signer is an account, indeed. It kinda feels a bit of a stretch to use the account representing the key as the ID for that key. Let's see what the others have to say about it. Cc @lukeg90 @Dudleyneedham

I think it could potentially be confusing for a developer to determine what string they should enter here as the ID, especially when signers can be both accounts and DIDs, depending on the context . But I don't really have a suggestion for improvement, other than making sure we have good documentation.

Of course if they always use the getSigner methods then they don't even have to care what the ID is.

I think that's the intended way for the signer to be created, indeed.

yeah this would mostly get unnoticed; there may be a getAccountSigners function or method that creates all signers for an account (using Addresses as ids) and a getSigners method on a DID class or something that returns all signers for its verificationMethods (where all ids are DID URLs). This higher level api is TBD in a follow-up PR, but I see a lot of potential

packages/utils/src/Signers.ts

Dudleyneedham

Not sure I like the term signers however, I couldn't come up with anything interesting.

Looks good.

Dudleyneedham · 2023-10-24T11:45:49Z

packages/core/src/delegation/DelegationNode.ts

-    }
-    const verificationMethod = delegateDid.verificationMethod?.find(
-      ({ id }) => id === fragment
+    const { byDid, verifiableOnChain } = Signers.select


What does byDid mean?

you select a Signer byDid (-> by it's association to a Did, via the DidDocument). I thought that was self-explanatory, how can this be improved?

I think on its own it might not make sense, but when it's exported with a named object, it made more sense to me, also the way it's used in the tests.

any suggestions here?

packages/did/src/Did.chain.ts

packages/did/src/DidDetails/FullDidDetails.ts

packages/legacy-credentials/src/Credential.ts

packages/utils/src/Signers.ts

packages/did/src/DidDetails/FullDidDetails.ts

packages/legacy-credentials/src/Credential.ts

lukeg90 · 2023-10-24T13:55:02Z

Good riddance signing callbacks!

ntn-x2

There's few incompatibilities with the underlying DID refactoring branch by now (sorry about that). But as far as I am concerned, this could be merged. Looking forward for the rest of the issues by others to be resolved.

* allows removing reimplementation signer factories * allows stronger typing of algorithm identifiers

rflechtner self-assigned this Oct 18, 2023

rflechtner changed the base branch from develop to vc-refactor October 18, 2023 14:52

rflechtner force-pushed the rf-signers branch from 893bf03 to 0d876eb Compare October 19, 2023 18:48

rflechtner changed the base branch from vc-refactor to aa/native-did-attempt-2 October 19, 2023 19:00

ntn-x2 mentioned this pull request Oct 20, 2023

feat!: native DIDDocuments and proper DID resolution interface #799

Merged

rflechtner force-pushed the rf-signers branch 3 times, most recently from c92a0f9 to 04eb5b3 Compare October 23, 2023 16:05

rflechtner marked this pull request as ready for review October 24, 2023 05:56

rflechtner requested a review from ntn-x2 October 24, 2023 05:57

ntn-x2 reviewed Oct 24, 2023

View reviewed changes

rflechtner requested review from lukeg90 and Dudleyneedham October 24, 2023 09:50

Dudleyneedham reviewed Oct 24, 2023

View reviewed changes

rflechtner commented Oct 24, 2023

View reviewed changes

lukeg90 reviewed Oct 24, 2023

View reviewed changes

packages/did/src/DidDetails/FullDidDetails.ts Outdated Show resolved Hide resolved

lukeg90 reviewed Oct 24, 2023

View reviewed changes

packages/legacy-credentials/src/Credential.ts Show resolved Hide resolved

ntn-x2 approved these changes Oct 25, 2023

View reviewed changes

Base automatically changed from aa/native-did-attempt-2 to vc-refactor October 26, 2023 07:56

rflechtner added 10 commits October 26, 2023 17:33

feat: SignerInterface type signers

b148622

refactor!: signer interface POC

605cec8

feat: check for sign capability in byDid

3b4c75e

chore: improve error reporting

9230dc5

chore: linting

90222b7

chore: remove all uses of SignCallback

efdb535

refactor!: replace GetStoreTxSignCallback with signer interface

74de98d

test: fix bundle tests

672e251

chore: make all signers params readonly

977fb43

feat: generic SignerInterface

7f45f80

rflechtner added 4 commits October 26, 2023 18:31

feat: allow overriding selectSigner(s) result type

b92155a

refactor: rename signer parameter keyUri to id

ec00441

chore: various review suggestions

edc234d

chore: rename byId to bySignerId

b39f32b

rflechtner force-pushed the rf-signers branch from f66d2d9 to b39f32b Compare October 26, 2023 16:39

rflechtner added 2 commits October 26, 2023 19:04

chore: improve an error message

8d93841

feat: use newest suites release

29d1534

* allows removing reimplementation signer factories * allows stronger typing of algorithm identifiers

rflechtner merged commit 88fb8c0 into vc-refactor Oct 31, 2023
11 of 14 checks passed

rflechtner deleted the rf-signers branch October 31, 2023 16:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor!: cryptosuite-style signers #801

refactor!: cryptosuite-style signers #801

rflechtner commented Oct 18, 2023 •

edited

Loading

ntn-x2 left a comment

ntn-x2 Oct 24, 2023

rflechtner Oct 24, 2023

ntn-x2 Oct 24, 2023

lukeg90 Oct 24, 2023

lukeg90 Oct 24, 2023

ntn-x2 Oct 24, 2023

rflechtner Oct 26, 2023

Dudleyneedham left a comment

Dudleyneedham Oct 24, 2023

rflechtner Oct 24, 2023

ntn-x2 Oct 24, 2023

rflechtner Oct 26, 2023

lukeg90 commented Oct 24, 2023

ntn-x2 left a comment

refactor!: cryptosuite-style signers #801

refactor!: cryptosuite-style signers #801

Conversation

rflechtner commented Oct 18, 2023 • edited Loading

fixes KILTProtocol/ticket#2962

TBD (possibly in a later PR)

How to test:

Checklist:

ntn-x2 left a 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

Dudleyneedham left a 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

lukeg90 commented Oct 24, 2023

ntn-x2 left a comment

Choose a reason for hiding this comment

rflechtner commented Oct 18, 2023 •

edited

Loading