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

Transition to a cardanonical API #1577

Open
noonio opened this issue Aug 19, 2024 · 5 comments · Fixed by #1578
Open

Transition to a cardanonical API #1577

noonio opened this issue Aug 19, 2024 · 5 comments · Fixed by #1578
Labels
api Items related to the Hydra client API 💭 idea An idea or feature request

Comments

@noonio
Copy link
Contributor

noonio commented Aug 19, 2024

The protocol-parameters schema has changed and, among other things, now definitions/Lovelace has been replaced with definitions/Value<AdaOnly> ( CardanoSolutions/cardanonical@cf11243#diff-fd323233e2e57f94dee3c36cf0c3f962610306bba1d44d8225af33d2727322e0R2794 ).

This was picked up by the following test:

cabal test hydra-node --test-options='--match "/Hydra.API.HTTPServer/API should respond correctly/GET /protocol-parameters/matches schema/"'
@noonio noonio added the bug 🐛 Something isn't working label Aug 19, 2024
@noonio
Copy link
Contributor Author

noonio commented Aug 19, 2024

Note that we actually vendor the cardano.json file here - https://github.com/cardano-scaling/hydra/tree/master/hydra-node/json-schemas/cardanonical - but we appear to not use it.

@noonio noonio changed the title Transition from Lovelace to Value<AdaOnly> Transition to new cardano.json API Aug 19, 2024
@noonio
Copy link
Contributor Author

noonio commented Aug 19, 2024

Fix for the immediate problem - #1577 - but this issue can serve as a broader place to discuss compatibility.

@noonio noonio linked a pull request Aug 19, 2024 that will close this issue
5 tasks
@noonio noonio reopened this Aug 20, 2024
@noonio noonio added 💬 feature A feature on our roadmap api Items related to the Hydra client API and removed bug 🐛 Something isn't working labels Aug 20, 2024
@ch1bo ch1bo added 💭 idea An idea or feature request and removed 💬 feature A feature on our roadmap labels Aug 20, 2024
@ch1bo ch1bo changed the title Transition to new cardano.json API Transition to a cardanonical API Aug 20, 2024
@ch1bo
Copy link
Collaborator

ch1bo commented Aug 20, 2024

As switching from

"Lovelace":
    { "title": "Lovelace"
    , "additionalProperties": false
    , "required": [ "lovelace" ]
    , "properties":
      { "lovelace":
        { "type": "integer"
        , "description": "A number of lovelace, possibly large when summed up."
        }
      }
    }

to

"Value<AdaOnly>":
    { "title": "Value<AdaOnly>"
    , "type": "object"
    , "additionalProperties": false
    , "required": [ "ada" ]
    , "properties":
      { "ada":
        { "type": "object"
        , "additionalProperties": false
        , "required": [ "lovelace" ]
        , "properties":
          { "lovelace": { "type": "integer" }
          }
        }
      }
    }

is a breaking change.

Do we want to break the API only for this or shall we consider switching other values too?

The change at hand is a bit surprising IMO, because fields like ProtocolParameters minFeeConstant are only a Coin in the ledger code, which is exactly that Lovelace from before / the ada part from a Value.

CC @KtorZ

@ch1bo
Copy link
Collaborator

ch1bo commented Aug 20, 2024

A related API consistency issue: #1543

@KtorZ
Copy link
Collaborator

KtorZ commented Aug 21, 2024

This was indeed changed after the first draft of the new API, with feedback I got from various parties. The rationale here is to be consistent and future-proof (in an hypothetical Babel Fees era...). So any Coin from the ledger is mapped to a Value. In the API though, they are still labeled as Value<AdaOnly> should one want to parse them as raw quantity instead of singletons.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Items related to the Hydra client API 💭 idea An idea or feature request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants