Use ledger presentation of multi-asset values directly. Lens to make this uniform over `ShelleyBasedEra` #360

newhoggy · 2023-11-08T14:39:56Z

Changelog

- description: |
    Use ledger presentation of multi-asset values directly.  Lens to make this uniform over `ShelleyBasedEra`.
    
    Delete `ByronToAllegraEra`.
    
    New module `Cardano.Api.Ledger.Lens`.
    
    Modify `TxOutValue` to have `TxOutValueByron` and `TxOutValueShelleyBased` instead of `TxOutAdaOnly` and `TxOutValue` respectively.  `TxOutValueShelleyBased` now directly uses the ledger type instead of the `Value` type. 
    
    These functions have changed to either `L.Value (ShelleyLedgerEra era)` instead of `Value` or eons or both:
    - `genValue`
    - `genValueDefault`
    - `genValueForMinting`
# uncomment types applicable to the change:
  type:
  # - feature        # introduces a new feature
  - breaking       # the API has changed in a breaking way
  # - compatible     # the API has changed but is non-breaking
  # - optimisation   # measurable performance improvements
  # - improvement    # QoL changes e.g. refactoring
  # - bugfix         # fixes a defect
  # - test           # fixes/modifies tests
  # - maintenance    # not directly related to the code
  # - release        # related to a new release preparation
  # - documentation  # change in code docs, haddocks...

Context

This PR introduces adapter lenses adaAssetL and multiAssetL.

The former provides a uniform way to access the ADA asset in ledger values uniformly across all of ShelleyBasedEra which simplifies code considerably.

The latter provides an easy way to access all the non-ADA assets from Mary onwards.

The change also deletes ByronToAllegraEra

Further simplifications are possible but are left to future PRs to keep this PR small.

How to trust this PR

Highlight important bits of the PR that will make the review faster. If there are commands the reviewer can run to observe the new behavior, describe them.

Checklist

Commit sequence broadly makes sense and commits have useful messages
New tests are added if needed and existing tests are updated. See Running tests for more details
Self-reviewed the diff

…multiAssetL

newhoggy · 2023-11-08T14:40:35Z

cardano-api/internal/Cardano/Api/Eon/ByronToAllegraEra.hs

-{-# LANGUAGE StandaloneDeriving #-}
-{-# LANGUAGE TypeFamilies #-}
-
-module Cardano.Api.Eon.ByronToAllegraEra


This PR deletes ByronToAllegraEra because we don't need it anymore.

This moves us towards to goal of having fewer eons.

newhoggy · 2023-11-08T14:41:10Z

cardano-api/internal/Cardano/Api/Eras/Case.hs

-  MaryEra    -> r MaryEraOnwardsMary
-  AlonzoEra  -> r MaryEraOnwardsAlonzo
-  BabbageEra -> r MaryEraOnwardsBabbage
-  ConwayEra  -> r MaryEraOnwardsConway


Deletes a case function because we don't need it anymore.

newhoggy · 2023-11-08T14:43:11Z

cardano-api/internal/Cardano/Api/Ledger/Lens.hs

+  caseShelleyToAllegraOrMaryEraOnwards
+    adaAssetShelleyToAllegraEraL
+    adaAssetMaryEraOnwardsL
+    sbe


We use lenses to present a consistent interface across all Shelley based eras. With this single lens we can access the ADA value inside the L.Value ledgerera.

It is a direct field lookup, not a map lookup like selectLovelace in cardano-api.

We need to move this lens to cardano-ledger in a follow up PR.

newhoggy · 2023-11-08T14:49:49Z

cardano-api/internal/Cardano/Api/TxBody.hs

+        )
+    => ShelleyBasedEra era
+    -> L.Value (ShelleyLedgerEra era)
+    -> TxOutValue era


This definition has two constructors like before, the main difference is that all Shelley eras are now treated the same and only Byron is different. This is as it should be because the ledger treats Byron as legacy so it doesn't even have lens support.

The TxOutValueByron constructor will eventually be removed because:

makeByronTransactionBody :: () => ByronEraOnly era -> TxBodyContent BuildTx era -> Either TxBodyError (TxBody era)

We don't need TxBodyContent BuildTx Era to construct a Byron tx as the majority of the fields aren't used in Byron. We are only interested in inputs, outputs and update proposals and we can parameterize makeByronTransactionBody as such.

We will follow the approach of ledger the ledger towards the Byron era as we don't want consumers of the api who are interested only in the latest shelley era dealing with Byron related types.

Most places where TxOutValueByron is called we see usage of the EmptyCase pragma either for the Byron era or the ShelleyBasedEra so the code is also hinting this to us.

newhoggy · 2023-11-08T14:50:23Z

cardano-api/internal/Cardano/Api/TxBody.hs

-    TxOutAdaOnly _ l -> l
-    TxOutValue _ v -> selectLovelace v
+    TxOutValueByron         _   l -> l
+    TxOutValueShelleyBased  sbe v -> coinToLovelace $ v ^. A.adaAssetL sbe


Example of where we use adaAssetL to access the ADA value.

newhoggy · 2023-11-08T14:51:11Z

cardano-api/internal/Cardano/Api/TxBody.hs

+          & L.datumTxOutL .~ toBabbageTxOutDatum' txoutdata
+          & L.referenceScriptTxOutL .~ refScriptToShelleyScript cEra refScript
+  where
+    cEra = shelleyBasedToCardanoEra sbe


Opportunity to simplify because L.mkBasicTxOut (toShelleyAddr addr) value is repeated. This wasn't the case previously.

newhoggy · 2023-11-08T14:52:07Z

cardano-api/internal/Cardano/Api/Value.hs

+lovelaceToCoin (Lovelace ll) = Shelley.Coin ll
+
+coinToLovelace :: Shelley.Coin -> Lovelace
+coinToLovelace (Shelley.Coin ll) = Lovelace ll


We could delete Lovelace and use the Coin ledger type directly to avoid conversions.

We should in another PR.

newhoggy · 2023-11-08T14:53:24Z

cardano-api/internal/Cardano/Api/Value.hs

+  caseShelleyToAllegraOrMaryEraOnwards
+    (const (coinToValue v))
+    (const (fromMaryValue v))
+    sbe


These conversions were functions were introduced to convert between Value and L.Value ledgerera. Value could actually be deleted if we use the ledger type directly everywhere and then these conversion functions go away.

cardano-api/internal/Cardano/Api/TxBody.hs

…implify the code by deleting the feature we no longer use

newhoggy · 2023-11-08T15:15:10Z

cardano-api/internal/Cardano/Api/TxBody.hs

-
-toShelleyTxOut _ (TxOut addr (TxOutValue MaryEraOnwardsAlonzo value) txoutdata _) =
-    L.mkBasicTxOut (toShelleyAddr addr) (toMaryValue value)
-    & L.dataHashTxOutL .~ toAlonzoTxOutDataHash txoutdata


This appears to be Alonzo only. Deleting this can simplify the code a lot.

newhoggy · 2023-11-09T06:40:54Z

cardano-api/cardano-api.cabal

@@ -223,6 +223,7 @@ library
                        -- exposed by cardano-api-ledger
                        Cardano.Api.Ledger

+  reexported-modules:   Cardano.Api.Ledger.Lens


New export so we can use eon enabled from the CLI and from tests.

Why not just expose it?

The code needs to live in the internal component because code in internal uses it. This doesn't apply to the other modules because those are never imported by code in internal.

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

newhoggy · 2023-11-09T06:56:38Z

cardano-api/internal/Cardano/Api/TxBody.hs

-                        -> StrictMaybe (L.DataHash StandardCrypto)
-                        -> TxOutDatum ctx era
-fromAlonzoTxOutDataHash _    SNothing  = TxOutDatumNone
-fromAlonzoTxOutDataHash s (SJust dh)   = TxOutDatumHash s (ScriptDataHash dh)


A bunch of alonzo specific code disappears because we remove the feature that only worked in Alonzo and choose to no longer support.

Datum hashes weren't deprecated. See in ledger: instance Crypto c => AlonzoEraTxOut (BabbageEra c) where. This is removing the possibility for users to specify datum hashes instead of inline datums!

newhoggy · 2023-11-09T06:57:45Z

cardano-api/internal/Cardano/Api/TxBody.hs

-  SNothing -> TxOutDatumNone
-  SJust dh
-    | Just d <- Map.lookup dh txdatums  -> TxOutDatumInTx' w (ScriptDataHash dh) (fromAlonzoData d)
-    | otherwise                         -> TxOutDatumHash w (ScriptDataHash dh)


A bunch of alonzo specific code disappears because we remove the feature that only worked in Alonzo and choose to no longer support.

newhoggy · 2023-11-09T06:58:10Z

cardano-api/internal/Cardano/Api/TxBody.hs

-toShelleyTxOutAny sbe (TxOut addr (TxOutValue MaryEraOnwardsBabbage value) txoutdata refScript) =
-    let cEra = shelleyBasedToCardanoEra sbe
-    in L.mkBasicTxOut (toShelleyAddr addr) (toMaryValue value)
-       & L.datumTxOutL .~ toBabbageTxOutDatum' txoutdata


Being able to remove this Alonzo specific one-liner allows toShelleyTxOutAny to be implemented with a single eon case.

newhoggy · 2023-11-09T07:00:42Z

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

-            lookupDRepDeposit
-            isRegPool
-            (toLedgerUTxO sbe utxo)
-            txbody


Because the lens adapter makes the ADA asset interface consistent across all of ShelleyBasedEra we no longer need to case on the eon to implement evaluateTransactionBalance.

newhoggy · 2023-11-09T07:02:01Z

cardano-api/internal/Cardano/Api/TxBody.hs

-          sbe
-
-  let addressInEra :: AddressInEra era
-      addressInEra = shelleyBasedEraConstraints sbe $ fromShelleyAddr sbe $ ledgerTxOut ^. L.addrTxOutL


More simplifications because the eon-enabled lens adapter makes ADA Asset appear uniform across all of ShelleyBasedEra.

newhoggy · 2023-11-09T07:03:32Z

cardano-api/cardano-api.cabal

@@ -170,6 +169,7 @@ library internal
                      , errors
                      , filepath
                      , formatting
+                      , groups


We import groups because ledger implements Group for multi-asset values which we can use to negate the entire L.Value ledgerera.

Jimbo4350 · 2023-11-09T13:22:21Z

cardano-api/internal/Cardano/Api/Ledger/Lens.hs

+  caseShelleyToAllegraOrMaryEraOnwards
+    adaAssetShelleyToAllegraEraL
+    adaAssetMaryEraOnwardsL
+    sbe


We need to move this lens to cardano-ledger in a follow up PR.

Jimbo4350 · 2023-11-09T13:28:04Z

cardano-api/cardano-api.cabal

@@ -223,6 +223,7 @@ library
                        -- exposed by cardano-api-ledger
                        Cardano.Api.Ledger

+  reexported-modules:   Cardano.Api.Ledger.Lens


Why not just expose it?

Jimbo4350 · 2023-11-09T13:29:00Z

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

-      evalMultiAsset
-      sbe
+  shelleyBasedEraConstraints sbe
+    $ TxOutValueShelleyBased sbe


Jimbo4350 · 2023-11-09T13:34:24Z

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

@@ -828,21 +809,22 @@ makeTransactionBodyAutoBalance sbe systemstart history lpp@(LedgerProtocolParame
    -- However, since at this point we know how much non-Ada change to give
    -- we can use the true values for that.

-    let outgoingNonAda = mconcat [filterValue isNotAda v | (TxOut _ (TxOutValue _ v) _ _) <- txOuts txbodycontent]
-    let incomingNonAda = mconcat [filterValue isNotAda v | (TxOut _ (TxOutValue _ v) _ _) <- Map.elems $ unUTxO utxo]
+    let outgoingNonAda = mconcat [v | (TxOut _ (TxOutValueShelleyBased _ v) _ _) <- txOuts txbodycontent]


Where has the isNotAda filter gone?

In the old code, isNotAda removes the ADA asset, which is equivalent to setting the ADA asset value to zero. Later the ADA asset value is set by way of addition so the zero value is necessary:

lovelaceToValue (Lovelace (2^(64 :: Integer)) - 1) <> nonAdaChange

The new code doesn't both to set it to zero because instead of adding nonAdaChange to the ADA asset value we overwrite it with nonAdaChange instead:

change & A.adaAssetL sbe .~ lovelaceToCoin maxLovelaceChange

Jimbo4350 · 2023-11-09T13:35:06Z

cardano-api/internal/Cardano/Api/Ledger/Lens.hs

+mkBasicTxOut sbe addr value =
+  shelleyBasedEraConstraints sbe $ L.mkBasicTxOut addr value
+
+mkAdaValue :: ShelleyBasedEra era -> L.Coin -> L.Value (ShelleyLedgerEra era)


Another function that should live in the ledger

This can't move to the ledger as is because it used ShelleyBasedEra, but there is n eon-free version of this that can move to the ledger.

Yes, ledger can implement a function that constructs a Value regardless of the era.

Jimbo4350 · 2023-11-09T13:39:15Z

cardano-api/internal/Cardano/Api/Value.hs

+lovelaceToCoin (Lovelace ll) = Shelley.Coin ll
+
+coinToLovelace :: Shelley.Coin -> Lovelace
+coinToLovelace (Shelley.Coin ll) = Lovelace ll


We should in another PR.

cardano-api/internal/Cardano/Api/TxBody.hs

Jimbo4350 · 2023-11-09T13:50:12Z

cardano-api/internal/Cardano/Api/TxBody.hs

-                        -> StrictMaybe (L.DataHash StandardCrypto)
-                        -> TxOutDatum ctx era
-fromAlonzoTxOutDataHash _    SNothing  = TxOutDatumNone
-fromAlonzoTxOutDataHash s (SJust dh)   = TxOutDatumHash s (ScriptDataHash dh)


Datum hashes weren't deprecated. See in ledger: instance Crypto c => AlonzoEraTxOut (BabbageEra c) where. This is removing the possibility for users to specify datum hashes instead of inline datums!

newhoggy · 2023-11-09T14:21:33Z

cardano-api/internal/Cardano/Api/TxBody.hs

-toAlonzoTxOutDataHash' (TxOutDatumInTx' _ (ScriptDataHash dh) _) = SJust dh
-toAlonzoTxOutDataHash' (TxOutDatumInline inlineDatumSupp _sd) =
-  case inlineDatumSupp :: BabbageEraOnwards AlonzoEra of {}
-


The code above gets the hash and the code below also gets the hash as a Datum.

Comments addressed

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

Jimbo4350 · 2023-11-09T15:47:02Z

cardano-api/internal/Cardano/Api/Ledger/Lens.hs

+  , mkAdaValue
+
+    -- * Lenses
+  , strictMaybeL
  , L.invalidBeforeL


We need a clear distinction between ledger and cardano-api. We are mixing lenses here which is confusing.

Jimbo4350

Nice work. I would like to sort out Cardano.Api.Ledger and Cardano.Api.Ledger.Lens before we release again.

…es-for-key-commands Command argument types for `key` commands

newhoggy added 8 commits November 9, 2023 01:29

Remove need for ByronToAllegraEra eon

a52cb17

Delete ByronToAllegraEra eon

0bef5e3

Delete ByronToMaryEra

46ad8e4

New smart constructor: mkAdaOnlyTxOut; and New lenses: adaAssetL and …

f4fca13

…multiAssetL

Tidy up toShelleyTxOut

d5e61c9

Rename TxOutAdaOnlyByron to TxOutValueByron

e462623

Use ledger types for TxOutValue

5cd6506

Delete caseByronOrShelleyToAllegraOrMaryEraOnwards

589fb20

newhoggy commented Nov 8, 2023

View reviewed changes

cardano-api/internal/Cardano/Api/TxBody.hs Outdated Show resolved Hide resolved

The dataHashTxOutL appears to be a feature only used Alonzo. We can s…

984ebfa

…implify the code by deleting the feature we no longer use

newhoggy commented Nov 8, 2023

View reviewed changes

Simplify TxOut construction

0008725

newhoggy commented Nov 9, 2023

View reviewed changes

newhoggy force-pushed the newhoggy/improved-TxOutValue-code branch from f27b67e to cf84f53 Compare November 9, 2023 06:51

Rename binding names to be more accurate

9774286

newhoggy force-pushed the newhoggy/improved-TxOutValue-code branch from cf84f53 to 9774286 Compare November 9, 2023 06:53

newhoggy commented Nov 9, 2023

View reviewed changes

cardano-api/internal/Cardano/Api/Fees.hs Show resolved Hide resolved

newhoggy commented Nov 9, 2023

View reviewed changes

newhoggy marked this pull request as ready for review November 9, 2023 07:04

newhoggy requested review from dcoutts, Jimbo4350, erikd, carbolymer, CarlosLopezDeLara and disassembler as code owners November 9, 2023 07:04

newhoggy changed the title ~~Use ledger presentation of multi-asset values~~ Use ledger presentation of multi-asset values directly. Lens to make this uniform over ShelleyBasedEra Nov 9, 2023

carbolymer approved these changes Nov 9, 2023

View reviewed changes

Jimbo4350 previously requested changes Nov 9, 2023

View reviewed changes

newhoggy commented Nov 9, 2023

View reviewed changes

newhoggy requested a review from Jimbo4350 November 9, 2023 15:18

Jimbo4350 reviewed Nov 9, 2023

View reviewed changes

cardano-api/internal/Cardano/Api/Fees.hs Show resolved Hide resolved

Jimbo4350 reviewed Nov 9, 2023

View reviewed changes

Jimbo4350 approved these changes Nov 9, 2023

View reviewed changes

newhoggy added this pull request to the merge queue Nov 10, 2023

Merged via the queue into main with commit f39ae4d Nov 10, 2023
25 checks passed

newhoggy deleted the newhoggy/improved-TxOutValue-code branch November 10, 2023 00:54

newhoggy added a commit that referenced this pull request Mar 11, 2024

Merge pull request #360 from input-output-hk/newhoggy/command-arg-typ…

eb0e52b

…es-for-key-commands Command argument types for `key` commands

Use ledger presentation of multi-asset values directly. Lens to make this uniform over ShelleyBasedEra #360

Use ledger presentation of multi-asset values directly. Lens to make this uniform over ShelleyBasedEra #360

Conversation

newhoggy commented Nov 8, 2023 • edited Loading

Changelog

Context

How to trust this PR

Checklist

newhoggy Nov 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

newhoggy Nov 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jimbo4350 Nov 9, 2023 • edited Loading

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

Choose a reason for hiding this comment

newhoggy Nov 9, 2023 • edited Loading

Choose a reason for hiding this comment

newhoggy Nov 9, 2023 • edited Loading

Choose a reason for hiding this comment

Jimbo4350 Nov 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

newhoggy Nov 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

newhoggy Nov 9, 2023 • edited Loading

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

newhoggy Nov 9, 2023 • edited Loading

Choose a reason for hiding this comment

Jimbo4350 Nov 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jimbo4350 Nov 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jimbo4350 Nov 9, 2023 • edited Loading

Choose a reason for hiding this comment

Jimbo4350 left a comment

Choose a reason for hiding this comment

Use ledger presentation of multi-asset values directly. Lens to make this uniform over `ShelleyBasedEra` #360

Use ledger presentation of multi-asset values directly. Lens to make this uniform over `ShelleyBasedEra` #360

newhoggy commented Nov 8, 2023 •

edited

Loading

newhoggy Nov 8, 2023 •

edited

Loading

newhoggy Nov 8, 2023 •

edited

Loading

Jimbo4350 Nov 9, 2023 •

edited

Loading

newhoggy Nov 9, 2023 •

edited

Loading

newhoggy Nov 9, 2023 •

edited

Loading

Jimbo4350 Nov 9, 2023 •

edited

Loading

newhoggy Nov 9, 2023 •

edited

Loading

newhoggy Nov 9, 2023 •

edited

Loading

newhoggy Nov 9, 2023 •

edited

Loading

Jimbo4350 Nov 9, 2023 •

edited

Loading

Jimbo4350 Nov 9, 2023 •

edited

Loading

Jimbo4350 Nov 9, 2023 •

edited

Loading