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-0068 | Small change in 333 FT sub standard #359

Merged
merged 5 commits into from
Oct 25, 2022

Conversation

alessandrokonrad
Copy link
Contributor

@alessandrokonrad alessandrokonrad commented Oct 20, 2022

decimals in metadata don't need to be big_int, but can be int also. @KtorZ pointed that out before for theversion field, but I have overseen it for decimals.
I also realized it makes little sense to follow the same constraints of the cardano-token-registry for logo, which only allows you to use PNGs as bytestrings. Here the image is in the datum and potentially in an inline datum. Having only the option to store the image as bytestring on-chain wouldn't be ideal. A URI is much better suited here, which gives more flexibility and allows you to use also other image types e.g. .jpg
@rphair

@@ -171,15 +171,16 @@ This is a low-level representation of the metadata, following closely the struct

```
; Explanation here: https://developers.cardano.org/docs/native-tokens/token-registry/cardano-token-registry/
; NOTE: 'logo' is an exception and is not a PNG bytestring, but a URI, so that links and data uris with all image types are possible (see below)
Copy link
Member

Choose a reason for hiding this comment

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

neat-picking: why specifically png ? I assume the intent is to forbid inline image data payload, regardless of their mime type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I expressed that wrong maybe, will rework that. In the cardano-token-registry you have to specify a PNG bytestring.
It's not even a valid URI. logo here (in the 333 standard) of course allows you to use a PNG, but not as bytestring, it needs to be a valid data uri if you want to inline it: data:image/png;....
I just wanted to be explicit here and say that using logo like in the token-registry is wrong and it cannot be used like that here.


metadata =
{
name : bounded_bytes, ; UTF-8
description : bounded_bytes, ; UTF-8
? ticker: bounded_bytes, ; UTF-8
? url: bounded_bytes, ; UTF-8
? logo: bounded_bytes, ; UTF-8
? decimals: big_int
? logo: bounded_bytes, ; UTF-8, URI (not a PNG bytestring like in the explanation. Valid formats: ("https://...", "ipfs://...", "data:...", etc.))
Copy link
Member

Choose a reason for hiding this comment

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

Might be worth defining an alias uri with a comment?

metadata = 
    { 
        ...
        ? logo: uri, 
        ...
    }

; A URI as a UTF-8 encoded bytestring.
; The URI scheme must be one of `https`, `ipfs` or `data`
; Do note encode plain file payloads as URI.  
uri = 
    bounded_bytes 

Also, side question: what for URIs that are longer than 64 bytes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, side question: what for URIs that are longer than 64 bytes?

I'm a bit confused about this one because in the serialization-lib there is this function to serialize bounded_bytes:

pub(crate) fn write_bounded_bytes<'se, W: Write>(
    serializer: &'se mut Serializer<W>,
    bytes: &[u8],
) -> cbor_event::Result<&'se mut Serializer<W>> {
    if bytes.len() <= BOUNDED_BYTES_CHUNK_SIZE {
        serializer.write_bytes(bytes)
    } else {
        // to get around not having access from outside the library we just write the raw CBOR indefinite byte string code here
        serializer.write_raw_bytes(&[0x5f])?;
        for chunk in bytes.chunks(BOUNDED_BYTES_CHUNK_SIZE) {
            serializer.write_bytes(chunk)?;
        }
        serializer.write_special(CBORSpecial::Break)
    }
}

Whenever my bytestring is longer than 64 bytes it is automatically split into 64 bytes chunks in an array. I assume this works, because plutus_data in the CDDL can be turned into an array [ *plutus_data ]? But a BuiltinByteString in PlutusCore will still be represented as single long string right?

I was under the impression this is somehow a standardised thing for bounded_bytes, but maybe it's only how the serialization-lib is handling it. Then we should be explicit about it and turn the type into: bounded_bytes / [ *bounded_bytes ].

Copy link
Member

Choose a reason for hiding this comment

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

That's indeed the behavior of the ledger / plutus regarding long indefinite bytestring. I was hoping to find a definition of bounded_bytes in the ledger's CDDL but there's none (nor there's any in the RFC describing CDDL...). So I guess It's fine to leave as such 🤷

Copy link
Contributor Author

@alessandrokonrad alessandrokonrad Oct 21, 2022

Choose a reason for hiding this comment

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

Ah damn it, now I changed it😅. Do you want me to revert it or leave it like that bounded_bytes / [ * bounded_bytes ]?

Copy link
Member

Choose a reason for hiding this comment

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

I'd say: we should provide a non ambiguous definition of bounded bytes somewhere, this CIP is likely not the right place. So maybe a PR to cardano-ledger to add a definition that somewhat reflects how implementations treat it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. Then I'm gonna revert my change. I could temporarily add a comment here to bounded_bytes until it's part of the cardano-ledger.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@KtorZ I found this in the CDDLs: https://github.com/input-output-hk/cardano-ledger/blob/8a8daef6d88b4f7b6f4ca7dafb0d5bde54617d8f/eras/babbage/test-suite/cddl-files/mock/extras.cddl#L36-L45

bounded_bytes is already described as we thought it behaves.

I'm going to revert my changes. I think the PR should be good then.

@rphair
Copy link
Collaborator

rphair commented Oct 21, 2022

Thanks @alessandrokonrad for tagging me in the OP but the best I can do with this discussion (which is beyond my expertise) is to follow the outcome of your dialogue with @KtorZ - as well as @SebastienGllmt if he wants to jump in 🤓

@alessandrokonrad
Copy link
Contributor Author

Thanks @alessandrokonrad for tagging me in the OP but the best I can do with this discussion (which is beyond my expertise) is to follow the outcome of your dialogue with @KtorZ - as well as @SebastienGllmt if he wants to jump in 🤓

I think it's looking good now. @KtorZ corrected me already :)

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.

as far as I can tell, last remaining issue has been resolved by 452dca2.

@rphair rphair merged commit 9abe8a7 into cardano-foundation:master Oct 25, 2022
@rphair rphair added the Update Adds content or significantly reworks an existing proposal label May 24, 2023
@rphair rphair changed the title CIP-0068: Small change in 333 FT sub standard CIP-0068 | Small change in 333 FT sub standard Jun 9, 2023
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.

4 participants