Avoid `IsShelleyBasedEra` and `IsCardanoEra` where possible #313

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

Changelog

- description: |
    Remove `IsShelleyBasedEra` and `IsCardanoEra` from all functions and types.
    Use `ShelleyBasedEra` and `CardanoEra` instead.
# 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

We often use a combination of witness and constraints, sometimes inconsistently.

This PR standardises on the the use of witnesses because those are eon-friendly, more flexible and have better type-inference.

Removal of the constraints means that callers no longer have to satisfy those constraints.

Checklist

Commit sequence broadly makes sense and commits have useful messages
The change log section in the PR description has been filled in
New tests are added if needed and existing tests are updated. These may include:
- golden tests
- property tests
- round trip tests
- integration tests
  See Running tests for more details
The version bounds in .cabal files are updated
CI passes. See note on CI. The following CI checks are required:
- Code is linted with hlint. See .github/workflows/check-hlint.yml to get the hlint version
- Code is formatted with stylish-haskell. See .github/workflows/stylish-haskell.yml to get the stylish-haskell version
- Code builds on Linux, MacOS and Windows for ghc-8.10.7 and ghc-9.2.7
Self-reviewed the diff

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

newhoggy · 2023-10-12T13:37:34Z

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

-        [ byronAddressInEra   <$> genAddressByron
-        , shelleyAddressInEra <$> genAddressShelley
+        [ byronAddressInEra       <$> genAddressByron
+        , shelleyAddressInEra sbe <$> genAddressShelley


shelleyAddressInEra is an example of a function that previously demanded a constraint, but now demands a witness instead. It is often the case that the witness is just available somewhere in the context.

newhoggy · 2023-10-12T13:38:54Z

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

-  parseJSON = withText "AddressInEra" $ \txt -> do
-    addressAny <- runParsecParser parseAddressAny txt
-    pure $ anyAddressInShelleyBasedEra addressAny
+  parseJSON =


We still retain the use of IsShelleyBasedEra and IsCardanoEra constraints for type class instances for now because we don't yet have a way to express these.

newhoggy · 2023-10-12T13:40:09Z

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

+
+  ShelleyBasedEra
+    :: ShelleyBasedEra era
+    -> CardanoEraStyle era


There is no need to embed both the constraint and the witness in the same constructor. They are convertible to each other.

newhoggy · 2023-10-12T13:41:00Z

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

+  in
+  case tx of
+    ShelleyTx _ tx' ->
+      let x = shelleyBasedEraConstraints sbe $ tx' ^. L.sizeTxF in Lovelace (a * x + b)


Often we need more constraints than just IsShelleyBasedEra so demanding it doesn't really help. We can summon all the constraints we need with just the eon witness.

newhoggy · 2023-10-12T13:43:24Z

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

+    ShelleyTxBody _ txbody _ _ _ _ -> makeShelleyBasedBootstrapWitness sbe nwOrAddr txbody sk
+
+makeShelleyBasedBootstrapWitness :: forall era. ()
+  => ShelleyBasedEra era


An example of where the witness is sufficient to summon all the constrains we need.

newhoggy · 2023-10-12T13:44:34Z

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

-      LegacyByronEra      -> makeByronTransactionBody
-      ShelleyBasedEra sbe -> makeShelleyTransactionBody sbe
+createAndValidateTransactionBody era =
+  case cardanoEraStyle era of


Using witness is less messy because we avoid having to give type-hints.

newhoggy · 2023-10-12T13:46:16Z

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

@@ -1840,8 +1840,7 @@ fromConwayPParams :: BabbageEraPParams ledgerera
                  -> ProtocolParameters
 fromConwayPParams = fromBabbagePParams

-checkProtocolParameters
-  :: forall era. IsCardanoEra era
+checkProtocolParameters :: ()
  => ShelleyBasedEra era


Sometimes we demanded both the witness and constraint and they don't match.

newhoggy · 2023-10-12T13:51:30Z

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

          (\w -> TxOutValue w (lovelaceToValue (Lovelace (2^(64 :: Integer)) - 1) <> nonAdaChange))
-          (cardanoEra @era)


Code that uses constraints are harder to maintain. For example we use cardanoEra @era here despite era' already being defined further down.

Jimbo4350

Can you open a PR in cardano-cli before we merge this? I see we only use IsShelleyBasedEra twice in cardano-cli so it should be fine.

newhoggy · 2023-10-14T14:40:06Z

Can you open a PR in cardano-cli before we merge this? I see we only use IsShelleyBasedEra twice in cardano-cli so it should be fine.

There is a POC here. Will clean up the POC some more.

IntersectMBO/cardano-cli#376

Jimbo4350

LGTM!

…lleyBasedEra module

…edEra

…ead of IsCardanoEra.

github-advanced-security bot found potential problems Oct 12, 2023

View reviewed changes

cardano-api/test/cardano-api-test/Test/Cardano/Api/Typed/CBOR.hs Fixed Show fixed Hide fixed

newhoggy commented Oct 12, 2023

View reviewed changes

newhoggy force-pushed the newhoggy/less-reliance-on-IsShelleyBasedEra branch from 97c880e to 099db6e Compare October 12, 2023 13:45

newhoggy commented Oct 12, 2023

View reviewed changes

newhoggy force-pushed the newhoggy/less-reliance-on-IsShelleyBasedEra branch from 099db6e to 153ac0b Compare October 12, 2023 13:53

newhoggy marked this pull request as ready for review October 12, 2023 13:53

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

Jimbo4350 reviewed Oct 12, 2023

View reviewed changes

newhoggy force-pushed the newhoggy/less-reliance-on-IsShelleyBasedEra branch 2 times, most recently from 3eb911a to 6dd0cce Compare October 14, 2023 14:34

newhoggy mentioned this pull request Oct 14, 2023

Avoid IsShelleyBasedEra and IsCardanoEra where possible IntersectMBO/cardano-cli#376

Closed

9 tasks

newhoggy requested a review from Jimbo4350 October 15, 2023 01:11

Jimbo4350 approved these changes Oct 16, 2023

View reviewed changes

newhoggy force-pushed the newhoggy/less-reliance-on-IsShelleyBasedEra branch from 6dd0cce to 5eb9017 Compare October 18, 2023 04:59

newhoggy requested a review from a team as a code owner October 18, 2023 05:45

newhoggy added 3 commits October 18, 2023 21:53

Move ShelleyBasedEraConstraints and shelleyBasedEraConstraints to She…

5f9537f

…lleyBasedEra module

Move CardanoEraConstraints and cardanoraConstraints to Core module

99f27e8

New functions anyShelleyBasedEra, inAnyCardanoEra and inAnyShelleyBas…

d1d6341

…edEra

Use ShelleyBasedEra instead of IsShelleyBasedEra. Use CardanoEra inst…

43027e5

…ead of IsCardanoEra.

newhoggy force-pushed the newhoggy/less-reliance-on-IsShelleyBasedEra branch from fdf5bad to 43027e5 Compare October 18, 2023 10:59

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

Merged via the queue into main with commit 44ffb48 Oct 18, 2023
20 checks passed

newhoggy deleted the newhoggy/less-reliance-on-IsShelleyBasedEra branch October 18, 2023 21:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid `IsShelleyBasedEra` and `IsCardanoEra` where possible #313

Avoid `IsShelleyBasedEra` and `IsCardanoEra` where possible #313

newhoggy commented Oct 12, 2023 •

edited

Loading

newhoggy Oct 12, 2023

newhoggy Oct 12, 2023

newhoggy Oct 12, 2023

newhoggy Oct 12, 2023 •

edited

Loading

newhoggy Oct 12, 2023

newhoggy Oct 12, 2023

newhoggy Oct 12, 2023

newhoggy Oct 12, 2023

Jimbo4350 left a comment •

edited

Loading

newhoggy commented Oct 14, 2023

Jimbo4350 left a comment

		(\w -> TxOutValue w (lovelaceToValue (Lovelace (2^(64 :: Integer)) - 1) <> nonAdaChange))
		(cardanoEra @era)

Avoid IsShelleyBasedEra and IsCardanoEra where possible #313

Avoid IsShelleyBasedEra and IsCardanoEra where possible #313

Conversation

newhoggy commented Oct 12, 2023 • edited Loading

Changelog

Context

Checklist

newhoggy Oct 12, 2023

Choose a reason for hiding this comment

newhoggy Oct 12, 2023

Choose a reason for hiding this comment

newhoggy Oct 12, 2023

Choose a reason for hiding this comment

newhoggy Oct 12, 2023 • edited Loading

Choose a reason for hiding this comment

newhoggy Oct 12, 2023

Choose a reason for hiding this comment

newhoggy Oct 12, 2023

Choose a reason for hiding this comment

newhoggy Oct 12, 2023

Choose a reason for hiding this comment

newhoggy Oct 12, 2023

Choose a reason for hiding this comment

Jimbo4350 left a comment • edited Loading

Choose a reason for hiding this comment

newhoggy commented Oct 14, 2023

Jimbo4350 left a comment

Choose a reason for hiding this comment

Avoid `IsShelleyBasedEra` and `IsCardanoEra` where possible #313

Avoid `IsShelleyBasedEra` and `IsCardanoEra` where possible #313

newhoggy commented Oct 12, 2023 •

edited

Loading

newhoggy Oct 12, 2023 •

edited

Loading

Jimbo4350 left a comment •

edited

Loading