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-0100 | Small clean up on example files #736

Merged
merged 5 commits into from
Jan 22, 2024

Conversation

Ryun1
Copy link
Collaborator

@Ryun1 Ryun1 commented Jan 8, 2024

Motivation

Whilst making some more example files, I have found a couple inconsistencies with the provided examples.

Changes

  • Removed proposal property which does not exist in the standard
  • Aligned capitalisation
  • Changed hashing algorithm from blake2b-224 to blake2b-256 as only 256 is the only supported
  • Changed links to reference github rather than CIPs website

cc @Quantumplation

@Ryun1
Copy link
Collaborator Author

Ryun1 commented Jan 8, 2024

One question;

The cips.cardano.org site seems to be broken for CIP-100 see here, so I am tempted to change all references to cips.cardano.org to direct to Github instead.

@Ryun1 Ryun1 added Correction Fixing minor issue or typo Category: Metadata Proposals belonging to the 'Metadata' category. labels Jan 8, 2024
@rphair
Copy link
Collaborator

rphair commented Jan 8, 2024

@Ryun1 for these links I think consistency is vital, and GitHub has always been a constant (since converting from CIP-XXXX.md to README.md files) and the source of truth with respect to cips.cardano.org and the CIPs on the Developer Portal. @KtorZ 's #109 (comment) suggests there may be another overhaul on cips.cardano.org still in the works, which by #436 (comment) might happen now that #389 is finally done.

Whatever revision is done on cips.cardano.org might not invalidate old pathnames but it would likely change canonical pathnames which are the only ones which make sense to use in a schema. I believe this confirms the importance of using GitHub for canonical links & relying upon user interpretation and parsing for human readers to visit the derived web sites instead.

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.

So assuming #736 (comment) is satisfactory, everything looks good to me.

@rphair rphair changed the title CIP-100 | Small clean up on example files CIP-0100 | Small clean up on example files Jan 8, 2024
@Ryun1
Copy link
Collaborator Author

Ryun1 commented Jan 8, 2024

GitHub has always been a constant

@rphair
Agreed, lets just switch to github links

@Quantumplation
Copy link
Contributor

It's worth noting that the urls in the context don't actually need to resolve to anything; the computer doesn't recursively fetch those documents. They are effectively just constants that everyone uses for disambiguation.

So, I'm fine with updating them now, but even if GitHub changes, it's important to not update them once there is data in the wild.

@KtorZ
Copy link
Member

KtorZ commented Jan 9, 2024

(side-note, I am checking why that CIP -- and possibly a few others -- is missing from the new website).

@Ryun1
Copy link
Collaborator Author

Ryun1 commented Jan 9, 2024

Also; in the examples I just changed hashing algorithm from blake2b-224 to blake2b-256 as only 256 is the only supported.

@Quantumplation
Copy link
Contributor

@Ryun1 did you update the resulting hash in the test vector in the readme.md?

@Ryun1
Copy link
Collaborator Author

Ryun1 commented Jan 22, 2024

@Ryun1 did you update the resulting hash in the test vector in the readme.md?

Good spot!
addressed via 41b9ec6

@Ryun1 Ryun1 merged commit a6267d5 into cardano-foundation:master Jan 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Metadata Proposals belonging to the 'Metadata' category. Correction Fixing minor issue or typo
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants