[ Feat ] Updated Direct-Send RPC-Api to accept list of UTXOs

[amitx13]

This PR fixes #1712 and addresses joinmarket-webui/jam#772
Changes Made:

• Updated the direct-send API to accept a list of UTXOs specified by the user.
• Implemented backend logic to handle the selection of these UTXOs for the transaction.
• Added necessary validation checks to ensure the specified UTXOs are part of the user's wallet.
• Ensured backward compatibility by making selected_utxos an optional property."

[amitx13]

Hey @AdamISZ and @kristapsk,

First of all, sorry for the disturbance. If you have a moment, could you please take a look at ~docs/api/wallet-rpc.yaml? I tried solving it on my own, but OpenAPI Diff keeps failing. I just want to add Coincontrol to JAM so that users can have more control over their funds. I apologize for being so annoying. I am in contract with JAM through the Summer of Bitcoin program and do not have much time left. I hope you understand, and thank you for your time and the great work you do.

[kristapsk]

@amitx13 Sorry, busy with other stuff, will try to find time and take a look.

[kristapsk]

I can't see the reason why CI is failing here. Bug in OpenAPI Diff? For some reason it thinks that selected_utxos is introduced as a required property, but it clearly isn't. Should we just ignore this?

@theborakompanioni Could you please test and confirm that this change does not break anything with Jam? That would be good enough for me to merge this.

[theborakompanioni]

@theborakompanioni Could you please test and confirm that this change does not break anything with Jam? That would be good enough for me to merge this.

I have now had time to test the changes. I exclusively tested on regtest via RPC api calls.

• Sending a single locked UTXO: Fails with "Transaction failed." ✔️
• Sending a single frozen UTXO: Fails with "Transaction failed." ✔️
• Sending an unfrozen and a frozen UTXO: Fails with "Transaction failed." ✔️
• Sending two unfrozen UTXO from different mixdepths: Fails with "Transaction failed." ✔️
• Sending a single locked but expired UTXO: Success! ✔️
• Sending selected_utxos as empty array: Success - as if no array was given - is this expected? ❓

When doing a sweep (amount_sats := 0), the selected_utxos is ignored!
This should probably fail, or sweep with only the selected UTXOs! Which approach is preferred?
I'd opt for failing, as a sweep with certain UTXOs can be done in a two phase: Freeze unselected UTXO, then sweep (don't provide selected_utxos).

So, from my point of view, the changes look good overall (except the sweep case).
I am happy as this brings Coin Control to JM/Jam, albeit only for direct sends.

However, I'd love to see some tests covering this behaviour and verifying the above manual test cases.
Great job @amitx13! And thanks for reviewing and considering this change @kristapsk! 🙏

[amitx13]

• Sending selected_utxos as empty array: Success - as if no array was given - is this expected? ❓

Yes, this is intentional. When no value is provided, selected_utxos is considered none, which ensures backward compatibility with the RPC-API.

When doing a sweep (amount_sats := 0), the selected_utxos is ignored! This should probably fail, or sweep with only the selected UTXOs! Which approach is preferred? I'd opt for failing, as a sweep with certain UTXOs can be done in a two phase: Freeze unselected UTXO, then sweep (don't provide selected_utxos).

I agree, failing makes more sense. Freezing unselected UTXOs and then sweeping with selected UTXOs would alter the original definition of a sweep. Instead of ignoring this case, we should fail it with Transaction failed.

Looking forward to hearing @kristapsk thoughts on this.

[roshii]

This (nullable) may be the line that breaks compatibility. Parameter is simply not required, and beside compatibility issue it makes no sense to supply the parameter with a null value.

[amitx13]

Hey @roshii, I appreciate your feedback, and thanks for catching that! I've updated the PR and removed nullable: true since it wasn't serving any purpose. However, I don't believe that's the reason behind the OpenAPI Diff failure.

[roshii]

That wasn't the reason for the failure indeed. OpenAPI Diff still catches selected_utxos as required 🤔
I will dig into it soon as as I can.

[amitx13]

Done with the changes requested by @roshii and fixed the sweep case that @theborakompanioni mentioned earlier. Please take a look. Thank you for your time, guys.