-
Notifications
You must be signed in to change notification settings - Fork 722
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
Script ToJSON instances serialises the full script #4138
Conversation
Note this is an alternative to #4130 which is less principled (still has orphan instance in a place there should be none) but easier to integrate. |
There used to be no orphan instance for ledger Script so we (Hydra) provided our own. This newly introduced instance overlaps with our own which would not be a problem, as we are fine using upstream instances, but for the fact the serialisation is lossy as cardano-api only serialised the hash of the script.
@Jimbo4350 I am trying to update my PR to address comments you made on @ch1bo 's PR #4130, but not sure what's the best approach. There's currently a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks reasonable to me.
Oh I see I should have approved #4223 which is the version rebased on the master branch. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, can you squash the commits?
@Jimbo4350 would need to open another branch on origin for that. I have squashed & rebased on the master PR. We would love to get it after |
This change was merged now on |
Looks like this has already been merged into the release branch. Closing. |
There used to be no orphan instance for ledger Script so we (Hydra)
provided our own. This newly introduced instance overlaps with our own
which would not be a problem, as we are fine using upstream instances,
but for the fact the serialisation is lossy as cardano-api only
serialised the hash of the script.