-
Notifications
You must be signed in to change notification settings - Fork 123
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
[preparation 1/2]: refactor to simplify send logic, prepare for new RPCs #799
Merged
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
Closed
guggero
force-pushed
the
prepare-refactor
branch
from
February 16, 2024 17:49
1f1fa26
to
9aead7d
Compare
guggero
changed the title
[preparation]: refactor to simplify send logic, prepare for new RPCs
[preparation 1/2]: refactor to simplify send logic, prepare for new RPCs
Feb 19, 2024
guggero
force-pushed
the
prepare-refactor
branch
from
February 19, 2024 16:03
9aead7d
to
938f6ed
Compare
guggero
force-pushed
the
prepare-refactor
branch
2 times, most recently
from
February 19, 2024 18:02
911ca66
to
89a88f5
Compare
Roasbeef
approved these changes
Feb 20, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🦄
We have the following weird package dependency chain: proof -> vm -> tapscript -> tappsbt This makes things weird and cumbersome. The only parts in tapscript that actually use the tappsbt stuff is send related. So to break that dependency chain between the vm and tappsbt, we split off all the send functionality into a new tapsend package.
We don't want to have a dependency from the proof package to the tappsbt package as that makes it impossible for us to have a proof within a virtual package. To resolve that potential circular dependency, we split the code of the ownership proving code into two pieces.
Now that we've resolved the circular package dependency between the proof and tappsbt packages, we can finally use the *proof.Proof type directly for the packet's input proof (and later the output's proof suffix).
We rename the variable to match the naming style of the other ones.
When interactively signing a transfer that goes to a TAP address, the participants might want to be able to check the proof courier delivery URL that will be used to transfer the proof. This information currently gets lost when converting a TAP address into a vPSBT and is transported in a different data structure inside of the freighter. For a flow that doesn't use the freighter (or just in the very end), there currently is no way to specify the proof courier address, which this commit now changes. A follow-up commit will refactor the freighter to use this new field.
We need a way to transport proofs that are complete in terms of MS-SMT proofs committing to the correct asset but incomplete in terms of the asset witness (in the ASSET_VERSION_V1 case). The virtual transaction output is as good a place as any other. This will also allow us to simplify some of the data structures around passive assets, as currently we need to carry those proofs around in a separate map.
In order to be able to pass the dummy outputs to the lnd FundPsbt call we need to make them look more like actual P2TR outputs.
We'll want to create proof suffixes outside of the chain porter, so we export the function and make it more generic so it can work with any number of related virtual transactions.
Since we now have a field to store the proof courier delivery address URL in within the VOutput, we no longer need to carry along extra state in the freighter.
This commit refactors the freighter in that we use the VPacket struct directly for the passive assets. We now have all information available directly in that struct and no longer need another wrapper. With that looking more similar to other code, we can then also re-use the updateAssetProofFile method which gets rid of some duplicated code.
guggero
force-pushed
the
prepare-refactor
branch
from
February 20, 2024 15:42
89a88f5
to
a2a95e7
Compare
GeorgeTsagk
approved these changes
Feb 20, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM + tACK ⚡
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.
This is a preparatory PR that does some of the refactoring required for #787. We also add two new fields to the vPSBT outputs:
ProofSuffix
andProofDeliveryAddress
which will be required to ship this missing information between different participants in a fully RPC driven send.