Delete `TxFeesExplicitInEra` and `TxFeesImplicitInEra` #244

newhoggy · 2023-09-14T07:05:04Z

Changelog

- description: |
    Delete `TxFeesExplicitInEra` and `TxFeesImplicitInEra`
    New `ByronEraOnly` feature
    Move `ShelleyBasedEra` into its own module as it is a legitimate feature
# 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

Feature types make these types unnecessary.

Checklist

cardano-api/internal/Cardano/Api/Feature/ByronEraOnly.hs

newhoggy · 2023-09-14T07:28:18Z

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

@@ -983,7 +981,7 @@ makeTransactionBodyAutoBalance systemstart history lpp@(LedgerProtocolParameters
    let (dummyCollRet, dummyTotColl) = maybeDummyTotalCollAndCollReturnOutput txbodycontent changeaddr
    txbody1 <- first TxBodyError $ -- TODO: impossible to fail now
               createAndValidateTransactionBody txbodycontent1 {
-                 txFee  = TxFeeExplicit explicitTxFees $ Lovelace (2^(32 :: Integer) - 1),
+                 txFee  = TxFeeExplicit sbe $ Lovelace (2^(32 :: Integer) - 1),


We can just provide sbe as the witness instead of make up another one.

newhoggy · 2023-09-14T07:28:54Z

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

+  caseByronOrShelleyBasedEra
+    TxFeeImplicit
+    (\w -> TxFeeExplicit w mempty)
+    (cardanoEra @era)


We can using casing functions now, avoiding the need to enumerate all eras.

newhoggy · 2023-09-14T07:29:32Z

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

-      fromShelleyLovelace $ body ^. L.feeTxBodyL
+  shelleyBasedEraConstraints sbe
+    $ TxFeeExplicit sbe
+    $ fromShelleyLovelace $ body ^. L.feeTxBodyL


We can use sbe here as well and not have to enumerate all eras and use constraints functions to summon constraints.

Jimbo4350

No, I disagree. The era itself isn't a feature, it represents a boundary between feature sets IMO.

Jimbo4350 · 2023-09-14T14:01:47Z

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

@@ -984,7 +981,7 @@ makeTransactionBodyAutoBalance systemstart history lpp@(LedgerProtocolParameters
    let (dummyCollRet, dummyTotColl) = maybeDummyTotalCollAndCollReturnOutput txbodycontent changeaddr
    txbody1 <- first TxBodyError $ -- TODO: impossible to fail now
               createAndValidateTransactionBody txbodycontent1 {
-                 txFee  = TxFeeExplicit explicitTxFees $ Lovelace (2^(32 :: Integer) - 1),
+                 txFee  = TxFeeExplicit sbe $ Lovelace (2^(32 :: Integer) - 1),


The explicit/implicit refactoring looks good.

newhoggy · 2023-09-15T06:42:13Z

No, I disagree. The era itself isn't a feature, it represents a boundary between feature sets IMO.

Maybe Feature is not the right word? Stuff like ShelleyToBabbageEra and ConwayEraOnwards are more like era selectors for other things that are features. BabbageEraOnly has that kind of shape.

If we had another word to replace Feature that fit, I think it would work.

Jimbo4350 · 2023-09-15T11:53:51Z

No, I disagree. The era itself isn't a feature, it represents a boundary between feature sets IMO.

Maybe Feature is not the right word? Stuff like ShelleyToBabbageEra and ConwayEraOnwards are more like era selectors for other things that are features. BabbageEraOnly has that kind of shape.

If we had another word to replace Feature that fit, I think it would work.

I don't think it's worth the churn. However the fee refactoring is good.

…nt module

Comments addressed

newhoggy · 2023-09-20T22:07:10Z

FeatureInEra has been renamed to Eon so the misnaming doesn't apply anymore.

…d-prefix-from-era-based-commands Remove `EraBased` prefix from era-based commands

github-advanced-security bot found potential problems Sep 14, 2023

View reviewed changes

cardano-api/internal/Cardano/Api/Feature/ByronEraOnly.hs Fixed Show fixed Hide fixed

newhoggy changed the title ~~Delete TxFeesExplicitInEra and TxFeesImplicitInEra`~~ Delete TxFeesExplicitInEra and TxFeesImplicitInEra Sep 14, 2023

newhoggy force-pushed the newhoggy/delete-TxFeesExplicitInEra-and-TxFeesImplicitInEra branch from ddef9c8 to 95bedc8 Compare September 14, 2023 07:17

newhoggy marked this pull request as ready for review September 14, 2023 07:18

newhoggy requested review from dcoutts, Jimbo4350, erikd, carbolymer, CarlosLopezDeLara and disassembler as code owners September 14, 2023 07:18

newhoggy force-pushed the newhoggy/delete-TxFeesExplicitInEra-and-TxFeesImplicitInEra branch from 95bedc8 to ac738db Compare September 14, 2023 07:24

newhoggy commented Sep 14, 2023

View reviewed changes

newhoggy force-pushed the newhoggy/delete-TxFeesExplicitInEra-and-TxFeesImplicitInEra branch 2 times, most recently from e0eff20 to 5f35c61 Compare September 14, 2023 08:43

Jimbo4350 previously requested changes Sep 14, 2023

View reviewed changes

Jimbo4350 reviewed Sep 14, 2023

View reviewed changes

carbolymer approved these changes Sep 15, 2023

View reviewed changes

newhoggy force-pushed the newhoggy/delete-TxFeesExplicitInEra-and-TxFeesImplicitInEra branch from 5f35c61 to 7e4c46e Compare September 19, 2023 02:59

newhoggy added 5 commits September 21, 2023 07:48

Rename FeatureInEra to Eon

c7b7e06

Rename featureInEra to forEon

b13bc50

As ShelleyBasedEra is actually a feature, move it to the Feature pare…

20ba878

…nt module

New ByronEraOnly feature

ee7c07a

Delete TxFeesExplicitInEra and TxFeesImplicitInEra

37d672e

newhoggy force-pushed the newhoggy/delete-TxFeesExplicitInEra-and-TxFeesImplicitInEra branch from 7e4c46e to 37d672e Compare September 20, 2023 22:03

newhoggy requested a review from Jimbo4350 September 20, 2023 22:03

newhoggy enabled auto-merge September 20, 2023 22:05

newhoggy added this pull request to the merge queue Sep 20, 2023

Merged via the queue into main with commit 039693b Sep 20, 2023
20 checks passed

newhoggy deleted the newhoggy/delete-TxFeesExplicitInEra-and-TxFeesImplicitInEra branch September 20, 2023 22:34

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

Merge pull request #244 from input-output-hk/newhoggy/remove-era-base…

76091aa

…d-prefix-from-era-based-commands Remove `EraBased` prefix from era-based commands

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Delete `TxFeesExplicitInEra` and `TxFeesImplicitInEra` #244

Delete `TxFeesExplicitInEra` and `TxFeesImplicitInEra` #244

newhoggy commented Sep 14, 2023 •

edited

Loading

newhoggy Sep 14, 2023

newhoggy Sep 14, 2023

newhoggy Sep 14, 2023

Jimbo4350 left a comment •

edited

Loading

Jimbo4350 Sep 14, 2023 •

edited

Loading

newhoggy commented Sep 15, 2023 •

edited

Loading

Jimbo4350 commented Sep 15, 2023

newhoggy commented Sep 20, 2023

Delete TxFeesExplicitInEra and TxFeesImplicitInEra #244

Delete TxFeesExplicitInEra and TxFeesImplicitInEra #244

Conversation

newhoggy commented Sep 14, 2023 • edited Loading

Changelog

Context

Checklist

newhoggy Sep 14, 2023

Choose a reason for hiding this comment

newhoggy Sep 14, 2023

Choose a reason for hiding this comment

newhoggy Sep 14, 2023

Choose a reason for hiding this comment

Jimbo4350 left a comment • edited Loading

Choose a reason for hiding this comment

Jimbo4350 Sep 14, 2023 • edited Loading

Choose a reason for hiding this comment

newhoggy commented Sep 15, 2023 • edited Loading

Jimbo4350 commented Sep 15, 2023

newhoggy commented Sep 20, 2023

Delete `TxFeesExplicitInEra` and `TxFeesImplicitInEra` #244

Delete `TxFeesExplicitInEra` and `TxFeesImplicitInEra` #244

newhoggy commented Sep 14, 2023 •

edited

Loading

Jimbo4350 left a comment •

edited

Loading

Jimbo4350 Sep 14, 2023 •

edited

Loading

newhoggy commented Sep 15, 2023 •

edited

Loading