interfaces: Stop exposing wallet destdata to gui #21353
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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, bitcoin#18608 could be considered as a followup.
jarolrod
reviewed
Mar 4, 2021
There was a problem hiding this comment.
ACK 53291c7, 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!
promag
reviewed
Mar 6, 2021
ryanofsky
commented
Mar 9, 2021
There was a problem hiding this comment.
Updated 53291c7 -> 758cbfa (pr/nodestg.2 -> pr/nodestg.3, compare) with review suggestions
jarolrod
reviewed
May 18, 2021
src/qt/test/wallettests.cpp
Outdated
| @@ -225,6 +225,7 @@ void TestGUI(interfaces::Node& node) | |||
| int initialRowCount = requestTableModel->rowCount({}); | |||
| QPushButton* requestPaymentButton = receiveCoinsDialog.findChild<QPushButton*>("receiveButton"); | |||
| requestPaymentButton->click(); | |||
| std::string address; | |||
There was a problem hiding this comment.
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"});There was a problem hiding this comment.
Thanks! Applied simplification
ryanofsky
commented
May 19, 2021
There was a problem hiding this comment.
Updated 758cbfa -> f5ba424 (pr/nodestg.3 -> pr/nodestg.4, compare) with suggestion
src/qt/test/wallettests.cpp
Outdated
| @@ -225,6 +225,7 @@ void TestGUI(interfaces::Node& node) | |||
| int initialRowCount = requestTableModel->rowCount({}); | |||
| QPushButton* requestPaymentButton = receiveCoinsDialog.findChild<QPushButton*>("receiveButton"); | |||
| requestPaymentButton->click(); | |||
| std::string address; | |||
There was a problem hiding this comment.
Thanks! Applied simplification
|
tACK f5ba424 |
|
Code review ACK f5ba424 |
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Jun 3, 2021
f5ba424 wallet: Add IsAddressUsed / SetAddressUsed methods (Russell Yanofsky) 62252c9 interfaces: Stop exposing wallet destdata to gui (Russell Yanofsky) 985430d test: Add gui test for wallet receive requests (Russell Yanofsky) Pull request description: 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. ACKs for top commit: jarolrod: tACK f5ba424 laanwj: Code review ACK f5ba424 Tree-SHA512: 5423df4786e537a59013cb5bfb9e1bc29a7ca4b8835360c00cc2165a59f925fdc355907a4ceb8bca0285bb4946ba235bffa7645537a951ad03fd3b4cee17b6b0
gwillen
pushed a commit
to ElementsProject/elements
that referenced
this pull request
Jun 1, 2022
ryanofsky
commented
Apr 7, 2023
| return batch.EraseDestData(EncodeDestination(dest), key); | ||
| } | ||
|
|
||
| const std::string value{"1"}; |
There was a problem hiding this comment.
In commit "wallet: Add IsAddressUsed / SetAddressUsed methods" (f5ba424)
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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.