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

Implement conversion for Lightning fee rate #678

Conversation

vladimirfomene
Copy link
Contributor

@vladimirfomene vladimirfomene commented Jul 21, 2022

This PR fixes #608.

Description

Lightning denotes transaction fee rate sats / 1000 weight units and sats / 1000 vbytes.
Here we add support for creating BDK FeeRate from lightning fee rate. We also move all FeeRate tests to
types.rs and rename as_sat_vb to as_sat_per_vb.

Notes to the reviewers

Matt was concerned that we might round down value in fee calculation in such a way that a transaction may not be relayed because it is below Bitcoin Core's min relay fee (1 sat/vbyte). I don't think we need to worry about that because we round up(ceil) during fee calculation, we don't round down. I will love to hear what you think. Is there something I'm missing? @johncantrell97, I will appreciate your review on this one.

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
  • I've updated CHANGELOG.md

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

Copy link
Member

@danielabrozzoni danielabrozzoni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK, this needs rebase

CHANGELOG.md Outdated Show resolved Hide resolved
src/types.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@csralvall csralvall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK

Copy link
Contributor

@rajarshimaitra rajarshimaitra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tACK 5c5efed

Needs another rebase..

Copy link
Contributor

@rajarshimaitra rajarshimaitra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tACK b75566d

Copy link
Member

@notmandatory notmandatory left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK b75566d

Copy link
Member

@danielabrozzoni danielabrozzoni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK b75566d

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
Lightning denotes transaction fee rate
sats / 1000 weight units and sats / 1000 vbytes.
Here we add support for creating BDK fee rate from
lightning fee rate. We also move all FeeRate test to
types.rs and rename as_sat_vb to as_sat_per_vb.
Copy link
Member

@danielabrozzoni danielabrozzoni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK de358f8

@danielabrozzoni danielabrozzoni merged commit 12507c7 into bitcoindevkit:master Aug 29, 2022
@afilini afilini mentioned this pull request Sep 8, 2022
21 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Status: Done
Development

Successfully merging this pull request may close these issues.

Support lighting's special feerates
6 participants