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.

Sign up for GitHub

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

Jump to bottom

Single `TxOutValue` constructor #366

Closed

newhoggy wants to merge 4 commits into main from newhoggy/single-TxOutValue-constructor

Collaborator

newhoggy commented Nov 12, 2023 •

edited

Loading

Changelog

- description: |
    Changes:
    - `TxOutValue` now has only one `TxOutValue` constructor and is no longer a GADT
    - Delete:
      - `ShelleyToAllegraEra`
      - `caseShelleyToAllegraOrMaryEraOnwards`
    - Add:
      - `ByronToAllegraEra` and related functions
      - `caseByronToAllegraOrMaryEraOnwards`
      - `caseByronOnlyOrShelleyToAllegraOrMaryEraOnwards`
      - `lovelaceToCoin`
      - `coinToLovelace`
    - Functions now taking `CardanoEra` instead:
      - `genValueForTxOut`
      - `mkAdaValue`
      - `adaAssetL
      - `negateLedgerValue`
      - `fromLedgerValue`
    - Functions now taking `ByronToAllegraEra` instead:
      - `adaAssetShelleyToAllegraEraL`
    - Type class instances no longer requiring `IsCardanoEra era`:
      - `ToJSON (TxOutValue era)`
      - `FromJSON (TxOutValue era)`
    - Functions now taking `MaryEraOnwards`:
      - `fromMaryValue`
# 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 builds on #361

Review without the first comment to ignore changes from #361.

The most substantive change here is that the data TxOutValue now only has one constructor which allows it to not be a GADT.

The adjustment simplifies some code. For instance Byron is now unified with all other eras and casing is only necessary when using era-specific features. Notably accessing the ADA asset feature can be uniform across all eras.

Not being a GADT also eliminates the need for some empty cases.

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


          Merge ShelleyLedgerEra and CardanoLedgerEra to a single type family L…

d30607b

…edgerEra

newhoggy commented

View reviewed changes

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

@@ @@ -59,6 +61,8 @@ import qualified Data.Text as Text @@
               import           Data.Type.Equality (TestEquality (..), (:~:) (Refl))
               import           Data.Typeable (Typeable, showsTypeRep, typeOf)
+              type instance L.Value (L.ByronEra _c) = L.Coin

Collaborator Author

newhoggy Nov 12, 2023 •

edited

Loading

This is the only ledger adapter we need to unify all the constructors in TxOutValue.

This can be moved into ledger if it would be accepted. Otherwise it can stay here as it is only one line.

newhoggy commented

View reviewed changes

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

+                      . showString "TxOutValue "
+                      . showsPrec 11 era
+                      . showString " "
+                      . showsPrec 11 v

Collaborator Author

newhoggy Nov 12, 2023 •

edited

Loading

We can rely on CardanoEra argument and cardanoEraConstraints to implement instances so there isn't a need to carry any additional constraints in the constructors.

newhoggy force-pushed the newhoggy/single-TxOutValue-constructor branch from a07e6e4 to fca1c36 Compare

November 12, 2023 13:48

newhoggy commented

View reviewed changes

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

+              data TxOutValue era =
+                TxOutValue
+                  (CardanoEra era)
+                  (L.Value (LedgerEra era))

Collaborator Author

newhoggy Nov 12, 2023

We now only have one constructor spanning all eras which also means we don't need a GADT.

newhoggy force-pushed the newhoggy/single-TxOutValue-constructor branch from fca1c36 to 198666a Compare

November 12, 2023 13:53

newhoggy commented

View reviewed changes

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

-                  (\w -> TxOutValueByron w ll)
-                  (\w -> TxOutValueShelleyBased w $ A.mkAdaValue w $ lovelaceToCoin ll)
-                  era
+                TxOutValue era $ A.mkAdaValue era $ lovelaceToCoin ll

Collaborator Author

newhoggy Nov 12, 2023 •

edited

Loading

The unification means that as long as client keeps to lens to access the ADA asset (not the multi-asset feature), no constraints summoning nor casing is necessary.

newhoggy commented

View reviewed changes

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


		TxOut (AddressInEra ByronAddressInAnyEra (ByronAddress _)) (TxOutValueShelleyBased w _) _ _ -> case w of {}

Collaborator Author

newhoggy Nov 12, 2023 •

edited

Loading

The unification means some cases like this go away.


          Ledger Value type instance for ByronEra

12a3bca

newhoggy force-pushed the newhoggy/single-TxOutValue-constructor branch from 198666a to 12a3bca Compare

November 12, 2023 13:57

newhoggy commented

View reviewed changes

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

@@ @@ -3099,10 +3084,7 @@ toShelleyTxOutAny :: forall ctx era ledgerera. @@
                               -> TxOut ctx era
                               -> Ledger.TxOut ledgerera
               toShelleyTxOutAny sbe = \case
-                TxOut _ (TxOutValueByron ByronEraOnlyByron _) _ _ ->
-                  case sbe of {}

Collaborator Author

newhoggy Nov 12, 2023

The unification means some cases like this go away.

newhoggy commented

View reviewed changes

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

@@ @@ -697,11 +695,8 @@ toShelleyTxOut :: forall era ledgerera. @@
                              => ShelleyBasedEra era
                              -> TxOut CtxUTxO era
                              -> Ledger.TxOut ledgerera
-              toShelleyTxOut sbe = \case -- jky simplify
-                TxOut _ (TxOutValueByron ByronEraOnlyByron _) _ _ ->
-                  case sbe of {}

Collaborator Author

newhoggy Nov 12, 2023

Unification means empty cases like this go away.

newhoggy commented

View reviewed changes

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

-                TxOut (AddressInEra ByronAddressInAnyEra (ByronAddress addr)) (TxOutValueByron _ value) _ _ ->
-                  Byron.TxOut addr <$> toByronLovelace value
-                TxOut (AddressInEra ByronAddressInAnyEra (ByronAddress _)) (TxOutValueShelleyBased w _) _ _ ->
-                  case w of {}

Collaborator Author

newhoggy Nov 12, 2023

Unification means empty cases like this go away.


          New ByronToAllegraEra eon

eb4d09a

newhoggy force-pushed the newhoggy/single-TxOutValue-constructor branch 2 times, most recently from ad68950 to 728c769 Compare

November 13, 2023 12:34

newhoggy changed the title ~~Newhoggy/single tx out value constructor~~ Single TxOutValue constructor


          Delete ShelleyToAllegraEra eon

7b6ca66

newhoggy force-pushed the newhoggy/single-TxOutValue-constructor branch from 728c769 to 7b6ca66 Compare

November 13, 2023 12:37

newhoggy marked this pull request as ready for review

November 14, 2023 09:48

newhoggy requested review from dcoutts, Jimbo4350, erikd, carbolymer, CarlosLopezDeLara and disassembler as code owners

November 14, 2023 09:48

newhoggy closed this

newhoggy added a commit that referenced this pull request


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

935d5e8

…es-for-governance-actions-commands

Make `governance action create-protocol-parameters-update` Conway onwards only

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

dcoutts Awaiting requested review from dcoutts

Jimbo4350 Awaiting requested review from Jimbo4350

erikd Awaiting requested review from erikd

carbolymer Awaiting requested review from carbolymer

CarlosLopezDeLara Awaiting requested review from CarlosLopezDeLara

disassembler Awaiting requested review from disassembler

Labels

None yet