(draft)Add transactions_sort_by

[reez]

Description

Adds transactions_sort_by.

The example is the sorting I wanted and would sort of expect as a user of bdk-ffi, so that's what I went for in actual implementation.

Notes to the reviewers

I've wanted this for myself so I'm grateful Steve added it in bdk.

Obviously could do this sorting on client side, but its nice to get something out of the box that sorts consistently if you are a consumer of bdk-ffi.

Screenshots

~After

~Before

Changelog notice

Added

• Exposed a new function transactions_sort_by in Wallet

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

[reez]

@thunderbiscuit

Wanted to get your feedback on this in general:

This is a pretty simplistic (and possibly opinionated from the ffi/mobile side) version of what we could do with this, it is similar to the example for good reason, IMO that's what you might expect out of the box (consistency + newest items first). (and is what I'd expect when building BDKSwiftExampleWallet and is why I was wanting it added in bdk and exposed in bdk-ffi!)

But there is obviously room for expansion of this and more flexibility, but the more I've thought about it I've not landed on something I'm immediately convinced to do right now. For example, I could add a SortOrder enum and then pass an additional parameter into transactions_sort_by for someone to have the option to choose whether they want it ascending/descending, and that's easy enough but I'm not even sure if that's worth adding at the moment. And then we could add even more optionality, but if I think of the main consumers of bdk-ffi the ability to have a default "here are the transactions where the order is newest to oldest" takes care of the bulk of what folks would want and then if not people still have the option to use existing transactions method.

[thunderbiscuit]

My only thought is that as is, the method would need to be renamed (something like transactions_sort_descending() or transactions_sort_newest_first().

But that's quite far from the Rust idea of passing it a sorting lambda. I'd need to look into the method a bit more to see if we can use the foreign trait implementation workflow of uniffi on this compare argument (and I don't like the look of those 2 lifetimes and the FnMut but it's maybe something we can get by). This would be the ultimate cleaner solution IMO.

[reez]

My only thought is that as is, the method would need to be renamed (something like transactions_sort_descending() or transactions_sort_newest_first().

But that's quite far from the Rust idea of passing it a sorting lambda. I'd need to look into the method a bit more to see if we can use the foreign trait implementation workflow of uniffi on this compare argument (and I don't like the look of those 2 lifetimes and the FnMut but it's maybe something we can get by). This would be the ultimate cleaner solution IMO.

Good thoughts, I'm going to think over them a little bit myself too now-

[rustaceanrob]

I believe a trait that has one associated function sort and takes (CanonicalTx, CanonicalTx) with an output of a new enum Ordering would be possible without a huge lift. I think we can take references internally as long as the UniFFI user passes two CanonicalTx

[reez]

I believe a trait that has one associated function sort and takes (CanonicalTx, CanonicalTx) with an output of a new enum Ordering would be possible without a huge lift. I think we can take references internally as long as the UniFFI user passes two CanonicalTx

yup that totally makes sense to me right now, was just messing with something somewhat similar to that right now (kind of taking inspiration from some of the stuff I did previously with callbacks on sync requests related to script inspection, etc), so thanks for spelling out your thoughts and the return type of the Ordering enum, that helped me clarify what this should look like, so that was helpful to get your feedback and specific idea!

[reez]

This actually has a few different directions it can go, so am going to save the discussion for the next bindings call after having tried out a few things and thought thru what I'd want/expect as an iOS dev to color my thinking, and then get everyone's feedback again-

[notmandatory]

I agree we either need to rename this if using a static sorting, or preferably have it take a compare function callback as a parameter.