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

Universal Wallet Conceptual Clarifications #46

Closed
kimdhamilton opened this issue Dec 5, 2020 · 21 comments
Closed

Universal Wallet Conceptual Clarifications #46

kimdhamilton opened this issue Dec 5, 2020 · 21 comments
Assignees

Comments

@kimdhamilton
Copy link
Collaborator

kimdhamilton commented Dec 5, 2020

Context

Reviewing the Universal Wallet spec vs source code, I identified some interesting differences in core wallet operations. Categorizing these differences led to some possible directions for improved clarity in the universal wallet spec.

There is already an issue tracking separation of the data model from the interface (or at least clarity on normativity status). This discussion sort of punts on that for now, pulling in data model and interface concepts in an attempt to gain clarity on what a wallet is.

Goals

Get feedback, develop a plan, and then submit more fine-grained issues/PRs.

Problem Statement

In source code, the Wallet interface is this:

interface Wallet {
  status: WalletStatus;
  contents: any[];
  passwordToKey: (password: string) => Promise<Uint8Array>;
  seedToId: (seed: Uint8Array) => Promise<string>;
  add: (content: any) => Wallet;
  remove: (contentId: string) => Wallet;
  lock: (password: string) => Promise<Wallet>;
  unlock: (password: string) => Promise<Wallet>;
  export: (password: string) => Promise<any>;
  import: (encryptedWalletCredential: any, password: string) => Promise<Wallet>;
  query: (map: any, reduce: any, initialValue: any) => any;
}

Note that some of these operations -- like lock, unlock, import, and export -- are present in the Universal Wallet (spec) interface, while others (e.g. add, remove) are not.

Following is a visual grouping of methods by:

  • In spec and code
  • just in code (not spec)
  • just in spec (not code)

pic1

The trailing comments are where I attempted to group operations by different types of wallet functionality. Some examples:

  • verify, issue, and prove are VC-related operations
  • Transfer is a cryptocurrency operation

This pointed to more general questions, such as:

  • Does it make sense to refine the universal wallet, and introduce clearer responsibilities and layers?
  • Which of the above operations make sense on all “wallets”, which can be abstracted into something that makes sense on all wallets, and which need to be a separate plugin/handler?

I also added spec Data Model elements in my consideration below.

Option 1ish: A more generic wallet

One path is to consider whether any abstractions covering credential/currency can be bubbled up to something that makes sense at the wallet level, and whether that’s even a worthwhile exercise (it may not be).

As a rough stab, I came up with a wallet managing "identities" and "assets".

  • Identities might be DIDs, crypto keys, or even logins
  • Assets might be money, VCs, etc

These are not clean abstractions because the examples do not fit neatly within.

pic1 5

Option 2ish: A simpler wallet

An alternative we can consider is whether it makes sense to extract any complicated methods like this from wallet, which would end up with different “brands” of wallet using the simpler wallet.

The first example shows a VC Wallet...

pic2

Vs a Currency Wallet...

pic3

Additional Thoughts

The Universal Wallet code already introduces a notion of plugins for EDV, did key, but where possible, I'd like to start building towards abstractions of these, e.g. storage plugins, key and DID plugins.

@OR13
Copy link
Collaborator

OR13 commented Dec 6, 2020

I have been leaning more towards option 2...

essentially any universal wallet plugin can define new data models, "json-ld objects" and use new methods to generate them, and then add them to wallet.contents.

@OR13
Copy link
Collaborator

OR13 commented Dec 6, 2020

because storing everything in a giant array probably won't scale, interfaces can be "read" or "write" oriented...

for example:

fancyWallet.getCredentialsMatchingMyCustomQuery => custom credentials list....

I would expect a plugin to define getCredentialsMatchingMyCustomQuery and allow it to "connect" to whatever is needed to resolve that request.

this list might then be used to to adjust the current wallet credentials, before a backup occurs, for example.

The main point being that functions are easier to work with than methods, so whenever possible a plugin should avoid calling wallet.contents directly... there are exceptions to that such as wallet.lock which needs to do that.

we could remove the concept of wallet.contents from all interfaces, and require it to be explicitly passed... that might make option 2 even better.

@NTonani
Copy link
Collaborator

NTonani commented Dec 7, 2020

I also endorse second option and see initial groundwork in support - #20.

Crucially, pluggability bifurcates the wallet, establishing clearer lines between core and non-core. A few benefits:

  • Empower community engagement and adoption
    • Allows for broader use and experimentation
  • Support iterative, evolving use cases, protocols, standards
    • Enables custom plugins with track toward formal support within spec / community
  • Encourages lean code bases within agents/clients
    • Potentially impactful for edge

because storing everything in a giant array probably won't scale, interfaces can be "read" or "write" oriented...

Agreed, and likely a third store-independent orientation - 'compute'.

@kimdhamilton
Copy link
Collaborator Author

I took a stab at sorting responsibilities. One thing I didn't highlight in my original comment was that I'm also starting to nudge towards abstractions for plugin groups; e.g. EDV Plugin conforming to a "Storage" interface (similar with did key plugin). I get that it may be premature, and that we may need to iterate on these over time. But interested in thoughts on the general direction.

wallet_simpler

Oh, also ignore the fact the "VC Wallet" box is separate for now. I'm still thinking through this.

@OR13
Copy link
Collaborator

OR13 commented Dec 11, 2020

I like the idea of pulling the storage interface out, the lock / unlock feature is currently connected too content so we would need to define lock / export in terms of query, and import / unlock in terms of add.

@sudeshrshetty
Copy link
Contributor

I am more inclined towards second option.

I just went through the spec & demo for the first time and please correct me if my understanding is wrong.

It is mentioned here that,

Interface functions are abstract, they take wallet contents and options as input, and they produce wallet contents and side effects as output.
This is bit confusing since it doesn't look like most of the interfaces need wallet contents as input.

Few of the interfaces in spec looks are more like JS oriented. Lets take an example of query. I am not sure how a wallet written in some other language like Golang or a REST based implementation of universal wallet can support arguments like map & reduce.

For example, if I have to add getCredentialsMatchingMyCustomQuery in my wallet which supports querying credentials using presentation exchange what should be the right approach?

  • Should I add/modify the properties in request model for query interface function?
  • or add new interface function for 'VC wallet'?

Shouldn't we also need predefined request-response models for each of the interfaces for more clarity and interop?

@OR13
Copy link
Collaborator

OR13 commented Jan 6, 2021

@sudeshrshetty certainly mosts of the interfaces are meant to be functions, map and reduce functions are convenient ways of handling JSON, but to they might not be needed at all, there could get more advanced query interfaces, for example encrypted data vaults queries are supported by the edv plugin.

If you want to create a new set of interfaces, I recommend you create a new plugin and define the functions in terms of objects, for example:

"send ethereum"

could be (web3, mnemonic, toAddress, fromIndex, amountWei) => ethTransaction

but it could also be (web3, toAddress, amountEth) => ethTransaction

"issue credential"

could be (vcTemplate, signingKey, format) => VC

Why functions?

They are safer from an interop perspective than assuming OOP or method oriented interfaces, for example, I could implement an "issueVc" interface of "keys"... then you could do the following:

const signingKey = await wallet.getKeyById("keyId");
const vc = await signingKey.issueVc(vcTemplate);

while this interface requires a signingKey class to be aware of every possible type of signature (including vanilla and capabilities)....

Imo its better to use functions, for example:

const signingKey = await wallet.getKeyById("keyId");
const vc = await wallet.issueVc(signingKey, vcTemplate);

But i could be convinced otherwise by a concrete proposal for how interfaces should be designed.

In general, I like functional typed interfaces, the actor pattern, etc...

I don't think these interface should be about network requests / response... i think it should be a set of abstract functions, if folks want to implement remote operation with did comm or grpc, how they features are exposed might be different.

My first objective was to explain how private keys are related to currency, capabilities and credentials, and DIDs... by using existing specs.

for example, the same did:key can be used to control ethereum, sign credentials and invoke capabilities. You might imagine constructing a wallet with these features with the builder pattern, for example:

const wallet = wallet.factory()
  .addPlugin(ethereum)
  .addPlugin(did)
  .addPlugin(capabilities)
  .addPlugin(credentials)
  
const txn = await wallet.sendEth(fromAddress, toAddress);  
const vc = await wallet.issue(vcTemplate);  
const capability = await wallet.delegateCapability(capability);  
const invoke = await wallet.invoke(capability);  

Shouldn't we also need predefined request-response models for each of the interfaces for more clarity and interop?

Yes, thats the idea by defining the functional inputs, outputs and side effects.

Do you have a preference for how the interfaces should be modeled?

@sudeshrshetty
Copy link
Contributor

sudeshrshetty commented Jan 6, 2021

@OR13 I like the approach of using functions.

My concern was regarding using functional inputs and outputs in interfaces. I was thinking about the scenario of interop with REST based wallet. May be I misunderstood the scope of interop here about supporting set of abstract interfaces.

A REST based wallet can not support 'query' interface from spec because of functional inputs parameters. It can support JSON request/response models.

@jgoodell2
Copy link

jgoodell2 commented Jan 6, 2021 via email

@OR13
Copy link
Collaborator

OR13 commented Jan 7, 2021

We had a chat about this, and the consensus was the wallet interfaces should be functions of JSON objects, and produce JSON objects.... this means that internally the function may convert the object to a class instance and then use it, but the function inputs and function outputs are limited to things that can be expressed in JSON... if you really want to write a function in JSON, you can... but I don't advise it :)

This issue now needs a PR which updates the spec to reflect this.

@sudeshrshetty
Copy link
Contributor

Thanks for the PR @OR13.

Is it a good idea to group interfaces in spec by plugins defined here?

Shall I bring back the 'query' interface by defining supported query types & query params.
For Ex: if a wallet supports JSON-LD framing

query({type:"QueryByFrame", dataQuery:{""}})

We can also introduce querying by presentation exchange here for VC wallets.

@OR13
Copy link
Collaborator

OR13 commented Feb 1, 2021

@sudeshrshetty yes, I think we should expose a top level "query by json" object, and then map that to specs.

@OR13
Copy link
Collaborator

OR13 commented Feb 1, 2021

Related to #52

I think we should provide full json documentation for the interfaces described by the spec, this will also help us align the storybook ui with the spec, instead of duplicating examples, we can link directly to the spec.

@sudeshrshetty
Copy link
Contributor

sudeshrshetty commented Feb 8, 2021

I will make a PR very soon

@sudeshrshetty
Copy link
Contributor

sudeshrshetty commented Feb 11, 2021

I liked the way @OR13 made a point about adding a data model & corresponding interface functions here

So I simplified the interfaces as below,

image

  1. Storage interface moved out of core wallet interfaces here. Adding/removing data models should be done through corresponding interface functions.

For example: If I am interested in implementing VC wallet only, exposing a generic add interface will force me to support any data models and also my query interface will have to support those data models.
Also this will apply to verifyRaw/signRaw interfaces too, where I can add/verify proofs through issue/prove/verify interface functions on data models I am interested in and I don't have to expose those generic interfaces.

  1. Also, remove, query interfaces can be wallet implementation specific.

sudeshrshetty added a commit to sudeshrshetty/universal-wallet-interop-spec that referenced this issue Feb 12, 2021
This update is based on discussions related to universal wallet
interfaces w3c-ccg#46 .
- added Query interface with query types `QueryByFrame` and
`PresentationExchange`.
- defined options in `Issue/Prove` interfaces.
- and few minor updates based on discussions in the issuer w3c-ccg#46.

Signed-off-by: sudesh.shetty <sudesh.shetty@securekey.com>
sudeshrshetty added a commit to sudeshrshetty/universal-wallet-interop-spec that referenced this issue Feb 13, 2021
This update is based on discussions related to universal wallet
interfaces w3c-ccg#46 .
- added Query interface with query types `QueryByFrame` and
`PresentationExchange`.
- defined options in `Issue/Prove` interfaces.
- and few minor updates based on discussions in the issuer w3c-ccg#46.

Signed-off-by: sudesh.shetty <sudesh.shetty@securekey.com>
sudeshrshetty added a commit to sudeshrshetty/universal-wallet-interop-spec that referenced this issue Feb 13, 2021
This update is based on discussions related to universal wallet
interfaces w3c-ccg#46 .
- added Query interface with query types `QueryByFrame` and
`PresentationExchange`.
- defined options in `Issue/Prove` interfaces.
- and few minor updates based on discussions in the issuer w3c-ccg#46.

Signed-off-by: sudesh.shetty <sudesh.shetty@securekey.com>
sudeshrshetty added a commit to sudeshrshetty/universal-wallet-interop-spec that referenced this issue Feb 16, 2021
This update is based on discussions related to universal wallet
interfaces w3c-ccg#46 .
- added Query interface with query types `QueryByFrame` and
`PresentationExchange`.
- defined options in `Issue/Prove` interfaces.
- and few minor updates based on discussions in the issuer w3c-ccg#46.

Signed-off-by: sudesh.shetty <sudesh.shetty@securekey.com>
sudeshrshetty added a commit to sudeshrshetty/universal-wallet-interop-spec that referenced this issue Feb 16, 2021
This update is based on discussions related to universal wallet
interfaces w3c-ccg#46 .
- added Query interface with query types `QueryByFrame` and
`PresentationExchange`.
- defined options in `Issue/Prove` interfaces.
- and few minor updates based on discussions in the issuer w3c-ccg#46.

Signed-off-by: sudesh.shetty <sudesh.shetty@securekey.com>
sudeshrshetty added a commit to sudeshrshetty/universal-wallet-interop-spec that referenced this issue Feb 16, 2021
This update is based on discussions related to universal wallet
interfaces w3c-ccg#46 .
- added Query interface with query types `QueryByFrame` and
`PresentationExchange`.
- defined options in `Issue/Prove` interfaces.
- and few minor updates based on discussions in the issuer w3c-ccg#46.

Signed-off-by: sudesh.shetty <sudesh.shetty@securekey.com>
@kimdhamilton
Copy link
Collaborator Author

@sudeshrshetty @OR13 reflecting on our discussion, I'm not sure that verifyRaw and signRaw belong in the list of wallet interface functions. Key seems much more like a supporting entity whose operations shouldn't necessarily bubble up to the wallet. As @sudeshrshetty points out, these operations will generally be wrapped by higher level ones like transfer or issue.

On that note, it might be worth distinguishing between auxiliary interfaces like Key and Store (which corresponds to Sudesh's "core" distinction above), vs interfaces providing distinct wallet functionality, e.g. enabling currency operations, VC operations, etc. Now that I think about it, perhaps this is where Sudesh was going with the use of "plugin" here. To help distinguish between that and the use of "plugin" in the reference implementation, I'll use "plugin interface" for what (I think) Sudesh is saying in that diagram.

Continuing down this rabbit hole, I wonder if terms Store vs Storage are causing confusion. There are two distinct store-like concepts that are interesting in the context of wallets -- one corresponding to "backing storage" and the other corresponding to the wallet as an entity that would participate in credential exchange ceremonies. If this concern doesn't make sense, please ignore. I will come back to it later.

Interested in your feedback for at least the first 2 paragraphs. This should give me enough fodder to break apart this issue into more actionable sub-issues, and finally close this.

@ChristopherA
Copy link

ChristopherA commented Mar 5, 2021

I don't know the spec, but signRaw sounds risky. In cryptocurrency wallets you don't want to ever allow arbitrary signing with keys, as it is an attack vector. The signer must parse what is being signed.

@kimdhamilton
Copy link
Collaborator Author

kimdhamilton commented Mar 7, 2021

Unfortunately I need to bring in "UML" (scare quotes intended).

wallet_interfaces

What we're calling "Key" and "Storage" (we can quibble over names) are "required interfaces" (as opposed to "provided interfaces"). The VC and Currency interfaces are more like wallet mixins.

The specific interfaces in the diagram are worth calling out because they are basic ones enabling our envisioned wallet use cases, but they are not necessarily the only ones of their types. Briefly:

  1. "Plugin interfaces" enable common functionality across wallet components.
  2. "Mixin interfaces" introduce new wallet capabilities that result in it behaving as a currency wallet, VC wallet, etc. They generally would also need to depend on plugin interfaces

In addition to providing new instantiations, implementors could introduce new interfaces of either category. We can (over time) iterate over discoverability of all of the above in some sort of wallet marketplace.

This distinction between "plugin interfaces" vs "mixin interfaces" (we can also quibble over these names) may be overly pedantic and we may not need to preserve this, but it's

@OR13
Copy link
Collaborator

OR13 commented Mar 8, 2021

I don't know the spec, but signRaw sounds risky. In cryptocurrency wallets you don't want to ever allow arbitrary signing with keys, as it is an attack vector. The signer must parse what is being signed.

As a developer, I DO want to sign raw, especially if I want to support multiple crypto currencies that require signatures over specific payloads, such as ethereum and bitcoin... a wallet interface should not assume your network...

and higher level interfaces are always built from lower level ones.

Its fine to warn a use not to use signRaw unless they know what they are doing... its terrible idea to block it... see WebAuthN signatures... impossible to use them for anything but auth... billed as a "feature" but makes the standard only useful for one thing, and therefore another standard is needed to do other things...

We need to be careful not to take away interfaces that are implied by plaintext private key visibility.... its a huge usability issue.

For example, IF I can see a P-256 curve private key, I CAN:

  1. ECDSA Sign
  2. ECDSA Verify
  3. ECDH deriveBits

If a wallet can see this key, a wallet plugin should support these operations...

https://nodejs.org/api/webcrypto.html#webcrypto_sign_and_verify

These interfaces are used to build the higher order interfaces of:

  1. JWS with ES256
  2. JWE with ECDH-ES+A256KW

These interfaces are in turn exposed in the yet higher order interfaces for:

  1. Issue Credential (VC-JWT / JsonWebSignature2020)
  2. Share Encrypted Data Vault Document

Sure the wallet does not need to expose 100% of the interfaces via a UI... but its a mistake to tell developers they are too stupid to use lower level interfaces when they need them, and especially bad idea to make a spec that supports only higher level interfaces... because there will be massive disagreement on the higher level interfaces, but lots of agreement on the lower level ones.

We should probably discuss conformance classes for wallets, and start with the lowest level... something like support for basic crypto operations like web crypto has.... and then build up to DID Wallets which only expose higher order interfaces like
transferCurrency and issueCredential... and don't even let a developer pick the crypto that is used to implement these interfaces.

@kimdhamilton
Copy link
Collaborator Author

kimdhamilton commented Mar 19, 2021

Finally trying to close this out. Some conceptual issues were clarified in other issues &PRs. There are 2 lingering issues that I opened to capture the remaining tasks

  1. Add section discussing interface grouping #70
  2. Add Add and Remove interfaces to spec #69

@OR13 @sudeshrshetty please let me know if you think there are any additional remaining tasks. Otherwise I think we can close this beast

@OR13
Copy link
Collaborator

OR13 commented Mar 22, 2021

I am going to close this issue, i think everything has been captured in smaller / more actionable issues.

@OR13 OR13 closed this as completed Mar 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants