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.

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

Add a summary of the signed transaction #1526

Merged
merged 4 commits into from
Feb 2, 2024

Conversation

OBorce
Copy link
Contributor

@OBorce OBorce commented Feb 1, 2024

Print out a summary of the singed transaction form the wallet CLI

@TheQuantumPhysicist TheQuantumPhysicist marked this pull request as draft February 1, 2024 16:10
@TheQuantumPhysicist
Copy link
Contributor

I will change a few things and make transactions generally printable. I converted the PR to draft.

@TheQuantumPhysicist TheQuantumPhysicist marked this pull request as ready for review February 1, 2024 21:09
Copy link
Contributor

@TheQuantumPhysicist TheQuantumPhysicist left a comment

Choose a reason for hiding this comment

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

Switched to ready to review. Please double-check that what I did makes sense.

Copy link
Contributor

@iljakuklic iljakuklic left a comment

Choose a reason for hiding this comment

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

This looks good to me.

A nuclear option would be to generalise this with traits, roughly like this:

  • Introduce a Summarize trait that has the same signature as std::fmt::Display but takes an extra chain_config parameter
  • Introduce a wrapper type Summary(T, &ChainConfig) which implements Display if T: Summarize. It now works everywhere you can use Display.
  • There could be even extension trait that makes the syntax even more convenient, changing from Summary(my_tx, &cc) to my_tx.summary(&cc)
  • The summary trait could be implemented for outputs, inputs, transactions, or whatever else needs to be summarized

The above reuses all the formatting machinery already in Rust and would integrate rather nicely with existing tools, for exmaple:

  • log::debug!("Summary of transaction: {}", my_tx.summary(&cc))
  • same with println!, format!, write! and such
  • my_tx.summary(&cc).to_string() would work too

Comment on lines +156 to +160
let s = match output {
TxOutput::Transfer(val, dest) => {
let val_str = fmt_val(val);
format!("Transfer({}, {val_str})", fmt_dest(dest))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The output printout could be taken out to a separate function in case someone wants to print outputs separately. Similarly with inputs.

@TheQuantumPhysicist TheQuantumPhysicist merged commit 79ecb2b into master Feb 2, 2024
27 checks passed
@TheQuantumPhysicist TheQuantumPhysicist deleted the feature/wallet-sign-tx-summary branch February 2, 2024 11:24
Copy link
Contributor

@TheQuantumPhysicist TheQuantumPhysicist left a comment

Choose a reason for hiding this comment

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

I will do the separation in a separate PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants