Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support getting verbose transactions #58

Closed

Conversation

TheCharlatan
Copy link
Contributor

This allows the caller to get extra information of a transaction, like the block hash of the block its included in and the number of
confirmations.

Some electrum servers do not support this method, which is reflected in the test of the method.

This allows the caller to get extra information of a transaction, like
the block hash of the block its included in and the number of
confirmations.

Some electrum servers do not support this method, which is reflected in
the test of the method.
@johncantrell97
Copy link

Anything holding this up? I would find this useful.

@tnull
Copy link
Contributor

tnull commented Aug 2, 2022

+1 for this. On the LDK side of things we might soon need this and/or find a workaround.

@notmandatory notmandatory added the enhancement New feature or request label Aug 2, 2022
@notmandatory
Copy link
Member

I just put this on the BDK roadmap so we can get some attention on it. Any others like this one you want to get some attention on let me know.

@afilini
Copy link
Member

afilini commented Aug 8, 2022

We recently merged #74 so that users can make raw calls without having to wait for us to implement them.

Now that we have that I'm not sure if it makes sense to merge methods that aren't supported by the all the main implementations, I think I would rather let people who know what they are doing implement this themselves in their own code.

@tnull
Copy link
Contributor

tnull commented Aug 8, 2022

We recently merged #74 so that users can make raw calls without having to wait for us to implement them.

Now that we have that I'm not sure if it makes sense to merge methods that aren't supported by the all the main implementations, I think I would rather let people who know what they are doing implement this themselves in their own code.

Hm, but transaction_get_verbose is just a regular call that an ElectrumClient should support? If you're talking about the BDK side of things I understand it's debatable what should or should not be part of the Blockchain interface, but IIUC there is currently no way to access the underlying client once once it's passed to Blockchain::from_client? So the users can't then implement their own logic on top of it?

@afilini
Copy link
Member

afilini commented Aug 8, 2022

Yeah my point was that apparently not all electrum implementations support this call, so maybe adding it to the client is a bit misleading because people would expect it to work everywhere. Maybe they test with just an implementation and it looks fine and then it breaks somewhere else. Since this is not an "ElectrumX" or "electrs" client specifically, we try to only implement stuff that work everywhere.

there is currently no way to access the underlying client once once it's passed to Blockchain::from_client

I think ElectrumClient should implement Deref<Target=Client>, if it doesn't I'll open a quick PR now

afilini added a commit to afilini/bdk that referenced this pull request Aug 8, 2022
As pointed out in bitcoindevkit/rust-electrum-client#58 (comment)
there was no way to keep using the client once it was given to BDK.
@TheCharlatan
Copy link
Contributor Author

We recently merged #74 so that users can make raw calls without having to wait for us to implement them.
Now that we have that I'm not sure if it makes sense to merge methods that aren't supported by the all the main implementations, I think I would rather let people who know what they are doing implement this themselves in their own code.

This is reasonable, I'm closing this PR again.

afilini added a commit to bitcoindevkit/bdk that referenced this pull request Aug 25, 2022
c5952dd Implement `Deref<Target=Client>` for `ElectrumBlockchain` (Alekos Filini)

Pull request description:

  ### Description

  As pointed out in bitcoindevkit/rust-electrum-client#58 (comment) there was no way to keep using the client once it was given to BDK.

  ### Checklists

  #### All Submissions:

  * [x] I've signed all my commits
  * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md)
  * [x] I ran `cargo fmt` and `cargo clippy` before committing

ACKs for top commit:
  rajarshimaitra:
    ACK c5952dd

Tree-SHA512: fbfbada51c9426266c8960da5508ee07b196808f0d670a09a51962bd6eda9ccf585e209f5b99b5ab78a3d17af774bdb3e33ef36ac4f4d1ce7f2c3398ae4f6d0c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Archived in project
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants