feat: esplora client get_tx

[reez]

Description

Adds get_tx to Esplora Client

Notes to the reviewers

Jurvis just added this method to pre-1.0 (thank you!), so I'm adding to 1.0; while similar to pre-1.0 it is not exactly the same but I tried to match the pre-1.0 functions naming conventions.

I was also going to add this method for Electrum but since that is slightly more different I decided to spin it out into a separate upcoming PR.

Changelog notice

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

[thunderbiscuit]

Just one small question to make sure I understand, but this looks good to go.

[thunderbiscuit]

Do I understand correctly that this is sort of remapping the error you might get from the Txid::from_str() into an EsploraError::Parsing?

I do think it probably makes sense here because otherwise you'd need to combine both the Txid parsing error and the Esplora error into one, which gets messy when there are multiple of those combinations across the codebase.

[thunderbiscuit]

Other observation: with the introduction of the bitcoin-ffi type Txid and the impl_string_custom_typedef macro, can that translation into the Rust Txid type happen automatically? It's a String on the target language side but becomes a true Rust Txid when you feed it to a Rust function? It's my first time using the TypeDef in practice so not 100% sure... but maybe?

[reez]

Do I understand correctly that this is sort of remapping the error you might get from the Txid::from_str() into an EsploraError::Parsing?

I do think it probably makes sense here because otherwise you'd need to combine both the Txid parsing error and the Esplora error into one, which gets messy when there are multiple of those combinations across the codebase.

yup that's correct.

Needing to remap the error that we get back from parsing which is different than the EsploraError type that we return from the function.

Looking at the Txid docs for from_str it seemed like the EsploraError::Parsing would make the most sense.

But yeah I played around with a few things, and what you mention about combining seemed a bit messy to me too.

The one thing I did think about doing was creating a conversion for it in error.rs like:

impl From<BdkHexToArrayError> for EsploraError {
    fn from(error: BdkHexToArrayError) -> Self {
        EsploraError::Parsing {
            error_message: format!("Invalid txid: {}", error),
        }
    }
}

but bitcoin::hashes::hex::HexToArrayError would need to be added as a dependency so I didn't think it was worth adding a new dependency based on one error to be able to clean up

     let txid = Txid::from_str(txid.as_str()).map_err(|e| EsploraError::Parsing {
            error_message: format!("Invalid txid: {}", e),
        })?;

to something like let txid = Txid::from_str(&txid)?;.

but as I'm writing this it's dawning on me that bdk_wallet probably exports that dependency so I dont need to explicitly add it which solves my initial hesitation to add the conversion of the error in error.rs.... so look out for a push with that change right now to make things look even nicer.

[reez]

Other observation: with the introduction of the bitcoin-ffi type Txid and the impl_string_custom_typedef macro, can that translation into the Rust Txid type happen automatically? It's a String on the target language side but becomes a true Rust Txid when you feed it to a Rust function? It's my first time using the TypeDef in practice so not 100% sure... but maybe?

From my understanding that impl_string_custom_typedef macro only helps with conversions at the FFI boundary not within Rust functions.

[reez]

followed up with 2 responses, a small code refactor, and rebased on master

[thunderbiscuit]

This might solve both your issues in one go. I tried it locally and everything compiles well. I think the neat part is that because you pass in a true Txid type and not a String, your parsing error will happen prior to you passing the Txid to the method (on the construction of the Txid type in theory, but now that I think of it we don't have a constructor for it... so where does the error get thrown exactly if you provide a malformed txid string?).

Anyway will test a bit more, but your Result is cleaner because it doesn't need to wrap two errors.

pub fn get_tx(&self, txid: Txid) -> Result<Option<Arc<Transaction>>, EsploraError> {
    let tx_opt = self.0.get_tx(&txid)?;
    Ok(tx_opt.map(|inner| Arc::new(Transaction::from(inner))))
}

[thunderbiscuit]

Ok after testing locally it works super well, but surfaces another potential issue we should discuss on bitcoin-ffi: if your Txid is malformed, you get a panic. So that crashes your app at runtime rather than return an error... not so good.

This error is uncatchable as far as I can tell, because it's never thrown.

val esploraClient: EsploraClient = EsploraClient(SIGNET_ESPLORA_URL)
try {
    val txid: Txid = "9d0a83df4a556d61338ff18cbc20bf49a3c732805e404a66e25e1579271597a"
    val transaction: Transaction? = esploraClient.getTx(txid)
    println("Transaction: ${transaction?.computeTxid()}, ${transaction?.isExplicitlyRbf()}")
} catch (t: Throwable) {
    println("Panic: $t")
}

[reez]

This might solve both your issues in one go. I tried it locally and everything compiles well. I think the neat part is that because you pass in a true Txid type and not a String, your parsing error will happen prior to you passing the Txid to the method (on the construction of the Txid type in theory, but now that I think of it we don't have a constructor for it... so where does the error get thrown exactly if you provide a malformed txid string?).

Anyway will test a bit more, but your Result is cleaner because it doesn't need to wrap two errors.

pub fn get_tx(&self, txid: Txid) -> Result<Option<Arc<Transaction>>, EsploraError> {
    let tx_opt = self.0.get_tx(&txid)?;
    Ok(tx_opt.map(|inner| Arc::new(Transaction::from(inner))))
}

Definitely tried changing the function signature to that exact thing while messing around with the best shape of this, so happy to expand this convo to potentially changing the function signature, but I opted to not at the time because it felt like changing to using a true Txid type instead of a String was something that might have other implications (you mention where error might get thrown if malformed txid string) and/or other spots in the codebase we might want to make the same change (BumpFeeTxBuilder?). So I kind of stopped myself moving forward in that direction, but I'm cool with either direction you want to take this, and am glad you are thinking about some similar things (and also things I may not have thought of)!

[thunderbiscuit]

Ok so I opened an issue relating our findings here: bitcoindevkit/bitcoin-ffi#20

[thunderbiscuit]

Small import change but otherwise looks good.

[thunderbiscuit]

ACK 87fd113. Feel free to merge, I'll rebase #584.