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

getrawtransaction implementation #777

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

BrandonOdiwuor
Copy link
Contributor

@BrandonOdiwuor BrandonOdiwuor commented Nov 14, 2023

Fixes #645.

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

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

@DrahtBot
Copy link
Contributor

DrahtBot commented Nov 14, 2023

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

No conflicts as of last run.

@willcl-ark
Copy link
Member

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

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

@BrandonOdiwuor BrandonOdiwuor changed the title GUI getrawtransaction implementation gui: getrawtransaction implementation Nov 14, 2023
@willcl-ark
Copy link
Member

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.

Copy link
Member

@luke-jr luke-jr left a comment

Choose a reason for hiding this comment

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

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

@hebasto hebasto changed the title gui: getrawtransaction implementation getrawtransaction implementation Nov 20, 2023
Copy link
Contributor

@pablomartin4btc pablomartin4btc left a comment

Choose a reason for hiding this comment

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

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.

Comment on lines 49 to 72
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;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

@hernanmarino hernanmarino left a comment

Choose a reason for hiding this comment

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

Concept ACK.

@BrandonOdiwuor
Copy link
Contributor Author

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

Copy link
Contributor

@pablomartin4btc pablomartin4btc left a comment

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

nit:

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

or ID?

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

Choose a reason for hiding this comment

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

nit:

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

@@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

@DrahtBot DrahtBot requested review from hernanmarino and removed request for hernanmarino December 28, 2023 14:24
@kristapsk
Copy link
Contributor

Concept ACK

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

@alfonsoromanz alfonsoromanz left a comment

Choose a reason for hiding this comment

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

Tested ACK 9659890

screen-recording

@DrahtBot DrahtBot requested review from hernanmarino and removed request for hernanmarino January 9, 2024 20:28
@hebasto
Copy link
Member

hebasto commented Feb 12, 2024

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

GUI users can use the RPC console.

@DrahtBot DrahtBot requested review from hernanmarino and removed request for hernanmarino February 12, 2024 13:36
@maflcko
Copy link
Contributor

maflcko commented Feb 13, 2024

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?

@DrahtBot
Copy link
Contributor

🐙 This pull request conflicts with the target branch and needs rebase.

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

Successfully merging this pull request may close these issues.

Bitcoin Core GUI getrawtransaction implementation
10 participants