Include vout when copying transaction ID from coin selection

meshcollider commented at 12:13 pm on September 27, 2021: contributor

Fixes #432

I think it makes sense to just add the vout to the existing function because I can’t imagine a situation where a user in the coin selection dialog would want just the transaction ID rather than the specific outpoint, and they can just delete it from the end anyway.

meshcollider added the label Wallet on Sep 27, 2021

laanwj commented at 12:16 pm on September 27, 2021: member

ACK c4ea11c919a49322578db549f31c6e68a2560540 FWIW we used to append the vout to the id when copying from the normal transaction list as well, a long time ago, but it always got a lot of flak from confused users. But I think it’s fine here, coin control is an advanced feature anyway.

jarolrod commented at 5:08 pm on September 27, 2021: member

Concept ACK

Can we do something to document this change in the text of the related context menu action? If the text states copy transaction ID then a user will expect ONLY the txid. We should set the correct expectations of what the action will do.

maybe the following?

0@@ -55,7 +55,7 @@ CoinControlDialog::CoinControlDialog(CCoinControl& coin_control, WalletModel* _m
1     contextMenu->addAction(tr("&Copy address"), this, &CoinControlDialog::copyAddress);
2     contextMenu->addAction(tr("Copy &label"), this, &CoinControlDialog::copyLabel);
3     contextMenu->addAction(tr("Copy &amount"), this, &CoinControlDialog::copyAmount);
4-    copyTransactionHashAction = contextMenu->addAction(tr("Copy transaction &ID"), this, &CoinControlDialog::copyTransactionHash);
5+    copyTransactionHashAction = contextMenu->addAction(tr("Copy transaction &ID/vout"), this, &CoinControlDialog::copyTransactionHash);
6     contextMenu->addSeparator();
7     lockAction = contextMenu->addAction(tr("L&ock unspent"), this, &CoinControlDialog::lockCoin);
8     unlockAction = contextMenu->addAction(tr("&Unlock unspent"), this, &CoinControlDialog::unlockCoin);

meshcollider commented at 11:23 pm on September 27, 2021: contributor

Modified the context menu text as per @jarolrod’s suggestion

in src/qt/coincontroldialog.cpp:232 in fafed470d4 outdated

230@@ -231,7 +231,8 @@ void CoinControlDialog::copyAddress()
231 // context menu action: copy transaction id
232 void CoinControlDialog::copyTransactionHash()

kristapsk commented at 0:15 am on September 28, 2021:

Method should be renamed as well as comment above, as it no more copies just transaction hash id alone.

meshcollider commented at 0:32 am on September 28, 2021:

Done :)

in src/qt/coincontroldialog.h:66 in 949e700149 outdated

62@@ -63,7 +63,7 @@ class CoinControlDialog : public QDialog
63 
64     QMenu *contextMenu;
65     QTreeWidgetItem *contextMenuItem;
66-    QAction *copyTransactionHashAction;
67+    QAction *copyTransactionOutpointAction;

shaavan commented at 1:38 pm on September 28, 2021:

The Clang format checker suggested the following change:

0    QAction* copyTransactionOutpointAction;

promag commented at 2:06 pm on September 28, 2021:

nit, also use m_ prefix and snake case.

in src/qt/coincontroldialog.cpp:231 in 949e700149 outdated

228@@ -229,9 +229,10 @@ void CoinControlDialog::copyAddress()
229 }
230 
231 // context menu action: copy transaction id

shaavan commented at 1:39 pm on September 28, 2021:

I guess you missed updating the comment.

0// context menu action: copy transaction id and output index

shaavan commented at 1:43 pm on September 28, 2021: contributor

Concept ACK

This PR adds the functionality of copying output index and transaction ID as a context menu option from the Coin Selection dialog box. I was able to successfully test this PR on Ubuntu 20.04 (Using Qt version 5.12.8).

The Difference with Master:

Master	PR
9f4cc49b1294299ae9bba26e5b48b82a032222f8c46d05e455602b298ff8bcba	9f4cc49b1294299ae9bba26e5b48b82a032222f8c46d05e455602b298ff8bcba:1

The changes in functions’ names aptly represent the change in functionality. I have mentioned some minor nits that shall be looked into.

Btw, I was just curious. Would it be beneficial to apply this functionality to the context menu action under the transaction tab?

in src/qt/coincontroldialog.cpp:234 in 949e700149 outdated

228@@ -229,9 +229,10 @@ void CoinControlDialog::copyAddress()
229 }
230 
231 // context menu action: copy transaction id
232-void CoinControlDialog::copyTransactionHash()
233+void CoinControlDialog::copyTransactionOutpoint()
234 {
235-    GUIUtil::setClipboard(contextMenuItem->data(COLUMN_ADDRESS, TxHashRole).toString());
236+    const QString outpoint = contextMenuItem->data(COLUMN_ADDRESS, TxHashRole).toString() + ":" + contextMenuItem->data(COLUMN_ADDRESS, VOutRole).toString();

promag commented at 2:04 pm on September 28, 2021:

nit, maybe a bit more clear:

0const auto address = contextMenuItem->data(COLUMN_ADDRESS, TxHashRole).toString();
1const auto vout = contextMenuItem->data(COLUMN_ADDRESS, VOutRole).toString();
2const auto outpoint = QString("%1:%2").arg(address).arg(vout);

promag commented at 2:09 pm on September 28, 2021: contributor

Code review ACK 949e700149a7d84e7b507f9c8d5dd44af1d82ef2. @laanwj I think we can have both actions copy-txid and copy-outpoint.

Left some nits.

meshcollider commented at 8:52 pm on September 28, 2021: contributor

Thanks for the reviews, will address the nits shortly.

Would it be beneficial to apply this functionality to the context menu action under the transaction tab?

IMO no, because unlike the coin selection which deals with UTXOs, transactions can have multiple outputs and most non-technical users don’t understand the idea of outputs anyway, so it would just be confusing.

Include vout when copying transaction ID from coin selection 10c6929d55

meshcollider commented at 10:17 pm on September 28, 2021: contributor

Addressed nits (and rebased)

kristapsk approved

kristapsk commented at 8:21 am on September 29, 2021: contributor

ACK 10c6929d55ba9bc203bbadfb834537445dbd67ce

in src/qt/coincontroldialog.cpp:58 in 10c6929d55

54@@ -55,7 +55,7 @@ CoinControlDialog::CoinControlDialog(CCoinControl& coin_control, WalletModel* _m
55     contextMenu->addAction(tr("&Copy address"), this, &CoinControlDialog::copyAddress);
56     contextMenu->addAction(tr("Copy &label"), this, &CoinControlDialog::copyLabel);
57     contextMenu->addAction(tr("Copy &amount"), this, &CoinControlDialog::copyAmount);
58-    copyTransactionHashAction = contextMenu->addAction(tr("Copy transaction &ID"), this, &CoinControlDialog::copyTransactionHash);
59+    m_copy_transaction_outpoint_action = contextMenu->addAction(tr("Copy transaction &ID and output index"), this, &CoinControlDialog::copyTransactionOutpoint);

promag commented at 10:10 am on September 29, 2021:

Anything against having both actions?

meshcollider commented at 10:31 am on September 29, 2021:

@promag seems a bit more GUI clutter for no purpose. It’s a manual action touching the clipboard so it can’t interfere with any automation, so the user can always just manually delete the :n on the end if they don’t want it. IMO txid alone is useless in this situation because this is a specific outpoint selection dialog.

hebasto commented at 10:52 am on September 29, 2021: member

Concept ACK.

hebasto approved

hebasto commented at 11:01 am on September 29, 2021: member

ACK 10c6929d55ba9bc203bbadfb834537445dbd67ce, tested on Linux Mint 20.2 (Qt 5.12.8).

shaavan approved

shaavan commented at 1:52 pm on September 29, 2021: contributor

ACK 10c6929

Changes since my last review:

copyTransactionOutpointAction has been renamed to m_copy_transaction_outpoint_action.
Comment has been aptly updated to match the change in functionality.
The copyTransactionOutpoint function has been expanded to make it more clear.
PR has been rebased on the current master.
Formatting has now been corrected.

Difference with Master (Screenshot):

Master	PR

hebasto merged this on Sep 29, 2021

hebasto closed this on Sep 29, 2021

sidhujag referenced this in commit 8048ecdb94 on Sep 29, 2021

meshcollider deleted the branch on Sep 29, 2021

bitcoin-core locked this on Sep 30, 2022

Include vout when copying transaction ID from coin selection #436