[Qt] prefer CRecipient over std::pair like in walletmodel #6205

pull Diapolo wants to merge 1 commits into bitcoin:master from Diapolo:Qt_CRecipient changing 8 files +30 −23

Diapolo commented at 1:31 PM on May 31, 2015: none
- this updates Qt code to core code, where CRecipient is used in wallet
- also makes things e.g. in paymentserver.cpp much more readable
Diapolo renamed this:
~~[Qt] prefer CReceipient over std::pair like in walletmodel~~
[Qt] prefer CRecipient over std::pair like in walletmodel
on May 31, 2015
Diapolo commented at 7:31 AM on June 1, 2015: none

@theuni What does this mean, can you help me? error: source directory already configured; run "make distclean" there first
luke-jr commented at 4:33 AM on June 2, 2015: member

"code code"? (commit message)
Diapolo commented at 7:47 AM on June 2, 2015: none

@luke-jr Strange... updated the commit message :). Thanks for catching.
fanquake commented at 2:33 PM on June 3, 2015: member

@laanwj Do you want to restart the build for this pull?

laanwj commented at 5:34 PM on June 3, 2015: member

There's some include file problem in the disable-wallet build:

In file included from ./wallet/walletdb.h:10:0,
from ./wallet/wallet.h:20,
from qt/paymentrequestplus.h:11,
from qt/walletmodel.h:8,
from qt/guiutil.cpp:10:
./wallet/db.h:21:20: fatal error: db_cxx.h: No such file or directory
compilation terminated.

Diapolo commented at 8:50 AM on June 6, 2015: none

@laanwj The problem I see is that CRecipient is defined in wallet.h and I couldn't get the walletless build to not error out yet -_- (there are quite some fancy dependencies). I'm not convinced CRecipient fits best in wallet.h anyway.
laanwj commented at 9:03 AM on June 6, 2015: member

I think it's the proper place- why would you need a recipient type at all when not concerned with the wallet?
sipa commented at 9:07 AM on June 6, 2015: member

The createrawtransaction RPC could use something like CRecipient, even without wallet support.
laanwj commented at 9:20 AM on June 6, 2015: member

I guess so, but that'd be completely internal to that call - which is already very simple, and a pure utility method at that. So you'd have two unconnected parts of the source code somehow using the same data type. No, I don't really see that as useful code change. We already have CTxOut. Other recipient metadata is likely application-specific.

e.g. see fSubtractFeeFromAmount in CWallet's recipient. The type is meant to be a wallet recipient.

[Qt] prefer CRecipient over std::pair like in walletmodel

- this updates Qt code to core code, where CRecipient is used in wallet
- also makes things e.g. in paymentserver.cpp much more readable

6f1d01eb15

Diapolo commented at 10:42 AM on June 6, 2015: none

Nice, seems I got it :), woohooo... all builds are passing.
laanwj added the label Wallet on Jun 6, 2015
laanwj commented at 8:12 AM on June 10, 2015: member

I've looked over this and I don't really think it's an improvement.

As said in my previous post, the CRecipient type is very much bound to our specific current wallet implementation. There's already at least one field in it that is irrelevant to payment requests. Also there is something to be said for keeping the payment server code somewhat more generic (e.g. future library).

If the goal is just to neaten the code a bit, I'd suggest to define a local recipient type in paymentrequestplus.h, eg a typedef of std::pair<CScript,CAmount>.
laanwj closed this on Jun 12, 2015
in src/qt/paymentserver.cpp:None in 6f1d01eb15
```
 588 |              return false;
 589 |          }
 590 |  
 591 |          // Extract and check amounts
 592 | -        CTxOut txOut(sendingTo.second, sendingTo.first);
 593 | +        CTxOut txOut(sendingTo.nAmount, sendingTo.scriptPubKey);
```
Diapolo commented at 9:56 AM on June 12, 2015:

@laanwj Well I think in terms of understanding the code this was an improvement.

Diapolo commented at 5:54 AM on June 15, 2015:

But not that much, so you would ACK it, right?

laanwj commented at 10:59 AM on June 15, 2015:

As said, I'm fine with defining a local recipient type here and using that to clarify the code. I just disagree with using the wallet's recipient type.
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels