Skip to content

tapd: allow skipping the funding step in CommitVirtualPsbts #1563

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

Merged

Conversation

bhandras
Copy link
Member

In order to be able to construct zero fee transactions (which do not need funding inputs) we want to allow CommitVirtualPsbts to skip the funding step altogether.

@coveralls
Copy link

coveralls commented May 23, 2025

Pull Request Test Coverage Report for Build 15272152109

Details

  • 61 of 76 (80.26%) changed or added relevant lines in 2 files are covered.
  • 35 unchanged lines in 6 files lost coverage.
  • Overall coverage increased (+18.6%) to 55.795%

Changes Missing Coverage Covered Lines Changed/Added Lines %
taprpc/assetwalletrpc/assetwallet.pb.go 0 5 0.0%
rpcserver.go 61 71 85.92%
Files with Coverage Reduction New Missed Lines %
asset/asset.go 2 79.49%
tapchannel/aux_leaf_signer.go 2 43.08%
commitment/tap.go 3 84.55%
asset/mock.go 6 71.16%
tapdb/multiverse.go 6 82.23%
address/mock.go 16 88.24%
Totals Coverage Status
Change from base Build 15255103485: 18.6%
Covered Lines: 52736
Relevant Lines: 94518

💛 - Coveralls

@guggero guggero self-requested a review May 26, 2025 07:18
Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

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

LGTM, just need to address the linter complaints.

@bhandras bhandras force-pushed the commitvirtualpsbt-skip-funding branch from 56478dc to 91f5663 Compare May 27, 2025 09:51
@bhandras bhandras requested a review from GeorgeTsagk May 27, 2025 09:59
Copy link
Member

@GeorgeTsagk GeorgeTsagk left a comment

Choose a reason for hiding this comment

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

LGTM, would like some test coverage but since it's involved I think it's OK to merge as is, given that it's a noop if you ignore the flag

@GeorgeTsagk GeorgeTsagk enabled auto-merge May 27, 2025 15:06
@GeorgeTsagk GeorgeTsagk added this pull request to the merge queue May 27, 2025
Merged via the queue into lightninglabs:main with commit 608bed8 May 27, 2025
35 of 36 checks passed
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.

4 participants