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.

Sign up for GitHub

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

Jump to bottom

Option for Electrum and Esplora APIs to include floating TxOuts in updates for fee calculation #1265

Closed
evanlinjin opened this issue Jan 10, 2024 · 6 comments · Fixed by #1403
Closed

Option for Electrum and Esplora APIs to include floating TxOuts in updates for fee calculation #1265

evanlinjin opened this issue Jan 10, 2024 · 6 comments · Fixed by #1403
Assignees
@LagginTimes
Labels
module-blockchain new feature New feature or request
Milestone
1.0.0-alpha

Comments

@evanlinjin
Copy link
Member

Describe the enhancement

BDK does not store the transaction fee in a separate field. It needs to be calculated from the difference between prev outputs and outputs created.

For transactions that are received from an external wallet, we would not have prev txout data by default. It'll be handy to have an option to get prev txouts for these transactions and have them inserted as floating txouts in the TxGraph.

Use case

Some wallets would want to be able to calculate fees for all transactions that it includes.

Additional context
@evanlinjin evanlinjin added the new feature New feature or request label Jan 10, 2024
@LLFourn
Copy link
Contributor

LLFourn commented Jan 22, 2024

In esplora it should not be an option it just should be done. The information is already fetched it just isn't inserted. With electrum I guess you have to make it an option.

@evanlinjin
Copy link
Member Author

That's a good point @LLFourn. Should be straightforward for Esplora then.

@evanlinjin evanlinjin added this to BDK Jan 22, 2024
@evanlinjin evanlinjin moved this to Todo in BDK Jan 22, 2024
@evanlinjin evanlinjin added this to the 1.0.0-beta milestone Jan 22, 2024
@LagginTimes
Copy link
Contributor

I can take a look at this.

@nondiremanuel nondiremanuel moved this from Todo to In Progress in BDK Jan 30, 2024
@nondiremanuel nondiremanuel modified the milestones: 1.0.0-beta, 1.0.0 Jan 30, 2024
@LagginTimes LagginTimes mentioned this issue Jan 31, 2024
Merged
6 tasks
danielabrozzoni added a commit that referenced this issue Feb 5, 2024
@danielabrozzoni
Merge #1308: feat(esplora): include previous TxOuts for fee calcula…
7aca884 
…tion

552f11c feat(esplora): include previous `TxOut`s for fee calculation The previous `TxOut` for transactions received from an external wallet are added as floating `TxOut`s to `TxGraph` to allow for fee calculation. (Wei Chen)

Pull request description:

  ### Description

  Partially implements #1265.

  The previous `TxOut` for transactions received from an external wallet are added as floating `TxOut`s to `TxGraph` to allow for fee calculation.

  ### Notes to the reviewers

  Currently only the `esplora` portion of #1265 has been implemented.
  The `electrum` portion will potentially be done in a new PR, as discussed on the 1/30/24 Lib call.

  ### Checklists

  #### To Do:
  * [ ] Implement `electrum` portion of #1265.

  #### 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

  #### New Features:

  * [x] I've added tests for the new feature
  * [x] I've added docs for the new feature

ACKs for top commit:
  evanlinjin:
    re-ACK 552f11c
  danielabrozzoni:
    ACK 552f11c

Tree-SHA512: 752a24ebd0b9ad7952c1b093ecb251473e346c77b860c1a80c73418130189227405a0f0d7652967cf8c7b89994e8c37df96cd52b52b6daff9cc8c88b5194069a
@adamjonas adamjonas mentioned this issue Mar 14, 2024
Open
6 tasks
@notmandatory notmandatory removed this from the 1.0.0-alpha milestone Mar 18, 2024
@ValuedMammal
Copy link
Contributor

possibly related #1419

@notmandatory notmandatory added this to the 1.0.0-alpha milestone May 2, 2024
@ValuedMammal ValuedMammal mentioned this issue May 7, 2024
Merged
5 tasks
@evanlinjin
Copy link
Member Author

@LagginTimes will also implement this for Electrum.

@LagginTimes
Copy link
Contributor

The electrum portion of this ticket has been addressed in #1403.

@notmandatory notmandatory moved this from In Progress to Needs Review in BDK May 10, 2024
@github-project-automation github-project-automation bot moved this from Needs Review to Done in BDK May 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@LagginTimes LagginTimes

Labels
module-blockchain new feature New feature or request
Projects
BDK
Archived in project
Milestone
1.0.0-alpha
Development

Successfully merging a pull request may close this issue.

Update bdk_electrum crate to use sync/full-scan structs LagginTimes/bdk
6 participants
@LLFourn @notmandatory @evanlinjin @nondiremanuel @ValuedMammal @LagginTimes