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

Remove multibase requirement from proofValue type #92

Closed
wants to merge 1 commit into from

Conversation

OR13
Copy link
Contributor

@OR13 OR13 commented Apr 16, 2023

Including this requirement forces implementers to bundle multibase which is not yet a standard.

it also forces them to bundle multformats which is not a standard.

Bundling base58btc and base64url and others...

I suggest leaving proofType a regular string, that is understood in the context of its associated cryptosuite.

In the future, when multibase becomes a standard, it seems wise to follow the "sorta convention" established by "did core", and call the multibase version of proof;

proofMultibase - https://www.w3.org/TR/did-core/#dfn-publickeymultibase

@OR13 OR13 requested review from msporny and mprorock as code owners April 16, 2023 20:39
@OR13 OR13 mentioned this pull request Apr 16, 2023
@mprorock
Copy link
Contributor

Approved, let's avoid down refs wherever possible

Copy link
Member

@msporny msporny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-1 for the following reasons:

  • The feature does not require "bundling multiformats", as @OR13 suggests.
  • The feature does not require downrefs to Multibase or Multiformats, as @mprorock suggests.
  • Removing this feature is actively harmful to auto-encoders, such as CBOR-LD, which relies on the multibase tag to convert the base-encoded text value to a binary value to save space (up to 33% for base64).
  • Removing this would be actively harmful for a variety of implementations that Digital Bazaar is doing for customers.
  • The Multibase work is currently in the process of being either 1) adopted at IETF via a mini-WG, or 2) going the ISE route (at which point we can provide a normative link).

Copy link
Contributor

@dlongley dlongley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-1 for the same reasons noted by @msporny above.

@msporny
Copy link
Member

msporny commented Apr 20, 2023

@OR13, given the discussion on the call yesterday, are you going to still pursue this PR, or are you comfortable now that what you were concerned about was not the intent of the spec text? If so, please close the PR. If not, please provide guidance on how the spec could change to convey what we discussed yesterday (ideally, by raising another PR).

@OR13
Copy link
Contributor Author

OR13 commented Apr 25, 2023

I'd like to see reference to base64url in the spec, here: https://w3c.github.io/vc-data-integrity/#multikey

As we discussed on the call, u +

examples for both keys and signatures.

@msporny
Copy link
Member

msporny commented Apr 25, 2023

I'd like to see reference to base64url in the spec, here: https://w3c.github.io/vc-data-integrity/#multikey

As we discussed on the call, u +

examples for both keys and signatures.

The normative requirements and the concrete examples for both keys and signatures happen in the cryptosuites:

https://w3c.github.io/vc-di-eddsa/#data-model

I expect the u Multibase value to be used in the vc-di-bbs cryptosuite.

Once they're in each cryptosuite, we might want to also mention them in https://w3c.github.io/vc-data-integrity/ ... but we might want to do that just before, or during CR. It could be argued to be an Editorial change since we're just moving normative content between a family of specifications.

My preference would be to have Multikey defined in the VC Data Integrity specification, but have been holding off on doing that until we have more people requesting that we consolidate the normative definitions.

If we can get more support from the WG to put just 'z' and 'u' Multibase definitions in VC Data Integrity, as well as the Multikey definitions in VC Data Integrity and just have the other specs refer to the core spec (but allow other cryptosuite definitions to extend/use different base and key formats, if they have good reason to do so), then that would be a good sign to consolidate.

In the very least, we need to know that people aren't going to object if we start defining that stuff more generally/normatively in VC Data Integrity.

@OR13
Copy link
Contributor Author

OR13 commented Apr 26, 2023

I'm going to close this, given the working group seems to have resolved to support multikey, and we are allowed to use both base58btc and base64url.

See the minutes related to #93

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants