Add era-independent "debug transaction view" command #840

smelc · 2024-07-16T16:02:35Z

Changelog

- description: |
    Add era-independent "debug transaction view" command
# 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...

Context

Fixes babbage transaction view on Babbage-era transaction shows some Conway era fields #799
When this PR is merged, I will create a follow-up issue to deprecate the [era] transaction view command after the next release.
This is somehow reverting the decision from transaction view: show proposals and votes #774 (comment) that was to show null for fields in irrelevant eras

How to trust this PR

The golden file for transaction view on this Conway transaction, which is visible here didn't change: the Conway fields stayed.
Golden files for non-Conway eras for transaction view outputs have changed.

Checklist

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

cardano-cli/src/Cardano/CLI/Json/Friendly.hs

                 ]
+              ++ ( caseByronToBabbageOrConwaysEraOnwards era $


cardano-cli/src/Cardano/CLI/Json/Friendly.hs

+                            ["governance actions" .= (friendlyLedgerProposals cOnwards $ toList lProposals)]
+                    )
+                 )
+              ++ ( caseByronToBabbageOrConwaysEraOnwards era $


cardano-cli/src/Cardano/CLI/Json/Friendly.hs

+                    )
+                 )
+              ++ ( caseByronToBabbageOrConwaysEraOnwards era $
+                    const $


cardano-cli/src/Cardano/CLI/Json/Friendly.hs

+                      ["currentTreasuryValue" .= toJSON (unFeatured <$> txCurrentTreasuryValue)]
+                 )
+              ++ ( caseByronToBabbageOrConwaysEraOnwards era $
+                    const $


cardano-cli/src/Cardano/CLI/Json/Friendly.hs

+        ( \sbe -> do
+            caseShelleyToBabbageOrConwayEraOnwards
+              (const [])
+              (\cOnwards -> f cOnwards)


smelc · 2024-07-17T12:56:49Z

cc @mkoura

palas

Looks good. Just a lint

palas · 2024-07-17T17:55:24Z

cardano-cli/src/Cardano/CLI/Json/Friendly.hs

+              ++ ( caseByronToBabbageOrConwaysEraOnwards
+                    ( \cOnwards ->
+                        case txProposalProcedures of
+                          Nothing -> []
+                          Just (Featured _ TxProposalProceduresNone) -> []
+                          Just (Featured _ (TxProposalProcedures lProposals _witnesses)) ->
+                            ["governance actions" .= (friendlyLedgerProposals cOnwards $ toList lProposals)]
+                    )
+                    era
+                 )
+              ++ ( caseByronToBabbageOrConwaysEraOnwards
+                    ( \cOnwards ->
+                        case txVotingProcedures of
+                          Nothing -> []
+                          Just (Featured _ TxVotingProceduresNone) -> []
+                          Just (Featured _ (TxVotingProcedures votes _witnesses)) ->
+                            ["voters" .= friendlyVotingProcedures cOnwards votes]
+                    )
+                    era
+                 )
+              ++ ( caseByronToBabbageOrConwaysEraOnwards
+                    (const ["currentTreasuryValue" .= toJSON (unFeatured <$> txCurrentTreasuryValue)])
+                    era
+                 )
+              ++ ( caseByronToBabbageOrConwaysEraOnwards
+                    (const ["treasuryDonation" .= toJSON (unFeatured <$> txTreasuryDonation)])
+                    era


Suggested change

++ ( caseByronToBabbageOrConwaysEraOnwards

( \cOnwards ->

case txProposalProcedures of

Nothing -> []

Just (Featured _ TxProposalProceduresNone) -> []

Just (Featured _ (TxProposalProcedures lProposals _witnesses)) ->

["governance actions" .= (friendlyLedgerProposals cOnwards $ toList lProposals)]

)

era

)

++ ( caseByronToBabbageOrConwaysEraOnwards

( \cOnwards ->

case txVotingProcedures of

Nothing -> []

Just (Featured _ TxVotingProceduresNone) -> []

Just (Featured _ (TxVotingProcedures votes _witnesses)) ->

["voters" .= friendlyVotingProcedures cOnwards votes]

)

era

)

++ ( caseByronToBabbageOrConwaysEraOnwards

(const ["currentTreasuryValue" .= toJSON (unFeatured <$> txCurrentTreasuryValue)])

era

)

++ ( caseByronToBabbageOrConwaysEraOnwards

(const ["treasuryDonation" .= toJSON (unFeatured <$> txTreasuryDonation)])

era

++ ( concat

( caseByronToBabbageOrConwaysEraOnwards

( \cOnwards ->

[ case txProposalProcedures of

Nothing -> []

Just (Featured _ TxProposalProceduresNone) -> []

Just (Featured _ (TxProposalProcedures lProposals _witnesses)) ->

["governance actions" .= (friendlyLedgerProposals cOnwards $ toList lProposals)]

, case txVotingProcedures of

Nothing -> []

Just (Featured _ TxVotingProceduresNone) -> []

Just (Featured _ (TxVotingProcedures votes _witnesses)) ->

["voters" .= friendlyVotingProcedures cOnwards votes]

, ["currentTreasuryValue" .= toJSON (unFeatured <$> txCurrentTreasuryValue)]

, ["treasuryDonation" .= toJSON (unFeatured <$> txTreasuryDonation)]

]

)

era

)

We can factor caseByronToBabbageOrConwaysEraOnwards out by using concat. We could also use concatMap but this way we don't even need the consts. Or we could use catMaybes, that would enforce that it is only one item, but probably much less readable

I think the code is easier to understand by not using concat, because it avoids nesting. And so it keeps the "one blob of code <==> one item in the list" structure that the items above have 👍

So I'll pass on this suggestion 👍

smelc · 2024-07-18T08:58:58Z

Maybe @Jimbo4350 you want to have a look before merging

carbolymer · 2024-07-18T13:02:02Z

cardano-cli/src/Cardano/CLI/Json/Friendly.hs

+    caseByronToBabbageOrConwaysEraOnwards :: (ConwayEraOnwards era -> [a]) -> CardanoEra era -> [a]
+    caseByronToBabbageOrConwaysEraOnwards f =
+      caseByronOrShelleyBasedEra
+        []
+        (caseShelleyToBabbageOrConwayEraOnwards (const []) f)


That's doing the same same as

monoidForEraInEonA :: () => Eon eon => Applicative f => Monoid a => CardanoEra era -> (eon era -> f a) -> f a

Nope, because Aeson.Value is not a monoid 🙂 (besides, I'm not sure the added abstraction would be worth the harder-to-understand code).

carbolymer · 2024-07-18T13:16:14Z

cardano-cli/src/Cardano/CLI/Run/Debug.hs

 renderDebugCmdError :: DebugCmdError -> Doc ann
-renderDebugCmdError DebugCmdFailed = "Debug command failed"
+renderDebugCmdError = \case
+  DebugCmdFailed -> "Debug command failed"
+  DebugTxCmdError err -> renderTxCmdError err


This should be an Error instance in the DebugCmdError type module.

Good point, changed 👍

carbolymer · 2024-07-18T13:17:05Z

cardano-cli/test/cardano-cli-golden/Test/Golden/TxView.hs

@@ -30,60 +19,6 @@ goldenDir, inputDir :: FilePath
 goldenDir = "test/cardano-cli-golden/files/golden"
 inputDir = "test/cardano-cli-golden/files/input"

-- TODO: Expose command to view byron tx files
-_hprop_golden_view_byron_yaml :: Property


I think this makes HLINT pragma unneccessary. Can you remove it if it's possible?

Yes indeed, changing 👍

…"[era] transaction view")

…eting corresponding unplugged tests

…(don't even show 'null')

Jimbo4350 · 2024-07-23T08:08:29Z

cardano-cli/src/Cardano/CLI/Run/Debug/TransactionView.hs

+        InAnyShelleyBasedEra era txbody <- pure $ unIncompleteCddlTxBody unwitnessed
+        -- Why are we differentiating between a transaction body and a transaction?
+        -- In the case of a transaction body, we /could/ simply call @makeSignedTransaction []@
+        -- to get a transaction which would allow us to reuse friendlyTxBS. However,


This is something the new api would fix because by default we would either produce an unsigned tx or a signed tx. No need to think about transaction bodies that are missing witnesses.

Jimbo4350

LGTM!

Jimbo4350 · 2024-07-23T08:18:02Z

cardano-cli/test/cardano-cli-golden/files/golden/help/debug.cli

+                           connect to a local node and log the epoch state to a
+                           file. The log file format is line delimited JSON. The
+                           command will not terminate.
+  transaction              Transaction commands


Transaction debugging commands?

Jimbo4350 · 2024-07-23T08:24:47Z

cardano-cli/src/Cardano/CLI/Json/Friendly.hs

@@ -74,6 +76,11 @@ import           GHC.Unicode (isAlphaNum)

 data FriendlyFormat = FriendlyJson | FriendlyYaml

+viewOutputFormatToFriendlyFormat :: ViewOutputFormat -> FriendlyFormat


I don't think FriendlyFormat is necessary. Why not propagate ViewOutputFormat to the friendly* functions?

smelc force-pushed the smelc/add-transaction-view-under-debug branch 2 times, most recently from c461329 to 414388c Compare July 17, 2024 12:03

github-advanced-security bot found potential problems Jul 17, 2024

View reviewed changes

smelc force-pushed the smelc/add-transaction-view-under-debug branch from 317ad96 to 1bdc3df Compare July 17, 2024 12:46

smelc marked this pull request as ready for review July 17, 2024 13:38

smelc requested review from Jimbo4350, newhoggy, carbolymer, palas, kevinhammond and disassembler as code owners July 17, 2024 13:38

palas approved these changes Jul 17, 2024

View reviewed changes

smelc mentioned this pull request Jul 18, 2024

Deprecate [era] transaction view #843

Closed

2 tasks

carbolymer approved these changes Jul 18, 2024

View reviewed changes

smelc added 7 commits July 18, 2024 16:08

Add "debug transaction view" which is era independent (as opposed to …

651f7f6

…"[era] transaction view")

Change tests to use "debug transaction view"

c59396c

Adapt golden files

956c8de

transaction view on byron tx is not going to become supported, so del…

b79e220

…eting corresponding unplugged tests

Simplifications and code sharing

893f7f6

transaction view: don't show Conway fields on non-Conway transaction …

a9e48ec

…(don't even show 'null')

Adapt golden files

e80203b

smelc force-pushed the smelc/add-transaction-view-under-debug branch from 1bdc3df to e80203b Compare July 18, 2024 14:08

smelc enabled auto-merge July 18, 2024 14:22

smelc disabled auto-merge July 18, 2024 15:04

smelc merged commit 4d3dbdb into main Jul 18, 2024
22 of 23 checks passed

smelc deleted the smelc/add-transaction-view-under-debug branch July 18, 2024 15:04

Jimbo4350 reviewed Jul 23, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add era-independent "debug transaction view" command #840

Add era-independent "debug transaction view" command #840

smelc commented Jul 16, 2024 •

edited

Loading

smelc commented Jul 17, 2024

palas left a comment

palas Jul 17, 2024

smelc Jul 18, 2024 •

edited

Loading

smelc commented Jul 18, 2024

carbolymer Jul 18, 2024

smelc Jul 18, 2024

carbolymer Jul 18, 2024

smelc Jul 18, 2024

carbolymer Jul 18, 2024

smelc Jul 18, 2024

Jimbo4350 Jul 23, 2024

Jimbo4350 left a comment

Jimbo4350 Jul 23, 2024

Jimbo4350 Jul 23, 2024

		@@ -74,6 +76,11 @@ import GHC.Unicode (isAlphaNum)

		data FriendlyFormat = FriendlyJson \| FriendlyYaml

		viewOutputFormatToFriendlyFormat :: ViewOutputFormat -> FriendlyFormat

Add era-independent "debug transaction view" command #840

Add era-independent "debug transaction view" command #840

Conversation

smelc commented Jul 16, 2024 • edited Loading

Changelog

Context

How to trust this PR

Checklist

smelc commented Jul 17, 2024

palas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smelc Jul 18, 2024 • edited Loading

Choose a reason for hiding this comment

smelc commented Jul 18, 2024

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

smelc commented Jul 16, 2024 •

edited

Loading

smelc Jul 18, 2024 •

edited

Loading