Implement Addresses list and Sign/Verify in Wallet Settings#561
Implement Addresses list and Sign/Verify in Wallet Settings#561pseudoramdom wants to merge 3 commits into
Conversation
johnny9
left a comment
There was a problem hiding this comment.
Can we try to reuse the toggle buttons in the NetworkTraffic page?
Good callout. |
|
Rebased on top of |
b9242e7 to
0418ffd
Compare
There was a problem hiding this comment.
Found a few things worth tackling in the context of this PR.
- Payment request flow shortcut requires some adjustment to the RequestPayment page logic.
- New address creation failures should be handled
- Addresses in the list can go stale, watch for changes.
- Segment picker layout needs adjustment
0418ffd to
98579c7
Compare
98579c7 to
414856d
Compare
|
Picked changes from #563 and rebased. Addressed the feedback as well. |
|
double check the functional tests. |
| #include <qml/models/coinslistmodel.h> | ||
| #include <qml/models/paymentrequest.h> | ||
| #include <qml/models/sendrecipientslistmodel.h> | ||
| #include <qml/models/signverifymessagemodel.h> |
There was a problem hiding this comment.
this is in the first commit, but it doesn't seem to be added until a later commit, so the first commit doesn't compile on its own
Not exactly a blocker for me, but worthwhile to fix
There was a problem hiding this comment.
The functional test regression came from c31ba7d which rewired EllipsisMenuToggleItem.qml. The likely fix is to call toggle() on the right object. It seems we now have a bunch of special cases for "clicked" so we should have the test bridge actually inject mouse pressed/release on the center of the object (x, y + width,height/2)
| return false; | ||
| } | ||
|
|
||
| const SecureString secure_passphrase{passphrase->toStdString()}; |
There was a problem hiding this comment.
we should stick to using our SecureStringFromQString function instead of passphrase->toStdString.
To avoid confusion in the future, may be worthwhile to make it a helper function and put it to use like so:
Details
diff --git a/qml/models/signverifymessagemodel.cpp b/qml/models/signverifymessagemodel.cpp
index 7470b00a0..226d03f00 100644
--- a/qml/models/signverifymessagemodel.cpp
+++ b/qml/models/signverifymessagemodel.cpp
@@ -7,7 +7,7 @@
#include <common/signmessage.h>
#include <key_io.h>
#include <qml/models/walletrelockguard.h>
-#include <support/allocators/secure.h>
+#include <qml/util.h>
namespace {
std::optional<PKHash> LegacyP2PKHFromAddress(const QString& address)
@@ -124,8 +124,10 @@ bool SignVerifyMessageModel::unlockForSigning(const std::optional<QString>& pass
return false;
}
- const SecureString secure_passphrase{passphrase->toStdString()};
- if (!m_wallet->unlock(secure_passphrase)) {
+ SecureString secure_passphrase{QmlUtil::SecureStringFromQString(*passphrase)};
+ const bool unlocked{m_wallet->unlock(secure_passphrase)};
+ QmlUtil::ClearSecureString(secure_passphrase);
+ if (!unlocked) {
setSigningStatus(tr("The wallet password you entered was incorrect."));
return false;
}
diff --git a/qml/models/walletqmlmodel.cpp b/qml/models/walletqmlmodel.cpp
index 56510f069..7a9904aa8 100644
--- a/qml/models/walletqmlmodel.cpp
+++ b/qml/models/walletqmlmodel.cpp
@@ -15,6 +15,7 @@
#include <qml/models/signverifymessagemodel.h>
#include <qml/models/walletrelockguard.h>
#include <qml/models/walletqmlmodeltransaction.h>
+#include <qml/util.h>
#include <chainparams.h>
#include <consensus/amount.h>
@@ -33,7 +34,6 @@
#include <wallet/coincontrol.h>
#include <wallet/wallet.h>
-#include <QByteArray>
#include <QDateTime>
#include <QMetaObject>
#include <QRegularExpression>
@@ -263,26 +263,6 @@ QString LocalizedString(const bilingual_str& value)
return QString::fromStdString(value.translated.empty() ? value.original : value.translated);
}
-SecureString SecureStringFromQString(const QString& value)
-{
- QByteArray bytes{value.toUtf8()};
- SecureString secure;
- secure.assign(bytes.constData(), bytes.constData() + bytes.size());
- if (!bytes.isEmpty()) {
- memory_cleanse(bytes.data(), static_cast<size_t>(bytes.size()));
- }
- return secure;
-}
-
-void ClearSecureString(SecureString& value)
-{
- if (value.empty()) {
- return;
- }
- memory_cleanse(value.data(), value.size());
- value.clear();
-}
-
QString OutputTypeId(OutputType type)
{
return QString::fromStdString(FormatOutputType(type));
@@ -514,9 +494,9 @@ bool WalletQmlModel::encryptWallet(const QString& passphrase)
return false;
}
- SecureString secure_passphrase{SecureStringFromQString(passphrase)};
+ SecureString secure_passphrase{QmlUtil::SecureStringFromQString(passphrase)};
const bool encrypted{m_wallet->encryptWallet(secure_passphrase)};
- ClearSecureString(secure_passphrase);
+ QmlUtil::ClearSecureString(secure_passphrase);
if (!encrypted) {
setSettingsError(tr("The wallet password could not be set."));
return false;
@@ -542,11 +522,11 @@ bool WalletQmlModel::changeWalletPassphrase(const QString& old_passphrase, const
return false;
}
- SecureString secure_old_passphrase{SecureStringFromQString(old_passphrase)};
- SecureString secure_new_passphrase{SecureStringFromQString(new_passphrase)};
+ SecureString secure_old_passphrase{QmlUtil::SecureStringFromQString(old_passphrase)};
+ SecureString secure_new_passphrase{QmlUtil::SecureStringFromQString(new_passphrase)};
const bool changed{m_wallet->changeWalletPassphrase(secure_old_passphrase, secure_new_passphrase)};
- ClearSecureString(secure_old_passphrase);
- ClearSecureString(secure_new_passphrase);
+ QmlUtil::ClearSecureString(secure_old_passphrase);
+ QmlUtil::ClearSecureString(secure_new_passphrase);
if (!changed) {
setSettingsError(tr("The current wallet password was incorrect."));
return false;
@@ -1125,7 +1105,7 @@ bool WalletQmlModel::prepareTransaction()
bool WalletQmlModel::prepareTransactionWithPassphrase(const QString& passphrase)
{
- return prepareTransactionInternal(std::optional<SecureString>{SecureStringFromQString(passphrase)});
+ return prepareTransactionInternal(std::optional<SecureString>{QmlUtil::SecureStringFromQString(passphrase)});
}
bool WalletQmlModel::prepareTransactionInternal(std::optional<SecureString> passphrase)
@@ -1133,7 +1113,7 @@ bool WalletQmlModel::prepareTransactionInternal(std::optional<SecureString> pass
clearTransactionStatus();
if (!m_wallet || !m_send_recipients || m_send_recipients->recipients().empty()) {
if (passphrase.has_value()) {
- ClearSecureString(*passphrase);
+ QmlUtil::ClearSecureString(*passphrase);
passphrase.reset();
}
setTransactionStatus(tr("Enter at least one valid recipient to continue."));
@@ -1499,7 +1479,7 @@ bool WalletQmlModel::unlockForAction(std::optional<SecureString>& passphrase, bo
relock = false;
if (!m_wallet || !m_wallet->isCrypted() || !m_wallet->isLocked()) {
if (passphrase.has_value()) {
- ClearSecureString(*passphrase);
+ QmlUtil::ClearSecureString(*passphrase);
passphrase.reset();
}
return true;
@@ -1509,7 +1489,7 @@ bool WalletQmlModel::unlockForAction(std::optional<SecureString>& passphrase, bo
}
const bool unlocked = m_wallet->unlock(*passphrase);
- ClearSecureString(*passphrase);
+ QmlUtil::ClearSecureString(*passphrase);
passphrase.reset();
if (!unlocked) {
setTransactionStatus(tr("The wallet password you entered was incorrect."));
diff --git a/qml/util.cpp b/qml/util.cpp
index c2532f02e..065491a57 100644
--- a/qml/util.cpp
+++ b/qml/util.cpp
@@ -4,9 +4,13 @@
#include <qml/util.h>
+#include <support/cleanse.h>
+
#include <cassert>
+#include <cstddef>
#include <string>
+#include <QByteArray>
#include <QQuickWindow>
#include <QSGRendererInterface>
#include <QString>
@@ -33,4 +37,24 @@ QString GraphicsApi(QQuickWindow* window)
assert(false);
}
+SecureString SecureStringFromQString(const QString& value)
+{
+ QByteArray bytes{value.toUtf8()};
+ SecureString secure;
+ secure.assign(bytes.constData(), bytes.constData() + bytes.size());
+ if (!bytes.isEmpty()) {
+ memory_cleanse(bytes.data(), static_cast<std::size_t>(bytes.size()));
+ }
+ return secure;
+}
+
+void ClearSecureString(SecureString& value)
+{
+ if (value.empty()) {
+ return;
+ }
+ memory_cleanse(value.data(), value.size());
+ value.clear();
+}
+
} // namespace QmlUtil
diff --git a/qml/util.h b/qml/util.h
index 6df5d2c2b..32930a5eb 100644
--- a/qml/util.h
+++ b/qml/util.h
@@ -5,6 +5,8 @@
#ifndef BITCOIN_QML_UTIL_H
#define BITCOIN_QML_UTIL_H
+#include <support/allocators/secure.h>
+
#include <QString>
#include <QtGlobal>
@@ -22,6 +24,9 @@ namespace QmlUtil {
*/
QString GraphicsApi(QQuickWindow* window);
+SecureString SecureStringFromQString(const QString& value);
+void ClearSecureString(SecureString& value);
+
} // namespace QmlUtil
#endif // BITCOIN_QML_UTIL_H
diff --git a/qml/walletqmlcontroller.cpp b/qml/walletqmlcontroller.cpp
index a16cd57bf..50c5af3f4 100644
--- a/qml/walletqmlcontroller.cpp
+++ b/qml/walletqmlcontroller.cpp
@@ -5,6 +5,7 @@
#include <qml/walletqmlcontroller.h>
#include <qml/models/walletqmlmodel.h>
+#include <qml/util.h>
#include <common/args.h>
#include <common/settings.h>
@@ -21,7 +22,6 @@
#include <stdexcept>
#include <utility>
-#include <QByteArray>
#include <QDir>
#include <QFileInfo>
#include <QMetaObject>
@@ -43,26 +43,6 @@ QString JoinWarnings(const std::vector<bilingual_str>& warnings)
return lines.join('\n');
}
-SecureString SecureStringFromQString(const QString& value)
-{
- QByteArray bytes{value.toUtf8()};
- SecureString secure;
- secure.assign(bytes.constData(), bytes.constData() + bytes.size());
- if (!bytes.isEmpty()) {
- memory_cleanse(bytes.data(), static_cast<size_t>(bytes.size()));
- }
- return secure;
-}
-
-void ClearSecureString(SecureString& value)
-{
- if (value.empty()) {
- return;
- }
- memory_cleanse(value.data(), value.size());
- value.clear();
-}
-
bool IsBackupLikeFile(const QFileInfo& file_info)
{
const QString file_name = file_info.fileName().toLower();
@@ -312,10 +292,10 @@ bool WalletQmlController::createSingleSigWallet(const QString &name, const QStri
setWalletCreateError(tr("Wallets are still loading. Try again in a moment."));
return false;
}
- SecureString secure_passphrase{SecureStringFromQString(passphrase)};
+ SecureString secure_passphrase{QmlUtil::SecureStringFromQString(passphrase)};
const std::string wallet_name{name.toStdString()};
auto wallet{m_node.walletLoader().createWallet(wallet_name, secure_passphrase, wallet::WALLET_FLAG_DESCRIPTORS, m_warning_messages)};
- ClearSecureString(secure_passphrase);
+ QmlUtil::ClearSecureString(secure_passphrase);
setWalletLoadWarnings(JoinWarnings(m_warning_messages));
if (wallet) {
const QString loaded_wallet_name = QString::fromStdString((*wallet)->getWalletName());
@@ -436,7 +416,7 @@ void WalletQmlController::migrateWallet(const QString& path, const QString& pass
setWalletMigrationError(tr("Wallets are still loading. Try again in a moment."));
return;
}
- startWalletMigration(path, SecureStringFromQString(passphrase));
+ startWalletMigration(path, QmlUtil::SecureStringFromQString(passphrase));
}
void WalletQmlController::clearWalletMigrationStatus()
@@ -937,7 +917,7 @@ void WalletQmlController::startWalletMigration(const QString& path, SecureString
clearWalletMigrationStatus();
if (path.trimmed().isEmpty()) {
- ClearSecureString(passphrase);
+ QmlUtil::ClearSecureString(passphrase);
setWalletMigrationError(tr("Choose a wallet to update."));
Q_EMIT walletMigrationFailed();
return;
@@ -945,7 +925,7 @@ void WalletQmlController::startWalletMigration(const QString& path, SecureString
const QString wallet_reference = resolveManagedWalletReference(path);
if (wallet_reference.isEmpty()) {
- ClearSecureString(passphrase);
+ QmlUtil::ClearSecureString(passphrase);
setWalletMigrationError(tr("The selected wallet is not available in the wallet directory."));
Q_EMIT walletMigrationFailed();
return;
@@ -960,7 +940,7 @@ void WalletQmlController::startWalletMigration(const QString& path, SecureString
QTimer::singleShot(0, m_worker, [this, wallet_reference, passphrase = std::move(passphrase)]() mutable {
auto result = m_node.walletLoader().migrateWallet(wallet_reference.toStdString(), passphrase);
- ClearSecureString(passphrase);
+ QmlUtil::ClearSecureString(passphrase);
if (!result) {
const QString error = QString::fromStdString(util::ErrorString(result).translated);| if (!destination) { | ||
| return; | ||
| if (!destination || !IsValidDestination(destination.value())) { | ||
| return false; |
There was a problem hiding this comment.
We should have retry logic like the qt widgets gui does, and here is some more context as to why: bitcoin/bitcoin#2904
There was a problem hiding this comment.
can check if isLocked first
There was a problem hiding this comment.
Ah interesting! Just today I learned why we might need to ask for passphrase if we need to replenish the key pool. I'll look in to this tomorrow.


Add support to view receive and change addresses. (Fixes #519)
addresslistmodelWalletQmlModelwhich provides helper methods related to addresses.SegmentedPicker.qmlfor segmented picker with multiple options.Implement Sign/verify message page in Wallet Settings. Fixes #524
Add SignVerifyMessageModel to coordinate signing/verifying and will be owned
by WalletQmlModel.
This change was dependent on generating a legacy P2PKH addresses which
was missing in the receive flow. Implemented a picker to choose address types.
Implement functional, unit tests.
Tests
Screenshots
Address list
Sign / Verify message