Isolation of Byron era 1/n #362

Jimbo4350 · 2023-11-09T19:31:48Z

Changelog

- description: |
    Isolation of Byron era 1/n. Changes made to cardano-api where Byron era was never being called in the first place.
# 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

Additional context for the PR goes here. If the PR fixes a particular issue please provide a link to the issue.

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

Jimbo4350 · 2023-11-09T20:42:54Z

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

@@ -2010,8 +2009,11 @@ createAndValidateTransactionBody :: ()
  => CardanoEra era
  -> TxBodyContent BuildTx era
  -> Either TxBodyError (TxBody era)
-createAndValidateTransactionBody =
-  caseByronOrShelleyBasedEra makeByronTransactionBody makeShelleyTransactionBody
+createAndValidateTransactionBody era txBodyContent =


This is just a stop gap. We would have a separate function for ~~Byron~~ basic txs and parameterize createAndValidateTransactionBody on ShelleyBasedEra era instead.

Jimbo4350 · 2023-11-09T20:43:32Z

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

@@ -69,8 +69,7 @@ data TxInMode where
  -- delegation certs.
  --
  TxInByronSpecial
-    :: ByronEraOnly era
-    -> Consensus.GenTx Consensus.ByronBlock
+    :: Consensus.GenTx Consensus.ByronBlock


The evidence is no longer necessary. We are already constructing Consensus.GenTx Consensus.ByronBlock

I don't think it was necessary in the first place, but it was a matter of convenience. Before you could write

eon <- forEraMaybeEon pure $ TxInByronSpecial eon foo

and after the change:

_ :: ByronEraOnly <- forEarMaybeEon pure $ TxInByronSpecial foo

so now you need to say twice that you're in byron.

Can you show me where you now have to say twice that we are in Byron? This is only called in Byron specific code in the cli.

In the current usages you don't have to, because byron was hardcoded everywhere. My remark was about general usage, where you could create an era-specific value, mentioning era just once: in the value constructor.
If we're going to isolate and hardcode byron code, I guess it won't matter much.

Jimbo4350 · 2023-11-09T20:50:21Z

cardano-api/test/cardano-api-test/Test/Cardano/Api/Json.hs

@@ -27,27 +27,27 @@ prop_json_roundtrip_alonzo_genesis = H.property $ do

 prop_json_roundtrip_utxo :: Property
 prop_json_roundtrip_utxo = H.property $ do


We are only calling Babbage.

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

Update FromJSON (TxOutValue era) to be constrained by IsShelleyBasedEra era Parameterize lovelaceToTxOutValue on ShelleyBasedEra era instead of CardanoEra era

…lly makeTransactionBodyAutoBalance Note makeTransactionBodyAutoBalance accepts ShelleyBasedEra era and not CardanoEra era

carbolymer · 2023-11-15T12:24:32Z

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

@@ -711,12 +710,46 @@ genTxTotalCollateral =
 genTxFee :: CardanoEra era -> Gen (TxFee era)
 genTxFee =
  caseByronOrShelleyBasedEra
-    (pure . TxFeeImplicit)
+    undefined -- (pure . TxFeeImplicit)


a comment explaining why someone sees an exception from here would be nice

or maybe something like error "not supported" would be better here?

This was a mistake

carbolymer · 2023-11-15T12:55:36Z

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

@@ -30,21 +30,21 @@ import           Test.Tasty.Hedgehog (testProperty)

 prop_roundtrip_txbody_CBOR :: Property
 prop_roundtrip_txbody_CBOR = H.property $ do
-  AnyCardanoEra era <- H.forAll $ Gen.element [minBound..AnyCardanoEra BabbageEra]
+  AnyShelleyBasedEra era <- H.forAll $ Gen.element [minBound..maxBound]


Nice! I wonder why it was only until babbage before

Notice the tests were only calling the Babbage era

removed once we have specific functionality to create "basic" transactions in any era.

newhoggy · 2023-11-16T09:02:39Z

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

@@ -838,8 +840,7 @@ deriving instance Show (TxInsReference build era)
 data TxOutValue era where

  TxOutValueByron
-    :: ByronEraOnly era
-    -> Lovelace
+    :: Lovelace


If we're going the path of pushing Byron to legacy status and have it separate we should move this constructor to its own type and then to a Byron-specific legacy module so users of our API never have to see/worry/handle it.

Definitely. This is my eventual goal but I'm doing it in small chunks.

Add anchor to vote-create command

Jimbo4350 commented Nov 9, 2023

View reviewed changes

Jimbo4350 force-pushed the jordan/basic-tx branch from c18a2bf to f649544 Compare November 10, 2023 13:15

github-advanced-security bot found potential problems Nov 10, 2023

View reviewed changes

cardano-api/gen/Test/Gen/Cardano/Api/Typed.hs Fixed Show fixed Hide fixed

Jimbo4350 force-pushed the jordan/basic-tx branch 2 times, most recently from a81c5f9 to 6492b07 Compare November 10, 2023 17:48

Jimbo4350 changed the title ~~Jordan/basic tx~~ Separation of Byron era 1/n Nov 10, 2023

Jimbo4350 changed the title ~~Separation of Byron era 1/n~~ Isolation of Byron era 1/n Nov 10, 2023

Jimbo4350 marked this pull request as ready for review November 10, 2023 20:23

Jimbo4350 requested review from dcoutts, erikd, newhoggy, carbolymer, CarlosLopezDeLara and disassembler as code owners November 10, 2023 20:23

Jimbo4350 added 4 commits November 14, 2023 12:08

Modify makeByronTransactionBody to take only inputs and outputs

fa6c5d6

Remove ByronEraOnly era from TxInByronSpecial constructor

2be1739

Remove ByronEraOnly era from TxOutValueByron

48cb500

Update FromJSON (TxOutValue era) to be constrained by IsShelleyBasedEra era Parameterize lovelaceToTxOutValue on ShelleyBasedEra era instead of CardanoEra era

Propagate updated lovelaceToTxOutValue to Cardano.Api.Fees, specifica…

4735476

…lly makeTransactionBodyAutoBalance Note makeTransactionBodyAutoBalance accepts ShelleyBasedEra era and not CardanoEra era

Jimbo4350 force-pushed the jordan/basic-tx branch from 74d05e6 to dd496b2 Compare November 14, 2023 16:08

carbolymer reviewed Nov 15, 2023

View reviewed changes

carbolymer approved these changes Nov 15, 2023

View reviewed changes

Jimbo4350 added 4 commits November 15, 2023 09:05

Update generators to be parameterized on ShelleyBasedEra era

aea7867

Update cardano-api tests to reflect changes in generators

8c1d5c1

Notice the tests were only calling the Babbage era

Re-add Byron related roundtrip tests and generators

67b9e62

Necessary to allow creation of byron outputs in the cli. This will be

26114c9

removed once we have specific functionality to create "basic" transactions in any era.

Jimbo4350 force-pushed the jordan/basic-tx branch from dd496b2 to 26114c9 Compare November 15, 2023 13:05

Jimbo4350 enabled auto-merge November 15, 2023 13:06

Jimbo4350 added this pull request to the merge queue Nov 15, 2023

Merged via the queue into main with commit afa7d7c Nov 15, 2023
20 checks passed

Jimbo4350 deleted the jordan/basic-tx branch November 15, 2023 13:35

newhoggy reviewed Nov 16, 2023

View reviewed changes

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

Merge pull request #362 from input-output-hk/ch/vote-anchor

4001f53

Add anchor to vote-create command

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Isolation of Byron era 1/n #362

Isolation of Byron era 1/n #362

Jimbo4350 commented Nov 9, 2023 •

edited

Loading

Jimbo4350 Nov 9, 2023 •

edited

Loading

Jimbo4350 Nov 9, 2023 •

edited

Loading

carbolymer Nov 15, 2023

Jimbo4350 Nov 15, 2023 •

edited

Loading

carbolymer Nov 15, 2023

Jimbo4350 Nov 9, 2023

carbolymer Nov 15, 2023 •

edited

Loading

carbolymer Nov 15, 2023 •

edited

Loading

Jimbo4350 Nov 15, 2023

carbolymer Nov 15, 2023 •

edited

Loading

newhoggy Nov 16, 2023 •

edited

Loading

Jimbo4350 Nov 16, 2023

		@@ -27,27 +27,27 @@ prop_json_roundtrip_alonzo_genesis = H.property $ do

		prop_json_roundtrip_utxo :: Property
		prop_json_roundtrip_utxo = H.property $ do

Isolation of Byron era 1/n #362

Isolation of Byron era 1/n #362

Conversation

Jimbo4350 commented Nov 9, 2023 • edited Loading

Changelog

Context

How to trust this PR

Checklist

Jimbo4350 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

carbolymer Nov 15, 2023

Choose a reason for hiding this comment

Jimbo4350 Nov 15, 2023 • edited Loading

Choose a reason for hiding this comment

carbolymer Nov 15, 2023

Choose a reason for hiding this comment

Jimbo4350 Nov 9, 2023

Choose a reason for hiding this comment

carbolymer Nov 15, 2023 • edited Loading

Choose a reason for hiding this comment

carbolymer Nov 15, 2023 • edited Loading

Choose a reason for hiding this comment

Jimbo4350 Nov 15, 2023

Choose a reason for hiding this comment

carbolymer Nov 15, 2023 • edited Loading

Choose a reason for hiding this comment

newhoggy Nov 16, 2023 • edited Loading

Choose a reason for hiding this comment

Jimbo4350 Nov 16, 2023

Choose a reason for hiding this comment

Jimbo4350 commented Nov 9, 2023 •

edited

Loading

Jimbo4350 Nov 9, 2023 •

edited

Loading

Jimbo4350 Nov 9, 2023 •

edited

Loading

Jimbo4350 Nov 15, 2023 •

edited

Loading

carbolymer Nov 15, 2023 •

edited

Loading

carbolymer Nov 15, 2023 •

edited

Loading

carbolymer Nov 15, 2023 •

edited

Loading

newhoggy Nov 16, 2023 •

edited

Loading