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

cardano-api: Drop ToJSON Script (ledger) instance #4130

Closed
wants to merge 1 commit into from

Conversation

ch1bo
Copy link
Contributor

@ch1bo ch1bo commented Jul 4, 2022

This orphan instance is not total and seemingly unused. It makes usage
of the cardano-api library very hard if you do not agree with having all
Alonzo.Script values be serialized as only their hashes.

CC co-author @abailly-iohk

This orphan instance is not total and seemingly unused. It makes usage
of the cardano-api library very hard if you do not agree with having all
Alonzo.Script values be serialized as only their hashes.
@ch1bo ch1bo changed the title Drop ToJSON Script (ledger) instance cardano-api: Drop ToJSON Script (ledger) instance Jul 4, 2022
@ch1bo
Copy link
Contributor Author

ch1bo commented Jul 4, 2022

It seems it's used in the cardano-cli though over here in the context of some DebugLedgerState: https://github.com/input-output-hk/cardano-node/blob/08e043a62476a69e04a0c5baa97e415e0efe1212/cardano-cli/src/Cardano/CLI/Shelley/Run/Query.hs#L1337

If that is the only use for these orphan instances, then I'm happily moving them to cardano-cli if that means we don't have them in cardano-api.

@ch1bo
Copy link
Contributor Author

ch1bo commented Jul 7, 2022

See #4138 for an alternative to this, which I would support instead. Still happy to have the discussion about these orphan instances.

@Jimbo4350
Copy link
Contributor

See #4138 for an alternative to this, which I would support instead. Still happy to have the discussion about these orphan instances.

I'd say move it over to the cli in this PR and I can approve @ch1bo

@ghost
Copy link

ghost commented Jul 11, 2022

Actually, this instance is used in the node. I got the following error when trying to move it to cardano-cli in #4138:

src/Cardano/Node/Protocol/Cardano.hs:133:7: error:
    • No instance for (ToJSON
                         (Cardano.Ledger.Alonzo.Scripts.Script
                            (Cardano.Ledger.Babbage.BabbageEra
                               Cardano.Ledger.Crypto.StandardCrypto)))
        arising from a use of ‘SomeConsensusProtocol’
    • In the expression: SomeConsensusProtocol CardanoBlockType

I would say, "done is better than perfect" so let's merge #4138 if no one has strong opinions against it.

@ch1bo
Copy link
Contributor Author

ch1bo commented Jul 27, 2022

Won't be having time to work on this for the time being and we merged the alternative of fixing the ToJSON Script instance to be complete.

@ch1bo ch1bo closed this Jul 27, 2022
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