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.

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

CIP-0025 | Update "image" Spec Based on Feedback #341

Merged
merged 4 commits into from
Oct 6, 2022

Conversation

thaddeusdiamond
Copy link
Contributor

Practical usage has led to a proliferation of URI mechanisms for the "image" field in CIP-0025 metadata. (See
https://twitter.com/pool_pm/status/1575219209491337216?s=20&t=KZKbGeCKIUnPyTZ0fzPyhQ for one thread on the topic). This change would be a simple clarification in one sub-bullet for newly introduced community members looking to get familiar with the standard.

Since IPFS and Arweave are the most common methods of data storage for NFT files, we explicitly call them out and provide more resources. Note that this is particularly important as certain free storage providers like NFT.Storage only return the version 1 CID in their file upload calls and lead to scheme being commonly omitted in the URI.

Practical usage has led to a proliferation of URI mechanisms for the
"image" field in CIP-0025 metadata.  (See
https://twitter.com/pool_pm/status/1575219209491337216?s=20&t=KZKbGeCKIUnPyTZ0fzPyhQ
for one thread on the topic).  This change would be a simple
clarification in one sub-bullet for newly introduced community members
looking to get familiar with the standard.

Since IPFS and Arweave are the most common methods of data storage
for NFT files, we explicitly call them out and provide more resources.
Note that this is particularly important as certain free storage
providers like NFT.Storage only return the version 1 CID in their file
upload calls and lead to scheme being commonly omitted in the URI.
@thaddeusdiamond
Copy link
Contributor Author

@KtorZ
Copy link
Member

KtorZ commented Oct 1, 2022

This sounds like a reasonable precision. I am not quite sure what you mean with the following sentence:

not merely the common identifier as NFT viewers may not be able to locate the file

Would it be worth clarifying with a few explicit example perhaps? Good examples usually help clearing out ambiguity.

Copy link
Collaborator

@rphair rphair left a comment

Choose a reason for hiding this comment

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

This seems like a good clarification to have & I would have appreciated it at my own level, especially with the link to OpenSea. Happy to approve pending the detail suggested in #341 (comment).

@rphair rphair added the Update Adds content or significantly reworks an existing proposal label Oct 1, 2022
@rphair rphair changed the title CIP-0025: Update "image" Spec Based on Feedback CIP-0025 | Update "image" Spec Based on Feedback Oct 1, 2022
This second commit provides some clarifying language and examples based
on initial feedback.
@thaddeusdiamond
Copy link
Contributor Author

I meant "content" not "common" woops (https://github.com/multiformats/cid). Perils of writing late at night. Including more examples and resubmitting. @rphair and @KtorZ let me know feedback on this second commit. I am attaching a photo of how it shows up in stackedit.io

Screen Shot 2022-10-02 at 2 49 42 PM

apologies to cram this into your branch: just need the clarity for other proofreading...
Copy link
Collaborator

@rphair rphair left a comment

Choose a reason for hiding this comment

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

Additional detail is very helpful... ready to approve given the reservation below 😎

CIP-0025/README.md Outdated Show resolved Hide resolved
@thaddeusdiamond
Copy link
Contributor Author

thaddeusdiamond commented Oct 3, 2022

Good callouts. Rebased the branch on top of @rphair's whitespace commit and removed the SVG/updated the link. Feel free to merge whenever this meets the review needs.

Copy link
Collaborator

@rphair rphair left a comment

Choose a reason for hiding this comment

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

I'm happy with it & believe it now fully satisfies #341 (comment) 😎

@KtorZ
Copy link
Member

KtorZ commented Oct 6, 2022

Nice. I believe that this also clarifies #345

@KtorZ KtorZ merged commit 4948eab into cardano-foundation:master Oct 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Update Adds content or significantly reworks an existing proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants