transaction submit: print transaction hash #925

smelc · 2024-10-04T14:55:57Z

Changelog

- description: |
    transaction submit: print transaction hash, like this:

    Transaction successfully submitted. Transaction hash is:
    {"txhash":"456c614d5d547b7fe197a4d18fbb86e086cb9080594dabf9059adf08b00cf2bd"}

    Previously it was:

    Transaction successfully submitted.
# 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 #896

How to trust this PR

We added a test in cardano-testnet: IntersectMBO/cardano-node#6003

Checklist

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

Jimbo4350

We shouldn't add more print statements here. A more principled approach would be to query the mempool to confirm that tx has been submitted and we can derive the txid from the tx itself. Why are we printing the txid here in the first place?

smelc · 2024-10-07T07:26:24Z

Why are we printing the txid here in the first place?

This PR is an answer to this ask by a user: #896. Design has been validated by @CarlosLopezDeLara.

Jimbo4350 · 2024-10-07T13:51:28Z

Why are we printing the txid here in the first place?

This PR is an answer to this ask by a user: #896. Design has been validated by @CarlosLopezDeLara.

Responded. We need to know what they intend to do with the information and that will inform what changes we should make, if any.

CarlosLopezDeLara · 2024-10-07T14:22:12Z

I believe this feature adds good value. After submitting a transaction, it's common to share the transaction ID (txid) with the recipient or check it in a blockchain explorer. Including the txid in the output seems like a natural addition, as it's the key information needed for any follow-up.

Of course, one can compute the tx-id with cardano-cli conway transaction txid but printing it at submit makes the process more convenient and user-friendly without requiring additional steps to retrieve this essential detail.

smelc · 2024-11-07T15:57:37Z

cardano-cli/src/Cardano/CLI/Types/Common.hs

+newtype TxIdSubmission = TxIdSubmission {txhash :: T.Text}
+  deriving (Show, Generic)
+
+instance FromJSON TxIdSubmission


I've put the FromJSON instance too, in case independent Haskell tooling wants to use it.

What's wrong with FromJSON TxId?

@Jimbo4350: see #925 (comment)

carbolymer · 2024-11-07T16:13:58Z

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

+        liftIO $ Text.putStrLn "Transaction successfully submitted. Transaction hash is:"
+        let hashText = T.decodeUtf8 $ serialiseToRawBytesHex (getTxId $ getTxBody tx)
+        liftIO $ LBS.putStrLn $ Aeson.encode $ TxIdSubmission hashText


The message should go to stderr, the hash should go to stdout. Otherwise it'll be a hassle to separate one from the other.

I'm not sure: do we do that already? It could surprise the user if we separate the two (because they are really both not errors!).

I wrote the JSON on a separate line so that ... | tail -n 1 works for retrieving it.

cc @CarlosLopezDeLara, in case you have insights on our conventions for printing so far.

I think we should start doing it. tail -n 1 is wonky, because it assumes hidden knowledge about the output format, which can change between commands and over time.

That's pretty much way of outputting things in various unix tools.

stdout for output data

stderr for diagnostic messages

Try find /proc 1>/dev/null and find /proc 2>/dev/null.

If we're not doing it alredy I believe this is a good place for a precedent.
@Jimbo4350 thoughts?

Agreed during yesterday's team meeting to do it, so it's implemented now 👍

cardano-cli/src/Cardano/CLI/Types/Common.hs

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

carbolymer · 2024-11-08T15:02:05Z

cardano-cli/src/Cardano/CLI/Types/Common.hs

+
+-- | Type used for serialization when printing the hash of a transaction
+-- after having submitted it.
+newtype TxIdSubmission = TxIdSubmission {txhash :: TxId}


I still don't like this type name 😄

The newtype is unnecessary. It's obvious that this is the txid of the submitted transaction and we are using the underlying TxId JSON instances. What is the benefit of this newtype definition?

If we were using this value in multiple places and could mix it up with other TxIds, I would agree with the newtype definition. However, it's used in one place, so it's overkill.

This newtype is so that we output this JSON on stdout:

{"txhash": "d9290bc8f8f412660109e3c9740d89c19f2ba6697508fd9d4b4b54f32a2ba02e"}

If we were to use TxID's JSON format directly, we would have:

"d9290bc8f8f412660109e3c9740d89c19f2ba6697508fd9d4b4b54f32a2ba02e"

which we ruled out during our team meeting a few weeks ago (because "it wasn't JSON"). So in the end, I'm not sure what you guys want!?

My gripe is with the type name. I understand it currently as "a submission of transaction id" which is unclear in my opinion. A better name would be "a result of a transaction submission", for example: TxSubmissionResult.

This is non-critical though. I'm leaving an approval to @Jimbo4350 since he blocked the PR originally.

By the way,

"d9290bc8f8f412660109e3c9740d89c19f2ba6697508fd9d4b4b54f32a2ba02e"

is a valid JSON so I don't get the argument. 😄

Decided during today's team meeting: let's go for the {"txhash": "d9290bc8f8f412660109e3c9740d89c19f2ba6697508fd9d4b4b54f32a2ba02e"} and separate JSON from English by using stdout and stderr, as @carbolymer suggested here.

smelc mentioned this pull request Oct 4, 2024

cardano-testnet: use new output of "transaction submit" IntersectMBO/cardano-node#6003

Draft

3 tasks

smelc marked this pull request as ready for review October 4, 2024 15:10

smelc requested review from Jimbo4350, newhoggy, carbolymer, palas, kevinhammond and disassembler as code owners October 4, 2024 15:10

Jimbo4350 requested changes Oct 4, 2024

View reviewed changes

smelc force-pushed the smelc-cl/print-tx-hash-after-submission branch from 7799011 to feffd34 Compare October 30, 2024 16:19

smelc force-pushed the smelc-cl/print-tx-hash-after-submission branch from feffd34 to 1ee583f Compare November 7, 2024 11:09

smelc commented Nov 7, 2024

View reviewed changes

carbolymer reviewed Nov 7, 2024

View reviewed changes

cardano-cli/src/Cardano/CLI/Types/Common.hs Outdated Show resolved Hide resolved

carbolymer reviewed Nov 7, 2024

View reviewed changes

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

smelc mentioned this pull request Nov 8, 2024

[FR] - QoL feature: display txhash on command line after submitting tx #896

Open

smelc force-pushed the smelc-cl/print-tx-hash-after-submission branch from 1ee583f to 9fbd897 Compare November 8, 2024 15:00

carbolymer reviewed Nov 8, 2024

View reviewed changes

transaction submit: print transaction hash as JSON

2e301c2

smelc force-pushed the smelc-cl/print-tx-hash-after-submission branch from 9fbd897 to 2e301c2 Compare November 13, 2024 10:23

carbolymer approved these changes Nov 13, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

transaction submit: print transaction hash #925

transaction submit: print transaction hash #925

smelc commented Oct 4, 2024 •

edited

Loading

Jimbo4350 left a comment

smelc commented Oct 7, 2024

Jimbo4350 commented Oct 7, 2024

CarlosLopezDeLara commented Oct 7, 2024

smelc Nov 7, 2024

Jimbo4350 Nov 8, 2024

smelc Nov 12, 2024

carbolymer Nov 7, 2024 •

edited

Loading

smelc Nov 8, 2024

carbolymer Nov 8, 2024 •

edited

Loading

smelc Nov 13, 2024

carbolymer Nov 8, 2024 •

edited

Loading

Jimbo4350 Nov 8, 2024 •

edited

Loading

Jimbo4350 Nov 8, 2024

smelc Nov 12, 2024

carbolymer Nov 12, 2024 •

edited

Loading

smelc Nov 12, 2024

transaction submit: print transaction hash #925

Are you sure you want to change the base?

transaction submit: print transaction hash #925

Conversation

smelc commented Oct 4, 2024 • edited Loading

Changelog

Context

How to trust this PR

Checklist

Jimbo4350 left a comment

Choose a reason for hiding this comment

smelc commented Oct 7, 2024

Jimbo4350 commented Oct 7, 2024

CarlosLopezDeLara commented Oct 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carbolymer Nov 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carbolymer Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carbolymer Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

Jimbo4350 Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carbolymer Nov 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smelc commented Oct 4, 2024 •

edited

Loading

carbolymer Nov 7, 2024 •

edited

Loading

carbolymer Nov 8, 2024 •

edited

Loading

carbolymer Nov 8, 2024 •

edited

Loading

Jimbo4350 Nov 8, 2024 •

edited

Loading

carbolymer Nov 12, 2024 •

edited

Loading