Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

transaction submit: print transaction hash #925

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

transaction submit: print transaction hash #925

wants to merge 2 commits into from
+18 −1

Conversation

smelc
Copy link
Contributor

@smelc smelc commented Oct 4, 2024 •
edited
Loading

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

@smelc smelc mentioned this pull request Oct 4, 2024
Draft
3 tasks
@smelc smelc marked this pull request as ready for review October 4, 2024 15:10
Jimbo4350
Jimbo4350 requested changes Oct 4, 2024
View reviewed changes
Copy link
Contributor

@Jimbo4350 Jimbo4350 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

smelc commented Oct 7, 2024

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
Copy link
Contributor

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
Copy link
Contributor

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 smelc force-pushed the smelc-cl/print-tx-hash-after-submission branch from 7799011 to feffd34 Compare October 30, 2024 16:19
@smelc smelc force-pushed the smelc-cl/print-tx-hash-after-submission branch from feffd34 to 1ee583f Compare November 7, 2024 11:09
smelc
smelc commented Nov 7, 2024
View reviewed changes
cardano-cli/src/Cardano/CLI/Types/Common.hs
newtype TxIdSubmission = TxIdSubmission {txhash :: T.Text}
deriving (Show, Generic)

instance FromJSON TxIdSubmission
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

carbolymer
carbolymer reviewed Nov 7, 2024
View reviewed changes
cardano-cli/src/Cardano/CLI/EraBased/Run/Transaction.hs
Comment on lines +1488 to +1490
liftIO $ Text.putStrLn "Transaction successfully submitted. Transaction hash is:"
let hashText = T.decodeUtf8 $ serialiseToRawBytesHex (getTxId $ getTxBody tx)
liftIO $ LBS.putStrLn $ Aeson.encode $ TxIdSubmission hashText
Copy link
Contributor

@carbolymer carbolymer Nov 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

carbolymer
carbolymer reviewed Nov 7, 2024
View reviewed changes
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 :: T.Text}
Copy link
Contributor

@carbolymer carbolymer Nov 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. The name should be TxSubmissionResult or something similar, it's a result of a submitting a transaction and not tx id.
  2. Why not use TxId? It has a JSON instance already. I mean:
newtype TxSubmissionResult = TxSubmissionResult {txhash :: TxId}

carbolymer
carbolymer reviewed Nov 7, 2024
View reviewed changes
cardano-cli/src/Cardano/CLI/EraBased/Run/Transaction.hs
Net.Tx.SubmitSuccess -> liftIO $ Text.putStrLn "Transaction successfully submitted."
Net.Tx.SubmitSuccess -> do
liftIO $ Text.putStrLn "Transaction successfully submitted. Transaction hash is:"
let hashText = T.decodeUtf8 $ serialiseToRawBytesHex (getTxId $ getTxBody tx)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This duplicates the ToJSON TxId instance. Can we avoid that?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carbolymer carbolymer carbolymer left review comments

@Jimbo4350 Jimbo4350 Jimbo4350 requested changes

@newhoggy newhoggy Awaiting requested review from newhoggy newhoggy is a code owner

@palas palas Awaiting requested review from palas palas is a code owner

@kevinhammond kevinhammond Awaiting requested review from kevinhammond kevinhammond is a code owner

@disassembler disassembler Awaiting requested review from disassembler disassembler is a code owner

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[FR] - QoL feature: display txhash on command line after submitting tx
4 participants
@smelc @Jimbo4350 @CarlosLopezDeLara @carbolymer