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

Improvements to the open up transaction in third-party link action #430

Merged
merged 4 commits into from
Sep 29, 2021

Conversation

jarolrod
Copy link
Member

@jarolrod jarolrod commented Sep 24, 2021

#4092 introduced the ability to open up a transaction in a block explorer. This improves the related code by simplifying the addition and connection of the action through an overloaded addAction function and prepends action description text to the host, "Show in". The reason to add this text is to make it clear what the action does. It also creates a clearer mental correlation between a user doing the work to add the 3rd-party tx link and this new menu action popping up.

This updates the setting text so that "third-party" is hyphenated. It should be hyphenated because it is being used as a modifier of both "URL" and "transaction URLs".

Additionally, this fixes #431 by ensuring that the seperator will be added before creating action.

Screenshots of visual changes:

Context menu actions

master pr
3pt-master 3pt-pr

Setting text
(tooltip text containing usage of "third-party" is also properly hyphenated)

master pr
unnamed pr-hyphenate

Simplify the creation, addition, and slot/signal connection of
a third part tx url context menu action by using an overloaded
addAction function.
The text for an open third-party tx URL action
is improved by appending the host name with "Show in".
This makes it self-explanatory what the action will do.
@jarolrod jarolrod added the UI All about "look and feel" label Sep 24, 2021
This ensures that if we're going to add an action to open up
a transaction in a third-party link (block explorer) that it
is seperated into it's own section.
@jarolrod
Copy link
Member Author

Updated from a70a980 -> 8177578 (pr430.01 -> pr430.02, diff)

Changes: include new commit to fix #431

Copy link
Contributor

@shaavan shaavan 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

This PR introduced the overloaded addAction function, which simplifies the code while keeping the functionality intact. The introduced translator’s comments also look good. I have a suggestion regarding them that I will come to in a minute. Finally, the added actions_created boolean allows solving the bug of separator not being added while also increasing the understandability of the code without additional comments.

src/qt/transactionview.cpp Show resolved Hide resolved
Our usage of "third-party" should be hyphenated
as it is being used as a modifier of both "URL"
and "transaction URLs".
@jarolrod
Copy link
Member Author

Updated from 8177578 -> 2ccde2f (pr430.02 -> pr430.03, diff)

Changes: included one more commit to address the proper hyphenation of "third-party".

Copy link
Contributor

@shaavan shaavan left a comment

Choose a reason for hiding this comment

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

The new commit looks good. I have tested the PR on Ubuntu 20.04 (using Qt version 5.12.8) with the recent commit, and the changes are reflected perfectly in the GUI.

Master PR
master pr

Copy link
Contributor

@stratospher stratospher 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 2ccde2f.
In agreement with the PR's objectives. The PR was tested and verified to be consistent with the proposed changes:

  1. Adding the description "Show in" - gave a clearer idea of the menu action.
  2. Hyphenating to “third-party”
  3. Fixing #431 so that the separator between third-party link actions and normal actions are in different sections even when the first string in a Third party transaction URL is empty. ( Example from #431 : | | https://mempool.space/tx/%s | https://blockstream.info/tx/%s)

Notes:

  • 9980f4a
    addAction() adds openThirdPartyTxUrl() to the menu's list of actions and returns it directly. This simplifies the code as compared to the previous version where connect() was used.
  • a70a980
    Adds the description "Show in" to get a clear picture that we want to show the transaction in a third party transaction URL.
  • 8177578
    Makes a separate section for third-party links
    Uses a boolean variable to identify the first non-empty string and then adds the separator instead of arbitrarily adding the separator after the first string in listUrls[].
  • 2ccde2f
    Makes “Third party” -> “Third-party”

Copy link
Contributor

@promag promag left a comment

Choose a reason for hiding this comment

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

Code view ACK 2ccde2f.

Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

Tested 2ccde2f on Linux Mint 20.2 (Qt 5.12.8) -- #431 is fixed.

An idea for a follow up -- introduce a warning message about possibility to break privacy while using external links.

@prayank23 What do you think about such a warning?

Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

ACK 2ccde2f

@hebasto hebasto merged commit ba1a82e into bitcoin-core:master Sep 29, 2021
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Sep 29, 2021
…n third-party link action

2ccde2f qt: hyphenate usage of third-party modifier (Jarol Rodriguez)
8177578 qt: ensure seperator when adding third-party transaction links (Jarol Rodriguez)
a70a980 qt: improve text for open third-party tx url action (Jarol Rodriguez)
9980f4a qt, refactor: simplify third-party tx url action through overload (Jarol Rodriguez)

Pull request description:

  [bitcoin#4092](bitcoin#4092) introduced the ability to open up a transaction in a block explorer. This improves the related code by simplifying the addition and connection of the action through an [overloaded](https://doc.qt.io/archives/qt-5.9/qmenu.html#addAction-5) `addAction` function and prepends action description text to the host, "Show in". The reason to add this text is to make it clear what the action does. It also creates a clearer mental correlation between a user doing the work to add the 3rd-party tx link and this new menu action popping up.

  This updates the setting text so that "third-party" is hyphenated. It should be hyphenated because it is being used as a modifier of both "URL" and "transaction URLs".

  Additionally, this fixes #431 by ensuring that the seperator will be added before creating action.

  Screenshots of visual changes:

  **Context menu actions**
  |   master   |   pr   |
  |--------------|--------|
  | <img width="248" alt="3pt-master" src="https://user-images.githubusercontent.com/23396902/134618354-00278ac6-5094-44ee-8ba7-fe648fdcb7d2.png"> | <img width="248" alt="3pt-pr" src="https://user-images.githubusercontent.com/23396902/134618364-ddb64269-e5ee-40af-a2a6-1922001b6f4e.png"> |

  **Setting text**
  (tooltip text containing usage of "third-party" is also properly hyphenated)
  |   master   |   pr   |
  |--------------|--------|
  | ![unnamed](https://user-images.githubusercontent.com/23396902/134854070-fb299ba5-3491-487f-b37f-c0cd96514353.png) | ![pr-hyphenate](https://user-images.githubusercontent.com/23396902/134854127-88630cc2-a178-4376-a569-f413f66eba0d.png) |

ACKs for top commit:
  stratospher:
    tested ACK 2ccde2f.
  promag:
    Code view ACK 2ccde2f.
  hebasto:
    ACK 2ccde2f

Tree-SHA512: 8dfcd539a1d41c8abf3c8208d150d1480d4ef81a008de826299e8bad1dfa6e3c49dc76d041c5946fafcf0b033eebb9b9fbd3d49ba6d8af93dd388c488e92f143
@ghost
Copy link

ghost commented Sep 29, 2021

An idea for a follow up -- introduce a warning message about possibility to break privacy while using external links.

Yes we can inform the user about privacy issues because using such block explorers when you are running your own node doesn't make sense. Can add one line about using open source explorers with Bitcoin Core so URL will be localhost.

@promag
Copy link
Contributor

promag commented Sep 29, 2021

The user might want to share the link with someone?

@ghost
Copy link

ghost commented Jan 20, 2022

The user might want to share the link with someone?

Makes sense. Warning can be avoided in that case or sharing transaction id without explorer links is also good enough as the other person can check it in the explorer they prefer to use.

I was testing different things in GUI today to see if attackers can get IP for users and a fun experiment to share problems with users can be:

  1. Create a DNS token: https://canarytokens.org/generate
  2. Save https://randomchars.canarytokens.com/tx/%s in Bitcoin Core settings
  3. Right click on any transaction
  4. Check your email for IP address

image

Why would someone use a random explorer? Attackers can use several ways to convince newbies with features that are not available in other explorers. Example: https://bitcointalk.org/index.php?topic=2100392.0

@bitcoin-core bitcoin-core locked and limited conversation to collaborators Jan 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
UI All about "look and feel"
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Separator can be omitted when adding third-party link actions to the transaction table
5 participants