getrawtransaction implementation #777

BrandonOdiwuor · 2023-11-14T11:36:45Z

Fixes #645.

Add getrawtransaction RPC to GUI on Tools -> Verify external txid. tab

Screencast.from.21-12-2023.06.35.04.ASUBUHI.webm

DrahtBot · 2023-11-14T11:36:48Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	pablomartin4btc, alfonsoromanz
Concept ACK	hernanmarino, kristapsk, hebasto

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#bitcoin/bitcoin/29790 ([DO NOT MERGE] cmake: Migrate CI scripts to CMake-based build system -- WIP by hebasto)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

willcl-ark · 2023-11-14T11:56:57Z

Cool, that's pretty nice!

Just thinking out loud, but it does make me wonder if perhaps a new Verify or Utils button, in line with "Send | Recieve | Transactions" could be even nicer? If we added it there, we might consider adding a few more utils to the GUI like "Decode raw transaction", "Decode Script", "Get Descriptor Info", "Verify Message" etc. which folks who use the gui generally use other software for today...

BrandonOdiwuor · 2023-11-14T14:42:34Z

Cool, that's pretty nice!

Just thinking out loud, but it does make me wonder if perhaps a new Verify or Utils button, in line with "Send | Recieve | Transactions" could be even nicer? If we added it there, we might consider adding a few more utils to the GUI like "Decode raw transaction", "Decode Script", "Get Descriptor Info", "Verify Message" etc. which folks who use the gui generally use other software for today...

@willcl-ark would love to hear more opinions on this, I also think Help -> Verify external txid tab might not work well with adding more utils

willcl-ark · 2023-11-14T14:46:10Z

Yeah I don't want to derail this pr, which I think looks good on its own!

If there was desire to add more utils we could always move this later; I think this looks nice.

luke-jr

Needs a lot of UX work. Definitely doesn't belong on the Help menu...

pablomartin4btc

Concept ACK

As @luke-jr perhaps we can create a separate menu item like "Tools" or "Utils" with this feature in it (and would be more suitable if we need to add for example other features as @willcl-ark mentioned).

Light CR.

pablomartin4btc · 2023-11-22T15:57:47Z

src/qt/utilitydialog.cpp

+            setWindowTitle(tr("About %1").arg(PACKAGE_NAME));
+
+            std::string licenseInfo = LicenseInfo();
+            /// HTML-format the license message from the core
+            QString licenseInfoHTML = QString::fromStdString(LicenseInfo());
+            // Make URLs clickable
+            QRegularExpression uri(QStringLiteral("<(.*)>"), QRegularExpression::InvertedGreedinessOption);
+            licenseInfoHTML.replace(uri, QStringLiteral("<a href=\"\\1\">\\1</a>"));
+            // Replace newlines with HTML breaks
+            licenseInfoHTML.replace("\n", "<br>");
+
+            ui->aboutMessage->setTextFormat(Qt::RichText);
+            ui->scrollArea->setVerticalScrollBarPolicy(Qt::ScrollBarAsNeeded);
+            text = version + "\n" + QString::fromStdString(FormatParagraph(licenseInfo));
+            ui->aboutMessage->setText(version + "<br><br>" + licenseInfoHTML);
+            ui->aboutMessage->setWordWrap(true);
+            ui->helpMessage->setVisible(false);
+            ui->labelTxId->setVisible(false);
+            ui->txidEdit->setVisible(false);
+            ui->labelBlockHash->setVisible(false);
+            ui->blockHashEdit->setVisible(false);
+            ui->submitButton->setVisible(false);
+            ui->verboseCheckbox->setVisible(false);
+            break;


Since you are here (refactoring commit 10b3cd1) I'd extract all these settings into a function like "initializeAboutModeWindow" or something like that, same for the other modes, making the code a bit more maintainable and easier to follow.

Add getrawtransaction RPC to GUI to tools > getrawtransactions

hernanmarino

Concept ACK.

BrandonOdiwuor · 2023-12-21T03:59:50Z

I have updated the PR based on the feedback given above:

Added Tools menu
Moved getrawtransaction RPC implementation to Tools > verify external txid

cc: @willcl-ark @luke-jr @pablomartin4btc

pablomartin4btc

utACK 9659890

I agree that the new place for this PR's feature looks better under the new "Tools" menu.

Also code-wise it's better to have a new separate dialog (toolsdialog) rather than re-using the existing help dialog (HelpMessageDialog). Thinking it loud, I wonder if we need to specify a more specific name instead of the current generic one (what other use cases would open this ToolsDialog? Other RPC commands?). Another thoughts: how this new feature (and future ones?) integrates with the current RPC console? Should we also have some kind of link on the form to open the console with the execution of the rpc command and its output? Should we create a new sub-menu under "Tools" like "RPC commands"? These are some questions that don't need to be answered right now nor stopping this PR to go ahead and get merged, just to keep the focus on what directions we want to give to the UI and follow it up.

pablomartin4btc · 2023-12-28T14:07:45Z

src/qt/forms/toolsdialog.ui

+       <item row="0" column="0">
+        <widget class="QLabel" name="labelTxId">
+         <property name="text">
+          <string>transaction id: </string>


nit:

Suggested change

<string>transaction id: </string>

<string>Transaction id: </string>

or ID?

pablomartin4btc · 2023-12-28T14:08:47Z

src/qt/forms/toolsdialog.ui

+       <item row="2" column="0">
+        <widget class="QLabel" name="labelBlockHash">
+         <property name="text">
+          <string>blockhash (optional): </string>


nit:

Suggested change

<string>blockhash (optional): </string>

<string>Blockhash (optional): </string>

pablomartin4btc · 2023-12-28T14:23:23Z

src/qt/bitcoingui.cpp

@@ -368,12 +369,17 @@ void BitcoinGUI::createActions()
    m_mask_values_action->setStatusTip(tr("Mask the values in the Overview tab"));
    m_mask_values_action->setCheckable(true);

+    getRawTransactionAction = new QAction(tr("&Verify external txid"), this);


nit:

Suggested change

getRawTransactionAction = new QAction(tr("&Verify external txid"), this);

getRawTransactionAction = new QAction(tr("&Verify transaction"), this);

Some thoughts: perhaps we use a more generic naming here and in the form we indicate if it's a wallet transaction with a checkbox (and then we change the form removing the blockhash option and call gettransaction?). Or maybe just on a follow-up.

kristapsk · 2023-12-28T16:46:09Z

Concept ACK

alfonsoromanz

Tested ACK 9659890

hebasto · 2024-02-12T13:36:28Z

Concept ACK. However, I still agree with @promag's bitcoin/bitcoin#19161 (comment):

GUI users can use the RPC console.

maflcko · 2024-02-13T08:42:06Z

Seems confusing to have the same feature present twice. Once in this pop-up and another time in the RPC pop-up?

If this is merged, then every RPC will be duplicated in a new GUI pop-up?

BrandonOdiwuor force-pushed the getrawtransactions_helperspage branch from 9485fb4 to e233da2 Compare November 14, 2023 12:56

DrahtBot added the CI failed label Nov 14, 2023

BrandonOdiwuor changed the title ~~GUI getrawtransaction implementation~~ gui: getrawtransaction implementation Nov 14, 2023

DrahtBot removed the CI failed label Nov 14, 2023

This was referenced Nov 14, 2023

Update about logo icon (colour) to denote the chain type of the QT instance in About/ Help Message Window/ Dialog #762

Open

Modify command line help to show support for BIP21 URIs #752

Merged

luke-jr suggested changes Nov 15, 2023

View reviewed changes

hebasto changed the title ~~gui: getrawtransaction implementation~~ getrawtransaction implementation Nov 20, 2023

pablomartin4btc reviewed Nov 22, 2023

View reviewed changes

BrandonOdiwuor force-pushed the getrawtransactions_helperspage branch from e233da2 to a17fb06 Compare December 20, 2023 12:53

Gui: getrawtransaction Implementation

9659890

Add getrawtransaction RPC to GUI to tools > getrawtransactions

BrandonOdiwuor force-pushed the getrawtransactions_helperspage branch from a17fb06 to 9659890 Compare December 20, 2023 14:19

DrahtBot added CI failed and removed CI failed labels Dec 20, 2023

hernanmarino approved these changes Dec 20, 2023

View reviewed changes

pablomartin4btc reviewed Dec 28, 2023

View reviewed changes

DrahtBot requested review from hernanmarino and removed request for hernanmarino December 28, 2023 14:24

DrahtBot requested review from hernanmarino and removed request for hernanmarino December 28, 2023 16:46

DrahtBot added CI failed and removed CI failed labels Jan 2, 2024

alfonsoromanz approved these changes Jan 9, 2024

View reviewed changes

DrahtBot requested review from hernanmarino and removed request for hernanmarino January 9, 2024 20:28

DrahtBot added the CI failed label Jan 14, 2024

DrahtBot requested review from hernanmarino and removed request for hernanmarino February 12, 2024 13:36

DrahtBot requested review from hernanmarino and removed request for hernanmarino February 13, 2024 08:42

DrahtBot mentioned this pull request Apr 9, 2024

[DO NOT MERGE] cmake: Migrate CI scripts to CMake-based build system -- WIP bitcoin/bitcoin#29790

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

getrawtransaction implementation #777

getrawtransaction implementation #777

BrandonOdiwuor commented Nov 14, 2023 •

edited

DrahtBot commented Nov 14, 2023 •

edited

willcl-ark commented Nov 14, 2023

BrandonOdiwuor commented Nov 14, 2023

willcl-ark commented Nov 14, 2023

luke-jr left a comment

pablomartin4btc left a comment

pablomartin4btc Nov 22, 2023

hernanmarino left a comment

BrandonOdiwuor commented Dec 21, 2023

pablomartin4btc left a comment

pablomartin4btc Dec 28, 2023

pablomartin4btc Dec 28, 2023

pablomartin4btc Dec 28, 2023

kristapsk commented Dec 28, 2023

alfonsoromanz left a comment

hebasto commented Feb 12, 2024

maflcko commented Feb 13, 2024

	<string>transaction id: </string>
	<string>Transaction id: </string>

	<string>blockhash (optional): </string>
	<string>Blockhash (optional): </string>

	getRawTransactionAction = new QAction(tr("&Verify external txid"), this);
	getRawTransactionAction = new QAction(tr("&Verify transaction"), this);

getrawtransaction implementation #777

Are you sure you want to change the base?

getrawtransaction implementation #777

Conversation

BrandonOdiwuor commented Nov 14, 2023 • edited

DrahtBot commented Nov 14, 2023 • edited

Reviews

Conflicts

willcl-ark commented Nov 14, 2023

BrandonOdiwuor commented Nov 14, 2023

willcl-ark commented Nov 14, 2023

luke-jr left a comment

Choose a reason for hiding this comment

pablomartin4btc left a comment

Choose a reason for hiding this comment

pablomartin4btc Nov 22, 2023

Choose a reason for hiding this comment

hernanmarino left a comment

Choose a reason for hiding this comment

BrandonOdiwuor commented Dec 21, 2023

pablomartin4btc left a comment

Choose a reason for hiding this comment

pablomartin4btc Dec 28, 2023

Choose a reason for hiding this comment

pablomartin4btc Dec 28, 2023

Choose a reason for hiding this comment

pablomartin4btc Dec 28, 2023

Choose a reason for hiding this comment

kristapsk commented Dec 28, 2023

alfonsoromanz left a comment

Choose a reason for hiding this comment

hebasto commented Feb 12, 2024

maflcko commented Feb 13, 2024

BrandonOdiwuor commented Nov 14, 2023 •

edited

DrahtBot commented Nov 14, 2023 •

edited