Make TxBodyContent an instance of Monoid #4458

LudvikGalois · 2022-09-21T04:44:58Z

TxBodyContent is now an instance of Monoid, allowing things like

foo :: TxBodyContent BuildTx BabbageEra
foo = mempty { txValidityRange = someValidityRange }

bar :: TxBodyContent BuildTx BabbageEra
bar = mempty { txIns = someTxIns, txOuts = someTxOuts }

baz :: TxBodyContent BuildTx BabbageEra
baz = mempty { txOuts = someMoreTxOuts }

transaction :: TxBodyContent BuildTx BabbageEra
transaction = mconcat [foo, bar, baz]

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

Jimbo4350

Looking good but with one comment

Jimbo4350 · 2022-10-07T12:07:59Z

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

+instance Semigroup (TxValidityLowerBound era) where
+    TxValidityNoLowerBound <> x = x
+    x <> TxValidityNoLowerBound = x
+    (TxValidityLowerBound wit l) <> (TxValidityLowerBound _ r) = TxValidityLowerBound wit (max l r)


I disagree with using max here. SlotNo has a Num instance, I think we should add the values together with (+). That way a user can adjust the validity lower bound as they see fit, with positive or negative values.

github-actions · 2022-11-22T02:10:47Z

This PR is stale because it has been open 45 days with no activity.

koslambrou · 2022-11-22T19:56:30Z

@LudvikGalois Is @Jimbo4350's comment the only thing preventing from merging this PR?

LudvikGalois · 2022-11-22T20:19:45Z

@koslambrou I think so? I haven't come back to this because the reason I wanted it disappeared.

koslambrou · 2022-11-22T21:37:36Z

@koslambrou I think so? I haven't come back to this because the reason I wanted it disappeared.

@LudvikGalois Do you want to continue the PR or should I take over?

TxBodyContent is now an instance of Monoid, allowing things like ```haskell foo :: TxBodyContent BuildTx BabbageEra foo = mempty { txValidityRange = someValidityRange } bar :: TxBodyContent BuildTx BabbageEra bar = mempty { txIns = someTxIns, txOuts = someTxOuts } baz :: TxBodyContent BuildTx BabbageEra baz = mempty { txOuts = someMoreTxOuts } transaction :: TxBodyContent BuildTx BabbageEra transaction = mconcat [foo, bar, baz] ```

newhoggy · 2023-03-01T04:45:44Z

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

+instance Semigroup (TxValidityLowerBound era) where
+    TxValidityNoLowerBound <> x = x
+    x <> TxValidityNoLowerBound = x
+    (TxValidityLowerBound wit l) <> (TxValidityLowerBound _ r) = TxValidityLowerBound wit (max l r)


The thing that bothers me is we throw away the witness on the RHS of <>. Is this safe to do?

Oh it's a type witness. Maybe it's okay?

It is safe to do for all the "feature supported in era" type witnesses. For any given era, they have at most one non-bottom value, so it doesn't matter "which one" you pick.

…pperBoundSupportedInEra and ValidityLowerBoundSupportedInEra

Jimbo4350 · 2023-03-01T15:53:51Z

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

@@ -13,6 +13,7 @@
 {-# LANGUAGE StandaloneDeriving #-}
 {-# LANGUAGE TypeApplications #-}
 {-# LANGUAGE TypeFamilies #-}
+{-# LANGUAGE UndecidableInstances #-}


We can remove the UndecidableInstances pragma with:

instance IsCardanoEra era => Monoid (TxBodyContent ViewTx era) where mempty = TxBodyContent { txIns = mempty , txInsCollateral = mempty , txInsReference = mempty , txOuts = mempty , txTotalCollateral = mempty , txReturnCollateral = TxReturnCollateralNone , txFee = mempty , txValidityRange = (mempty, mempty) , txMetadata = mempty , txAuxScripts = mempty , txExtraKeyWits = mempty , txProtocolParams = Nothing <$ (mempty :: BuildTxWith ViewTx ()) , txWithdrawals = mempty , txCertificates = mempty , txUpdateProposal = TxUpdateProposalNone , txMintValue = mempty , txScriptValidity = mempty } instance IsCardanoEra era => Monoid (TxBodyContent BuildTx era) where mempty = TxBodyContent { txIns = mempty , txInsCollateral = mempty , txInsReference = mempty , txOuts = mempty , txTotalCollateral = mempty , txReturnCollateral = TxReturnCollateralNone , txFee = mempty , txValidityRange = (mempty, mempty) , txMetadata = mempty , txAuxScripts = mempty , txExtraKeyWits = mempty , txProtocolParams = Nothing <$ (mempty :: BuildTxWith BuildTx ()) , txWithdrawals = mempty , txCertificates = mempty , txUpdateProposal = TxUpdateProposalNone , txMintValue = mempty , txScriptValidity = mempty }

newhoggy · 2023-03-06T00:03:53Z

Hmm, this actually doesn't work. For example when I try to rewrite TxBodyContent to mempty here:

  dummyTx :: TxBodyContent BuildTx era
  dummyTx = TxBodyContent {
      txIns = [( TxIn "dbaff4e270cfb55612d9e2ac4658a27c79da4a5271c6f90853042d1403733810" (TxIx 0)
               , BuildTxWith $ KeyWitness KeyWitnessForSpending )]
    , txInsCollateral = TxInsCollateralNone
    , txInsReference = TxInsReferenceNone
    , txOuts = []
    , txFee = mkTxFee 0
    , txValidityRange = (TxValidityNoLowerBound, mkTxValidityUpperBound 0)
    , txMetadata = metadata
    , txAuxScripts = TxAuxScriptsNone
    , txExtraKeyWits = TxExtraKeyWitnessesNone
    , txProtocolParams = BuildTxWith Nothing
    , txWithdrawals = TxWithdrawalsNone
    , txCertificates = TxCertificatesNone
    , txUpdateProposal = TxUpdateProposalNone
    , txMintValue = TxMintNone
    , txScriptValidity = TxScriptValidityNone
    , txReturnCollateral = TxReturnCollateralNone
    , txTotalCollateral = TxTotalCollateralNone
    }

I get the error:

src/Cardano/Api/TxBody.hs:2618:5: error:
    • Ambiguous type variable ‘build0’ arising from a use of ‘mempty’
      prevents the constraint ‘(Monoid
                                  (BuildTxWith build0 ()))’ from being solved.
      Probable fix: use a type annotation to specify what ‘build0’ should be.
      These potential instances exist:
        instance Monoid a => Monoid (BuildTxWith BuildTx a)
          -- Defined at src/Cardano/Api/TxBody.hs:1207:10
        instance Semigroup a => Monoid (BuildTxWith ViewTx a)
          -- Defined at src/Cardano/Api/TxBody.hs:1204:10
    • In the expression: mempty
      In the expression:
        mempty
          {txIns = fromLedgerTxIns era body,
           txInsCollateral = fromLedgerTxInsCollateral era body,
           txInsReference = fromLedgerTxInsReference era body,
           txOuts = fromLedgerTxOuts era body scriptdata,
           txTotalCollateral = fromLedgerTxTotalCollateral era body,
           txReturnCollateral = fromLedgerTxReturnCollateral era body,
           txFee = fromLedgerTxFee era body,
           txValidityRange = fromLedgerTxValidityRange era body,
           txWithdrawals = fromLedgerTxWithdrawals era body,
           txCertificates = fromLedgerTxCertificates era body,
           txUpdateProposal = fromLedgerTxUpdateProposal era body,
           txMintValue = fromLedgerTxMintValue era body,
           txExtraKeyWits = fromLedgerTxExtraKeyWitnesses era body,
           txProtocolParams = ViewTx, txMetadata, txAuxScripts,
           txScriptValidity = scriptValidity}
      In an equation for ‘fromLedgerTxBody’:
          fromLedgerTxBody era scriptValidity body scriptdata mAux
            = mempty
                {txIns = fromLedgerTxIns era body,
                 txInsCollateral = fromLedgerTxInsCollateral era body,
                 txInsReference = fromLedgerTxInsReference era body,
                 txOuts = fromLedgerTxOuts era body scriptdata,
                 txTotalCollateral = fromLedgerTxTotalCollateral era body,
                 txReturnCollateral = fromLedgerTxReturnCollateral era body,
                 txFee = fromLedgerTxFee era body,
                 txValidityRange = fromLedgerTxValidityRange era body,
                 txWithdrawals = fromLedgerTxWithdrawals era body,
                 txCertificates = fromLedgerTxCertificates era body,
                 txUpdateProposal = fromLedgerTxUpdateProposal era body,
                 txMintValue = fromLedgerTxMintValue era body,
                 txExtraKeyWits = fromLedgerTxExtraKeyWitnesses era body,
                 txProtocolParams = ViewTx, txMetadata, txAuxScripts,
                 txScriptValidity = scriptValidity}
            where
                (txMetadata, txAuxScripts) = fromLedgerTxAuxiliaryData era mAux
     |
2618 |     mempty
     |     ^^^^^^

newhoggy · 2023-03-06T20:57:35Z

Closing this PR in favour of some other strategy. Ongoing work will be tracked by #4941

newhoggy · 2023-03-06T21:37:22Z

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

+instance IsCardanoEra era => Monoid (TxValidityUpperBound era) where
+    mempty = case cardanoEra @era of
+      ByronEra -> TxValidityNoUpperBound ValidityNoUpperBoundInByronEra
+      ShelleyEra -> TxValidityUpperBound ValidityUpperBoundInShelleyEra maxBound


ShelleyEra -> TxValidityUpperBound ValidityUpperBoundInShelleyEra maxBound

Was this how it was in the original code?

Why is Shelly era special that it should use maxBound?

-- Note that the 'ShelleyEra' /does not support/ omitting a validity upper -- bound. It was introduced as a /required/ field in Shelley and then made -- optional in Allegra and subsequent eras. -- -- The Byron era supports this by virtue of the fact that it does not support -- validity ranges at all.

Jimbo4350 reviewed Sep 21, 2022

View reviewed changes

cardano-api/src/Cardano/Api/TxBody.hs Outdated Show resolved Hide resolved

LudvikGalois force-pushed the ludvikgalois/transaction-body-content-monoid branch 2 times, most recently from 4ecda34 to 9977a6d Compare September 23, 2022 07:46

LudvikGalois marked this pull request as ready for review September 23, 2022 07:48

LudvikGalois requested review from dcoutts, erikd, newhoggy and JaredCorduan as code owners September 23, 2022 07:48

Jimbo4350 reviewed Oct 7, 2022

View reviewed changes

github-actions bot added the Stale label Nov 22, 2022

github-actions bot removed the Stale label Nov 23, 2022

newhoggy requested a review from CarlosLopezDeLara as a code owner January 7, 2023 02:13

newhoggy self-assigned this Feb 28, 2023

newhoggy reviewed Mar 1, 2023

View reviewed changes

Semigroup instances for ValidityUpperBoundSupportedInEra, ValidityNoU…

28f43b0

…pperBoundSupportedInEra and ValidityLowerBoundSupportedInEra

newhoggy force-pushed the ludvikgalois/transaction-body-content-monoid branch from 9977a6d to 28f43b0 Compare March 1, 2023 06:14

Jimbo4350 reviewed Mar 1, 2023

View reviewed changes

newhoggy mentioned this pull request Mar 5, 2023

Combinators for TxBodyContent and related types #4941

Merged

newhoggy closed this Mar 6, 2023

newhoggy reviewed Mar 6, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make TxBodyContent an instance of Monoid #4458

Make TxBodyContent an instance of Monoid #4458

LudvikGalois commented Sep 21, 2022

Jimbo4350 left a comment

Jimbo4350 Oct 7, 2022

github-actions bot commented Nov 22, 2022

koslambrou commented Nov 22, 2022

LudvikGalois commented Nov 22, 2022

koslambrou commented Nov 22, 2022 •

edited

Loading

newhoggy Mar 1, 2023 •

edited

Loading

newhoggy Mar 1, 2023

LudvikGalois Mar 1, 2023

Jimbo4350 Mar 1, 2023

newhoggy commented Mar 6, 2023

newhoggy commented Mar 6, 2023

newhoggy Mar 6, 2023

newhoggy Mar 6, 2023

Make TxBodyContent an instance of Monoid #4458

Make TxBodyContent an instance of Monoid #4458

Conversation

LudvikGalois commented Sep 21, 2022

Jimbo4350 left a comment

Choose a reason for hiding this comment

Jimbo4350 Oct 7, 2022

Choose a reason for hiding this comment

github-actions bot commented Nov 22, 2022

koslambrou commented Nov 22, 2022

LudvikGalois commented Nov 22, 2022

koslambrou commented Nov 22, 2022 • edited Loading

newhoggy Mar 1, 2023 • edited Loading

Choose a reason for hiding this comment

newhoggy Mar 1, 2023

Choose a reason for hiding this comment

LudvikGalois Mar 1, 2023

Choose a reason for hiding this comment

Jimbo4350 Mar 1, 2023

Choose a reason for hiding this comment

newhoggy commented Mar 6, 2023

newhoggy commented Mar 6, 2023

newhoggy Mar 6, 2023

Choose a reason for hiding this comment

newhoggy Mar 6, 2023

Choose a reason for hiding this comment

koslambrou commented Nov 22, 2022 •

edited

Loading

newhoggy Mar 1, 2023 •

edited

Loading