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.

Sign up for GitHub

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

Jump to bottom

CIP-0030: update to api.signData() #148

Merged
merged 4 commits into from
Mar 16, 2022
Merged

CIP-0030: update to api.signData() #148

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0a0900d
CIP-0030: update to api.signData()
rooooooooob Nov 18, 2021
a748442
CIP-30 signData - add address protected header
rooooooooob Nov 19, 2021
a3ac380
CIP30: data sign add COSEKey to return value and change kid header
rooooooooob Jan 13, 2022
4dc5beb
CIP30 data sign: Use Address instead of cbor<address>
rooooooooob Feb 15, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 32 additions & 9 deletions CIP-0030/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,10 @@ The API specified in this document will count as version 0.1.0 for version-check

## Data Types

### Address

A string represnting an address in either bech32 format, or hex-encoded bytes. All return types containing `Address` must return the bech32 format, but must accept either format for inputs.

### Bytes

A hex-encoded string of the corresponding bytes.
Expand All @@ -37,6 +41,15 @@ A hex-encoded string of the corresponding bytes.
A hex-encoded string representing [CBOR](https://tools.ietf.org/html/rfc7049) corresponding to `T` defined via [CDDL](https://tools.ietf.org/html/rfc8610) either inside of the [Shelley Mult-asset binary spec](https://github.com/input-output-hk/cardano-ledger-specs/blob/0738804155245062f05e2f355fadd1d16f04cd56/shelley-ma/shelley-ma-test/cddl-files/shelley-ma.cddl) or, if not present there, from the [CIP-0008 signing spec](https://github.com/cardano-foundation/CIPs/blob/master/CIP-0008/CIP-0008.md).
This representation was chosen when possible as it is consistent across the Cardano ecosystem and widely used by other tools, such as [cardano-serialization-lib](https://github.com/Emurgo/cardano-serialization-lib), which has support to encode every type in the binary spec as CBOR bytes.

### DataSignature

```
type DataSignature = {|
signature:cbor\<COSE_Sign1>,
key: cbor\<COSE_Key>,
|};
```

### TransactionUnspentOutput

If we have CBOR specified by the following CDDL referencing the Shelley-MA CDDL:
Expand Down Expand Up @@ -93,7 +106,6 @@ DataSignErrorCode {
ProofGeneration: 1,
AddressNotPK: 2,
UserDeclined: 3,
InvalidFormat: 4,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}
type DataSignError = {
code: DataSignErrorCode,
Expand All @@ -104,7 +116,6 @@ type DataSignError = {
* ProofGeneration - Wallet could not sign the data (e.g. does not have the secret key associated with the address)
* AddressNotPK - Address was not a P2PK address and thus had no SK associated with it.
* UserDeclined - User declined to sign the data
* InvalidFormat - If a wallet enforces data format requirements, this error signifies that the data did not conform to valid formats.

### PaginateError

Expand Down Expand Up @@ -203,25 +214,25 @@ Errors: `APIError`

Returns the total balance available of the wallet. This is the same as summing the results of `api.getUtxos()`, but it is both useful to dApps and likely already maintained by the implementing wallet in a more efficient manner so it has been included in the API as well.

### api.getUsedAddresses(paginate: Paginate = undefined): Promise\<cbor\<address>[]>
### api.getUsedAddresses(paginate: Paginate = undefined): Promise\<Address[]>

Errors: `APIError`

Returns a list of all used (included in some on-chain transaction) addresses controlled by the wallet. The results can be further paginated by `paginate` if it is not `undefined`.

### api.getUnusedAddresses(): Promise\<cbor\<address>[]>
### api.getUnusedAddresses(): Promise\<Address[]>

Errors: `APIError`

Returns a list of unused addresses controlled by the wallet.

### api.getChangeAddress(): Promise\<cbor\<address>>
### api.getChangeAddress(): Promise\<Address>

Errors: `APIError`

Returns an address owned by the wallet that should be used as a change address to return leftover assets during transaction creation back to the connected wallet. This can be used as a generic receive address as well.

### api.getRewardAddresses(): Promise\<cbor\<address>[]>
### api.getRewardAddresses(): Promise\<Address[]>

Errors: `APIError`

Expand All @@ -233,13 +244,25 @@ Errors: `APIError`, `TxSignError`

Requests that a user sign the unsigned portions of the supplied transaction. The wallet should ask the user for permission, and if given, try to sign the supplied body and return a signed transaction. If `partialSign` is true, the wallet only tries to sign what it can. If `partialSign` is false and the wallet could not sign the entire transaction, `TxSignError` shall be returned with the `ProofGeneration` code. Likewise if the user declined in either case it shall return the `UserDeclined` code. Only the portions of the witness set that were signed as a result of this call are returned to encourage dApps to verify the contents returned by this endpoint while building the final transaction.

### api.signData(addr: cbor\<address>, sigStructure: cbor\<Sig_structure>): Promise\<Bytes>
### api.signData(addr: Address, payload: Bytes): Promise\<DataSignature>

Errors: `APIError`, `DataSignError`

This endpoint is due to be updated/finalized soon, see [discussion in the initial PR](https://github.com/cardano-foundation/CIPs/pull/88#issuecomment-954436243).
This endpoint utilizes the [CIP-0008 signing spec](https://github.com/cardano-foundation/CIPs/blob/master/CIP-0008/CIP-0008.md) for standardization/safety reasons. It allows the dApp to request the user to sign a payload conforming to said spec. The user's consent should be requested and the message to sign shown to the user. The payment key from `addr` will be used for base, enterprise and pointer addresses to determine the EdDSA25519 key used. The staking key will be used for reward addresses. This key will be used to sign the `COSE_Sign1`'s `Sig_structure` with the following headers set:

* `alg` (1) - must be set to `EdDSA` (-8)
* `kid` (4) - Optional, if present must be set to the same value as in the `COSE_key` specified below. It is recommended to be set to the same value as in the `"address"` header.
* `"address"` - must be set to the raw binary bytes of the address as per the binary spec, without the CBOR binary wrapper tag
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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,

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@rooooooooob rooooooooob Nov 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

Copy link

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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


The payload is not hashed and no `external_aad` is used.

If the payment key for `addr` is not a P2Pk address then `DataSignError` will be returned with code `AddressNotPK`. `ProofGeneration` shall be returned if the wallet cannot generate a signature (i.e. the wallet does not own the requested payment private key), and `UserDeclined` will be returned if the user refuses the request. The return shall be a `DataSignature` with `signature` set to the hex-encoded CBOR bytes of the `COSE_Sign1` object specified above and `key` shall be the hex-encoded CBOR bytes of a `COSE_Key` structure with the following headers set:

This endpoint utilizes the [CIP-0008 signing spec](https://github.com/cardano-foundation/CIPs/blob/master/CIP-0008/CIP-0008.md) for standardization/safety reasons. It allows the dApp to request the user to sign data conforming to said spec. The user's consent should be requested and the details of `sig_structure` shown to them in an informative way. The Please refer to the CIP-0008 spec for details on how to construct the sig structure.
* `kty` (1) - must be set to `OKP` (1)
* `kid` (2) - Optional, if present must be set to the same value as in the `COSE_Sign1` specified above.
* `alg` (3) - must be set to `EdDSA` (-8)
* `crv` (-1) - must be set to `Ed25519` (6)
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

* `x` (-2) - must be set to the public key bytes of the key used to sign the `Sig_structure`

### api.submitTx(tx: cbor\<transaction>): Promise\<hash32>

Expand Down