bitcoindevkit / bdk

A modern, lightweight, descriptor-based wallet library written in Rust!
Other
862 stars 311 forks source link

The wallet should have a utility method to return standard transaction data #1401

Open thunderbiscuit opened 7 months ago

thunderbiscuit commented 7 months ago

Since the purpose of the bdk_wallet crate is to provide useful/opinionated ways to perform standard wallet tasks, it feels to me like a simple-to-use "get me all important—nicely bundled if you don't mind—info about this wallet's transactions" method is missing from our API.

The data is there, but currently requires multiple round trips to bundle it all together, yet that data is what all user interfaces will need for one of the most common use cases, a transactions history screen. Note that this type of data object is what we previously had with the TransactionDetails struct.

The exact data that might be useful here is up for discussion, but let us assume a structure like this one would suffice for most use cases:

pub struct TxDetails<'a> {
    pub received: u64,
    pub sent: u64,
    pub fee: u64,
    pub fee_rate: FeeRate,
    pub txid: Txid,
    pub chain_position: ChainPosition<&'a ConfirmationTimeHeightAnchor>,
}

The current way to bundle this up ready for UI consumption is something like this:

let transactions = wallet.transactions();

let tx_details: Vec<TxDetails> = transactions.map(|tx| {
        let (sent, received) = wallet.sent_and_received(tx.tx_node.tx);
        let fee = wallet.calculate_fee(tx.tx_node.tx).unwrap();
        let fee_rate = wallet.calculate_fee_rate(tx.tx_node.tx).unwrap();
        let txid = tx.tx_node.txid;
        let chain_position = tx.chain_position.clone();

        TxDetails {
            received,
            sent,
            fee,
            fee_rate,
            txid,
            chain_position,
        }
    })
    .collect();

for tx in &tx_details {
    let ui_details = format!(
        "Received: {}\nSent: {}\nTotal fee: {} sat\nFee rate: {} sat/vbyte\nTxid: {}\nChain position: {:?}\n",
        tx.received,
        tx.sent,
        tx.fee,
        tx.fee_rate.to_sat_per_vb_ceil(),
        tx.txid,
        tx.chain_position
    );
    println!("{}", ui_details);
}

Notice the individual calls to the wallet for for fee, fee_rate, and sent_and_received values, as well as the pulling out of the txid. It gets even more unruly if you want to build fields like confirmed: Boolean, confirmation_block: Option<u64>, confirmation_timestamp: Option<u64> instead of using the ChainPosition type, etc.

I propose we add a method (name tbd, but maybe something like get_txs_details(), which would take care of building such a data structure for the user, instead of making all users develop their utility method on top of wallet.transactions().

LLFourn commented 6 months ago

Since the purpose of the bdk_wallet crate is to provide useful/opinionated ways to perform standard wallet tasks, it feels to me like a simple-to-use "get me all important—nicely bundled if you don't mind—info about this wallet's transactions" method is missing from our API.

Just querying what you want is the most simple-to-use method for obtaining what you want. Why bundle it? important is application dependent. Perhaps we add ways of querying new "important" information. Adding these as fields to this struct is then a breaking change but adding a new method is not.

I don't find the code above to be unruly. It seems idiomatic and flexible.

notmandatory commented 6 months ago

Since this requires more discussion and can be added later (and we've got so much left to do for 1.0) I'm moving this out of the 1.0 milestone.

matthiasdebernardini commented 6 months ago

Hey I'm actually using this code to summarize my wallet to the user, I am syncing with electrum (on signet) and it seems that the example code doesn't sync certain outputs which are needed to calculate_fee which throws an error in tx_graph.rs

Scanning keychain [External] 1   2   3   4   5   6   7   8   9   10  11  12  13  14  15  16  17  18  19  20  21  22  23  24  25  26  27  28  29  30  31  32  33  34  35  36  37  38  39  40  41  42  43  44  45  46  47  48  49  50  51  52  53  54  55  56  57  58  59  60  61  62  63  64 
Scanning keychain [Internal] 1   2   3   4   5   6   7   8   9   10  11  12  13  14  15  16  17  18  19  20  21  22  23  24  25  26  27  28  29  30  31  32  33  34  35  36  37  38  39  40  41  42  43  44  45  46  47  48  49  50  51  52  53  54  55  56  57  58  59  60  61  62  63  64  65  66  67  68  69  70  71  72  73  74  75  76  77  78  79  80  81  82  83  84  85  86  87  88  89  90  91  92  93  94  95  96  97  98  99  100 101 102 103 104 105 106 107 108 109 110 111 112 113 114 115 116 117 118 119 120 121 122 123 124 125 126 127 128 129 130 131 132 133 134 135 136 137 138 139 140 141 142 143 144 145 146 147 148 149 150 151 152 153 154 155 156 157 158 159 160 161 162 163 164 165 166 167 168 169 170 171 172 173 174 175 176 177 178 179 180 181 182 183 184 185 186 187 188 189 190 191 192 193 194 195 196 197 198 199 200 201 202 203 204 205 206 207 208 209
thread 'main' panicked at src/main.rs:69:61:
called `Result::unwrap()` on an `Err` value: MissingTxOut([OutPoint { txid: 0x7bd97a2f739fde1c69468abd4d4f15495f9382507bc1941594123b8aaedc69c2, vout: 2 }])
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

I haven't been able to figure out how to sync/include these missing outpoints and I'm curious what the best way to do it is.

For your convenience I've made a replit that shows my issue, anyone interested should be able to run the example. Let me know if it doesn't run.

I'd be happy to amend the electrum example to include the snippet of code to sync the missing outpoints, since syncing with electrum and calculating fees seems like a common use case.

Thanks!

matthiasdebernardini commented 5 months ago

I'll just add this here in case anyone else needs to use it, this is what I am using.

pub fn get_tx_details(wallet: &Wallet) -> anyhow::Result<Vec<TxDetail>> {
    wallet
        .transactions()
        .map(|tx| {
            let txid = tx.tx_node.txid;
            let chain_position = tx.chain_position;
            let tx = tx.tx_node.tx.as_ref();
            let (sent, received) = wallet.sent_and_received(tx);
            let fee = wallet.calculate_fee(tx)?;
            let fee_rate = wallet.calculate_fee_rate(tx)?;
            let fee_rate = floating_rate!(fee_rate);

            let tx_detail = TxDetail {
                received,
                sent,
                fee,
                fee_rate,
                txid,
                chain_position,
            };

            Ok(tx_detail)
        })
        .collect()
}
tnull commented 2 months ago

I'm +1 for re-adding this functionality. Previously, I used the TransactionDetails returned from TxBuilder::finish to run some checks (sent/received amount, adherence to a fee limit, etc) before proceeding to sign and broadcast the transaction. Not being able to do that seems like a feature regression that should be fixed.

I think I'm now able to re-implement some checks manually, but I'd much prefer for BDK to provide some method to retrieve these details, notably before the transaction is signed/extracted/broadcasted/inserted into the TxGraph (as you should ~never sign anything you don't intend to publish, and in some scenarios (e.g., remote signing) signing might be costly).