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-0068 | Allow [* bounded_bytes] for images #809

Merged
merged 6 commits into from
Jun 2, 2024
Merged

CIP-0068 | Allow [* bounded_bytes] for images #809

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
544ee2a
cip-68 image update
nicholas-maselli May 6, 2024
28d11cf
bounded bytes on second uri
nicholas-maselli May 6, 2024
737d285
Update README.md
Crypto2099 May 6, 2024
bc440a6
version update
nicholas-maselli May 8, 2024
2460f9b
Merge branch 'cip-68-image-update' of https://github.com/NicholasMase…
nicholas-maselli May 8, 2024
54a354f
adding version commit
nicholas-maselli May 8, 2024
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
17 changes: 11 additions & 6 deletions CIP-0068/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,7 @@ metadata =
; A valid Uniform Resource Identifier (URI) as a UTF-8 encoded bytestring.
; The URI scheme must be one of `https` (HTTP), `ipfs` (IPFS), `ar` (Arweave) or `data` (on-chain).
; Data URLs (on-chain data) must comply to RFC2397.
uri = bounded_bytes ; UTF-8
uri = bounded_bytes / [ * bounded_bytes ] ; UTF-8
Crypto2099 marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not use bytes(*) instead of array(*) in the first place?

Copy link

@slowbackspace slowbackspace May 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

To add to @mmahut question:

Lucid does encode long strings as bytes(*) (using Data.to(new Constr(0, [Data.fromJson(metadata)...).

// excerpt from cbor.me of an image uri encoded as bytes(*) 5F tag
  5F                             # bytes(*)
            58 40                       # bytes(64)
               697066733A2F2F516D5543584D546376754A7077484633674142527236396365515232754547324673696B3943795768384D556F51766572796C6F6E67757269 # "ipfs://QmUCXMTcvuJpwHF3gABRr69ceQR2uEG2Fsik9CyWh8MUoQverylonguri"
            58 19                       # bytes(25)
               646566696E6974656C796D6F72657468616E36346279746573 # "definitelymorethan64bytes"
            FF                          # primitive(*)

Does the change proposed in this PR mean that array(*) should be use instead?

// excerpt from cbor.me of an image uri encoded as array(*) 9F tag
 9F                             # array(*)
            58 40                       # bytes(64)
               68747470733A2F2F6173736574317870743436647A336E39377979336C3937633035666A6C73727976357267677274306E6361372E6170652E6E667463646E2E # "https://asset1xpt46dz3n97yy3l97c05fjlsryv5rggrt0nca7.ape.nftcdn."
            58 40                       # bytes(64)
               696F2F696D6167653F73697A653D32353626746B3D506E307935746651766165654C6A4277764C646A506158684A322D445F735352664E4A6856386854336C6F # "io/image?size=256&tk=Pn0y5tfQvaeeLjBwvLdjPaXhJ2-D_sSRfNJhV8hT3lo"
            FF                          # primitive(*)

Copy link
Author

Choose a reason for hiding this comment

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

Both bytes() and array() should be fine. In both instances it splits the values up into 64 byte chunks.

Copy link
Author

Choose a reason for hiding this comment

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

A few people including myself assumed this was already part of the standard since it works for CIP-25 as well and many dapps already support the array format

Copy link

Choose a reason for hiding this comment

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

I am not good at reading CDDL, does [ * bounded_bytes ] allows for both indefinite byte string (5F) and indefinite array (9F) tags?

Since ref implementation is using lucid, which implements this using indefinite byte string (5F), shouldn't that be the preffered way?

Copy link
Collaborator

Choose a reason for hiding this comment

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

CDDL does not really have a concept of a "chunked, indefinite length string".

Essentially the expected behavior for a decoder would be to convert from an array into a concatenated string whether an indefinite byte string or an array of max-length 64 byte strings...

As Nick mentioned, this is how it has "always worked" since the early days of CIP-25 and my presumption is that most decoders are already accounting for this possible scenario. Adding it as an option to the spec I don't think warrants a version bump as I'm fairly confident we could find multiple instances of this already being used and supported even though not officially documented.

Copy link

@slowbackspace slowbackspace May 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

CDDL does not really have a concept of a "chunked, indefinite length string".

Ah, interesting and rather disappointing. Thanks for the insight.

Essentially the expected behavior for a decoder would be to convert from an array into a concatenated string whether an indefinite byte string or an array of max-length 64 byte strings...

As Nick mentioned, this is how it has "always worked" since the early days of CIP-25 and my presumption is that most decoders are already accounting for this possible scenario. Adding it as an option to the spec I don't think warrants a version bump as I'm fairly confident we could find multiple instances of this already being used and supported even though not officially documented.

What about other fields such as src and description. I would assume the same presumption applies?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fairly confident we could find multiple instances of this already being used and supported even though not officially documented (...)

The fact that various instance of this has been implemented doesn't mean we can break the standard.

The standard says the URI is of bounded_bytes and changing that breaks the standard.

For example, why Nick opened this PR in the first place is that Blockfrost honors this CIP (while other explorers do not). We are okay to implement this as CIP68v3.

; Custom user defined plutus data.
; Setting data is optional, but the field is required
Expand All @@ -200,7 +200,7 @@ extra = plutus_data
datum = #6.121([metadata, version, extra])
version = 1 / 2
version = 1 / 2 / 3
```

Example datum as JSON:
Expand Down Expand Up @@ -307,7 +307,7 @@ metadata =
; A valid Uniform Resource Identifier (URI) as a UTF-8 encoded bytestring.
; The URI scheme must be one of `https` (HTTP), `ipfs` (IPFS), `ar` (Arweave) or `data` (on-chain).
; Data URLs (on-chain data) must comply to RFC2397.
uri = bounded_bytes ; UTF-8
uri = bounded_bytes / [ * bounded_bytes ] ; UTF-8
; Custom user defined plutus data.
; Setting data is optional, but the field is required
Expand All @@ -316,7 +316,7 @@ extra = plutus_data
datum = #6.121([metadata, version, extra])
version = 1 / 2
version = 1 / 2 / 3
```

Example datum as JSON:
Expand Down Expand Up @@ -442,7 +442,7 @@ extra = plutus_data
datum = #6.121([metadata, version, extra])
version = 2
version = 3
```

Example datum as JSON:
Expand Down Expand Up @@ -550,7 +550,8 @@ version of these tokens from any point in time with the following format:
1. [6d897eb](https://github.com/cardano-foundation/CIPs/tree/6d897eb60805a58a3e54821fe61284d5c5903764/CIP-XXXX)
2. [45fa23b](https://github.com/cardano-foundation/CIPs/tree/45fa23b60806367a3e52231e552c4d7654237678/CIP-XXXX)
3. **Current**
3. [bfc6fde](https://github.com/cardano-foundation/CIPs/tree/bfc6fde340280d8b51f5a7131b57f4cc6cc5f260/CIP-XXXX)
4. **Current**
```

Each time a new version is introduced the previous version's link MUST be updated to match the last commit corresponding
Expand All @@ -573,6 +574,10 @@ versions of the affected tokens. `asset_name_labels` **MUST** only be marked obs

- Added new RFT asset class (444)

#### version 3

- Added [* bounded_bytes] support to the image and src tags on the metadata

## Rationale: how does this CIP achieve its goals?

Without separation of `reference NFT` and `user token` you lose all flexibility and moving the `user token` would be
Expand Down