Transaction.hs: reduce boilerplate and move towards ShelleyBasedEra #783

smelc · 2024-06-07T09:35:42Z

Changelog

- description: |
    Transaction.hs: reduce boilerplate and move towards ShelleyBasedEra
# 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
  - refactoring    # QoL changes
  # - 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...

How to trust this PR

It doesn't actually change the runtime behavior
It's more deletion that additions 🥇

Checklist

Commit sequence broadly makes sense and commits have useful messages
Self-reviewed the diff

cardano-cli/src/Cardano/CLI/Types/Errors/TxValidationError.hs

smelc · 2024-06-07T09:52:31Z

cardano-cli/src/Cardano/CLI/Types/Errors/TxValidationError.hs

 -- TODO: Because we have separated Byron related transaction
-- construction into separate commands, we can parameterize this
-- on ShelleyBasedEra era and remove Either TxCertificatesValidationError
+-- construction into separate commands, we can remove
+-- Either TxNotSupportedInEraValidationError


Does this comment mean I should just remove the call to conjureWitness?

Well, sbe == supported in the function body, so this conjureWitness will never fail, so i'd say yes. The same for other places in this PR, where we changed CardanoEra to ShelleyBasedEra.

carbolymer

Good job!

carbolymer · 2024-06-13T08:59:27Z

cardano-cli/src/Cardano/CLI/Types/Errors/TxValidationError.hs

+    -- | First argument is the kind of data that is not supported.
+    -- Second argument in the era that doesn't support the data.
+    TxNotSupportedInAnyCardanoEraValidationError T.Text AnyCardanoEra
+    -- | First argument is the kind of data that is not supported.


I'm not sure if this haddock is 100% correct - I think it should be after |, as it references the second constructor.

@carbolymer> nah it's all good:

carbolymer · 2024-06-13T09:03:49Z

cardano-cli/src/Cardano/CLI/Types/Errors/TxValidationError.hs

 -- TODO: Because we have separated Byron related transaction
-- construction into separate commands, we can parameterize this
-- on ShelleyBasedEra era and remove Either TxCertificatesValidationError
+-- construction into separate commands, we can remove
+-- Either TxNotSupportedInEraValidationError


Well, sbe == supported in the function body, so this conjureWitness will never fail, so i'd say yes. The same for other places in this PR, where we changed CardanoEra to ShelleyBasedEra.

carbolymer · 2024-06-13T09:10:54Z

cardano-cli/src/Cardano/CLI/Types/Errors/TxValidationError.hs

+validateTxCurrentTreasuryValue sbe mCurrentTreasuryValue =
+  case mCurrentTreasuryValue of
+    Nothing -> Right Nothing
+    Just (TxCurrentTreasuryValue { unTxCurrentTreasuryValue }) ->


suggestion:

Suggested change

validateTxCurrentTreasuryValue sbe mCurrentTreasuryValue =

case mCurrentTreasuryValue of

Nothing -> Right Nothing

Just (TxCurrentTreasuryValue { unTxCurrentTreasuryValue }) ->

validateTxCurrentTreasuryValue sbe =

mapM $ \(TxCurrentTreasuryValue { unTxCurrentTreasuryValue }) ->

or you can use forM, ...or neither 😄

Since I rebased this PR on main (instead of #778) this is irrelevant now, but I'll keep it mind when revisiting #778 👍

carbolymer · 2024-06-13T09:11:13Z

cardano-cli/src/Cardano/CLI/Types/Errors/TxValidationError.hs

+  => ShelleyBasedEra era
+  -> Maybe TxTreasuryDonation
+  -> Either (TxNotSupportedInEraValidationError era) (Maybe (Featured ConwayEraOnwards era L.Coin))
+validateTxTreasuryDonation sbe mTreasuryDonation =


same suggestion as above

Since I rebased this PR on main (instead of #778) this is irrelevant now, but I'll keep it mind when revisiting #778 👍

Jimbo4350 · 2024-06-13T13:51:11Z

cardano-cli/src/Cardano/CLI/Types/Errors/TxValidationError.hs

-instance Error TxProtocolParametersValidationError where
-  prettyError (ProtocolParametersNotSupported e) =
-    "Transaction protocol parameters are not supported in " <> pretty e
-
 validateProtocolParameters
  :: CardanoEra era


This will only fail if we call validateProtocolParameters with ByronEra. However the code won't compile if we call this with ByronEra:

ghci> validateProtocolParameters ByronEra Nothing <interactive>:22:1: error: [GHC-39999] • No instance for ‘IsShelleyBasedEra ByronEra’ arising from a use of ‘print’ • In a stmt of an interactive GHCi command: print it

What we should do is change the parameter from CardanoEra era to ShelleyBasedEra era but actually in this case no validation is done!

So we can remove this function and directly use setTxProtocolParams.

Have a look at the other functions that demand CardanoEra era but should be parameterized on ShelleyBasedEra era. If no validation is done, we can just drop them. I'd do that in a separate PR.

OK will do in a follow-up PR right away 👍

@Jimbo4350> so can you approve in the meantime?

I can't because it would be adding churn. If you do what I explained above it should also result in the removal of the unnecessary error constructors.

@Jimbo4350> done 👍 Please rereview.

cardano-cli/src/Cardano/CLI/EraBased/Run/Transaction.hs

Jimbo4350

LGTM! However you can remove the TxNotSupportedInAnyCardanoEraValidationError constructor.

Jimbo4350 · 2024-06-18T08:39:09Z

cardano-cli/src/Cardano/CLI/Types/Errors/TxValidationError.hs

+data TxNotSupportedInEraValidationError era =
+    -- | First argument is the kind of data that is not supported.
+    -- Second argument is the era that doesn't support the data.
+    TxNotSupportedInAnyCardanoEraValidationError T.Text AnyCardanoEra


This constructor can actually be removed.

Good catch, done 👍

It should return AnyShelleyBasedEra because: https://github.com/IntersectMBO/cardano-cli/pull/783/files#diff-a38343c1825e6e334224d0a8a1ba78a458652970f00c54f5fb0d040be28f822eR606

@Jimbo4350> not really, because this constructor is passed as a function to conjureWitness and there remains one single caller to conjureWitness that uses a CardanoEra era value.

Once this last caller is eliminated, we will be able to tighten TxNotSupportedInAnyCardanoEraValidationError's type.

We could use shelleyBasedToCardanoEra.

@Jimbo4350> but this is awkward because conjureWitness expects a function to build the error, to which it applies the CardanoEra. If I want to keep conjureWitness existing type (which I think is nice), I need to do something like this:

which is weird, because the value passed by conjureWitness gets ignored when building the error.

…ustom Text message instead.

github-advanced-security bot found potential problems Jun 7, 2024

View reviewed changes

cardano-cli/src/Cardano/CLI/Types/Errors/TxValidationError.hs Fixed Show fixed Hide fixed

smelc force-pushed the smelc/reduce-boilerplate-modernize-TxValidationErrors branch from 5a540fb to d53b701 Compare June 7, 2024 09:45

smelc commented Jun 7, 2024

View reviewed changes

smelc marked this pull request as ready for review June 7, 2024 09:52

smelc requested review from Jimbo4350, newhoggy, carbolymer, palas, kevinhammond and disassembler as code owners June 7, 2024 09:52

smelc force-pushed the smelc/treasury-donations branch from 528f47e to 2096e19 Compare June 12, 2024 15:15

carbolymer approved these changes Jun 13, 2024

View reviewed changes

smelc force-pushed the smelc/treasury-donations branch from 2096e19 to b2b9781 Compare June 13, 2024 09:40

smelc force-pushed the smelc/reduce-boilerplate-modernize-TxValidationErrors branch from d53b701 to f42b8c6 Compare June 13, 2024 09:42

Jimbo4350 requested changes Jun 13, 2024

View reviewed changes

smelc force-pushed the smelc/reduce-boilerplate-modernize-TxValidationErrors branch from f42b8c6 to bfef03a Compare June 13, 2024 13:54

smelc changed the base branch from smelc/treasury-donations to main June 13, 2024 14:28

smelc force-pushed the smelc/reduce-boilerplate-modernize-TxValidationErrors branch from bfef03a to 4f9fda2 Compare June 13, 2024 15:35

smelc mentioned this pull request Jun 17, 2024

Add --current-treasury-value and --treasury-donation to transaction build and friends #778

Merged

6 tasks

Jimbo4350 reviewed Jun 18, 2024

View reviewed changes

cardano-cli/src/Cardano/CLI/EraBased/Run/Transaction.hs Outdated Show resolved Hide resolved

cardano-cli/src/Cardano/CLI/EraBased/Run/Transaction.hs Outdated Show resolved Hide resolved

smelc force-pushed the smelc/reduce-boilerplate-modernize-TxValidationErrors branch from cf4e491 to 639d6a0 Compare June 18, 2024 08:22

Jimbo4350 approved these changes Jun 18, 2024

View reviewed changes

Remove a number of error types, replace by a generic error with one c…

b843840

…ustom Text message instead.

smelc force-pushed the smelc/reduce-boilerplate-modernize-TxValidationErrors branch from 639d6a0 to b843840 Compare June 18, 2024 08:53

smelc enabled auto-merge June 18, 2024 08:53

smelc added this pull request to the merge queue Jun 18, 2024

smelc removed this pull request from the merge queue due to a manual request Jun 18, 2024

smelc added this pull request to the merge queue Jun 18, 2024

Merged via the queue into main with commit fccfd72 Jun 18, 2024
22 checks passed

smelc deleted the smelc/reduce-boilerplate-modernize-TxValidationErrors branch June 18, 2024 11:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Transaction.hs: reduce boilerplate and move towards ShelleyBasedEra #783

Transaction.hs: reduce boilerplate and move towards ShelleyBasedEra #783

smelc commented Jun 7, 2024 •

edited

Loading

smelc Jun 7, 2024

carbolymer Jun 13, 2024

carbolymer left a comment

carbolymer Jun 13, 2024

smelc Jun 13, 2024

carbolymer Jun 13, 2024

carbolymer Jun 13, 2024

smelc Jun 13, 2024

carbolymer Jun 13, 2024

smelc Jun 13, 2024

Jimbo4350 Jun 13, 2024

smelc Jun 13, 2024

Jimbo4350 Jun 14, 2024

smelc Jun 17, 2024

Jimbo4350 left a comment

Jimbo4350 Jun 18, 2024

smelc Jun 18, 2024

Jimbo4350 Jun 18, 2024

smelc Jun 18, 2024

Jimbo4350 Jun 18, 2024

smelc Jun 18, 2024

Transaction.hs: reduce boilerplate and move towards ShelleyBasedEra #783

Transaction.hs: reduce boilerplate and move towards ShelleyBasedEra #783

Conversation

smelc commented Jun 7, 2024 • edited Loading

Changelog

How to trust this PR

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carbolymer 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jimbo4350 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

smelc commented Jun 7, 2024 •

edited

Loading