Skip to content

Implement ordering for TransactionDetails#812

Merged
danielabrozzoni merged 1 commit intobitcoindevkit:masterfrom
benthecarman:order-tx-details
Dec 19, 2022
Merged

Implement ordering for TransactionDetails#812
danielabrozzoni merged 1 commit intobitcoindevkit:masterfrom
benthecarman:order-tx-details

Conversation

@benthecarman
Copy link
Copy Markdown
Contributor

@benthecarman benthecarman commented Dec 6, 2022

Description

Pulled from MutinyWallet/mutiny-node#189

Wallets should be able to sort the transactions easily, this makes it so you can just all sort on a list of tx details instead of needing to implement the sort_by yourself

Checklists

All Submissions:

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

Comment thread src/types.rs Outdated
// if timestamps are equal, compare by txid
match (&self.confirmation_time, &other.confirmation_time) {
(Some(a), Some(b)) => {
let cmp = a.timestamp.cmp(&b.timestamp);
Copy link
Copy Markdown
Contributor

@tnull tnull Dec 6, 2022

Choose a reason for hiding this comment

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

Maybe it would make sense to implement Ord also for BlockTime, which would first compare the height then the timestamp. Here we then can just to compare the entire confirmation_time?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good idea, done

Comment thread src/types.rs Outdated
// if timestamps are equal, compare by txid
match (&self.confirmation_time, &other.confirmation_time) {
(Some(a), Some(b)) => {
let cmp = a.cmp(&b);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think you can make this (and below) quite a bit more concise by chaining the cmps via then_with.

Btw., Ord is implemented for Option<T>, so you should be able to just do self.confirmation_time.cmp(&other.confirmation_time).then_with(...), no need for the match.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done, thanks!

Comment thread src/types.rs Outdated
Copy link
Copy Markdown
Contributor

@tnull tnull 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 to me.

Comment thread src/types.rs Outdated
@benthecarman benthecarman force-pushed the order-tx-details branch 2 times, most recently from c8c5038 to 1b5efb0 Compare December 6, 2022 09:28
@tnull
Copy link
Copy Markdown
Contributor

tnull commented Dec 6, 2022

Ah, this probably would also benefit from a really basic unit test checking that the ordering works as expected.

@benthecarman
Copy link
Copy Markdown
Contributor Author

Added a test

Copy link
Copy Markdown
Contributor

@tnull tnull 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 from my side!

@notmandatory notmandatory added the new feature New feature or request label Dec 16, 2022
@notmandatory
Copy link
Copy Markdown
Member

Looks like a good reasonable default ordering scheme to me. But you need to fix a clippy error, and rebase to fix a recent CI issue.

@benthecarman
Copy link
Copy Markdown
Contributor Author

Looks like a good reasonable default ordering scheme to me. But you need to fix a clippy error, and rebase to fix a recent CI issue.

fixed

Copy link
Copy Markdown
Member

@danielabrozzoni danielabrozzoni left a comment

Choose a reason for hiding this comment

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

utACK d3d0756

Thanks for taking care of this! :)

@danielabrozzoni danielabrozzoni merged commit 634a057 into bitcoindevkit:master Dec 19, 2022
@benthecarman benthecarman deleted the order-tx-details branch December 19, 2022 08:57
@notmandatory notmandatory mentioned this pull request Dec 26, 2022
24 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

new feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants