Bring back TxDetails

[thunderbiscuit]

Fixes #100.

The TxDetails feature was put aside to focus on the 1.0 release, but now that we're done with that I want to revive the idea.

This is very much WIP. I'm trying to figure out exactly what we want in this data structure, and I'm not even sure my way to get at the data is the most efficient way to do it.

Notes to the reviewers

Changelog notice

Added:
    - Wallet::get_tx_details method [#201]

[#201]: https://github.com/bitcoindevkit/bdk_wallet/pull/201

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

[coveralls]

Pull Request Test Coverage Report for Build 15187463781

Details

• 21 of 21 (100.0%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.04%) to 85.38%

---

Totals
Change from base Build 15166368710:	0.04%
Covered Lines:	7294
Relevant Lines:	8543

---

💛 - Coveralls

[thunderbiscuit]

Who TH is this coveralls bot?

@coveralls explain yourself and tell me why I shouldn't just cut you out of my codebase my man

[ValuedMammal]

Concept ACK. I did a first pass looks great so far.

[thunderbiscuit]

Tagging some folks who had expressed interest in the feature: @matthiasdebernardini and @tnull.

I have a nagging feeling that there is probably more we want to have in this struct, and if we're going to build one then we might as well put the good stuff in it. Just wondering if you (or anyone else) has anything else they'd like to see as part of a struct that represent transaction details.

[matthiasdebernardini]

Hey thanks! I'd also want a net impact in terms of balance on a per transaction basis. We had to build this ourselves but I think it would be very useful.

[thunderbiscuit]

Ah yes I remember! Do you remember the issue number for the brainstorm/PR on this? I'll need to refresh my memory on what exactly we landed on. But that's a good idea.

[matthiasdebernardini]

I think it was on discord, this is what we have implemented

pub struct WalletHistoryRecord {
    pub height: Option<u32>,
    pub node_info: TxNodeInfo,
    pub received: u64,
    pub sent: u64,
    pub delta: i128,
    pub balance: i128,
    pub txid: Txid,
}

pub struct TxNodeInfo {
    pub block_timestamp: Option<u32>,
    pub mempool_time: Option<u64>,
}

We use this to check if a wallet is spending too much money in a given time period (app specific) but I think its useful for building wallet summaries anyways.

If you want to implement this, you will need to also figure out a way to canonically sort these transactions if theres multiple transactions in the same block, if its random then youll get a different history each time you call it.

So far, I like what you have, but I would add wallet balance and the net change to the wallet.

[thunderbiscuit]

Just linking here so I don't have so search for it later on: #31.

[thunderbiscuit]

Thanks for the feedback. @matthiasdebernardini I decided to go with balance_delta, as I thought it more explicit (otherwise it's a delta of what? not as clear).

@ValuedMammal I know you'd talked about using the name net_value, but again I wasn't sure if that conveyed the meaning clearly. The value of something (what?), and then "net" of what? In this case I mean I know exactly what of course, but figured balance_delta was clearer on first read. Happy to bikeshed on this if others have different opinions!

@matthiasdebernardini I left out for now the idea of balance as part of this struct, because I feel like that's a bit different. It's not about the transaction per se, but really about the balance of the wallet at the time (and here the struct assumes the given transaction is the latest tx I guess?). I'm not again adding it, but I'm not yet convinced of the true value.

Again thanks for the feedback, this is ready for some more bikeshedding if needed!

[matthiasdebernardini]

@thunderbiscuit, if youre not going to add it, then you should provide a method that calculates a balance at a given height, that would be more useful, maybe for another PR. But yeah balance delta is a good name!

[thunderbiscuit]

You know I had not thought about this before but it does sound interesting. "Balance at block X" type of method...

I'll open an issue and we can look at how best to add this.

[ValuedMammal]

ACK

I made some non-blocking comments.

[ValuedMammal]

@ValuedMammal I know you'd talked about using the name net_value, but again I wasn't sure if that conveyed the meaning clearly. The value of something (what?), and then "net" of what? In this case I mean I know exactly what of course, but figured balance_delta was clearer on first read. Happy to bikeshed on this if others have different opinions!

net_change would sound ok too.

[notmandatory]

Overall LGTM and +1 on Mammal's comments.

[thunderbiscuit]

All comments addressed.

@ValuedMammal I considered net_change earlier on in my thinking about this PR, but decided using the word change here potentially clashes with our other uses of the word change, (change keychains, sats coming back as change, etc.).

In that sense I thought the field unclear; "net" change? Is that different than other change? etc. Here the word change would be used as meaning "the change in the wallet's balance", but it's not obvious at first read of a struct that handles sent/received values and whatnot. You might think it's just a trio of "what was sent", "what was received", "what was change" (even thought that's weird and doesn't make a lot of sense). In any case I decided against using change here and took "delta" from @matthiasdebernardini (thanks for the idea!).

[ValuedMammal]

tACK 3cd8921

[notmandatory]

utACK 3cd8921