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

[Task] Change DID publicKey formats to publicKeyMultibase according to DID standard #296

Closed
3 of 9 tasks
JelleMillenaar opened this issue Jul 8, 2021 · 6 comments · Fixed by #443
Closed
3 of 9 tasks
Assignees
Labels
Enhancement New feature or improvement to an existing feature

Comments

@JelleMillenaar
Copy link
Collaborator

JelleMillenaar commented Jul 8, 2021

Description

Implement support for publickeyMultibase according to the new DID standard.

Motivation

Stay compliant with W3C standard

Resources

To-do list

Create a task-specific to-do list . Please link PRs that match the To-do list item behind the item after it has been submitted.

Change checklist

Add an x to the boxes that are relevant to your changes, and delete any items that are not.

  • The feature or fix is implemented in Rust and across all bindings whereas possible.
  • The feature or fix has sufficient testing coverage
  • All tests and examples build and run locally as expected
  • Every piece of code has been document according to the documentation guidelines.
  • If conceptual documentation (mdbook) and examples highlighting the feature exist, they are properly updated.
  • If the feature is not currently documented, a documentation task Issue has been opened to address this.
@JelleMillenaar JelleMillenaar added the Enhancement New feature or improvement to an existing feature label Jul 8, 2021
@l1h3r
Copy link
Contributor

l1h3r commented Jul 9, 2021

The publicKeyMultibase property is OPTIONAL. This feature is non-normative.

I don't really want to rush to make this change. It seems it was added to the standard fairly recently and is still being debated in various GitHub issues over at did-core.

We should still update the version of the DID standard but preferably to a more notable release like the previous Candidate Recommendation from March 18.

After updating to this version we could include the publicKeyJwk field which is included in the later versions and doesn't appear to be at risk like publicKeyBase58 was.

Thoughts?

@cycraig
Copy link
Contributor

cycraig commented Jul 9, 2021

While the publicKeyMultibase feature is non-normative it has still supplanted publicKeyBase58 (which was also non-normative) in the did-core specification, which we use in various places. The main discussion at w3c/did-core#707 seems conclusive in that at least. The aim in the change seems to be future extensibility for new bases without having to revise the supported publicKey<base> values for each one added.

I am of the opinion that we should add support for publicKeyMultibase now, in addition to publicKeyBase58 for temporary backwards compatibility, and phase out uses of the latter over time until we can remove it with a major release, e.g. 1.0.
The reason I prefer a phased removal is because the did-core specification still retains several examples using publicKeyBase58 (whether intentionally or erroneously) so it is unclear to me if we should still support it in the future. Once their examples have been updated I would be happy to push for removing it from the codebase entirely.

After updating to this version we could include the publicKeyJwk field which is included in the later versions and doesn't appear to be at risk like publicKeyBase58 was.

Agreed, publicKeyJwk is normative so it takes a higher priority. It also needs its own issue that should be added to the list in #196.

@cycraig
Copy link
Contributor

cycraig commented Jul 9, 2021

Regarding implementing publicKeyMultibase:

The multibase crate seems lightweight and supports all the encodings on the Multibase draft specification.

It runs on the wasm32-unknown-unknown target and has relatively few dependencies, of which syn and data-encoding already exist in our dependency tree.

multibase v0.9.1
├── base-x v0.2.8
├── data-encoding v2.3.2
└── data-encoding-macro v0.1.12
    ├── data-encoding v2.3.2
    └── data-encoding-macro-internal v0.1.10 (proc-macro)
        ├── data-encoding v2.3.2
        └── syn v1.0.73 (*)

It also shows similar performance to the bs58 crate currently used for publicKeyBase58. Average timings from Criterion benchmarks when processing random Ed25519 public keys to/from Base58-Bitcoin:

bs58 multibase
encode 1247 ns 683 ns
decode 590 ns 462 ns

An alternative crate is multiformats. However, it has unnecessary features (for our use-case) and dependencies as it implements hashing algorithms and other codecs in addition to Multibase, so it is overall less suited.

Given that the crate seems to have low overhead, and that Multibase specifies 22 encodings, I am of the opinion that we should simply use the multibase crate. Other alternatives such as using multiple individual crates for each base or implementing a subset ourselves are less attractive options and I don't see any advantages other than avoiding the extra dependency.

Aside: one thing to note is that we cannot support the identity (0x00) base since arbitrary bytes from a key cannot be represented as a UTF-8 string in general. Since publicKeyMultibase is specified as a UTF-16 string, it shouldn't ever be a problem.

@cycraig cycraig changed the title [Task] Change DID publicKey formats to "publickeyMultibase" according to DID standard [Task] Change DID publicKey formats to publicKeyMultibase according to DID standard Jul 13, 2021
@cycraig cycraig linked a pull request Jul 14, 2021 that will close this issue
10 tasks
@cycraig cycraig removed a link to a pull request Jul 14, 2021
10 tasks
@cycraig
Copy link
Contributor

cycraig commented Jul 14, 2021

Link to PR marking publicKeyMultibase as at-risk in the did-core specification: w3c/did-core#772

@JelleMillenaar
Copy link
Collaborator Author

@cycraig This can be closed, right?

@cycraig
Copy link
Contributor

cycraig commented Sep 23, 2021

While the multibase functionality is complete, I believe we still default to publicKeyBase58 instead of publicKeyMultibase in several components and examples, such as verification methods.

Whether or not to remove publicKeyBase58 entirely for some/all components is also still up for debate, but several examples in did-core still use it so I'm leaning towards keeping it for backwards compatibility.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement New feature or improvement to an existing feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants