Improved validity range handling #332

newhoggy · 2023-10-20T01:11:36Z

Changelog

- description: |
    Split the `txValidityRange` field into two fields.
    New function `allegraEraOnwardsToByronAndAllegraOnwardsEra`
# 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

Split the txValidityRange field into two fields. The two values are represented in the field as two parts to a tuple which makes it irregular and unwieldy.

Ledger groups them together but we don't necessarily need to follow what the ledger does and in any case the ledger does not use a tuple to do the grouping.

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

newhoggy · 2023-10-20T13:04:46Z

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

-genTxValidityRange era =
-  (,)
-    <$> genTxValidityLowerBound era
-    <*> genTxValidityUpperBound era


Without tupling, we no longer need this generator.

newhoggy · 2023-10-20T13:06:52Z

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

    -> TxValidityUpperBound era

  TxValidityUpperBound
    :: ShelleyBasedEra era
-    -> SlotNo
+    -> Maybe SlotNo


This is slightly closer the to ledger representation since the ledger uses SMaybe SlotNo.

TxValidityNoUpperBound simplifies to ByronEraOnly because it is no longer used to represent no bounds set for Shelley onwards. Instead TxValidityUpperBound Nothing serves this purpose instead.

newhoggy · 2023-10-20T13:07:35Z

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

-       txValidityRange      :: (TxValidityLowerBound era,
-                              TxValidityUpperBound era),
+       txValidityLowerBound :: TxValidityLowerBound era,
+       txValidityUpperBound :: TxValidityUpperBound era,


This is the core of the PR. We are splitting this field into to make TxBodyContent more regular.

newhoggy · 2023-10-20T13:09:36Z

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

@@ -1837,15 +1840,11 @@ createTransactionBody sbe txBodyContent =
  in case sbe of
       ShelleyBasedEraShelley -> do
        update <- convTxUpdateProposal sbe (txUpdateProposal txBodyContent)
-        let (_, upperBound) = txValidityRange txBodyContent


Notice also how the old code retrieves both the lower and upper bounds then throws away the lower bound. This no longer happens in the new code. We just retrieve what we need and throw nothing away.

newhoggy · 2023-10-20T13:10:24Z

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

@@ -1818,7 +1822,6 @@ createTransactionBody sbe txBodyContent =
      certs = convCertificates sbe $ txCertificates txBodyContent
      txAuxData = toAuxiliaryData sbe (txMetadata txBodyContent) (txAuxScripts txBodyContent)
      scripts = convScripts apiScriptWitnesses
-      validityInterval = convValidityInterval $ txValidityRange txBodyContent


This is duplication. We call txValidityRange here and then later again here: https://github.com/input-output-hk/cardano-api/pull/332/files#r1366954769

newhoggy · 2023-10-20T13:11:35Z

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

+--
+-- 'invalidHereAfterTxBodyL' lens over both with a 'Maybe SlotNo' type representation.  Withing the
+-- Shelley era, setting Nothing will set the ttl to 'maxBound' in the underlying ledger type.
+invalidHereAfterTxBodyL :: ShelleyBasedEra era -> Lens' (L.TxBody (ShelleyLedgerEra era)) (Maybe SlotNo)


This compatibility lens allows us to treat the upper bound the same in the shelley era and subsequent eras.

newhoggy · 2023-10-20T13:12:15Z

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

-                & L.ttlTxBodyL    .~ ttl
-                & L.updateTxBodyL .~ update
+                & L.certsTxBodyL                .~ certs
+                & L.invalidHereAfterTxBodyL sbe .~ convValidityUpperBound sbe (txValidityUpperBound txBodyContent)


In the Shelley era we set the upper bound just as we do in subsequent eras thanks to this lens: https://github.com/input-output-hk/cardano-api/pull/332/files#r1366957308

newhoggy · 2023-10-20T13:13:32Z

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

+        Nothing -> TxValidityNoLowerBound
+        Just s  -> TxValidityLowerBound w s
+    )
+    sbe


Using eons and the split of the validity range tuple, we can reduce a large case expression into a two small functions above and below.

That's a win!

newhoggy · 2023-10-20T13:15:16Z

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

-           & L.updateTxBodyL .~ update
-           & L.ttlTxBodyL    .~ case upperBound of
-                                  TxValidityNoUpperBound era' -> case era' of {}
-                                  TxValidityUpperBound _ ttl  -> ttl


Adhoc case matching and Shelley era specific code eliminated.

newhoggy · 2023-10-20T13:17:36Z

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

+                & L.certsTxBodyL                .~ certs
+                & L.updateTxBodyL               .~ update
+                & L.invalidBeforeTxBodyL aOn    .~ convValidityLowerBound (txValidityLowerBound txBodyContent)
+                & L.invalidHereAfterTxBodyL sbe .~ convValidityUpperBound sbe (txValidityUpperBound txBodyContent)


The above two lens required a eon each. This will in a separate PR serve to help reduce the amount of code by allowing the large case expression in this function to be eliminated.

newhoggy · 2023-10-20T13:18:27Z

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

@@ -1857,12 +1856,15 @@ createTransactionBody sbe txBodyContent =
              apiScriptValidity

       ShelleyBasedEraAllegra -> do
+        let aOn = AllegraEraOnwardsAllegra


Some additional lines to define the eon witness. These are hard coded currently because we still have the large case expression over sbe. When the case expression is eliminated, this will disappear.

carlhammann

I don't feel qualified to approve this, but since @carbolymer has already done so, I'm only leaving a few comments.

carlhammann · 2023-10-23T11:22:04Z

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

@@ -241,6 +242,16 @@ shelleyToAlonzoEraToShelleyToBabbageEra = \case
  ShelleyToAlonzoEraMary -> ShelleyToBabbageEraMary
  ShelleyToAlonzoEraAlonzo -> ShelleyToBabbageEraAlonzo

+allegraEraOnwardsToByronAndAllegraOnwardsEra :: ()


I don't see this being used anywhere?

You are right in that it isn't used. I had intended to use it but later opted to do it differently instead. The function is still potentially useful.

We should not be adding more Eon related code. We are moving away from this.

carlhammann · 2023-10-23T11:24:54Z

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

+
+import           Lens.Micro
+
+strictMaybeL :: Lens' (StrictMaybe a) (Maybe a)


This is an Iso, but microlens doesn't have such a type. Why are we using microlens?

I don't have the background to that decision unfortunately.

The reason I heard is that microlens is much lighter which shortens compilation time. I don't know if there are any other.

carlhammann · 2023-10-23T11:25:59Z

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

+    s SNothing  Nothing  = SNothing
+    s SNothing  (Just x) = SJust x
+    s (SJust _) Nothing  = SNothing
+    s (SJust _) (Just x) = SJust x


Suggested change

s SNothing Nothing = SNothing

s SNothing (Just x) = SJust x

s (SJust _) Nothing = SNothing

s (SJust _) (Just x) = SJust x

s _ Nothing = SNothing

s _ (Just x) = SJust x

Thanks for the simplification. I took it further and did this:

s _ = maybe SNothing SJust

carlhammann · 2023-10-23T11:41:04Z

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

+        Nothing -> TxValidityNoLowerBound
+        Just s  -> TxValidityLowerBound w s
+    )
+    sbe


That's a win!

carlhammann · 2023-10-23T11:44:49Z

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

@@ -1301,8 +1302,11 @@ setTxReturnCollateral v txBodyContent = txBodyContent { txReturnCollateral = v }
 setTxFee :: TxFee era -> TxBodyContent build era -> TxBodyContent build era
 setTxFee v txBodyContent = txBodyContent { txFee = v }

-setTxValidityRange :: (TxValidityLowerBound era, TxValidityUpperBound era) -> TxBodyContent build era -> TxBodyContent build era
-setTxValidityRange v txBodyContent = txBodyContent { txValidityRange = v }
+setTxValidityLowerBound :: TxValidityLowerBound era -> TxBodyContent build era -> TxBodyContent build era


I know this and the other functions are here for completeness, but I don't see them being used. What's your opinion on using (automatically generated) lenses here as well?

I'd like to avoid anything that involves Template Haskell, but I'm partial to generic-lens.

New functions: * invalidBeforeTxBodyL * invalidBeforeL * invalidHereAfterL * invalidBeforeStrictL * invalidHereAfterStrictL * strictMaybeL

… bounds

CODEOWNERS: remove core-tech

newhoggy force-pushed the newhoggy/improved-validity-range-handling branch 2 times, most recently from 64b9bad to 6faa2e1 Compare October 20, 2023 13:02

newhoggy commented Oct 20, 2023

View reviewed changes

newhoggy marked this pull request as ready for review October 20, 2023 13:18

newhoggy requested review from dcoutts, Jimbo4350, erikd, carbolymer, CarlosLopezDeLara and disassembler as code owners October 20, 2023 13:18

carbolymer approved these changes Oct 23, 2023

View reviewed changes

carlhammann reviewed Oct 23, 2023

View reviewed changes

newhoggy added 7 commits October 23, 2023 23:21

New allegraEraOnwardsToByronAndAllegraOnwardsEra function

a506fd5

Simplify fromLedgerTxValidityRange

a739143

New Cardano.Api.Ledger.Lens module:

2f8dc0f

New functions: * invalidBeforeTxBodyL * invalidBeforeL * invalidHereAfterL * invalidBeforeStrictL * invalidHereAfterStrictL * strictMaybeL

Split txValidityRange into txValidityLowerBound and txValidityLowerBound

1a510f3

Simplify with lenses

5999d5a

Set invalidBefore and invalidHereAfter separately on txbody

e64d106

Treat shelley the same as other shelley based eras for validity upper…

69464d4

… bounds

newhoggy force-pushed the newhoggy/improved-validity-range-handling branch from 6faa2e1 to 69464d4 Compare October 23, 2023 12:26

newhoggy enabled auto-merge October 23, 2023 12:28

newhoggy added this pull request to the merge queue Oct 23, 2023

Merged via the queue into main with commit 807342e Oct 23, 2023
20 checks passed

newhoggy deleted the newhoggy/improved-validity-range-handling branch October 23, 2023 13:10

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

Merge pull request #332 from input-output-hk/sl/codeowners

b0e03ff

CODEOWNERS: remove core-tech

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improved validity range handling #332

Improved validity range handling #332

newhoggy commented Oct 20, 2023 •

edited

newhoggy Oct 20, 2023

newhoggy Oct 20, 2023

newhoggy Oct 20, 2023

newhoggy Oct 20, 2023

newhoggy Oct 20, 2023

newhoggy Oct 20, 2023

newhoggy Oct 20, 2023

newhoggy Oct 20, 2023 •

edited

carlhammann Oct 23, 2023

newhoggy Oct 20, 2023

newhoggy Oct 20, 2023

newhoggy Oct 20, 2023

carlhammann left a comment

carlhammann Oct 23, 2023

newhoggy Oct 23, 2023

Jimbo4350 Oct 23, 2023

carlhammann Oct 23, 2023

newhoggy Oct 23, 2023

carbolymer Oct 23, 2023

carlhammann Oct 23, 2023

newhoggy Oct 23, 2023

carlhammann Oct 23, 2023

carlhammann Oct 23, 2023 •

edited

newhoggy Oct 23, 2023


		import Lens.Micro

		strictMaybeL :: Lens' (StrictMaybe a) (Maybe a)

Improved validity range handling #332

Improved validity range handling #332

Conversation

newhoggy commented Oct 20, 2023 • edited

Changelog

Context

How to trust this PR

Checklist

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 Oct 20, 2023 • edited

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

carlhammann 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlhammann Oct 23, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

newhoggy commented Oct 20, 2023 •

edited

newhoggy Oct 20, 2023 •

edited

carlhammann Oct 23, 2023 •

edited