Skip to content

Add basic fee estimation to on-chain wallet #585

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

benthecarman
Copy link

Related to #578

This adds basic fee estimation to the on-chain wallet. To not just completely copy-paste everything from send_to_address I refactored out a lot of the logic into its own function that send_to_address now uses. Then exposed the functionality and added it to tests.

@ldk-reviews-bot
Copy link

ldk-reviews-bot commented Jun 20, 2025

👋 Thanks for assigning @tnull as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

@ldk-reviews-bot ldk-reviews-bot requested a review from jkczyz June 20, 2025 20:38
@benthecarman benthecarman force-pushed the on-chain-fee-estimation branch from 2c1a567 to ef56520 Compare June 20, 2025 21:45
@tnull tnull requested review from tnull and removed request for jkczyz June 22, 2025 16:50
/// a reasonable estimate from the configured chain source.
///
/// [`BalanceDetails::total_anchor_channels_reserve_sats`]: crate::BalanceDetails::total_anchor_channels_reserve_sats
pub fn estimate_send_to_address(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, as discussed elsewhere, this is kind of an odd API as it is inherently TOCTOU race-y (based on fee rate updates, but also wallet state changes), and we don't offer any API that would allow to set exactly that net. fee returned here. I think generally the 'staging' approach as described over at #578 would be preferable, but that of course also interacts with UTXO locking, etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants