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

Extend AssetDetails data type #40

Merged
merged 3 commits into from
Dec 18, 2023

Conversation

euonymos
Copy link
Contributor

No description provided.

@sorki
Copy link
Contributor

sorki commented Oct 5, 2023

LGTM!

@euonymos
Copy link
Contributor Author

euonymos commented Oct 6, 2023

Thank you @sorki! I will add more changes since we need extra fields from CIP25. It's not obvious how to implement it to preserve backward compatibility, so for now, I added a Value to keep everything and an additional parser to read "standard" fields. Writing a manual instance is an alternative. Please let me know if you have any ideas about improving it.

@sorki
Copy link
Contributor

sorki commented Oct 11, 2023

Your solution is actually neat as writing a custom instance would add a lot of boilerplate and the additional parsing step is not visible to user / API anyway.

@sorki
Copy link
Contributor

sorki commented Nov 8, 2023

Ready? 😸

@euonymos
Copy link
Contributor Author

euonymos commented Dec 11, 2023

@sorki probably it is. We decided to change the way we encode metadata -- basically move most parts into the first cip68 argument. Now we see the following in the responses:

"onchain_metadata": {
    "GameY": "a449416c69676e6d656e74474e65757472616c46736368656d614d47616d65595f4578616d706c654d736368656d6156657273696f6e014776657273696f6e01",
    "description": "Description goes here",
    "games": "9f4547616d65584547616d6559ff",
    "image": "9f5840697066733a2f2f516d5439387267536b50376f766b76645857424568466e506d6258424364326d58674d334b687350334b574559792f746f6b656e3734323635442e6a7067ff",
    "mediaType": "image/jpg",
    "name": "000643b052464c58545f545354233036",
    "powered by": "4572666c7874"
},

So not all parts of metadata are decoded - some are still CBOR-encoded. While it might be not ideal for those who cannot/don't want to work with CBOR it's not a problem for us, but I cannot get the pattern here. I would say only short strings are decoded - but not, the last field powered by seems to me structurally identical to mediaType. So we guess only some known fields and decoded. Not sure we really want to upstream the corresponding changes though. Looks like a good set of defaults.

@sorki
Copy link
Contributor

sorki commented Dec 12, 2023

@sorki probably it is. We decided to change the way we encode metadata -- basically move most parts into the first cip68 argument. Now we see the following in the responses:

Cool, this seems to be flexible enough to include additional fields.

So not all parts of metadata are decoded - some are still CBOR-encoded. While it might be not ideal for those who cannot/don't want to work with CBOR it's not a problem for us, but I cannot get the pattern here. I would say only short strings are decoded - but not, the last field powered by seems to me structurally identical to mediaType. So we guess only some known fields and decoded. Not sure we really want to upstream the corresponding changes though. Looks like a good set of defaults.

+1, I think that decoding the rest is out of scope and IIRC it can only be done for some fields that decode to ascii/text but the fields can contain arbitrary data so we would need to try/guess which might lead to wrong results. It is done in similar fashion in our API, decode the known ones and keep the rest as CBORs so you are right about the handling.

@sorki sorki marked this pull request as ready for review December 18, 2023 08:21
@sorki
Copy link
Contributor

sorki commented Dec 18, 2023

I'll merge this, add a changelog and release it in the next round soonish.

@sorki sorki merged commit b275c69 into blockfrost:master Dec 18, 2023
2 checks passed
@sorki
Copy link
Contributor

sorki commented Dec 18, 2023

Thank you!

sorki added a commit that referenced this pull request Dec 18, 2023
@sorki
Copy link
Contributor

sorki commented Dec 18, 2023

Released in api-0.9 & client-0.8

@mmahut mmahut mentioned this pull request Dec 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants