I prefer having a single function (as opposed to keeping the logic at the call site) for the following reasons:
- We avoid duplicating code at the RPC call sites:
- The logic for validating SFFO inputs would need to be duplicated at
send, fundrawtx and walletcreatefundedpsbt. This is the code that checks that the ints passed are not duplicated and in bounds and previously used to be inside rpc/FundTransaction
- We have one place to validate all SFFO inputs:
- Currently,
sendmany SFFO inputs are not validated (documented in the added functional tests)
- Having one function for parsing and validating makes it cleaner to add validation for all inputs in a follow-up PR. The only reason I’m not doing it in this PR is that its a behavior change (it could make calls to
sendmany invalid that were previously accepted) and I’d like to keep this strictly a refactor
we don’t need second parameter for this function
In the case of sendmany, we do need the array of destinations to lookup their position. In the case of send, walletcreatefundedpsbt, and fundrawtransaction, we need the array of destinations to make sure our ints are in bounds. There are definitely other ways to do it, but it seems fine to use the same vector for lookups and size. More just making the point that the argument is always used.
we can combine NormalizeOutputs and ParseOutputs. they are used always together
Actually, the main goal here was to separate the logic in AddOutputs to get rid of duplicate validation logic in AddOutputs and also the original ParseRecipients function. sendtoaddress and sendtoaddress need the validation logic, but don’t need NormalizeOutputs since those RPCs already only allow the user to pass a key-value pair for address and amounts.
The other RPCs (send, fundrawtransaction, walletcreatefundedpsbt) need the logic for parsing how the univalue is passed (NormalizeOutputs) and validating the addresses (ParseOutputs).
