Update Cardano.Api.ProtocolParameters to ease integration with Alonzo #2634

Jimbo4350 · 2021-04-20T13:20:19Z

Prepare Cardano.Api.ProtocolParameters for Alonzo era.

In the Alonzo era we introduce 6 new protocol parameters and remove 1
protocol parameter. In anticipation of the Alonzo era, we make some
modifications to the ProtocolParameters type.

minUTxOValue becomes a Maybe (This parameter gets deprecated in Alonzo)

We introduce the following but note that they are currently ignored:

adaPerUTxoByte - Cost in ada per byte of UTxO storage (replaces
minUTxOValue)
costmdls - The cost models for plutus scripts
prices - The prices of execution units for plutus scripts
maxTxExUnits - Max total script execution resource units allowed per tx
maxBlockExUnits - Max total script execution resource units allowed per block
maxValSize - Max size of a Value in a tx output

We introduce an era parameter in the ProtocolParameters type in order to
reduce boilerplate. We have avoided the typical GADT pattern in
cardano-api to prevent an explosion of types.

Jimbo4350

~~TODO: Break up Cardano.Api.ProtocolParameters module~~

cardano-cli/src/Cardano/CLI/Shelley/Run/Transaction.hs

cardano-api/test/Test/Cardano/Api/Typed/Gen.hs

newhoggy · 2021-04-21T03:13:05Z

cabal.project

@@ -200,3 +213,7 @@ constraints:

 package comonad
  flags: -test-doctests
+
+allow-newer:
+  monoidal-containers:aeson,


You may not need monoidal-containers:aeson anymore as of now.

See IntersectMBO/cardano-ledger#2245

dcoutts

Looking good!

cardano-api/src/Cardano/Api/Fees.hs

cardano-api/src/Cardano/Api/Script.hs

cardano-api/test/Test/Cardano/Api/Typed/Gen.hs

dcoutts · 2021-04-27T10:01:48Z

cardano-api/src/Cardano/Api/ProtocolParameters.hs

@@ -101,7 +115,7 @@ import           Cardano.Api.Value
 --
 -- There are also paramaters fixed in the Genesis file. See 'GenesisParameters'.
 --
-data ProtocolParameters =
+data ProtocolParameters era =


Why did we need the era type param? We don't seem to use it below.

If we really don't need it to be era-parameterised, then it should simplify things elsewhere (e.g. in the cli code).

~~This appears to be leftover, I'll remove it~~ We need it for the JSON instances https://github.com/input-output-hk/cardano-node/pull/2634/files?file-filters%5B%5D=.hs#diff-6d43cdca78f34a488eb77b4b8597a6b02f173b078695ca5f6a2b9371bcb30531R595

Hmm. I've had a look and I don't think that's the case. You're using it to restrict the fields we render to those from a specific era, but I don't see any need to do that. We can just render them all.

Yes, I think the strategy we're following here with the ProtocolParameters is that we just have all of them, taking the union of them from all eras, and in some eras we just done use them all. I've not yet seen any reason that that strategy will not work out ok.

In which case to follow the strategy we really should not have an era param at all, since nothing inside it needs it. And other functions like JSON serialisation also do not need to know the era because the strategy we're following is to take the collection of all params, from all eras.

If you think that's not going to work out for some reason then lets review it and we might need to change strategy and have a fully era-typed style where we enforce that only the params that are available are those that exist for each specific era. But let's not do a hybrid strategy. Typed or untyped, but not a mish-mash.

cardano-api/src/Cardano/Api/ProtocolParameters.hs

Jimbo4350 · 2021-04-28T17:26:46Z

The hydra error will go away once the node's deps are updated and I rebase on master

refi93 · 2021-05-03T08:34:18Z

@Jimbo4350 I assume maxValSize is the maximum size of the utxo "value" field in the serialized transaction in bytes - do we know what the value of maxValSize in Alonzo era will be? In Mary it was 4000 bytes if I understood correctly: https://github.com/input-output-hk/cardano-ledger-specs/pull/2099/files#diff-1dbf0b3dcad4b1ddcdf86e9df16469b461f3e9f906cf42d925d6fb3a386dd6c2R258

Jimbo4350 · 2021-05-03T08:54:41Z

@Jimbo4350 I assume maxValSize is the maximum size of the utxo "value" field in the serialized transaction in bytes - do we know what the value of maxValSize in Alonzo era will be? In Mary it was 4000 bytes if I understood correctly: https://github.com/input-output-hk/cardano-ledger-specs/pull/2099/files#diff-1dbf0b3dcad4b1ddcdf86e9df16469b461f3e9f906cf42d925d6fb3a386dd6c2R258

We haven't decided on any of the new protocol parameters introduced in the Alonzo era as yet. This will happen closer to the fork.

dcoutts

There's obviously some overlap to make this PR and #2657. That's fine for the moment so they both pass CI independently.

Given that #2657 seems nearly ready and this one will need a bit more work, I think it probably makes most sense for this one to be rebased after that PR goes in (or rebased on that one's branch in the mean time).

cardano-api/src/Cardano/Api/ProtocolParameters.hs

dcoutts · 2021-05-11T23:46:25Z

cardano-api/src/Cardano/Api/ProtocolParametersUpdate.hs

+       -- | Cost models for non-native script languages.
+       protocolUpdateCostModels :: Map AnyScriptLanguage CostModel,
+
+       -- | Map AnyScriptLanguage ExecutionUnitPrices of execution units (for non-native script languages).


These are the other uses of the "native" script language terminology.

cardano-api/src/Cardano/Api/Eras.hs

cardano-api/src/Cardano/Api/Fees.hs

dcoutts · 2021-05-11T23:57:06Z

cardano-api/src/Cardano/Api/ProtocolParameters.hs

@@ -101,7 +115,7 @@ import           Cardano.Api.Value
 --
 -- There are also paramaters fixed in the Genesis file. See 'GenesisParameters'.
 --
-data ProtocolParameters =
+data ProtocolParameters era =


Yes, I think the strategy we're following here with the ProtocolParameters is that we just have all of them, taking the union of them from all eras, and in some eras we just done use them all. I've not yet seen any reason that that strategy will not work out ok.

In which case to follow the strategy we really should not have an era param at all, since nothing inside it needs it. And other functions like JSON serialisation also do not need to know the era because the strategy we're following is to take the collection of all params, from all eras.

If you think that's not going to work out for some reason then lets review it and we might need to change strategy and have a fully era-typed style where we enforce that only the params that are available are those that exist for each specific era. But let's not do a hybrid strategy. Typed or untyped, but not a mish-mash.

dcoutts · 2021-05-12T00:43:33Z

cardano-api/src/Cardano/Api/ProtocolParametersUpdate.hs

+instance HasTextEnvelope UpdateProposal where
+    textEnvelopeType _ = "UpdateProposalShelley"
+
+--TODO: Jordan UpdateProposal needs to be parameterized by era or have access to the era


If we think we might need the era just for this purpose, then we can either:

Drop CBOR serialisation for these types. Do we really need separate serialisation for them?

Do independent CBOR serialisation that does not involve converting to the underlying ledger type. This will not work in future precisely because we're taking the simple untyped approach of taking the union of all params for all eras.

dcoutts · 2021-05-12T00:45:03Z

cardano-api/src/Cardano/Api/ProtocolParametersUpdate.hs

+    --TODO decide how to handle parameter validation
+    UpdateProposal (Map.fromList [ (kh, params) | kh <- genesisKeyHashes ])
+
+newtype CostModel = CostModel (Map.Map Text Integer)


Oops, looks like we've duplicated these types from the ProtocolParameters module. A good reason to unsplit the modules (once we make the original less enormous again) :-)

dcoutts · 2021-05-12T00:46:54Z

cardano-api/src/Cardano/Api/ProtocolParametersUpdate.hs

+-- ----------------------------------------------------------------------------
+-- Conversion functions
+--


I can't tell what we've changed here since we've split the module. Can I suggest that's another good reason to not split them in this PR, or at least not split them and change them in a single commit.

dcoutts · 2021-05-12T00:50:08Z

cardano-api/src/Cardano/Api/Script.hs

+
+
+instance Aeson.FromJSONKey AnyScriptLanguage where
+    fromJSONKey = Aeson.FromJSONKeyValue parseJSON


I don't understand how this is supposed to work. A JSON key must be just a JSON string but this reuses the FromJSON above right? But that uses a JSON object not a JSON string. Am I missing something?

dcoutts · 2021-05-12T00:51:31Z

cardano-api/src/Cardano/Api/Script.hs

@@ -835,6 +871,7 @@ fromAllegraTimelock timelocks = go

 instance ToJSON (Script lang) where
  toJSON (SimpleScript _ script) = toJSON script
+  toJSON (PlutusScript _ _) = Aeson.Null


Perhaps better to leave this as an error "TODO ..." rather than Null. The latter is too easy to forget or miss in a grep TODO.

In the Alonzo era we introduce 6 new protocol parameters and remove 1 protocol parameter. In anticipation of the Alonzo era, we make some modifications to the ProtocolParameters type. minUTxOValue becomes a Maybe (This parameter gets deprecated in Alonzo) We introduce the following but note they are currently ignored: - adaPerUTxoByte - Cost in ada per byte of UTxO storage (replaces minUTxOValue) - costmdls - The cost models for plutus scripts - prices - The prices of execution units for plutus scripts - maxTxExUnits - Max total script execution resource units allowed per tx - maxBlockExUnits - Max total script execution resource units allowed per block - maxValSize - Max size of a Value in a tx output We introduce an era parameter in the ProtocolParameters type in order to reduce boilerplate. We have avoided the typical GADT pattern in cardano-api to prevent an explosion of types.

Jimbo4350 · 2021-05-17T08:41:38Z

Closed in favour of #2699

Jimbo4350 requested review from DavidEichmann, dcoutts, erikd and newhoggy as code owners April 20, 2021 13:20

Jimbo4350 commented Apr 20, 2021

View reviewed changes

cardano-cli/src/Cardano/CLI/Shelley/Run/Transaction.hs Outdated Show resolved Hide resolved

newhoggy reviewed Apr 21, 2021

View reviewed changes

cardano-api/test/Test/Cardano/Api/Typed/Gen.hs Show resolved Hide resolved

newhoggy reviewed Apr 21, 2021

View reviewed changes

Jimbo4350 force-pushed the jordan/alonzo-protocol-parameters-integration branch 6 times, most recently from d46dc08 to 0cd5588 Compare April 23, 2021 08:40

Jimbo4350 mentioned this pull request Apr 26, 2021

Basic Alonzo node #2657

Merged

dcoutts requested changes Apr 27, 2021

View reviewed changes

Jimbo4350 force-pushed the jordan/alonzo-protocol-parameters-integration branch 2 times, most recently from 0bd64ea to afa4b9a Compare April 28, 2021 15:07

refi93 mentioned this pull request May 3, 2021

refactor(core/cardano): decouple address parameters validation vacuumlabs/trezor-firmware#60

Closed

Jimbo4350 force-pushed the jordan/alonzo-protocol-parameters-integration branch 7 times, most recently from 56cdaba to 2cf08e4 Compare May 11, 2021 12:33

dcoutts requested changes May 12, 2021

View reviewed changes

Jimbo4350 added 5 commits May 12, 2021 12:55

Review fixes

f0d93a3

Split Cardano.Api.ProtocolParameters module to reduce compilation time

93a903e

Update NodeProtocolConfiguration to include Alonzo

a7c9f21

Point node deps at alonzo pre-release branch

aca5565

WIP: Review fixes

32a1c83

Jimbo4350 force-pushed the jordan/alonzo-protocol-parameters-integration branch from 2cf08e4 to 32a1c83 Compare May 12, 2021 15:29

dcoutts mentioned this pull request May 14, 2021

Extend Cardano.Api.ProtocolParameters with Alonzo params #2699

Merged

Jimbo4350 closed this May 17, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Cardano.Api.ProtocolParameters to ease integration with Alonzo #2634

Update Cardano.Api.ProtocolParameters to ease integration with Alonzo #2634

Jimbo4350 commented Apr 20, 2021 •

edited

Loading

Jimbo4350 left a comment •

edited

Loading

newhoggy Apr 21, 2021 •

edited

Loading

dcoutts left a comment

dcoutts Apr 27, 2021

Jimbo4350 Apr 27, 2021 •

edited

Loading

dcoutts May 11, 2021

dcoutts May 11, 2021

Jimbo4350 commented Apr 28, 2021

refi93 commented May 3, 2021

Jimbo4350 commented May 3, 2021

dcoutts left a comment

dcoutts May 11, 2021

dcoutts May 11, 2021

dcoutts May 12, 2021

dcoutts May 12, 2021

dcoutts May 12, 2021

dcoutts May 12, 2021

dcoutts May 12, 2021

Jimbo4350 commented May 17, 2021



		instance Aeson.FromJSONKey AnyScriptLanguage where
		fromJSONKey = Aeson.FromJSONKeyValue parseJSON

Update Cardano.Api.ProtocolParameters to ease integration with Alonzo #2634

Update Cardano.Api.ProtocolParameters to ease integration with Alonzo #2634

Conversation

Jimbo4350 commented Apr 20, 2021 • edited Loading

Jimbo4350 left a comment • edited Loading

Choose a reason for hiding this comment

newhoggy Apr 21, 2021 • edited Loading

Choose a reason for hiding this comment

dcoutts left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jimbo4350 Apr 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jimbo4350 commented Apr 28, 2021

refi93 commented May 3, 2021

Jimbo4350 commented May 3, 2021

dcoutts left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jimbo4350 commented May 17, 2021

Jimbo4350 commented Apr 20, 2021 •

edited

Loading

Jimbo4350 left a comment •

edited

Loading

newhoggy Apr 21, 2021 •

edited

Loading

Jimbo4350 Apr 27, 2021 •

edited

Loading