CIP-0025 inconsistencies

cardano-foundation / CIPs

Cardano Improvement Proposals (CIPs)

https://cips.cardano.org/

Creative Commons Attribution 4.0 International

517 stars 322 forks source link

CIP-0025 inconsistencies #303

Open klntsky opened 2 years ago

klntsky commented 2 years ago

I'm working on implementing CIP-25 in CTL. Here are some problems I encountered:

The general structure section contains something that looks like a loosely defined JSON schema, however, it's not clear why do we need to encode long strings as arrays in JSON, where there's no limit for string byte lengths. I think different schemas should be provided for Json and Plutus data (note that CDDL is not a good solution to define plutus data - despite there's little room for arbitrary decisions, it's still underspecified)
The version property in V2 defined as optional in the description, but required in CDDL. I think it should be required, because otherwise some metas can be decoded using both V1 and V2 and the contents would be different.
Since policy_id is ScriptHash, its length limit should be 28 bytes (not reflected in CDDL)
asset_name is TokenName and thus must be 0-32 bytes (not reflected in CDDL)

klntsky commented 2 years ago

CC @alessandrokonrad @KtorZ

klntsky commented 2 years ago

That's how we approached the differences in CTL:

Made JSON instance output long strings instead of limited-length chunks
Made version a required field in V2
enforced policy_id length to be 28 bytes
enforced asset_name maximum length of 32 bytes

gitmachtl commented 2 years ago

@klntsky What do you mean with "... there's no limit for string byte lengths." ? the metadata attached to a transaction still must be within the specs: https://github.com/input-output-hk/cardano-node/blob/master/doc/reference/tx-metadata.md Or do i get your question wrong?

klntsky commented 2 years ago

@klntsky What do you mean with "... there's no limit for string byte lengths." ? the metadata attached to a transaction still must be within the specs: https://github.com/input-output-hk/cardano-node/blob/master/doc/reference/tx-metadata.md Or do i get your question wrong?

I mean splitting the string in chunks is needed only for plutus data encoding (it's format-specific), but it does not make sense with JSON with no string length restrictions.

euonymos commented 5 months ago

Some wording should be improved I suggest:

In version 2 the the raw bytes of the policy_id are used.

Shouldn't there be "hex-encoded bytes"?

Also would be nice to mention somewhere in the document that v.2 isn't supported by major indexers like Blockfrost and Maestro.

SmaugPool commented 5 months ago

Shouldn't there be "hex-encoded bytes"?

No in version 2, it must be raw bytes, so actual binary bytes in the CBOR data. Hex encoded policy IDs as strings are not valid in version 2.

euonymos commented 5 months ago

@SmaugPool Thanks, I misplaced the initial quote, I intended to mention the analogous part for asset_name:

In version 1 the asset_name must be utf-8 encoded and in text format for the key in the metadata map. In version 2 the the raw bytes of the asset_name are used.

It looks like we are doing it the wrong way in CTL @klntsky, this might be the reason BF/Maestro were not able to read CIP25 v2 tokens minted. I will take a look later on.

SmaugPool commented 5 months ago

It looks like we are doing it the wrong way in CTL @klntsky, this might be the reason BF/Maestro were not able to read CIP25 v2 tokens minted. I will take a look later on.

It's the same for asset_name, only raw bytes are supported in version 2. A lot get this wrong indeed and use hex encoded strings which is not correct.

klntsky commented 3 months ago

@euonymos yes, and we patched it for one of the clients

rphair commented 3 months ago

@klntsky do you think it would help to talk about this at the next CIP meeting (Tuesday 06 August) to get some broader attention?