bdk icon indicating copy to clipboard operation
bdk copied to clipboard

feat(wallet): add transactions_sort_by function

Open notmandatory opened this issue 1 year ago • 5 comments

Description

Added new type alias WalletTx which represents a CanonicalTx<'a, Arc<Transaction>, ConfirmationTimeHeightAnchor> and new Wallet::transactions_sort_by that returns a Vec<WalletTx> sorted by the given compare function.

Notes to the reviewers

fixes #794

Changelog notice

  • Add new type alias WalletTx which represents a CanonicalTx<'a, Arc<Transaction>, ConfirmationTimeHeightAnchor>.
  • Add Wallet::transactions_sort_by() that returns a Vec<WalletTx> sorted by a given compare function.

Checklists

All Submissions:

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

New Features:

  • [x] I've added tests for the new feature
  • [x] I've added docs for the new feature

notmandatory avatar Jun 18 '24 01:06 notmandatory

I'm curious if users will also be interested in a list that includes transactions that may not be in the current active chain as was mentioned in #794 (comment)

I like the idea of being able to show TX that are not in the valid chain due to RBF or reorg for a future PR. The idea with this one is to provide only the basic sort functionality for lang bindings to expose.

notmandatory avatar Jun 18 '24 16:06 notmandatory

This only adds a new function and doesn't break any existing APIs so I'm moving this to the beta milestone so we can focus on breaking changes.

notmandatory avatar Jun 19 '24 14:06 notmandatory

Also, we'll still need to work on https://github.com/bitcoindevkit/bdk_wallet/issues/106 in the future, right ?

oleonardolima avatar Jun 19 '24 19:06 oleonardolima

Also, we'll still need to work on bitcoindevkit/bdk_wallet#106 in the future, right ?

Yes because this is a simple collect() into a Vec and then sorting the vec in the Wallet API. bitcoindevkit/bdk_wallet#106 is in the Chain API.

storopoli avatar Jun 19 '24 21:06 storopoli

@notmandatory I'm happy to merge this now tbh. It doesn't conflict or break anything.

evanlinjin avatar Jun 20 '24 03:06 evanlinjin

Just adding a +1 for merging this PR 🚀

reez avatar Sep 03 '24 14:09 reez

Rebased and fixed docs.

notmandatory avatar Sep 05 '24 03:09 notmandatory

Back by popular demand, merging this now instead of later.

notmandatory avatar Sep 05 '24 05:09 notmandatory

woohoo!

reez avatar Sep 07 '24 16:09 reez