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.

Sign up for GitHub

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

Jump to bottom

[GUI] Cold staking, alert user if the owner address is not from the wallet #1121

Merged
merged 1 commit into from Nov 28, 2019
Merged

[GUI] Cold staking, alert user if the owner address is not from the wallet #1121

merged 1 commit into from Nov 28, 2019

Conversation

furszy
Copy link

@furszy furszy commented Nov 20, 2019

Cold staking delegation, popup alerting user if the owner address is not from the wallet.

Preview.
Screen Shot 2019-11-20 at 10 59 46 AM

@furszy furszy self-assigned this Nov 20, 2019
random-zebra
random-zebra suggested changes Nov 20, 2019
View reviewed changes
Copy link

@random-zebra random-zebra left a comment •
edited

Choose a reason for hiding this comment

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

I don't like too much the wording.
Would make it simpler, something like

Alert: the owner-address (%%address%%) private key is not present.
The transaction outputs will not be spendable by this wallet.
Do you wish to proceed?
Yes-No

(replacing %%address%% with the actual address in question).
Personal taste, I guess. Better have inputs from native speakers.

@random-zebra random-zebra added this to the 4.0.0 milestone Nov 20, 2019
@furszy
Copy link
Author

furszy commented Nov 20, 2019 •
edited

Two points:

  1. No need to present the owner address again in the dialog, the delegation screen has an edit box with a big subtitle that points to it. Just the alert message.

  2. "regular" users don't know what outputs/inputs are, they only know that they have "coins" in "addresses". Terminology must be friendly enough to make them understand how dangerous is this. Users will not be able to spend the delegated coins.

@Fuzzbawls
Copy link
Collaborator

I think this may be a better warning text:

WARNING!
Sending to an external owner address.

The delegated coins will NOT be spendable by this wallet. Spending these coins will need to be done from the wallet or device containing the owner address.

Do you wish to proceed?

No Yes

not perfect, but I don't think we can cover each and every use case (paper wallets, for example) without risking an overly complicated and lengthy message.

@random-zebra
Copy link

Agree with @Fuzzbawls version

@furszy
Copy link
Author

furszy commented Nov 24, 2019 •
edited

Agree on the main message, the first line needs some polish, it's not user friendly enough, thinking on Delegating to an owner address that doesn't belong to this wallet!.

The rest is fine for me.

@random-zebra
Copy link

Wouldn't say that grammar errors are more "user friendly". :P
Sending/Delegating to an external owner address is clear enough imo.

@furszy
Copy link
Author

furszy commented Nov 24, 2019

If i have to state that i'm not native after all this time we are in troubles.. you got what i meant.

Mrs-X
Mrs-X approved these changes Nov 28, 2019
View reviewed changes
Copy link

@Mrs-X Mrs-X left a comment

Choose a reason for hiding this comment

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

I'm fine with the final text, so utACK 6daa383

random-zebra
random-zebra approved these changes Nov 28, 2019
View reviewed changes
Copy link

@random-zebra random-zebra left a comment

Choose a reason for hiding this comment

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

utACK 6daa383

random-zebra added a commit that referenced this pull request Nov 28, 2019
@random-zebra
Merge #1121: [GUI] Cold staking, alert user if the owner address is n…
0216fd3 
…ot from the wallet

6daa383 [GUI] Cold staking, alert user if the owner address is not from the wallet. (furszy)

Pull request description:

  Cold staking delegation, popup alerting user if the owner address is not from the wallet.

  Preview.
  <img width="633" alt="Screen Shot 2019-11-20 at 10 59 46 AM" src="https://user-images.githubusercontent.com/5377650/69245395-80579200-0b85-11ea-9209-b981492b8f4f.png">

ACKs for top commit:
  Mrs-X:
    I'm fine with the final text, so utACK 6daa383
  random-zebra:
    utACK 6daa383

Tree-SHA512: c5095790aff0590edae9f4606263fb5be2ff886e46f0709289a0b44bb79b6b2fb218fa1b3a4ae2ebc5178dfc6e2694ccaaf615aa0bb763f5e22a7387ad054f37
@random-zebra random-zebra merged commit 6daa383 into PIVX-Project:master Nov 28, 2019
Fuzzbawls pushed a commit that referenced this pull request Dec 14, 2019
@furszy @Fuzzbawls
[GUI] Cold staking, alert user if the owner address is not from the w…
33c7084 
…allet.

Github-Pull: #1121
Rebased-From: 6daa383
@Fuzzbawls Fuzzbawls mentioned this pull request Dec 14, 2019
Merged
random-zebra added a commit that referenced this pull request Dec 15, 2019
@random-zebra
Merge #1196: [Backport] 4.0.0 backports
e5f3fe9 
fa1987b [GUI] ColdStaking widget: fix "total staking" amount (random-zebra)
7ad5385 Update copyright headers (Fuzzbawls)
31433e2 [Docs] 4.0.0 release notes (Fuzzbawls)
8cfd8b5 [GUI] Border image workaround. (furszy)
82069c3 [Qt] Update translations from transifex (Fuzzbawls)
1894425 [GUI][Wallet] Allow spending of P2CS without coincontrol selection Github-Pull: #1193 Rebased-From: b8e4df8 (random-zebra)
102dfd5 [Consensus] Define MainNet changeover block for PIVX v4.0 Github-Pull: #1191 Rebased-From: 8f7dd00 (random-zebra)
9b9dc62 [GUI][BUG] Accept P2CS locked coins in coincontrol (random-zebra)
9d9909b [GUI] Use QRegexValidator instead of the QDoubleValidator to validate QEditLine amounts. (furszy)
ed0a93a [GUI] Feature/Bug CoinControl Update on Open (Liquid369)
6009c24 [GUI] Validate wallet password on enter key press (warrows)
3caf895 [Network] Add SPORK 17 and 18 to the fMissingSporks flag + move the validation below the old protocol and the connected to ourself check. No need to send the message if we are going to end up disconnecting from the peer. (furszy)
a7205ad [GUI] set focus when showing dialogs (random-zebra)
5f0d3e6 [GUI][Trivial] Make amount optional in staking address gen dialog Github-Pull: #1185 Rebased-From: a2d67d6 (random-zebra)
630e040 [GUI][Trivial] Fix tx detail dialog expanding policy Github-Pull: #1187 Rebased-From: 7e5253d (random-zebra)
96f240b [GUI][Trivial] move caps lock warning in askpassphrase dialog Github-Pull: #1186 Rebased-From: 5bd5bbc (random-zebra)
08ee144 [GUI][Bug] cold staking screen, fixing show receive addresses list in owner address editLine. (furszy)
c71bc13 [GUI] Remove every pushButton focus property. (furszy)
01c95c6 [Doc] Remove stale image files (Fuzzbawls)
3cafa3a [Doc] Update gitian building docs (Fuzzbawls)
90d27bd [Utils] Update gitian-build.py for multi-os compatibility (Fuzzbawls)
ddbceb4 Dead link to wrong PIVX website (NoobieDev12)
c99d712 [Build] Add random-zebra gitian GPG public key fingerprint Github-Pull: #1180 Rebased-From: a88bff8 (random-zebra)
6cae0c9 [Trivial] fix minColdStakingAmount comment and type Github-Pull: #1174 Rebased-From: 35a5065 (random-zebra)
a7f6d17 [GUI][Trivial] access Params() only from walletModel Github-Pull: #1174 Rebased-From: 4ad9492 (random-zebra)
d41f9c6 [Cleanup] Remove extra calls to getDisplayUnit in ColdStakingWidget Github-Pull: #1174 Rebased-From: 5c3162b (random-zebra)
5c1a1a9 [GUI] Cold Staking Widget: set minColdStakingAmount from chain params Github-Pull: #1174 Rebased-From: 2369644 (random-zebra)
c8e1762 [GUI] Removing cold staking "unconfirmed balance will not be shown" warning label. (furszy)
3a302aa [Wallet] GetAvailableP2CSCoins permitting unconfirmed tx. (furszy)
53901c0 [GUI] Cold staking, always update staking total amount (furszy)
9527147 [GUI][Bug] Cold staking screen, delegations list, total amount was not being updated. This solves the issue + re organize the UI form to present it aligned to the texts in the left. (furszy)
c3d8f60 [Qt] properly copy IPv6 externalip MN info (Fuzzbawls)
b1641f4 [Trivial] Clean time offset warning when it gets back within range Github-Pull: #1167 Rebased-From: c24d2b1 (random-zebra)
c3ec18b [Qt] Periodic make translate (Fuzzbawls)
02b2a04 [GUI][Model] Do not request password when the wallet is unlocked to lock it for staking only (furszy)
3971d91 [GUI] Custom change address, editable box getting focus when the dialog is launched. (furszy)
6565097 [GUI] Masternodes wizard, IPv6 correctly stored between brackets. (furszy)
6cb9217 [GUI] tooltip css changes. (furszy)
4dff02c [GUI] Decreasing the tooltip padding for #1076 windows issue. (furszy)
0e2dd1d Rewording text under Change Wallet Passphrase Github-Pull: #1151 Rebased-From: 95bba7a (NoobieDev12)
38d3218 [GUI][Trivial] Rewording of Error message while trying to send with the transaction with wallet unlocked for staking only (NoobieDev12)
107a452 [Travis] Increase functional tests reserved time (Warrows)
7d94126 [Bug] Coded properly the URI from file read.. (furszy)
32a32f3 [GUI] Regtest corrections. (furszy)
b4d5b57 [Bug] Fix segfault on GUI initialization for cold staker wallet (random-zebra)
33c7084 [GUI] Cold staking, alert user if the owner address is not from the wallet. (furszy)
de5cf4a [Deployment] Windows taskbar icon pixelated. (furszy)
5dbb514 [GUI] Inform if open pivx conf and open backups folder failed due some OS incapacity/restriction. Plus minor cleanup over the touched files. (furszy)
ffd7dce [Qt] Fix segfault when running GUI client with --help or -? (Fuzzbawls)
3d39ce6 Revert "[Params] TESTNET ONLY, do not merge in master." (Fuzzbawls)

Pull request description:

  This updates the `4.0` branch with the PRs that have been merged since the branch-off point. Additionally, it reverts the locking to testnet (2e85666).

  List of included PRs:
  #1139
  #1125
  #1127
  #1121
  #1142
  #1141
  #1131
  #1157
  #1159
  #1151
  #1156
  #1158
  #1161
  #1160
  #1162
  #1167
  #1166
  #1165
  #1174
  #1180
  #1179
  #1176
  #1173
  #1178
  #1186
  #1187
  #1185
  #1183
  #1177
  #1163
  #1190
  #1124
  #1192
  #1191
  #1193
  #1195
  #1188
  #1181
  #1197
  #1194

ACKs for top commit:
  Warrows:
    utACK fa1987b, diff looks good.
  random-zebra:
    utACK fa1987b and merging...

Tree-SHA512: f65d04e5764fda9a9899276bb0de8d2b8b6e8152d471d32a8fccfd7f79acdd526d4f9b03d848f07c54defe0173448c01b573e39e26164281cfd05c66c9e9af5a
@furszy furszy deleted the 2019_cs_owner_address_alert branch November 29, 2022 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@random-zebra random-zebra random-zebra approved these changes

@Mrs-X Mrs-X Mrs-X approved these changes

Assignees

@furszy furszy

Labels
GUI
Projects
None yet
Milestone
4.0.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@furszy @Fuzzbawls @random-zebra @Mrs-X