multiformats / multicodec

Compact self-describing codecs. Save space by using predefined multicodec tables.
MIT License
336 stars 201 forks source link

Propose codes for signature algorithms #290

Closed Gozala closed 1 year ago

Gozala commented 1 year ago

As per #289 this pr proposes to register common signature algorithms used in UCANs.

Gozala commented 1 year ago

lgtm; some of the numbering choices seem a little arbitrary, do you want to think a bit more about their placement and spacing before proceeding?

Thanks @rvagg! I think @expede had a reasoning to choose specific codes, but sadly I can't seem to find the thread with an elaboration. Personally I have ok with arbitrary codes as long as we have them & personally would prefer smaller ones.

@expede mind considering @rvagg's feedback here before we proceed ?

rvagg commented 1 year ago

As per my comments above in the varsig entry, I'm not really clear on what the purpose of that entry is anymore, nor am I clear on why the byte forms are important for these. It doesn't seem to align with the proposal in #289 where we're doing varints, because the new comments here are about the importance of the bytes rather than the int codes. What am I missing?

expede commented 1 year ago

nor am I clear on why the byte forms are important for these [...] What am I missing?

@rvagg It makes them easier to remember for the humans using these codes. There's quite a lot of number punning in this list — for example, 0xec for ECDSA and 0xed for EdDSA.

where we're doing varints

My understanding (as is the one in did:key AFAICT) is that the codes in this table can be encoded many ways, including but not limited to varints. The signature use case for a "varsig" would be encoded as a varint, but when looking up in this table we don't use varint, and a memotic is potentially helpful

I'm surprised you're not going to make a claim for a single-byte range for this

Actually backing up — do you mean "why not register a single code in this table, and then have a separate standard for varint subcodes elsewhere?" If so, we had discussed that, but there appear to be ranges in this table for public keys, hashes, etc. But we could be wrong!

You're not really using it as a signal value to say "here follows a varsig", but a catch-all of some kind.

That's a good point. @Gozala I think this was in your original proposal in ucan-ipld — thoughts?

I'm not completely against just counting up from 0xd000,

@rvagg this really isn't an "over my dead body" -- if we're "holding it wrong", we can just update this PR to use 0xd000, 0xd001, 0xd002, etc (or something). You tell me!

Gozala commented 1 year ago

@expede @rvagg I have responded in https://github.com/multiformats/multicodec/pull/290#discussion_r981891960 but unless I"m misunderstanding / missing something we just need to decide between

  1. Short codes
  2. Codes that would map more closely with corresponding key codes

I personally think optimizing for size is probably better choice that optimizing for resemblance, that said I'm happy either way. But please let's move this forward as we're about to deploy code using this into production and it would be a lot more painful to change codes afterwards.

expede commented 1 year ago

@rvagg @gozala to grease the wheels if we decide to switch to short codes, here's #293

rvagg commented 1 year ago

Thanks for clarifying, I'm really just wanting to know that you have clarity about how these are going to be used and whether it maps to these entries because it sounded a little like there might have been a disconnect. I'm fine with this PR as long as you both agree and can see the path forward from merging this PR. @Gozala has access to merge and this has been approved. If you want to go with #293 then that's fine too. No objections from me on either.

Gozala commented 1 year ago

@expede codes in #293 still uses 3 bytes to encode so I'm not sure it provides any benefit. I think we either use 0x0d00, 0x0d01, ... codes and shave off 1 byte or we just land this as is.

Gozala commented 1 year ago

I've decided to merge this as 1 byte overhead seem negligible. If it becomes a problem we could always changes those codes as those are in draft status anyway