wallet, refactor: FundTransaction(): return out-params as util::Result structure
#26129
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
b2d3453 to
1cbbbcd
Compare
| int change_pos, | ||
| bool lockUnspents, | ||
| const std::set<int>& setSubtractFeeFromOutputs, | ||
| CCoinControl coinControl) |
There was a problem hiding this comment.
since we are here, coin control could be passed as a ref so we avoid copying it.
There was a problem hiding this comment.
Thanks, done. This is potentially more than just a refactor since the passed in coin control object from the caller is now modified, but as far as I can see this is no problem.
src/wallet/spend.h
Outdated
| struct FundedTransactionResult | ||
| { | ||
| CAmount fee; | ||
| int change_pos; | ||
|
|
||
| FundedTransactionResult(CAmount _fee, int _change_pos) | ||
| : fee(_fee), change_pos(_change_pos) {} | ||
| }; |
There was a problem hiding this comment.
In 038ed83:
Instead of creating a new struct for this (which duplicates part of what we have in the CreatedTransactionResult struct), I would make a base class for CreatedTransactionResult that only contains the fee, fee calc and change pos fields. Then make CreatedTransactionResult a child of it. And use the new base class directly here.
1cbbbcd to
de30125
Compare
|
Force-pushed with suggestions from @furszy (thanks for reviewing!) and also rebased on master. |
| @@ -180,15 +180,22 @@ std::optional<SelectionResult> SelectCoins(const CWallet& wallet, CoinsResult& a | |||
| const CAmount& nTargetValue, const CCoinControl& coin_control, | |||
| const CoinSelectionParams& coin_selection_params) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet); | |||
|
|
|||
| struct CreatedTransactionResult | |||
| struct FundedTransactionResult | |||
There was a problem hiding this comment.
What about naming it TransactionDetails or something similar.
The double "result" wording in stuff like Util::Result<FundedTransactionResult> sounds redundant.
(for the other one, CreatedTransactionResult could do the same and remove the "result" wording too)
There was a problem hiding this comment.
Removing the redundancy seems to make sense. Not sure about TransactionDetails -- if one of the two structs is named like this, what would be the name of the other? ({More,Less}TransactionDetails? 😛 )
de30125 to
d7f6f44
Compare
|
Rebased on master. The suggestion in #26129 (comment) is a good improvement idea, but as this PR is only focused on the |
d7f6f44 to
a065889
Compare
This PR cleans up the interfaces of the
FundTransactionfunctions by returning the out-parameters (fee, change output, error) asutil::Resultwith a newly created structureFundedTransactionResult. It can be seen as a late follow-up to #20640 which did a similar operation to theCreateTransaction{Internal}functions. Note that there are actually two functionsFundTransactionwith the same name:bitcoin/src/wallet/spend.h
Line 160 in 0b02ce9
bitcoin/src/wallet/rpc/spend.cpp
Line 489 in 0b02ce9
Only the first returns an error and hence needs to be wrapped into
util::Result, the other one can directly return the result structure.