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

feat(wallet): add transactions_sort_by function #1477

Merged

Conversation

notmandatory
Copy link
Member

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:

  • 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

@notmandatory notmandatory self-assigned this Jun 18, 2024
@notmandatory notmandatory added new feature New feature or request module-wallet labels Jun 18, 2024
@notmandatory notmandatory added this to the 1.0.0-alpha milestone Jun 18, 2024
@notmandatory notmandatory force-pushed the feat/add_wallet_tx_sort branch 2 times, most recently from 92daee4 to 06e695e Compare June 18, 2024 02:05
@notmandatory notmandatory marked this pull request as ready for review June 18, 2024 02:07
Copy link
Contributor

@storopoli storopoli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A minor comment.

crates/wallet/src/wallet/mod.rs Show resolved Hide resolved
@notmandatory notmandatory force-pushed the feat/add_wallet_tx_sort branch from 06e695e to fb1599d Compare June 18, 2024 15:30
Copy link
Contributor

@ValuedMammal ValuedMammal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK fb1599d

This looks good. 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)

@notmandatory
Copy link
Member Author

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.

Copy link
Member

@evanlinjin evanlinjin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK fb1599d

@notmandatory
Copy link
Member Author

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 notmandatory modified the milestones: 1.0.0-alpha, 1.0.0-beta Jun 19, 2024
Copy link
Contributor

@oleonardolima oleonardolima left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! I just left a single comment about the documentation.

crates/wallet/src/wallet/mod.rs Show resolved Hide resolved
@oleonardolima
Copy link
Contributor

Also, we'll still need to work on #1333 in the future, right ?

@storopoli
Copy link
Contributor

Also, we'll still need to work on #1333 in the future, right ?

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

@evanlinjin
Copy link
Member

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

@notmandatory notmandatory modified the milestones: 1.0.0-beta, 1.1.0 Aug 9, 2024
@reez
Copy link

reez commented Sep 3, 2024

Just adding a +1 for merging this PR 🚀

@notmandatory notmandatory force-pushed the feat/add_wallet_tx_sort branch from fb1599d to 00d24ff Compare September 5, 2024 03:56
@notmandatory
Copy link
Member Author

Rebased and fixed docs.

@notmandatory notmandatory force-pushed the feat/add_wallet_tx_sort branch from 0003980 to 90b393a Compare September 5, 2024 04:19
Added type WalletTx<'a> as an alias for CanonicalTx<'a, Arc<Transaction>, ConfirmationBlockTime>.
@notmandatory notmandatory force-pushed the feat/add_wallet_tx_sort branch from 90b393a to 83a0247 Compare September 5, 2024 04:44
@notmandatory notmandatory modified the milestones: 1.1.0, 1.0.0-beta Sep 5, 2024
@notmandatory
Copy link
Member Author

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

@notmandatory notmandatory merged commit f69eab6 into bitcoindevkit:master Sep 5, 2024
21 checks passed
@reez
Copy link

reez commented Sep 7, 2024

woohoo!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module-wallet new feature New feature or request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Transaction Sorting and Querying
6 participants