New `debug transaction echo` command #709

newhoggy · 2024-04-09T12:08:07Z

Changelog

- description: |
    New `debug transaction echo` 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
  # - 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

New command is useful for testing if decode/encode roundtrip has problems.

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

smelc · 2024-04-09T13:14:13Z

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

+    , Just
+        $ subParser "echo"
+        $ Opt.info pTransactionEcho
+        $ Opt.progDesc "Echo a transaction"


Can you provide more context as to when this command is useful?

Yep, I don't understand why we need this command.

I gather that's verifying that we're capable of reading the transaction and saving it again. I don't know if saving can fail in this scenario, but reading should be verifiable with transaction view imo.

Yes, reading and saving the transaction again and verifying that the new transaction is byte-wise identical to the old.

Related to this investigation. https://input-output-rnd.slack.com/archives/CR599HMFX/p1712568383003499

Sooo... what's the advantage over transaction view ? 😃

transaction view doesn't do a roundtrip. It reads a transaction and writes it in some other format.

Using transaction view doesn't give us any information about whether the tx could have been generated by cardano-api or cardano-cli.

This looks like a thing we should have a property test for.

So should we have a roundtrip test instead of a command?

I guess both? The command could be useful too I think.

smelc · 2024-04-09T13:15:33Z

Code looks fine to me 👍

Not approving to give a chance to @Jimbo4350 and @CarlosLopezDeLara to chime in on the design.

carbolymer

I'd like to have this clarified first https://github.com/IntersectMBO/cardano-cli/pull/709/files#r1557852979

github-actions · 2024-05-26T01:51:07Z

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

carbolymer · 2024-05-27T09:17:03Z

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

+                                             )
+                                             --out-file FILE
+
+  Echo a transaction


This needs more explanation what does this command exactly do.

carbolymer

LGTM, but it needs more explanation why an user would like to use this command. Also I'm not sure if it wouldn't be better to make this command live under debug hierarchy, since it's not very useful in normal transaction processing.

Jimbo4350 · 2024-06-18T14:46:38Z

cardano-cli/src/Cardano/CLI/Options/Debug/Transaction.hs

+            ]
+    , subParser "echo"
+        $ Opt.info (DebugTransactionCmds <$> pDebugTransactionEcho)
+        $ Opt.progDesc "Echo a transaction"


Can you add more info why specifically we are adding this? I can approve after.

I was going to say the same. Something like "it parses and rewrites the transaction" or "it deserialises and then reserialises a transaction" or "it re-formats the transaction". And it would be nice to say why this may be useful too, e.g: "because we may want to update/reformat the transaction using the format of the current version of the cli".

palas

This should be very useful to work with changes to transaction format. I have included a couple of suggestions that I think could improve clarity

palas · 2024-06-18T20:10:58Z

cardano-cli/src/Cardano/CLI/Options/Debug/Transaction.hs

+            ]
+    , subParser "echo"
+        $ Opt.info (DebugTransactionCmds <$> pDebugTransactionEcho)
+        $ Opt.progDesc "Echo a transaction"


I was going to say the same. Something like "it parses and rewrites the transaction" or "it deserialises and then reserialises a transaction" or "it re-formats the transaction". And it would be nice to say why this may be useful too, e.g: "because we may want to update/reformat the transaction using the format of the current version of the cli".

palas · 2024-06-18T20:19:45Z

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

+      unwitnessed <- firstExceptT DebugCmdTextEnvCddlError . newExceptT $ readFileTxBody txbodyFile
+
+      case unwitnessed of
+        IncompleteCddlTxBody anyTxBody -> do
+          InAnyShelleyBasedEra sbe txbody <- pure anyTxBody
+
+          let tx = makeSignedTransaction [] txbody
+
+          firstExceptT DebugCmdWriteFileError . newExceptT
+            $ writeLazyByteStringFile outTxFile
+            $ shelleyBasedEraConstraints sbe
+            $ textEnvelopeToJSON Nothing tx


Suggested change

unwitnessed <- firstExceptT DebugCmdTextEnvCddlError . newExceptT $ readFileTxBody txbodyFile

case unwitnessed of

IncompleteCddlTxBody anyTxBody -> do

InAnyShelleyBasedEra sbe txbody <- pure anyTxBody

let tx = makeSignedTransaction [] txbody

firstExceptT DebugCmdWriteFileError . newExceptT

$ writeLazyByteStringFile outTxFile

$ shelleyBasedEraConstraints sbe

$ textEnvelopeToJSON Nothing tx

IncompleteCddlTxBody (InAnyShelleyBasedEra sbe txbody)

<- firstExceptT DebugCmdTextEnvCddlError . newExceptT $ readFileTxBody txbodyFile

let tx = makeSignedTransaction [] txbody

firstExceptT DebugCmdWriteFileError . newExceptT

$ writeLazyByteStringFile outTxFile

$ shelleyBasedEraConstraints sbe

$ textEnvelopeToJSON Nothing tx

This case is unnecessary because it only has one option (it is a newtype)

palas · 2024-06-18T20:21:40Z

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

+      anyTx <- lift (readFileTx inputTxFile) & onLeft (left . DebugCmdTextEnvCddlError)
+
+      InAnyShelleyBasedEra sbe tx <- pure anyTx


Suggested change

anyTx <- lift (readFileTx inputTxFile) & onLeft (left . DebugCmdTextEnvCddlError)

InAnyShelleyBasedEra sbe tx <- pure anyTx

InAnyShelleyBasedEra sbe tx <- lift (readFileTx inputTxFile) & onLeft (left . DebugCmdTextEnvCddlError)

I was thinking that this would be equivalent to:

let InAnyShelleyBasedEra sbe tx = anyTx

But it is not. The let version doesn't compile, interesting 🤔
But still can do the suggestion, unless you are doing it for clarity.

palas · 2024-06-18T20:23:24Z

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

+      anyTx <- lift (readFileTx inputTxFile) & onLeft (left . TxCmdTextEnvCddlError)
+
+      InAnyShelleyBasedEra sbe tx <- pure anyTx


Suggested change

anyTx <- lift (readFileTx inputTxFile) & onLeft (left . TxCmdTextEnvCddlError)

InAnyShelleyBasedEra sbe tx <- pure anyTx

InAnyShelleyBasedEra sbe tx <- lift (readFileTx inputTxFile) & onLeft (left . TxCmdTextEnvCddlError)

Same here

palas · 2024-06-18T20:23:51Z

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

+      unwitnessed <- firstExceptT TxCmdTextEnvCddlError . newExceptT $ readFileTxBody txbodyFile
+
+      case unwitnessed of
+        IncompleteCddlTxBody anyTxBody -> do
+          InAnyShelleyBasedEra sbe txbody <- pure anyTxBody
+
+          let tx = makeSignedTransaction [] txbody
+
+          firstExceptT TxCmdWriteFileError . newExceptT
+            $ writeLazyByteStringFile outTxFile
+            $ shelleyBasedEraConstraints sbe
+            $ textEnvelopeToJSON Nothing tx


Suggested change

unwitnessed <- firstExceptT TxCmdTextEnvCddlError . newExceptT $ readFileTxBody txbodyFile

case unwitnessed of

IncompleteCddlTxBody anyTxBody -> do

InAnyShelleyBasedEra sbe txbody <- pure anyTxBody

let tx = makeSignedTransaction [] txbody

firstExceptT TxCmdWriteFileError . newExceptT

$ writeLazyByteStringFile outTxFile

$ shelleyBasedEraConstraints sbe

$ textEnvelopeToJSON Nothing tx

IncompleteCddlTxBody (InAnyShelleyBasedEra sbe txbody)

<- firstExceptT TxCmdTextEnvCddlError . newExceptT $ readFileTxBody txbodyFile

let tx = makeSignedTransaction [] txbody

firstExceptT TxCmdWriteFileError . newExceptT

$ writeLazyByteStringFile outTxFile

$ shelleyBasedEraConstraints sbe

$ textEnvelopeToJSON Nothing tx

And here

palas · 2024-07-15T22:01:58Z

Backup: https://github.com/IntersectMBO/cardano-cli/tree/backup/newhoggy/new-transaction-echo-command

palas · 2024-07-15T23:23:37Z

FYI: I have rebased your branch because we have done changes to the formatting. I have made a copy of the unrebased branch that you can find in my previous comment in this PR.

github-actions · 2024-08-30T01:57:21Z

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

newhoggy marked this pull request as ready for review April 9, 2024 12:08

newhoggy requested review from Jimbo4350, carbolymer, smelc, palas, kevinhammond and disassembler as code owners April 9, 2024 12:08

newhoggy force-pushed the newhoggy/new-transaction-echo-command branch from 0accede to 73045d3 Compare April 9, 2024 12:09

smelc reviewed Apr 9, 2024

View reviewed changes

carbolymer requested changes Apr 9, 2024

View reviewed changes

newhoggy requested a review from carbolymer April 10, 2024 10:27

github-actions bot added Stale and removed Stale labels May 26, 2024

carbolymer reviewed May 27, 2024

View reviewed changes

carbolymer approved these changes May 27, 2024

View reviewed changes

newhoggy force-pushed the newhoggy/new-transaction-echo-command branch 2 times, most recently from c288b0e to e264596 Compare June 18, 2024 14:20

newhoggy changed the title ~~New transaction echo command~~ New debug transaction echo command Jun 18, 2024

newhoggy requested a review from smelc June 18, 2024 14:24

Jimbo4350 reviewed Jun 18, 2024

View reviewed changes

palas approved these changes Jun 18, 2024

View reviewed changes

newhoggy added 2 commits July 16, 2024 00:43

New transaction echo command

6837a78

Move transaction echo command to under 'debug' command group.

1bba1f5

palas force-pushed the newhoggy/new-transaction-echo-command branch from e264596 to 1bba1f5 Compare July 15, 2024 22:45

github-actions bot added the Stale label Aug 30, 2024

github-actions bot removed the Stale label Oct 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New `debug transaction echo` command #709

New `debug transaction echo` command #709

newhoggy commented Apr 9, 2024 •

edited

Loading

smelc Apr 9, 2024

Jimbo4350 Apr 9, 2024

carbolymer Apr 9, 2024

newhoggy Apr 10, 2024 •

edited

Loading

newhoggy Apr 10, 2024

carbolymer Apr 10, 2024 •

edited

Loading

newhoggy May 26, 2024 •

edited

Loading

carbolymer May 27, 2024

Jimbo4350 Jun 18, 2024

carbolymer Jun 18, 2024

smelc commented Apr 9, 2024

carbolymer left a comment

github-actions bot commented May 26, 2024

carbolymer May 27, 2024

carbolymer left a comment •

edited

Loading

Jimbo4350 Jun 18, 2024

palas Jun 18, 2024

palas left a comment

palas Jun 18, 2024

palas Jun 18, 2024

palas Jun 18, 2024

palas Jun 18, 2024

palas Jun 18, 2024

palas commented Jul 15, 2024

palas commented Jul 15, 2024

github-actions bot commented Aug 30, 2024

		anyTx <- lift (readFileTx inputTxFile) & onLeft (left . DebugCmdTextEnvCddlError)

		InAnyShelleyBasedEra sbe tx <- pure anyTx

New debug transaction echo command #709

Are you sure you want to change the base?

New debug transaction echo command #709

Conversation

newhoggy commented Apr 9, 2024 • edited Loading

Changelog

Context

How to trust this PR

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

newhoggy Apr 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carbolymer Apr 10, 2024 • edited Loading

Choose a reason for hiding this comment

newhoggy May 26, 2024 • edited Loading

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 Apr 9, 2024

carbolymer left a comment

Choose a reason for hiding this comment

github-actions bot commented May 26, 2024

Choose a reason for hiding this comment

carbolymer left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

palas 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

palas commented Jul 15, 2024

palas commented Jul 15, 2024

github-actions bot commented Aug 30, 2024

New `debug transaction echo` command #709

New `debug transaction echo` command #709

newhoggy commented Apr 9, 2024 •

edited

Loading

newhoggy Apr 10, 2024 •

edited

Loading

carbolymer Apr 10, 2024 •

edited

Loading

newhoggy May 26, 2024 •

edited

Loading

carbolymer left a comment •

edited

Loading