Add transactions method on wallet

[reez]

Description

Adds transactions method on Wallet.

Notes to the reviewers

Simplified return type to list of Transactions.

Changelog notice

Checklists

All Submissions:

• I've signed all my commits
• I followed the contribution guidelines
• I ran cargo fmt and cargo clippy before committing

New Features:

• I've added tests for the new feature
• I've added docs for the new feature

Bugfixes:

• This pull request breaks the existing API
• I've added tests to reproduce the issue which are now passing
• I'm linking the issue being fixed by this PR

[thunderbiscuit]

I think this works in practice, but I have 2 questions about the theory I'm still wondering about:

1. The API doc for the method is one line: Iterate over the transactions in the wallet.. I wonder if (a) this is meant to be used for more than just returning the transactions, and (b) only returns transactions "owned" by the wallet. For example I see that it's basically traversing the TxGraph... but if I understand correctly you can add any transactions to this TxGraph (?)
2. The type of CanonicalTx.TxNode.tx is &'a T, which I don't understand well. Will you always be able to get a Transaction type out of it when you apply the .into() method on it?

[reez]

Great Q’s/Feedback Thunder! I think I caught some of the conversation about this on the dev call this morning, and I think(?) what I heard as the conclusion is that we are leaving this transactions function as is but are just making sure we expose isMine to the users at some point so they can filter based on isMine.

If that is all correct than is it cool to add isMine in a follow up PR?

If my understanding is incorrect I can update to whatever is needed, happy to do whatever needed, and I enjoyed the conversation and questions around this PR so far.

[thunderbiscuit]

Yeah roughly my take on it is that isMine is implied in the the transaction method at the moment. We can add isMine and tell people to filter, but that feels like a weird API, in part because not all users will get the message that they in fact need to do both, and also because we don't actually offer any methods (at the moment at least) to add anything to your TxGraph, so at the moment the isMine filter would be redundant.

My current take on it is this (happy to hear your thoughts): we should add the isMine filter to the method directly. This ensures the method is explicit in returning only wallet-related txs (even thought that's really all it can return at the moment). We open an issue on the bdk repo to discuss this weird API and see what might have been the thinking behind it. One good option proposed by @notmandatory was to add an argument onlyMine to the method (at the rust layer) or something like that, to make it explicit that the method can return much more than that if that flag is not set.

Edit: I forgot to write it initially but the problem with this of course is that we're transforming the API, and so far we've been trying to keep them in very close sync; this would be a departure from that, so should be thought through seriously. We'll also see what comes out of the discussion in #1239

[thunderbiscuit]

I'm changing my mind on this. I think we should expose the API as is on the Rust side, and open issues/fix it upstream if there are things to improve. For now, exposing it as is is the better/cleaner way. We also don't offer ways to impact the TxGraph manually at this point on the ffi side, so the problems that this could cause are limited.

[thunderbiscuit]

Just a small nit. This is ready to go afterwards IMO. I tested locally.

[reez]

@thunderbiscuit PR is updated now per suggestions and tests pass, thanks!

[thunderbiscuit]

ACK a1a4599.