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

Add missing to/from JSON on Plutus types #103

Merged
merged 2 commits into from
Aug 12, 2022
Merged

Conversation

SebastienGllmt
Copy link
Contributor

Can you think a reason this was missing? Seems like just an oversight to me

Copy link
Contributor

@rooooooooob rooooooooob left a comment

Choose a reason for hiding this comment

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

It was either an oversight or I didn't think they were that useful at the time, but it makes sense to have them for consistency. We still derived the traits so they can be used as part of bigger structs.

We should either remove the to_from_json!() macro for ConstrPlutusData or add it for the other plutus datum types. We have a dedicated plutus datum to/from JSON functionality based on the cardano node's format, but then we also have (inaccessible when using directly e.g. no PlutusMap::from_json()) the serde format used from those derives. We needed to have the serde format as well since it's contained within other types with a to/from JSON e.g. Transaction`.

We could try and unify the two and explicitly define the serde traits to use the cardano node's format, which probably isn't hard, but it gets a bit more complicated when it comes to the .ts definitions as I'm not sure how you'd implement JsonSchema in accordance manually. And you'd NEED to have that trait defined rather than being able to just handcode the .json file for the schema, since we need that trait defined in order to derive it for all containing types.

@SebastienGllmt SebastienGllmt merged commit 5a1846d into develop Aug 12, 2022
@SebastienGllmt SebastienGllmt deleted the add-missing-json branch August 12, 2022 00:37
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.

2 participants