CIP-0030: update to api.signData()

[rooooooooob]

The current specification of signData() suffers from several issues:

1. There is no way to get the verification keys to verify the signature
   returned without prior knowledge
2. The specification did not completely say what would be returned, if
   it was merely hex-encoded bytes of the signature or what.
3. The COSE_Sign1/COSE_Sign object would need to be constructed
   identically on both the wallet and dApp side which was not covered by
   the spec

This update should address these as for 1) the COSE_Sign1 object returned is specified to contain in the kid header the verification key. 2) is resolved as well as we no longer have untyped bytes, and 3) is also resolved by nature of explicitly returning it.

The endpoint as a result is simpler and does not cover more complex
CIP-0008/COSE situations but these are likely not needed for dApps, and
if they ever are, it would be better to simply add in another endpoint
to cover them as this should cover the standard case for verifying
ownership of an address(and associated payment key) in a simpler way.

Other alterntives were previously discussed in the original CIP-0030 PR:
#88

[rooooooooob]

@alessandrokonrad @SebastienGllmt

[rooooooooob]

This shouldn't be necessary anymore as the user only provides the payload, not the entire Sig_structure. Not providing proper hex bytes falls under the broader APIError.InvalidRequest realm

[rooooooooob]

Do we want to directly store the Ed25519 pubkey bytes here? Or do we want to create a COSE_key structure and put that here instead? We're only supporting Ed25519 for the endpoint signing so context should be enough.

Do we need any other headers or should these be enough?

[rooooooooob]

One argument for COSE_key is that while in this situation it's not ambiguous at all as we only support Ed25519, without it someone looking at the produced COSE_Sign1 object only knows that it's EdDSA, but doesn't know which curve is used so I guess they could technically think it's Ed448. This goes doubly for people using general-purpose COSE libraries to handle CIP-8. I don't think CIP-8 explicitly mandates the 25519 curve either (but does mention the address relation but it's optional in the spec), but maybe it should? CIP-8 is still in draft status so this might be the first time there's a major use of it

[KtorZ]

Seems like the right place for keys would be a COSE_key object indeed. I don't think that taking shortcut (by shoving the key as a kid) gives any benefits. Rather, the kid could be the address itself that was used initially to determine the signing key (using the key itself or key hash also work just fine...) identifying a key object in the key map. This makes it much more future proof in case where other type of signing algorithm are introduced later.

[rooooooooob]

I added an explicit COSE_Key return. I think using the address in the kid value is a good idea, however it is making me think about CIP-8 which has its own dedicated header for storing the address since now we'd have two headers with the same value. @SebastienGllmt what do you think of updating CIP-8 to just mandate that the kid header be repurposed for that? Or do we want do leave it open for others to be able to use whatever they want for that instead?

[rooooooooob]

Do we want to bump the API version from 0.1.0 to 1.0.0 (since there's a breaking API change) or will we only start that once the CIP is in Active or Proposed instead of Draft stage?

[rooooooooob]

@SebastienGllmt this is what I understood from the CIP-8 spec but it doesn't explicitly say which format to use.

here's the quote:

For P1, the mapping of public keys to addresses has two main cases:

1. for Shelley addresses, one payment key maps to many addresses (base, pointer, enterprise)
2. for Byron addresses, chaincode and address attributes need to be combined with the public key to generate an address

To resolve this, one SHOULD add the full address to the protected header when using a v2 address. The v2 addresses contain the chaincode + attributes and we can verify the address matches combining it with the verification public key.

? address: bstr,

[alessandrokonrad]

What does "without the CBOR binary wrapper tag" mean? So in case of the serialization-lib, when I convert the addres to bytes, would that be correct or does that contain any CBOR tag?

[rooooooooob]

I wanted to write it without referring to any specific library. There are a couple crypto-related things in cardano-serialization-lib that have their to_bytes() have just some raw binary buffer for cases where people often are importing from non-CBOR contexts the bytes, whereas others will encode them in CBOR like BYTES(len) then the actual buffer. In the binary spec you have them all as just bytes which includes the tag though.

EDIT:
The Address struct in cardano-serialization-lib when using from_bytes() will use the raw-bytes format, without making it CBOR (e.g. 1-2 bytes CBOR bytes tag/len followed by 29 or 57 bytes for the address). It will just be those 29 or 57 bytes on their own as raw binary which I think is what CIP-8 wants but I'm not 100% sure since it just says bstr. Those bytes would then be, inside of the serialized CBOR wrapped in their own CBOR.

So on that point, with your code like

protectedHeaders.set_header(
    Loader.Message.Label.new_text('address'),
    Loader.Message.CBORValue.new_bytes(Buffer.from(address, 'hex'))
);

it would be an issue (CIP-8/CIP-30-wise) as due to how CIP-30 defines cbor\<address> it would need to include the CBOR bytes tag - but then that would make cardano-serialization-lib's Address::from_bytes() fail since it is one of those structures that due to the to/from bytes being designed primarily with interop with non-CBOR things (because it's just raw bytes like a hash or pubkey or address) rather than just as something that is a more complex CBOR structure that didn't already have a binary format on its own outside of the CBOR spec. On the other hand if you already had something like

let someAddress = Address.from_bech32("addr1u8pcjgmx7962w6hey5hhsd502araxp26kdtgagakhaqtq8sxy9w7g");
protectedHeaders.set_header(
    Loader.Message.Label.new_text('address'),
    Loader.Message.CBORValue.new_bytes(someAddress.to_bytes())
);

It would be fine (CIP-8/CIP-30 wise). We're going to have to take a second look at any hash/address/key in the CIP-30 spec now.

[rooooooooob]

I made an issue in the serialization lib to discus this Emurgo/cardano-serialization-lib#263

[alessandrokonrad]

let someAddress = Address.from_bech32("addr1u8pcjgmx7962w6hey5hhsd502araxp26kdtgagakhaqtq8sxy9w7g");
protectedHeaders.set_header(
Loader.Message.Label.new_text('address'),
Loader.Message.CBORValue.new_bytes(someAddress.to_bytes())
);

How is this different? You use to_bytes() here also, which gives you the same result as when just taking the address directly in hex/bytes?

it would be an issue (CIP-8/CIP-30-wise) as due to how CIP-30 defines cbor<address>

I think many assume here it's simply the address in hex format the cardano ledger takes. If it's not truly cbor, then we should change the definition in CIP-30.

[KtorZ]

About addresses:

API consistency is better indeed so, if the rest already works with Base16 then let's stick to it. CBOR here is bonkers though.

Alternatively, it wouldn't be much more complicated to support multiple encoding, defaulting to Base16 or Bech32, whatever makes the most sense.

About the payload:

Well, the payload isn't necessarily a valid UTF-8 byte sequence, so it's only waiting for errors to use a plain string here.

Of course, wallets will want to present the information to users but I doubt they would present the full payload anyway since it's mostly gibberish from a user standpoint. Rather, wallets can and shall, whenever possible, extract information that are relevant to show for approval.

[mkazlauskas]

For consistency I would say. In CIP-0030 we return everything in bytes/hex. So when you grab the address there, you can directly insert it in that form into the api.signData endpoint.

What is the value of using CBOR elsewhere besides implicitly referring to ledger specs (and not having to update CIP when the specs are updated)? I think dApp developer UX is extremely important and it would be better with javascript object-based API.

[rooooooooob]

@KtorZ

API consistency is better indeed so, if the rest already works with Base16 then let's stick to it. CBOR here is bonkers though.

Yeah the CBOR thing was more of that is how it is technically defined right now by nature of referring to the ledger binary spec, but I agree it makes more sense to change to just raw bytes (encoded by hex).

Well, the payload isn't necessarily a valid UTF-8 byte sequence, so it's only waiting for errors to use a plain string here.

I believe this was a mistake, it was meant to be a hex-encoded string type, to be consistent with CIP-0008 (message signing spec).

Alternatively, it wouldn't be much more complicated to support multiple encoding, defaulting to Base16 or Bech32, whatever makes the most sense.

What about for returns? It's easier to accept multiple encodings in parameters but it becomes weird for return types. If you're always returning a specific one (bech or hex) so that users can easilly decode it, it would make usage with the other of the two less consistent imo.

@mkazlauskas

What is the value of using CBOR elsewhere

CBOR encoding is fairly ubiquitous in the Cardano ecosystem and it has support with various tooling already in a standardized way. If we had our own JSON encoding for everything here (which would amount to a lot of types once you think about how complciated some of these types are) you would still need to code conversions between that and CBOR (or worse, to other tool-specific encodings/constructions) in order to use other libraries together with CIP-30, unless you build some monolithic library that takes care of everything you could possibly need all within this CIP-30 specific format. The way it's defined here you can just call YourLib.Transaction.fromCborBytes(bytes); and everything is fine. I could see if there were some standardized JSON format for all these types that already existed, that we could use that instead. This was the reason we used cbor encoded bytes here at least despite the EIP-0012 (dapp connector spec for the Ergo blockchain) that this was initially based on we used JSON as that had a fairly standardized format there.

[mkazlauskas]

you would still need to code conversions between that and CBOR (or worse, to other tool-specific encodings/constructions) in order to use other libraries together with CIP-30

The way it's defined here you can just call YourLib.Transaction.fromCborBytes(bytes);

Do you have any specific tools in mind? Using json schema for signTx/submitTx would remove dependency on cardano-serialization-lib for most dApps, which I think is great as it makes dApp development easier.

I could see if there were some standardized JSON format for all these types that already existed, that we could use that instead.

cardano-js-sdk has types we could use as a reference. It also provides utils for type conversion between those types and cardano-serialization-lib

[rhyslbw]

Using json schema for signTx/submitTx would remove dependency on cardano-serialization-lib for most dApps, which I think is great as it makes dApp development easier.

..and reduces the application's size by around 2MB

[crptmppt]

@alessandrokonrad / @rooooooooob is this ready for review / processing?

[rooooooooob]

@crptmppt I think the general idea is here but there are still a couple smaller details that need to be decided on the format for the key if we want a whole COSE_Key object or just like raw bytes of Ed25519 (see the possible ambiguity argument). It's possible we might want to update CIP-8 to disambiguate some things as well such as the address format in that relation. I assume it's the 29 or 57 bytes as specified in the comments of the binary spec but without any CBOR tags at all but it doesn't mention the format explicitly.

[KtorZ]

Wouldn't an ArrayBuffer be more suitable than a raw String for the payload? A string supposes that it is encoded in some form, whereas this is more likely some raw byte string which is not necessarily representable as a valid UTF-8 sequence.

[SebastienGllmt]

Message passing only supports JSON-serializable objects which ArrayBuffer isn't

[mkazlauskas]

Message passing only supports JSON-serializable objects which ArrayBuffer isn't

I don't think that's a constraint for the API - injected script can do serialization/deserialization.

[rooooooooob]

I don't think that's a constraint for the API - injected script can do serialization/deserialization.

Even if it weren't for this it is potentially worthwhile to maintain a JSON-representable (e.g. very widely supported, not specific to any tech) API as this protocol could get new extensions e.g. something like wallet connect and those could have similar restrictions. Although the representation of bytes as hex string could be specific to those extensions.

[KtorZ]

Seems like the right place for keys would be a COSE_key object indeed. I don't think that taking shortcut (by shoving the key as a kid) gives any benefits. Rather, the kid could be the address itself that was used initially to determine the signing key (using the key itself or key hash also work just fine...) identifying a key object in the key map. This makes it much more future proof in case where other type of signing algorithm are introduced later.

[KtorZ]

Note that, addresses are one of the few things in the entire Cardano binary objects that are NOT CBOR-encoded (except Byron address, but let's ignore those for this specification -- although the spec should perhaps mention this decision!).

One thing I don't quite understand here is: why even use bytes as an interface here? Shelley addresses are typically presented and stored as bech32 strings. While it is very much likely that a wallet implementing CIP-0030 would support bech32 encoding/decoding for its own sake, it's less likely that a dApp would. Or at least, requiring bytes here pushes that constraint on dApp developers.

I'd argue that we should use plain string as bech32 for this interface (for address), and as stated above however, use an ArrayBuffer for the payload.

[rooooooooob]

I'd argue that we should use plain string as bech32 for this interface (for address), and as stated above however, use an ArrayBuffer for the payload.

Do we prefer bech32 over base16 bytes for addresses then?

The arraybuffer is an overreaching API thing across the entire spec not just with this parameter. I think that if we do something like that then we would want to potentially update anywhere else bytes are used. What about the cbor bytes? Shall those be a byte buffer of CBOR bytes rather than a hex string representing the CBOR bytes?

There's that issue @SebastienGllmt mentioned above with there being restrictions in message passing to JSON-only, but as @mkazlauskas mentioned this could be translated within injected code so that whatever method things go from there to the wallet it could be converted beforehand. It's an interesting thought, but I've mentioned before that it can be useful to have the entire API be JSON-compliant, but I guess it depends on how much it degrades the dApp dev experience or not by enforcing that.

What are your thoughts @alessandrokonrad ?

[alessandrokonrad]

Do we prefer bech32 over base16 bytes for addresses then?

Would be fine for me. Only downside is consistency, but definitely easier for a developer to work with bech32 encoded addresses.

[KtorZ]

I wasn't aware of the limitations of the browser API regarding the JSON format. It's unfortunate. Sticking to base16-encoded strings for most bytes payload seems then reasonable.

For addresses however, moving to bech32 really seems like a natural move though I agree this would have to be consistent across the entire API. It can be introduced in a backward-compatible manner though, by having wallets support both format -- at least for a while. Since the bech32 prefix for addresses is known, it is quite trivial to select the right decoder / convert the address to the format of choice of the wallet by looking at the first few characters of the encoded strings (if (addr.startsWith("addr_") ...).

@rooooooooob what do you think?

[rooooooooob]

it seems more people are in favor of using bech32 for addresses. Could we update that in this PR or should we have a separate PR for that? It would affect other endpoints besides just this one.

[alessandrokonrad]

Just to double check is Ed25519 (6) correct here. The message-signing library returns 5 for CurveType.Ed25519. I don't know which is correct, but just want to bring this up in case there is something wrong.

[rooooooooob]

The message-signing library returns 5 for CurveType.Ed25519

@alessandrokonrad

You should open an issue in the message signing library if that is the case. It should be serializing it as 6 in the CBOR when converting from the enum to Label, but it's possible that if you convert it into a number implicitly that it will give you the ordering starting from 0 and increasing by 1. We had to do something weird like this since negative enums aren't supported by wasm_bindgen but a lot of the COSE labels are negative. It's been a while since I've looked at the code but it's possible we could hide the internal enum even more and have the only way to access any value whatsoever from it be a to_label function so implicit conversions like this don't happen. I don't think many people besides you have used the library yet so there's bound to be some oddities and things that need fixing/changing/etc. That should change once we update this data sign endpoint though. Thanks for pointing this out.

enum definition: https://github.com/Emurgo/message-signing/blob/master/rust/src/builders.rs#L151
macro it uses: https://github.com/Emurgo/message-signing/blob/master/rust/src/utils.rs#L82

I can take a look into this on Monday and maybe change the macro we were using to define those enums. IIRC the idea was you'd be using those enums with the stuff in builder.rs and not directly as their number values that wasm_bindgen gives them in JS.

I would recommend using the builder::EdDSA25519Key class to construct this COSE_Key as it should simplify almost all of this.

[alessandrokonrad]

Thanks @rooooooooob

I would recommend using the builder::EdDSA25519Key class to construct this COSE_Key as it should simplify almost all of this.

That is very useful, I just tried it out, however it also returns 5 forEd25519 😅. This is what I get:
{1: 1, 3: -8, -1: 5, -2: h'D058ED6FA27CA8FA454F1BAF2AAC58EDB774531DD2813E7BC91600D3CA8F0D4B'}

I opened up an issue here on message-siging lib: Emurgo/message-signing#9

[rooooooooob]

It can be introduced in a backward-compatible manner though, by having wallets support both format

@KtorZ what do you suggest for returns? We could easily accept both, but should we return bech32 or the old format?

[KtorZ]

I'd use bech32 all-the-way, it is straightforward to convert from bech32 to hexadecimal and vice-versa anyway.

[KtorZ]

@rooooooooob any update on that one ?

[rooooooooob]

@KtorZ I think it's read for review now. I just updated the spec to use an Address string type which can be either the old hex bytes or bech32. I think that's is it for comments on this besides out-of-scope large changes like not using cbor anywhere mentioned above.

I changed the Address format in other endpoints for consistency, but we can do that in another PR if we feel it is necessary. There's also the issue of the API version. Should we be incrementing this by a major version here or do we only want to start doing that once CIP-30 moves out of draft status?

[mkazlauskas]

How about adding a "content type" argument? Wallets could try to display the message being signed to the user.

[KtorZ]

cc @SebastienGllmt @rphair

[rphair]

As I remember from today's meeting this was OK with everyone, but don't remember if we resolved that @SebastienGllmt should check before merging.