[Qt] Permanently store requested payments in wallet #3521

cozz commented at 4:39 AM on January 13, 2014: contributor

see title.

Did some quick testing, seems to work...

laanwj commented at 7:07 AM on January 13, 2014: member

Thanks for having a shot at this.

I agree with the UI part. However the core part should be more generic. The core doesn't need to know about UI-only data structures such as CReceiveRequestEntry.

My idea was to offer a interface for storing general-purpose data on destinations in the wallet. See #3281 and store payment requests and bitcoin URIs in there.

However, it is nearly impossible to get changes to the core wallet code reviewed (and you need very good tests/testplan), and I had tons of other things to do, this needs to be re-thought post-0.9.

cozz commented at 1:54 PM on January 13, 2014: contributor

How about using Boost.Serialization, serializing the SendCoinsRecipient as xml/string and storing this in the wallet. Could this work / Is a good idea?

laanwj commented at 2:03 PM on January 13, 2014: member

I'd rather not import another serialization format.

We can use the bitcoin serialization format (used for most stuff in the wallet) or google protobuf (as used for payment requests).

The important part is that the serialization happens on the src/qt side, and not in the core, as to not burden the core with knowledge it doesn't need to know about.

cozz commented at 2:28 PM on January 13, 2014: contributor

Ah yes, having 3 serialization formats makes no sense.

So even if I implement this in on top of #3281 , there is no chance of getting "storing the requests permanently" into 0.9?

laanwj commented at 2:40 PM on January 13, 2014: member

It's most likely if you keep the core changes as small and non-invasive as possible. Basing on #3281 is a good idea I think as it only adds a minimal amount of code.

I can't guarantee anything though. I'll just merge GUI changes, but core changes need more extensive scrutiny.

cozz commented at 2:51 PM on January 13, 2014: contributor

Ok thanks, I will take a look at this. Though I cant promise anything at this point.

So close for now and maybe reopen on top of #3281...

cozz closed this on Jan 13, 2014

laanwj commented at 3:36 PM on January 13, 2014: member

Ok, in any case it'd be very welcome!

laanwj referenced this in commit 66a8829a33 on Jan 13, 2014

cozz reopened this on Jan 14, 2014

cozz commented at 4:43 AM on January 14, 2014: contributor

Now based on #3281

When loading from wallet the sorting of the table is different than before. But I would want to add sorting in general to the table. So lets leave this sorting-problem for another pull.

laanwj commented at 11:36 AM on January 14, 2014: member

Implementation looks good, I plan to test this in the coming week.

laanwj commented at 11:38 AM on January 14, 2014: member

Locking: Don't forget that the DestData functions on CWallet (just like the other address book manipulation functions) require a lock on cs_wallet before calling, as they don't acquire the lock themselves.

cozz commented at 4:19 PM on January 14, 2014: contributor

Sounds good. Added the lock.

laanwj commented at 9:05 AM on January 17, 2014: member

Indeed, deleting multiple rows at once seems to be broken. It deletes a different subset than asked (only visible after a restart).

Apart from that it works great. Changing the ordering is not a big issue.

wallet: add interface for storing generic data on destinations b10e147096

[Qt] Permanently store requested payments in wallet 8476d5d407

[Qt] Add sorting feature to the requested payments table 4d901023b7

cozz commented at 5:32 PM on January 19, 2014: contributor

Yes, I did not realize you are supposed to delete multiple rows there... Now fixed. @emmix Many thanks for pointing this out.

I also added a third commit "Add sorting feature". Default is by date desc, fixing the ordering issue.

BitcoinPullTester commented at 5:41 PM on January 19, 2014: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/4d901023b732efb492d89cebd8555c689ab7663e for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.

cozz commented at 7:42 AM on January 20, 2014: contributor

I have some questions/suggestions. What do you think about them?

display (no amount) instead of 0.00, similar to (no label) (no message)
rename table headline "Requested payments" => "History"
Change description from "Use this form to request payments." to "Use this form to create bitcoin addresses and URIs" I dont like the term request payment. Someone may think you are actually requesting something from the network, but you are just creating a request.
Rename button "Request payment" to "Create Address/URI" or "Create payment information" (Could also say "generate" instead of create)
Move this button to the left aligned with the textfields, otherwise the mouse ways are long. Put Clear button right to this button or even remove Clear button, with only 3 fields there, the Clear button is not so important.
Write a sentence above the 2 fields amount and message: "Amount and message are useful when using bitcoin URIs (bitcoin:1abc...) They appear in someones client when clicking the URI" Otherwise people may be confused and think they always have to specify the amount now, to receive coins.
What is the intention with the message field? Shouldnt this appear in sendcoinsdialog, when I click an URI. Is this field even used anywhere currently? Should I implement this? If I understand this correctly the message is ignored when clicking on a simple URI.
Emphasize all fields being optional a little better: "All fields are optional!" or "All fields are OPTIONAL" or even write next to every label (optional)
Draw a border around the upper form elements.
How about instead of the checkbox "Reuse an existing address" make a combo box "Address:" default is "Generate new bitcoin address (recommended)" and then fill the combo with all addresses ordered by label, address? Combo box could be placed below "Label" above message and amount.

laanwj commented at 8:21 AM on January 20, 2014: member

ok
History may be too general, don't forget there is transaction history too

3/4) But eventually this form will be used to create payment requests (BIP 0070). It doesn't do that yet, which may be confusing, sure... Maybe a better word would be "invoice"?

Aligning the 'action' button to the right and the clear button to the left is consistent with the 'send' tab. Don't change this individually.
Fine with me
The message is a standard field that gets embedded in the bitcoin: URI. It is a message for the recipient of the "invoice". Our client doesn't use it currently, but the idea is that it gets saved with the transaction at the sender (it does not get sent over the network). Maybe hide/remove it for now?
Good idea. I like "All fields are OPTIONAL". Don't write it next to every label, I tried that and it's ugly (also tried using * and put the message below, but it looks strange if it applies to all fields).
Fine with me
The number of addresses in the wallet can be huge, please don't put them in a combo box. We want to go away from the idea of addresses being something you have easily graspable, limited quantity of. I'm not even sure we should have this option at all.

laanwj commented at 8:27 AM on January 20, 2014: member

Please make a new issue for those changes. This pull request should be ready for testing/merging right? No need to hold it up.

in src/qt/walletmodel.cpp:None in 4d901023b7

 564 | +                vReceiveRequests.push_back(item2.second);
 565 | +}
 566 | +
 567 | +bool WalletModel::saveReceiveRequest(const std::string &sAddress, const int64_t nId, const std::string &sRequest)
 568 | +{
 569 | +    CTxDestination dest = CBitcoinAddress(sAddress).Get();

Diapolo commented at 10:12 AM on January 20, 2014:

Any need to handle or any chance to see invalid addresses here?

laanwj commented at 11:48 AM on January 20, 2014:

I don't think so: a payment request is always generated with one of our own addresses, which are correct by definition.

Edit: though I suppose it wouldn't hurt to check to prevent storing information on CTXNoDestination

in src/qt/recentrequeststablemodel.h:None in 4d901023b7

  17 | +class RecentRequestEntry
  18 |  {
  19 | +public:
  20 | +    RecentRequestEntry() : nVersion(RecentRequestEntry::CURRENT_VERSION), id(0) { }
  21 | +
  22 | +    static const int CURRENT_VERSION=1;

Diapolo commented at 10:13 AM on January 20, 2014:

Minor personal nit: missing spaces

in src/qt/recentrequeststablemodel.h:None in 4d901023b7

  45 | +class RecentRequestEntryLessThan
  46 | +{
  47 | +public:
  48 | +    RecentRequestEntryLessThan(int nColumn, Qt::SortOrder fOrder):
  49 | +        column(nColumn), order(fOrder) {}
  50 | +    bool operator()(RecentRequestEntry &left, RecentRequestEntry &right ) const;

Diapolo commented at 10:14 AM on January 20, 2014:

Minor personal nit: space after right

cozz commented at 4:51 PM on January 20, 2014: contributor

@laanwj @Diapolo I do prevent storing to CNoDestination, but in wallet.cpp, CWallet::AddDestData. I hope this is ok. @laanwj Thanks for your feedback, I will open a new pull for the other changes. Yes this one is ready for testing. I did not follow any testplan, just randomly sorted, added, deleted rows, then restart to check, if everything gets restored correctly. Works for me so far. @Diapolo Unless there is any other update to this pull, I will put your nits in a future pull request.

laanwj referenced this in commit ceab53b41d on Jan 22, 2014

laanwj merged this on Jan 22, 2014

laanwj closed this on Jan 22, 2014

MathyV referenced this in commit 70157f542e on Aug 5, 2014

DrahtBot locked this on Sep 8, 2021