Skip to content

Show permissions instead of whitelisted #34

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

Merged
merged 1 commit into from
Jul 16, 2020

Conversation

laanwj
Copy link
Member

@laanwj laanwj commented Jul 15, 2020

Show detailed permissions instead of legacy "whitelisted" flag in the peer list details.
These are formatted with & in between just like services flags. It reuses the "N/A" translation message if there are no special permissions.
This removes the one-but-last use of legacyWhitelisted.

Show detailed permissions instead of legacy "whitelisted" flag.
These are formatted with `&` in between just like services flags.
It reuses the "N/A" translation message if not.
This removes the one-but-last use of `legacyWhitelisted`.
@hebasto
Copy link
Member

hebasto commented Jul 15, 2020

Concept ACK.

@maflcko
Copy link
Contributor

maflcko commented Jul 16, 2020

Screenshot from 2020-07-16 08-03-03

Tested aCK

@maflcko maflcko changed the title gui: Show permissions instead of whitelisted Show permissions instead of whitelisted Jul 16, 2020
@maflcko maflcko merged commit 9a714c5 into bitcoin-core:master Jul 16, 2020
Copy link
Member

@jonatack jonatack left a comment

Choose a reason for hiding this comment

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

post-merge ACK 784ef8b

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 review ACK 784ef8b.

@@ -1120,7 +1120,15 @@ void RPCConsole::updateNodeDetail(const CNodeCombinedStats *stats)
ui->peerSubversion->setText(QString::fromStdString(stats->nodeStats.cleanSubVer));
ui->peerDirection->setText(stats->nodeStats.fInbound ? tr("Inbound") : tr("Outbound"));
ui->peerHeight->setText(QString::number(stats->nodeStats.nStartingHeight));
ui->peerWhitelisted->setText(stats->nodeStats.m_legacyWhitelisted ? tr("Yes") : tr("No"));
Copy link
Contributor

Choose a reason for hiding this comment

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

Why wasn't m_legacyWhitelisted removed?

Copy link
Contributor

Choose a reason for hiding this comment

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

See OP

This removes the one-but-last use of legacyWhitelisted.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh right, miss interpreted "one-but-last", sorry.

Copy link
Contributor

Choose a reason for hiding this comment

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

@@ -1120,7 +1120,15 @@ void RPCConsole::updateNodeDetail(const CNodeCombinedStats *stats)
ui->peerSubversion->setText(QString::fromStdString(stats->nodeStats.cleanSubVer));
ui->peerDirection->setText(stats->nodeStats.fInbound ? tr("Inbound") : tr("Outbound"));
ui->peerHeight->setText(QString::number(stats->nodeStats.nStartingHeight));
ui->peerWhitelisted->setText(stats->nodeStats.m_legacyWhitelisted ? tr("Yes") : tr("No"));
if (stats->nodeStats.m_permissionFlags == PF_NONE) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit, could #include <net_permissions.h>.

@Sjors
Copy link
Member

Sjors commented Jul 16, 2020

Post merge concept ACK, nice!

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jul 18, 2020
784ef8b gui: Show permissions instead of whitelisted (Wladimir J. van der Laan)

Pull request description:

  Show detailed permissions instead of legacy "whitelisted" flag in the peer list details.
  These are formatted with `&` in between just like services flags. It reuses the "N/A" translation message if there are no special permissions.
  This removes the one-but-last use of `legacyWhitelisted`.

Top commit has no ACKs.

Tree-SHA512: 11982da4b9d408c74bc56bb3c540c0eb22506be6353aa4d4d6c64461d140f0587be194e2daad1612fddaa2618025a856b33928ad89041558f418f721f6abd407
apoelstra added a commit to apoelstra/elements that referenced this pull request Dec 3, 2020
gwillen pushed a commit to ElementsProject/elements that referenced this pull request Mar 19, 2021
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Sep 1, 2021
Summary:
Show detailed permissions instead of legacy "whitelisted" flag.
These are formatted with `&` in between just like services flags.
It reuses the "N/A" translation message if not.

This is a backport of [[bitcoin-core/gui#34 | core-gui#34]]

Test Plan:
` ninja && src/qt/bitcoin-qt`
Menu Window > Peers. Select a peer to see its permissions.

Reviewers: #bitcoin_abc, majcosta

Reviewed By: #bitcoin_abc, majcosta

Differential Revision: https://reviews.bitcoinabc.org/D10011
@bitcoin-core bitcoin-core locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants