[wallet 2/3]: refactor funding logic to use allocation code #1407
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
guggero
changed the title
Pull Request Test Coverage Report for Build 13661304934Details
💛 - Coveralls |
guggero
force-pushed
the
allocation-refactor
branch
from
08b7805 to
941d26e
Compare
guggero
changed the title
guggero
force-pushed
the
allocation-refactor
branch
from
941d26e to
06ea429
Compare
guggero
force-pushed
the
group-key-refactor
branch
2 times, most recently
from
8c3d631 to
c28aacf
Compare
guggero
force-pushed
the
allocation-refactor
branch
from
06ea429 to
8939b42
Compare
guggero
force-pushed
the
group-key-refactor
branch
from
c28aacf to
f2b9b0f
Compare
guggero
force-pushed
the
allocation-refactor
branch
2 times, most recently
from
ee66eb9 to
86112a5
Compare
guggero
force-pushed
the
group-key-refactor
branch
from
f2b9b0f to
d416a6f
Compare
ffranr
reviewed
Mar 3, 2025
Roasbeef
reviewed
Mar 4, 2025
| @@ -328,7 +332,7 @@ func sortPiecesWithProofs(pieces []*piece) { | |||
| // asset outputs (asset UTXOs of different sizes from different tranches/asset | |||
| // IDs) according to the distribution rules provided as "allocations". | |||
| func DistributeCoins(inputs []*proof.Proof, allocations []*Allocation, | |||
| chainParams *address.ChainParams, | |||
| chainParams *address.ChainParams, interactive bool, | |||
There was a problem hiding this comment.
Still getting through the PR, but is the idea here that we'll start to use DistributeCoins more widely in the codebase? As otherwise right now, it's always used in an interactive setting.
There was a problem hiding this comment.
Yeah, exactly. We'll want to use the allocation code for the wallet's funding logic, which we'll use for the channel funding and in the future for the grouped asset on-chain send.
guggero
force-pushed
the
allocation-refactor
branch
from
86112a5 to
cdc7cb2
Compare
We'll generalize the allocation code in future commits so we can re-use it. We need to make sure we don't create circular package dependencies, so we move the code to a more appropriate location.
The version of the virtual packet has an influence on the commitment versions of the outputs created from it, so it's important that it can be configured in a more generic use of the allocation code.
This is a preparatory commit that refactors the piece allocation code a bit. This will make the actual changes in the next commit more easy to understand. This commit changes nothing in the behavior of the code.
The allocation code was previously only used for channel outputs, where all virtual transactions are always interactive. But we want to use the same code for non-interactive transfers (address based send logic) as well, which requires a couple of tweaks.
Since we're going to be using the allocation code for all send logic, we need to add all fields of the vOutput to the allocation, so the values will be applied correctly. Since we also add the explicit LockTime, we need to rename the CLTV field so it's more clear that this field is only used for the HTLC tiebreaking sort procedure. Finally, we add a new Validate method to the allocation that makes sure all required input fields are set.
guggero
force-pushed
the
allocation-refactor
branch
from
cdc7cb2 to
be499e2
Compare
Previously, we generated one allocation per channel asset. That was wrong and only worked because we never actually had multiple asset pieces in a channel. The idea of an allocation is that you only have one per on-chain output and that the different asset pieces are then distributed between the different outputs automatically. Because that means we need a way to give each virtual output of each asset piece a unique script key (where required), we need to turn the field into a functional type. In some places (e.g. the channel funding output) each vOutput of each different vPacket will have the same script key. In other places (e.g. cooperative close), we derived script keys per asset ID upfront so we need to use those for things to work correctly.
Because we'll need to convert a funding template that's potentially given to us over the RPC interface into an allocation, we need to add custom code for that. This will be used in the freighter to interpret the user-provided virtual packet into an allocation in the normal send logic.
With the new funding logic now potentially adding the change output first, we need to make sure the channel funding logic doesn't use hard-coded indexes.
We are now validating the input amount and types when creating the allocations and no longer need this function.
guggero
force-pushed
the
allocation-refactor
branch
from
be499e2 to
bb88417
Compare
Roasbeef
approved these changes
Mar 6, 2025
There was a problem hiding this comment.
LGTM 🐎
Reviewed 4 of 16 files at r4, 8 of 9 files at r8, 2 of 2 files at r9, 8 of 8 files at r10, all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @ffranr and @guggero)
tapchannel/aux_closer.go line 272 at r10 (raw file):
return accu + o.Amount.Val } localSum := fn.Reduce(commitState.LocalAssets.Val.Outputs, sumAmounts)
😎
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Refactors the wallet's funding code to use the pre-existing allocation logic to distribute coins from grouped assets.
Part of the group key support saga:
This change is