cardano-scaling / hydra

Implementation of the Hydra Head protocol
https://hydra.family/head-protocol/
Apache License 2.0
274 stars 84 forks source link

Transition to a cardanonical API #1577

Open noonio opened 3 weeks ago

noonio commented 3 weeks ago

The protocol-parameters schema has changed and, among other things, now definitions/Lovelace has been replaced with definitions/Value<AdaOnly> ( https://github.com/CardanoSolutions/cardanonical/commit/cf11243697802ff0ba4a9cec50bc55f8f300379b#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 commented 3 weeks ago

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 commented 3 weeks ago

Fix for the immediate problem - https://github.com/cardano-scaling/hydra/issues/1577 - but this issue can serve as a broader place to discuss compatibility.

ch1bo commented 3 weeks ago

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 commented 3 weeks ago

A related API consistency issue: https://github.com/cardano-scaling/hydra/issues/1543

KtorZ commented 3 weeks ago

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.