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

Integrate latest ledger dependencies #5013

Merged
merged 14 commits into from
Apr 14, 2023
Merged

Conversation

lehins
Copy link
Contributor

@lehins lehins commented Mar 23, 2023

The goal of this PR is to integrate latest changes in the cardano-ledger, which accrued over the period of the last few months. Most notable breaking changes in ledger are:

  • Addition of Conway era
  • Implementation of versioned CBOR serialization
  • Switching to lenses for manipulating of PParams
  • Changes to CostModel serialization
  • Many more ...

Besides integrating ledger this PR also:

  • Accounts for recent changes in consensus
  • Changes representation of CostModel
  • Fixes a bug where only metadata from TxAuxData was hashed upon transaction body creation with createTransactionBody
  • Reduces a whole lot of duplication
  • Moves all orphan instances for types defined in cardano-base and cardano-ledger
  • Fixes issues with calculateMinimumUTxO funciton:

@lehins lehins force-pushed the lehins/update-ledger-conway branch 3 times, most recently from 4e608b3 to 7479aca Compare March 29, 2023 00:47
@newhoggy newhoggy force-pushed the lehins/update-ledger-conway branch 2 times, most recently from 3325f00 to 68e0cfa Compare March 30, 2023 05:54
@newhoggy
Copy link
Contributor

LGTM

@lehins lehins force-pushed the lehins/update-ledger-conway branch from 68e0cfa to f1d99ff Compare March 30, 2023 14:04
Copy link
Contributor

@Jimbo4350 Jimbo4350 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @lehins for propagating the changes from ledger. I just have a couple comments

  1. I think the ProtocolParameter related improvments that are not directly related to ledger changes should be in a separate PR.
  2. I'd prefer to not use lenses if we don't have to. We're not updating the fields of deeply nested structures and I think the lenses further obfuscate the code vs record accessors which immediately give me information/context about the data structure I am updating.

There are many good refactors here 👍 . We should definitely organize a call next week so you can take us through the changes.

cardano-api/src/Cardano/Api/Genesis.hs Outdated Show resolved Hide resolved
cardano-api/src/Cardano/Api/Genesis.hs Show resolved Hide resolved
cardano-api/src/Cardano/Api/ProtocolParameters.hs Outdated Show resolved Hide resolved
cardano-api/src/Cardano/Api/TxBody.hs Show resolved Hide resolved
cardano-api/src/Cardano/Api/Fees.hs Outdated Show resolved Hide resolved
@lehins
Copy link
Contributor Author

lehins commented Mar 31, 2023

I think the ProtocolParameter related improvements that are not directly related to ledger changes should be in a separate PR.

They are all related to the ledger changes, so they can't be extracted into a separate PR. PParams and PParamsUpdate have been completely redone in the latest version of ledger. Here is the relevant PR: IntersectMBO/cardano-ledger#3242

I'd prefer to not use lenses if we don't have to.

We have to. Every single field in almost every ledger type family is now to be manipulated by a dedicated lens.

We're not updating the fields of deeply nested structures and I think the lenses further obfuscate the code vs record accessors which immediately give me information/context about the data structure I am updating.

In the matter of fact we are. This is how you'd add inputs to a transaction:

> let addInputs tx ins = tx & bodyTxL . inputsTxBodyL <>~ ins
> :t addInputs
addInputs
  :: EraTx era => Tx era -> Set (TxIn (EraCrypto era)) -> Tx era

It is important to note that this function here will work for all eras. This is the power of lenses from ledger, not only that they allow to modify nested structure.

Moreover. Both Tx and TxBody are nested structures themselves for every era. We historically provided pattern synonyms with record fields, that allowed us to pretend like they are not nested structures. But this is a maintenance burden and it does not help us with avoiding duplication. Lenses do. For example, I'd love to see an alternative approach that will let you compute a total output of any transaction in any era:

> let txTotalAda tx = fold $ fmap (\txOut -> txOut ^. coinTxOutL) (tx ^. bodyTxL . outputsTxBodyL)
> :t txTotalAda
txTotalAda :: EraTx era => Tx era -> Coin

Same applies to PParams. For example here is a lens that will let you set and get protocol version from PParams for any era:

> :t ppProtocolVersionL 
ppProtocolVersionL :: EraPParams era => Lens' (PParams era) ProtVer

If a field was added at a later era then it will be guarded by a specific type class that allows using the lens from specific era onwards, eg ppCostModelsL can be used in Alonzo, Babbage..., but not with PParams from eras prior to Alonzo:

> :t ppCostModelsL 
ppCostModelsL :: AlonzoEraPParams era => Lens' (PParams era) CostModels

Before that we were using PParams with the hacky getField @"_protocolVersion" pp approach, but it was too ugly and limited in functionality. It only allowed for accessing, not modifying. And with respect to other types, like Tx, TxBody, TxOut we had no way to deal in an era agnostic fashion.

Long story short, lenses are the new designated way to deal with ledger types that can change across eras.

There are many good refactors here +1 . We should definitely organize a call next week so you can take us through the changes.

Thank you. Sounds good with me.

@lehins lehins force-pushed the lehins/update-ledger-conway branch 3 times, most recently from 372096a to ce36e79 Compare April 4, 2023 21:34
@newhoggy
Copy link
Contributor

newhoggy commented Apr 4, 2023

FWIW, I think lenses are way easier than type families provided the lens API is discoverable.

In your example, how easy is it to find all the accessors for the expression:tx ^. bodyTxL?

For example you have outputsTxBodyL. What other fields are there?

@lehins
Copy link
Contributor Author

lehins commented Apr 4, 2023

In your example, how easy is it to find all the accessors for the expression: tx ^. bodyTxL?
For example you have outputsTxBodyL. What other fields are there?

Those are great questions! We have a proper cardano-ledger-api in the works. For now we export all of the lenses, but there will be a lot more exported later with proper documentation and examples. For instance here are all the lenses that are available for TxBody: https://github.com/input-output-hk/cardano-ledger/blob/302e46f4d83e5ad8dafec68130ea5b3c07942f52/libs/cardano-ledger-api/src/Cardano/Ledger/Api/Tx/Body.hs#L12-L56

@lehins lehins force-pushed the lehins/update-ledger-conway branch 3 times, most recently from cb79e12 to b1b4e38 Compare April 11, 2023 14:20
* Account for recent changes in consensus and ledger
* Reduce a whole lot of duplication
* Move all orphan instances for types defined in cardano-base and
  cardano-ledger into their own respective packages
* Fix a bug where only metadata from TxAuxData was hashed upon
  transaction body creation with `createTransactionBody`
* Change representation of CostModel. It is no longer a mapping from
  param name to values, but instead a list with values, where order
  of value dictates the mapping to param names of a plutus
  cost model for a particular plutus version
* ToJSON instance for CostModel and consequently for ProtocolParameters
  will now produce a list of values instead of a key value mapping.
@lehins lehins force-pushed the lehins/update-ledger-conway branch from 4dbf214 to b5265b5 Compare April 14, 2023 17:37
@lehins lehins enabled auto-merge April 14, 2023 18:38
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.

6 participants