Skip to content

tapd: bump maxFeeRatio for funded psbts #1545

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 1 commit into
base: main
Choose a base branch
from

Conversation

GeorgeTsagk
Copy link
Member

No description provided.

@GeorgeTsagk GeorgeTsagk self-assigned this May 16, 2025
@coveralls
Copy link

coveralls commented May 16, 2025

Pull Request Test Coverage Report for Build 15071168916

Details

  • 0 of 4 (0.0%) changed or added relevant lines in 2 files are covered.
  • 50 unchanged lines in 9 files lost coverage.
  • Overall coverage decreased (-0.02%) to 36.78%

Changes Missing Coverage Covered Lines Changed/Added Lines %
rpcserver.go 0 1 0.0%
wallet_anchor.go 0 3 0.0%
Files with Coverage Reduction New Missed Lines %
address/address.go 2 67.47%
asset/group_key.go 2 57.89%
tappsbt/create.go 2 26.74%
asset/mock.go 3 65.3%
commitment/tap.go 4 71.82%
tapdb/universe.go 4 80.35%
tapgarden/caretaker.go 4 68.59%
tapchannel/aux_invoice_manager.go 6 80.51%
asset/asset.go 23 47.13%
Totals Coverage Status
Change from base Build 15055985108: -0.02%
Covered Lines: 26496
Relevant Lines: 72040

💛 - Coveralls

wallet_anchor.go Outdated
// carry relatively small bitcoin amounts, we want to bump the allowed ratio
// between fees paid and total produced output amount. This can prove useful in
// high fee environments where we'd otherwise fail to fund the psbt.
const psbtMaxFeeRatio = 0.75
Copy link
Member

Choose a reason for hiding this comment

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

What's the rationale for this specific value? Why not 1?

Copy link
Member Author

Choose a reason for hiding this comment

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

There isn't really any rationale other than it's quite greater than 0.2 (the default) and also not very close to 1.0

my rationale against 1.0 is that you probably want some leftovers to eventually move the assets again in the future, without bringing in more inputs

Nevertheless I'm fine with changing to 1.0, just wanted to dump some thoughts on it

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good to me. I think we should add that rationale to the comment.

And perhaps we should actually just make it configurable as suggested in #1524 as well? Then we can set the new, higher default value of 0.75 but also allow the user to bump it if it's not sufficient.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah makes sense to add it to this PR.

Added some comments on that issue, will pause this PR until we reach an agreement there.

@GeorgeTsagk GeorgeTsagk force-pushed the bump-psbt-maxfeeratio branch from 99bb86a to e9d48fa Compare May 16, 2025 14:46
@levmi levmi moved this from 🆕 New to 🏗 In progress in Taproot-Assets Project Board May 19, 2025
@lightninglabs-deploy
Copy link

@GeorgeTsagk, remember to re-request review from reviewers when ready

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

Successfully merging this pull request may close these issues.

4 participants