Stop giving GUI access to destdata rows in database. Replace with narrow API just for saving and reading receive request information.
This simplifies code and should prevent the GUI from interfering with other destdata like address-used status. It also adds some more GUI test coverage.
There are no changes in behavior.
Make sure wallet receive requests are saved and deleted correctly by GUI
code
Co-authored-by: Jarol Rodriguez <jarolrod@tutanota.com>
Stop giving GUI access to destdata rows in database. Replace with narrow
API just for saving and reading receive request information.
This simplifies code and should prevent the GUI from interfering with
other destdata like address-used status.
Note: No user-visible behavior is changing in this commit. New
CWallet::SetAddressReceiveRequest() implementation avoids a bug in
CWallet::AddDestData() where a modification would leave the previous
value in memory while writing the new value to disk. But it doesn't
matter because the GUI doesn't currently expose the ability to modify
receive requests, only to add and erase them.
This simplifies code and adds a less cumbersome interface for accessing
address used information than CWallet AddDestData / EraseDestData /
GetDestData methods.
There is no change in behavior. Lower-level walletdb DestData methods
are also still available and not affected by this change. If there is
interest in consolidating destdata logic more and making it internal to
walletdb, #18608 could be considered as a followup.
ACK 53291c71256d7d15347d2ea46c3cd2692b6ef138, tested on macOS 11.2 Qt 5.15.2
Tested that the functionality remained the same between master and this PR. Specifically with populating the recentrequeststablemodel and actions like creating and removing a request.
This is a nice refactor and yay, more GUI test coverage!
3811 | @@ -3811,6 +3812,18 @@ std::vector<std::string> CWallet::GetDestValues(const std::string& prefix) const 3812 | return values; 3813 | } 3814 | 3815 | +bool CWallet::SetAddressReceiveRequest(WalletBatch& batch, const CTxDestination& dest, const std::string& id, const std::string& value) 3816 | +{ 3817 | + const std::string key{"rr" + id}; // "rr" prefix = "receive request" in destdata 3818 | + CAddressBookData& data = m_address_book.at(dest); 3819 | + if (value.empty()) { 3820 | + return data.destdata.erase(key) && batch.EraseDestData(EncodeDestination(dest), key);
dd8a9d019aa449f3a6c5ad989cb6bdd9f03d1306
Should erase from destdata iif EraseDestData succeeds?
re: #21353 (review)
Should erase from
destdataiifEraseDestDatasucceeds?
Good suggestion! No need to keep previous behavior that makes a missing key look like a failed write.
3817 | + const std::string key{"rr" + id}; // "rr" prefix = "receive request" in destdata 3818 | + CAddressBookData& data = m_address_book.at(dest); 3819 | + if (value.empty()) { 3820 | + return data.destdata.erase(key) && batch.EraseDestData(EncodeDestination(dest), key); 3821 | + } else { 3822 | + data.destdata[key] = value;
dd8a9d019aa449f3a6c5ad989cb6bdd9f03d1306
Should update destdata iff WriteDestData succeeds?
Updated 53291c71256d7d15347d2ea46c3cd2692b6ef138 -> 758cbfa37e8e1805d7d53ef203d91db3733248e9 (pr/nodestg.2 -> pr/nodestg.3, compare) with review suggestions
224 | @@ -225,6 +225,7 @@ void TestGUI(interfaces::Node& node) 225 | int initialRowCount = requestTableModel->rowCount({}); 226 | QPushButton* requestPaymentButton = receiveCoinsDialog.findChild<QPushButton*>("receiveButton"); 227 | requestPaymentButton->click(); 228 | + std::string address;
Any reason to use std::string here? If we just use QString we could avoid conversions on lines 238 and 274
diff --git a/src/qt/test/wallettests.cpp b/src/qt/test/wallettests.cpp
index 3acf9d6b2..708c3cc92 100644
--- a/src/qt/test/wallettests.cpp
+++ b/src/qt/test/wallettests.cpp
@@ -225,7 +225,7 @@ void TestGUI(interfaces::Node& node)
int initialRowCount = requestTableModel->rowCount({});
QPushButton* requestPaymentButton = receiveCoinsDialog.findChild<QPushButton*>("receiveButton");
requestPaymentButton->click();
- std::string address;
+ QString address;
for (QWidget* widget : QApplication::topLevelWidgets()) {
if (widget->inherits("ReceiveRequestDialog")) {
ReceiveRequestDialog* receiveRequestDialog = qobject_cast<ReceiveRequestDialog*>(widget);
@@ -234,9 +234,9 @@ void TestGUI(interfaces::Node& node)
QString uri = receiveRequestDialog->QObject::findChild<QLabel*>("uri_content")->text();
QCOMPARE(uri.count("bitcoin:"), 2);
QCOMPARE(receiveRequestDialog->QObject::findChild<QLabel*>("address_tag")->text(), QString("Address:"));
- QVERIFY(address.empty());
- address = receiveRequestDialog->QObject::findChild<QLabel*>("address_content")->text().toStdString();
- QVERIFY(!address.empty());
+ QVERIFY(address.isEmpty());
+ address = receiveRequestDialog->QObject::findChild<QLabel*>("address_content")->text();
+ QVERIFY(!address.isEmpty());
QCOMPARE(uri.count("amount=0.00000001"), 2);
QCOMPARE(receiveRequestDialog->QObject::findChild<QLabel*>("amount_tag")->text(), QString("Amount:"));
@@ -271,7 +271,7 @@ void TestGUI(interfaces::Node& node)
QCOMPARE(entry.nVersion, int{1});
QCOMPARE(entry.id, int64_t{1});
QVERIFY(entry.date.isValid());
- QCOMPARE(entry.recipient.address, QString::fromStdString(address));
+ QCOMPARE(entry.recipient.address, address);
QCOMPARE(entry.recipient.label, QString{"TEST_LABEL_1"});
QCOMPARE(entry.recipient.amount, CAmount{1});
QCOMPARE(entry.recipient.message, QString{"TEST_MESSAGE_1"});
Thanks! Applied simplification
Updated 758cbfa37e8e1805d7d53ef203d91db3733248e9 -> f5ba424cd44619d9b9be88b8593d69a7ba96db26 (pr/nodestg.3 -> pr/nodestg.4, compare) with suggestion
tACK f5ba424cd44619d9b9be88b8593d69a7ba96db26
Code review ACK f5ba424cd44619d9b9be88b8593d69a7ba96db26
3770 | + if (!used) { 3771 | + if (auto* data = util::FindKey(m_address_book, dest)) data->destdata.erase(key); 3772 | + return batch.EraseDestData(EncodeDestination(dest), key); 3773 | + } 3774 | + 3775 | + const std::string value{"1"};
In commit "wallet: Add IsAddressUsed / SetAddressUsed methods" (f5ba424cd44619d9b9be88b8593d69a7ba96db26)
Note: behavior accidentally changed here. Used addresses previously were written with "p" values, now they are written with "1" values. The change should not be significant because the values are currently ignored, but future code should continue to treat "1" and "p" the same