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. |
theStack
force-pushed
the
202209-refactor-wallet-fundtransaction_return_out_param_in_utilresult
branch
from
b2d3453 to
1cbbbcd
Compare
src/wallet/spend.cpp
Outdated
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
There was a problem hiding this comment.
In 038ed838:
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.
theStack
force-pushed
the
202209-refactor-wallet-fundtransaction_return_out_param_in_utilresult
branch
from
1cbbbcd to
de30125
Compare
|
Force-pushed with suggestions from @furszy (thanks for reviewing!) and also rebased on master. |
src/wallet/spend.h
Outdated
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? 😛 )
theStack
force-pushed
the
202209-refactor-wallet-fundtransaction_return_out_param_in_utilresult
branch
from
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 |
theStack
force-pushed
the
202209-refactor-wallet-fundtransaction_return_out_param_in_utilresult
branch
from
d7f6f44 to
a065889
Compare
6 participants
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.