wallet, refactor: FundTransaction(): return out-params as `util::Result` structure #26129

pull theStack wants to merge 2 commits into bitcoin:master from theStack:202209-refactor-wallet-fundtransaction_return_out_param_in_utilresult changing 3 files +38 −39

theStack commented at 12:13 AM on September 20, 2022: contributor

This PR cleans up the interfaces of the FundTransaction functions by returning the out-parameters (fee, change output, error) as util::Result with a newly created structure FundedTransactionResult. It can be seen as a late follow-up to #20640 which did a similar operation to the CreateTransaction{Internal} functions. Note that there are actually two functions FundTransaction with the same name:

https://github.com/bitcoin/bitcoin/blob/0b02ce914e8594e8938e527c91c07f57def4e943/src/wallet/spend.h#L160

https://github.com/bitcoin/bitcoin/blob/0b02ce914e8594e8938e527c91c07f57def4e943/src/wallet/rpc/spend.cpp#L489

Only the first returns an error and hence needs to be wrapped into util::Result, the other one can directly return the result structure.
DrahtBot commented at 7:49 AM on September 20, 2022: contributor


The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Reviews

See the guideline for information on the review process. A summary of reviews will appear here.



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #25273 (wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction by achow101)
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.
glozow added the label Wallet on Sep 23, 2022
glozow added the label Refactoring on Sep 23, 2022
davidjoshuashelton commented at 4:07 PM on October 4, 2022: none

.
theStack force-pushed on Nov 3, 2022
in src/wallet/spend.cpp:1115 in 038ed838d6 outdated
```
1091 | +        CWallet& wallet,
1092 | +        CMutableTransaction& tx,
1093 | +        int change_pos,
1094 | +        bool lockUnspents,
1095 | +        const std::set<int>& setSubtractFeeFromOutputs,
1096 | +        CCoinControl coinControl)
```
furszy commented at 4:08 PM on November 3, 2022:

since we are here, coin control could be passed as a ref so we avoid copying it.

theStack commented at 2:04 AM on November 27, 2022:

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.
in src/wallet/spend.h:216 in 038ed838d6 outdated
```
 192 | +    CAmount fee;
 193 | +    int change_pos;
 194 | +
 195 | +    FundedTransactionResult(CAmount _fee, int _change_pos)
 196 | +        : fee(_fee), change_pos(_change_pos) {}
 197 | +};
```
furszy commented at 4:11 PM on November 3, 2022:

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 commented at 2:05 AM on November 27, 2022:

Thanks, done.
theStack force-pushed on Nov 27, 2022
theStack commented at 2:06 AM on November 27, 2022: contributor

Force-pushed with suggestions from @furszy (thanks for reviewing!) and also rebased on master.
in src/wallet/spend.h:198 in f39caa1bfc outdated
```
 179 | @@ -180,15 +180,22 @@ std::optional<SelectionResult> SelectCoins(const CWallet& wallet, CoinsResult& a
 180 |                                             const CAmount& nTargetValue, const CCoinControl& coin_control,
 181 |                                             const CoinSelectionParams& coin_selection_params) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet);
 182 |  
 183 | -struct CreatedTransactionResult
 184 | +struct FundedTransactionResult
```
furszy commented at 2:13 PM on November 27, 2022:

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)

theStack commented at 4:29 PM on November 27, 2022:

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? :stuck_out_tongue: )
DrahtBot added the label Needs rebase on Dec 10, 2022
theStack force-pushed on Jan 3, 2023
theStack commented at 1:40 AM on January 3, 2023: contributor

Rebased on master. The suggestion in #26129 (review) is a good improvement idea, but as this PR is only focused on the FundTransaction result output, I decided to use the same naming scheme than the CreateTransaction result output (i.e. CreatedTransactionResult and FundedTransactionResult), and I'm also still not sure which names are proper (is simply CreatedTransaction and FundedTransaction descriptive enough?).
DrahtBot removed the label Needs rebase on Jan 3, 2023
wallet: FundTransaction(): return out-params via `util::Result` structure bfbb2ed609
wallet: rpc-internal FundTransaction(): return out-params via structure a0658894d3
theStack force-pushed on Jan 25, 2023
achow101 closed this on Apr 25, 2023
bitcoin locked this on Apr 24, 2024

Contributors

Labels

Linked (view graph)

#20640 wallet, refactor: return out-params of CreateTransaction() as optional struct

wallet, refactor: FundTransaction(): return out-params as `util::Result` structure #26129

Reviews

Conflicts