Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The fact that the Wallet.transactions() API can return more than wallet-related txs is not explicit #1239

Closed
thunderbiscuit opened this issue Dec 14, 2023 · 2 comments · Fixed by #1779
Assignees
Labels
api A breaking API change discussion There's still a discussion ongoing module-wallet
Milestone

Comments

@thunderbiscuit
Copy link
Member

One might expect that the Wallet.transactions() method replaced the Wallet.list_transactions() and will return similar transactions (the transactions related to the descriptors the wallet tracks). This is not the case, as the method returns an iterator that traverses the entire TxGraph. If I understand this TxGraph correctly, any number of transactions can be added to it, even conflicting and never mined txs. The API docs are simply: Iterate over the transactions in the wallet.

For typical use, a wallet will likely want to see transactions related to itself, and I assume the user is expected to then filter the transactions in the iterator using the is_mine() method. Let me know if I misunderstand the intended use of the API!

But if the above is correct, I find the API more cumbersome than it needs to be for the most typical uses and what one might expect of something like a Wallet.transactions() method. Here are a few thoughts I have to maybe make the API simpler/more explicit. Would love to hear what others think:

  1. The transactions() method could take an argument mine_only: bool that would ensure the method returns only wallet-related txs, or maybe the opposite, an argument full_tx_graph: bool that would return what it currently returns.
  2. The transactions() method could be renamed entirely to imply its more complex nature, and a new, simpler transactions() method could take its place and return a more filtered list of transactions that relate to the wallet.

Let me know if there is more to this that I haven't understood well!

@thunderbiscuit
Copy link
Member Author

Connecting this to bitcoindevkit/bdk-ffi#432

@thunderbiscuit thunderbiscuit added the discussion There's still a discussion ongoing label Dec 14, 2023
@LLFourn
Copy link
Contributor

LLFourn commented Dec 19, 2023

One might expect that the Wallet.transactions() method replaced the Wallet.list_transactions() and will return similar transactions (the transactions related to the descriptors the wallet tracks). This is not the case, as the method returns an iterator that traverses the entire TxGraph. If I understand this TxGraph correctly, any number of transactions can be added to it, even conflicting and never mined txs. The API docs are simply: Iterate over the transactions in the wallet.

I think it should actually return similar things than before. To be specific it returns transactions in the chain that have been inserted into the wallet. Most likely this will be only relevant transactions (but applications can always insert irrelevant ones if they want!).

The documentation should explain this nuance. You could make this method more restrictive or add another one which made sure it was only relevant transactions (i.e. sent or received some bitcoin in the wallet) which should be easy to check.

@nondiremanuel nondiremanuel added this to BDK Jan 2, 2024
@nondiremanuel nondiremanuel modified the milestones: 1.0.0-alpha.4, 1.0.0 Jan 2, 2024
@nondiremanuel nondiremanuel moved this to Todo in BDK Jan 6, 2024
@evanlinjin evanlinjin moved this from Todo to Discussion in BDK Jan 25, 2024
@notmandatory notmandatory added module-wallet api A breaking API change labels Mar 18, 2024
@nondiremanuel nondiremanuel removed this from the 1.0.0-alpha milestone Mar 21, 2024
@notmandatory notmandatory added this to the 1.0.0-beta milestone Dec 17, 2024
@notmandatory notmandatory moved this from Discussion to In Progress in BDK Dec 17, 2024
@notmandatory notmandatory moved this from In Progress to Needs Review in BDK Dec 17, 2024
@github-project-automation github-project-automation bot moved this from Needs Review to Done in BDK Dec 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api A breaking API change discussion There's still a discussion ongoing module-wallet
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants