Make fee and timestamp in TransactionDetails Options

[RCasatta]

Description

Make fields fee (renamed from fees to be coherent with rpc) and timestamp Option to better handle the missing information.

Specify in docs timestamp refer to the timestamp of the block the tx is included in (removed the initialization to now() for sending tx)

UPDATE: as per @LLFourn suggestion, timestamp and height have been grouped in the confirmation_time field

Fix #369

Notes to the reviewers

all tests are modified to use unwrap_or(0) in place where fee was used

In case a tx is confirmed but later reorged and included in the reorged block at the same height, the timestamp will be wrong. The fix would be detrimental for performance in the most common cases so I didn't implement it.

Checklists

All Submissions:

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

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

[thunderbiscuit]

Great stuff!

Do I understand correctly that this Option will basically return one of two things, either the proper timestamp/fee, or return None? (just reading about the Option type).

[LLFourn]

Concept ACK. Given that both height and timestamp will be None in the same case can we roll these into the same struct like ConfirmationTime or something?

[RCasatta]

Do I understand correctly that this Option will basically return one of two things, either the proper timestamp/fee, or return None? (just reading about the Option type).

yes, exactly

Given that both height and timestamp will be None in the same case can we roll these into the same struct like ConfirmationTime or something?

will do

[RCasatta]

I added simple, because you can't launch tests singularly if they share a common prefix with others

[LLFourn]

ACK 0bbfa5f it's a little interesting that you have to have this ::new() function to check if they are both not options. It would be nice if this could be enforced during de-serialization instead.

[afilini]

I haven't reviewed the code thoroughly yet, but I'm assuming this affects the database representation of our structs somehow. What happens if I try to load a db created with the older version?

[RCasatta]

I haven't reviewed the code thoroughly yet, but I'm assuming this affects the database representation of our structs somehow. What happens if I try to load a db created with the older version?

If my test is correct it works and it simply initializes fee and confirmation_time to None.

I used bdk-cli master, synced a wallet, then switched to this branch https://github.com/RCasatta/bdk-cli/tree/updated asked the balance of the same wallet, synced again and it works (my local bdk is on the options branch)