CIP25 merge V1/V2 in API and get rid of redundant types #216
Conversation
V1 vs V2 from a user perspective are identical but are encoded differently e.g. as text (v1) vs bytes (v2). A user should not need to care about this. We also previously had 3 different types each for asset names and policy ids: cip25 v1, cip25 v2 and the cml-chain ones. This modifies the outer map part of cip25 to directly use the cml-chain ones and remove the cip25 structs. The serialization for this map had already been hand-edited to allow for permissive parsing so having to move this to utils and hand-edit it shouldn't be much of an issue. Places where hand-editing was done is commented and the other code was taken directly from the auto-generated code from before. Doing this gives better structural checking too as we didn't do any of that for the v1/v2 policy ids. This is why the noise_metadata test had to be edited. As a result of this CIP25 has about half the types and should have a much less confusing API.
| let metadata = CIP25Metadata::new(LabelMetadata::new_label_metadata_v2( | ||
| LabelMetadataV2::new(v2), | ||
| )); | ||
| let policy_id_bytes = [ |
There was a problem hiding this comment.
Our old tests were using invalid policy IDs. This was never noticed since generating from the spec couldn't do things like say the text must be hex, only that it's text, etc. The bytes from the official .cddl also don't specify length for policy ids properly. Now that we're directly using cml_chain::PolicyId we ran into this so I updated the tests.
|
I'll add the WASM bindings later once we decide if this is the approach we want. The previous one is #193 but IMO this one gives a much cleaner API for users. It doesn't really even hurt regenerability much either since we had to modify the outer map deserialization code a bunch for permissive parsing anyway. I marked the few additional places we hand-edit here with comments in case we do ever need to regenerate. Other than that I just pasted the v1/v2 table parsing code into the new types and did those few single-line changes to directly use |
cip25/rust/src/utils.rs
Outdated
| serializer.write_text(policy_id.to_hex())?; | ||
| serializer.write_map(cbor_event::Len::Len(assets.len() as u64))?; | ||
| for (asset_name, details) in assets.iter() { | ||
| // hand-edit: write hex string |
There was a problem hiding this comment.
Oops. It's not hex actually. You can only encode string asset names in V1 AFAIK. What should we do here? Here are the first things that come to mind:
- Throw an error. I am against this since, while cbor_event::Serialize::serialize() works with
Result, in practice for us we should never throw an error as we use a vector serializer in most cases for our to/from bytes methods which assume that doing such will cause serialize() to never fail. - Encode as V2 as a fall-back if some of the asset names don't work with V1.
- Get rid of pub access to
LabelMetadata's fields and have all setters throw an error if you either try to set the version to v1 and you have non-text asset names, or add an NFT with a non-text asset name while you're on v1.
Thoughts? I am leaning slightly towards 2) out of those options. Maybe there's another way too.
There was a problem hiding this comment.
Personally I feel like (3) seems the most intuitive behavior, but I agree it's kind of weird that the struct would contain this internal implicit constraint on how you can use it, which is why I have a slight preference to just wrap things similar to pallas-traverse instead of merging the structs like this
There was a problem hiding this comment.
We have #193 that tried wrapping instead of merging. The issue exists there too, you just shift where this restriction on v1 pops up in the API by doing that. In the wrapper there you'd instead have the setter have to return an error if you're trying to set a non-displayable asset name in V1, so any setting/creation using AssetName would have to return a Result to cover that possibility. I think we're stuck with either implicit v2 fallback or having an error somewhere if we want to abstract the version diffs away (either with merging or wrappers).
And I don't think leaving things how they are without any version-independent API is a good idea because the part of the API that should matter least (just using policy ids/asset names which are concepts/types that already exist) takes up the majority of the API. What is PolicyId vs PolicyIdV1 vs PolicyIdV2? (compared to reducing the burden on the user who would already know the PolicyId type from the rest of the library). The first version of the docs David made for this lib before I think give a good idea of what someone might think if they don't already know the version differences and try to make sense of the API. i.e. that it's confusing instead of having users just focus on the actual MetadataDetails that impact the NFT info and how to attach this to/parse from metadata. You could partially work around it with documentation in the first place but it's still nowhere near as clean.
Shall I try out 3) when I have time?
There was a problem hiding this comment.
Shall I try out 3) when I have time?
Yeah I think fail-fast is better than the other options which delay the error occurring until later
There was a problem hiding this comment.
Oh yeah I'm definitely against 1) due to that (+ breaking our assumption we use in to_bytes() that serialization won't fail).
- wouldn't have any error though, it would just switch to V2 format if you tried creating a V1 one with non-printable asset names. I guess it depends what you mean by error.
There was a problem hiding this comment.
The error is not when the silent switch happens - the error would be when you try and use it somewhere where you expected a V1 format and didn't realize it was silently converted to a V2 format and crashes your pipeline
There was a problem hiding this comment.
imo option 3 is the most intuitive for end-users. I think we should also add a comment about the differences between v1 and v2 near LabelMetadata::new
There was a problem hiding this comment.
overall seems to be the right approach, thanks @rooooooooob. the v1 encoding should be fixed though as discussed
V1 vs V2 from a user perspective are identical but are encoded differently e.g. as text (v1) vs bytes (v2). A user should not need to care about this. We also previously had 3 different types each for asset names and policy ids: cip25 v1, cip25 v2 and the cml-chain ones.
This modifies the outer map part of cip25 to directly use the cml-chain ones and remove the cip25 structs. The serialization for this map had already been hand-edited to allow for permissive parsing so having to move this to utils and hand-edit it shouldn't be much of an issue. Places where hand-editing was done is commented and the other code was taken directly from the auto-generated code from before.
Doing this gives better structural checking too as we didn't do any of that for the v1/v2 policy ids. This is why the noise_metadata test had to be edited.
As a result of this CIP25 has about half the types and should have a much less confusing API.