CIP-0068 | Improve image types and URI descriptions #562

SmaugPool · 2023-07-26T08:58:16Z

This patch tries to improve 222, 333 & 444 media types and URIs descriptions.
It also proposes to remove the optional top level mediaType in 222 & 444 for the following reasons:
- because it is optional, it cannot be relied on and is therefore basically useless at best
- it has actually been counterproductive in CIP 25 as some users have minted NFTs with video/... or model/... media (mime) type for image URI, ignoring the image/* constraint (I will propose to remove it too in CIP25 if this is accepted here)
It adds the Arweave scheme (ar://) in the list of supported URI schemes. I wonder though if the CIP should have such a hard list of supported schemes or just suggestions?
It improves the References section and uses links instead of text

PS: Not sure how to avoid the duplication of information in each label. It would also be nice to link to related information in CIP25 about URIs and data URLs but not sure how.

rphair

I don't work on these implementations but this looks to me like it clarifies the use of URIs & images and I think it would be an improvement. As soon as we can agree on whether or not a version bump for 222 and 444 is needed I'm happy to sign off on this.

PS: Not sure how to avoid the duplication of information in each label.

The only portable way I know of would be to put it in a separate file... but people would expect that for a schema and not for written text.

Maybe someday it will work better as a schema?
Could there be any schema format that's even somewhat as human readable as what's there now?

It would also be nice to link to related information in CIP25 about URIs and data URLs but not sure how.

As it stands now, not even section anchored links (e.g. #Abstract) between CIPs are portable. Since line numbers will also change, I think the only approach now is to refer to statements in other CIPs literally (e.g. see CIP-XXXX > Heading > Subheading > quote).

This deficiency has been around since #109 and first we are trying to fix the raw text of all CIPs via #389 before someone can really work on better scripts for the web sites derived from GitHub source.

@SmaugPool you have my ✔️ on this if we can find developers commenting here who say unanimously that we don't need a version bump because the dropped parameter. Otherwise I think we could sign off on it right away if you add the version bumps.

The editors have agreed to be cautious here to avoid having to do something like #523 again in a hurry (amazingly this PR is still not merged; so any bumps here would have to be done relative to those bumps 😜). cc @KtorZ @Ryun1 @mmahut

CIP-0068/README.md

rphair · 2023-07-26T19:24:55Z

CIP-0068/README.md

 metadata = 
  {
    name : bounded_bytes, ; UTF-8
-    image : bounded_bytes, ; UTF-8
-    ? mediaType : bounded_bytes, ; UTF-8


(potentially requiring version bump to 2 or 3)

image is not removed, and mediaType was optional, so I'm not sure of the benefit of bumping the version?

Because officially now including a mediaType would be erroneous, while it wouldn't have been an error before whether it was missing or present. How such an error would be handled I believe would depend upon the implementor: so we would be giving them that option by bumping the version. See problem reported in #520.

It would not be erroneous because additional properties are allowed:

; ... Additional properties are allowed

thanks @SmaugPool - I missed that! 🤓 @Ryun1 we can talk about any other reasons why we might need the version bump @ the CIP meeting today, but it seems like we are OK without one (cc @KtorZ).

Crypto2099 · 2023-07-27T15:06:37Z

It also proposes to remove the optional top level mediaType in 222 & 444 for the following reasons:

because it is optional, it cannot be relied on and is therefore basically useless at best

it has actually been counterproductive in CIP 25 as some users have minted NFTs with video/... or model/... media (mime) type for image URI, ignoring the image/* constraint (I will propose to remove it too in CIP25 if this is accepted here)

I'm not sure I understand the rationale to remove an optional top-level mediaType. If anything, I believe it would be more helpful to refine and iterate the version of the standard passing in a well-formed object for all files such as:

file-ref = [
  media-type,
  uri
]

image-file-ref = [
    image-media-type,
    uri
]

image-media-type = 'image/jpeg' | 'image/png' | 'image/svg+xml'

We cannot write standards with the expectation that we will have to make exceptions because people will incorrectly implement the standards. We should write standards in such a way that if it is followed correctly, all the relevant and pertinent information a user might need will be where they expect it to be for ease of integration.

It adds the Arweave scheme (ar://) in the list of supported URI schemes. I wonder though if the CIP should have such a hard list of supported schemes or just suggestions?

I think it makes sense that the standard should define which URI schemes should be supported. There are other schemes (ftp:, network:, etc) that would pass validation as a "valid URI" but would most likely break all integrations due to lack of support.

SmaugPool · 2023-07-27T21:36:52Z

I'm not sure I understand the rationale to remove an optional top-level mediaType.

The rationale is that I'm aware of 0 use case where it has been useful, and that's because it's already constrained to image/*, and is optional so implementations should work without anyway (contrary to the one in files). On the other hand I'm aware of several cases where it has been misused leading to broken NFTs (with CIP25 but it's the same).

Therefore I think we should either remove it or make it mandatory. Browsers and most image viewers are able to detect most image types, so I'm not sure it's really useful to make it mandatory as long as only images are supported.

If we worry about exotic image formats and think it could be useful in this case, an alternative could be to list explicitly supported image formats.

Admittedly I may be biased by my own experience, so if anyone is aware of a single real-world use case where it has proved useful, please share it.

rphair

I believe the question of versioning (i.e. not strictly required) has been answered in #562 (comment) ... the guideline ... Additional properties are allowed effectively makes any property OK to use, so I now fully see @SmaugPool's point that having a property in this short list strongly encourages its use and, according to the OP's argument, its misuse.

So the remaining question as per #562 (comment) is whether that misuse should be addressed here in this PR:

This could be out of scope for this PR, but it would really be confusing to delete this property and then have to reintroduce it later to add a more rigorous definition as @Crypto2099 suggests above. That would be item # 2 to discuss at the CIP editors' meeting today.
Item # 1 will still be to call for any other motivation for bumping the version number (in my opinion we are OK without it but we need to be sure).

The definition question is beyond my technical scope as a reviewer so in the meantime I'm approving it here and will leave further reviews to reflect practical experience about current & potential use cases of mediaType.

SmaugPool · 2023-08-01T13:16:40Z

Note that my initial motivation to clarify the image mediaTypesituation is that CIP68 currently does not restrict it to image/* in 222 and 444 labels. I believe it was @alessandrokonrad goal to mimic CIP25 metadata structure and keep this restriction but I will let him confirm.

Without this important restriction and with the field being optional, any file type could theoretically be linked from image, potentially without any mime type indicated, making it very difficult to render them. It could be argued that the image field name is clear enough to restrict types but past experience showed it's not.

(Note: I have updated the patch to clarify the image media type restrictions for 222, 333 and 444).

…URIs

rphair · 2023-08-01T17:35:11Z

@SmaugPool at the CIP editors' meeting today we resolved for @Crypto2099 to re-review based on your last changes & if this does clarify the issue (as we expect) then we can merge this.

Crypto2099

Aside from the couple of points, particularly surrounding cross-compatibility with the Token Registry standard, I believe that this PR is acceptable to be merged as-is. Given that we are dropping an optional field that is also covered by the fact that any and all additional properties are accepted as part of the existing object, I do not believe that a version iteration is needed to implement these changes and these edits provide greater clarity to the existing standard rather than fundamentally modifying it.

Crypto2099 · 2023-08-01T17:30:45Z

CIP-0068/README.md

    ? url: bounded_bytes, ; UTF-8
-    ? logo: uri,
    ? decimals: int
+
+    ; 'logo' does not follow the explanation of the token-registry, it needs to be a valid URI and not a plain bytestring.
+    ; The logo URI must point to a resource with media type (mime type) `image/png`, `image/jpeg` or `image/svg+xml`.
+    ? logo: uri,


In this section I believe we should keep the naming convention consistent with the Cardano Token Registry to ensure minimal disruption when moving between one standard or the other. Perhaps it is better to move this comment out of the CDDL and use a separate "logo-uri" object that explicitly declares that there is NOT support for data: scheme URIs.

Crypto2099 · 2023-08-01T17:31:35Z

CIP-0068/README.md

+; 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


Drop mention of support for the data: scheme when working with fungible token logo-uri field to be compatible and match the CIP-26 formatting of the Token Registry.

Point of clarification cause I may have misunderstood here, the token registry stores the image in ONLY data: format by default and that is actually an acceptable URI in this case but we just want it to be a "plain text" URI rather than a raw bytestring?

I think this part was written by @alessandrokonrad & @AndrewWestberg, maybe they can clarify as I believe I just moved things around.

In the token registry, images are base64 encoded PNGs, they are not data URLs, but I think data urls are valid here.

@alessandrokonrad can comment on this. My addition was only to add 444 which is a copy paste of parts of 222 and 333.

KtorZ

Seems like a case of "reality that has caught up with the specs", and the clarification seems fair 👍

…URIs (cardano-foundation#562)

rphair added the Update Adds content or significantly reworks an existing proposal label Jul 26, 2023

rphair changed the title ~~CIP-0068 | Improve image types and URIs descriptions~~ CIP-0068 | Improve image types and URI descriptions Jul 26, 2023

rphair self-assigned this Jul 26, 2023

rphair reviewed Jul 26, 2023

View reviewed changes

rphair removed their assignment Jul 26, 2023

rphair requested review from KtorZ and Ryun1 July 26, 2023 19:33

rphair approved these changes Aug 1, 2023

View reviewed changes

SmaugPool force-pushed the master branch from b6f9eed to e532711 Compare August 1, 2023 13:12

CIP-0068 | Clarify & add consistenty to 222, 333 & 444 image types & …

112c85d

…URIs

SmaugPool force-pushed the master branch from e532711 to 112c85d Compare August 1, 2023 13:22

Crypto2099 approved these changes Aug 1, 2023

View reviewed changes

KtorZ approved these changes Aug 15, 2023

View reviewed changes

rphair merged commit b79bb40 into cardano-foundation:master Aug 15, 2023

Ryun1 approved these changes Aug 15, 2023

View reviewed changes

Ryun1 pushed a commit to Ryun1/CIPs that referenced this pull request Nov 17, 2023

CIP-0068 | Clarify & add consistenty to 222, 333 & 444 image types & …

b9c20c7

…URIs (cardano-foundation#562)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CIP-0068 | Improve image types and URI descriptions #562

CIP-0068 | Improve image types and URI descriptions #562

SmaugPool commented Jul 26, 2023 •

edited

rphair left a comment •

edited

rphair Jul 26, 2023

Ryun1 Jul 30, 2023

SmaugPool Jul 31, 2023 •

edited

rphair Jul 31, 2023 •

edited

SmaugPool Jul 31, 2023

rphair Aug 1, 2023

Crypto2099 commented Jul 27, 2023

SmaugPool commented Jul 27, 2023 •

edited

rphair left a comment •

edited

SmaugPool commented Aug 1, 2023 •

edited

rphair commented Aug 1, 2023

Crypto2099 left a comment

Crypto2099 Aug 1, 2023

Crypto2099 Aug 1, 2023

Crypto2099 Aug 2, 2023

SmaugPool Aug 2, 2023

AndrewWestberg Aug 2, 2023

KtorZ left a comment

CIP-0068 | Improve image types and URI descriptions #562

CIP-0068 | Improve image types and URI descriptions #562

Conversation

SmaugPool commented Jul 26, 2023 • edited

rphair left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SmaugPool Jul 31, 2023 • edited

Choose a reason for hiding this comment

rphair Jul 31, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Crypto2099 commented Jul 27, 2023

SmaugPool commented Jul 27, 2023 • edited

rphair left a comment • edited

Choose a reason for hiding this comment

SmaugPool commented Aug 1, 2023 • edited

rphair commented Aug 1, 2023

Crypto2099 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KtorZ left a comment

Choose a reason for hiding this comment

SmaugPool commented Jul 26, 2023 •

edited

rphair left a comment •

edited

SmaugPool Jul 31, 2023 •

edited

rphair Jul 31, 2023 •

edited

SmaugPool commented Jul 27, 2023 •

edited

rphair left a comment •

edited

SmaugPool commented Aug 1, 2023 •

edited