-
Notifications
You must be signed in to change notification settings - Fork 123
assets swap deposits [2/N]: extend the loop tapd client with funding, signing, and swap support utilities #980
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
base: master
Are you sure you want to change the base?
Conversation
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.
Summary of Changes
Hello @bhandras, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request significantly expands the loop
tapd
client's capabilities, particularly in the realm of asset swaps and deposits. It introduces a suite of new methods for advanced transaction construction, including funding and signing virtual PSBTs, managing asset proofs, and monitoring asset transfer events. Furthermore, it lays the groundwork for atomic swaps by adding dedicated support for Taproot Asset HTLCs and general utilities for sweeping assets.
Highlights
- Extended
tapd
Client Functionality: TheTapdClient
has been significantly extended with new methods for funding, signing, and publishing virtual PSBTs, which are crucial for constructing and broadcasting Taproot Asset transactions. This includesFundAndSignVpacket
,PrepareAndCommitVirtualPsbts
, andLogAndPublish
. - Asset Balance Management and Key Derivation: New utilities have been added to query asset balances, specifically
GetAssetBalance
andGetUnEncumberedAssetBalance
, providing detailed insight into spendable assets. The client also gainsDeriveNewKeys
for generating new internal and script keys for asset management. - Proof Handling and Import: The
TapdClient
now supports importing and managing asset proofs throughImportProof
andImportProofFile
, which is essential for verifying asset ownership and maintaining a complete asset history within thetapd
database. - Asset Event Monitoring: Capabilities to subscribe to and monitor asset send and receive events have been introduced with
WaitForReceiveComplete
andWaitForSendComplete
, enabling real-time tracking and automation of asset transfers. - Taproot Asset HTLC Support: New packages (
assets/htlc
) have been added to provide comprehensive support for constructing and managing Taproot Asset Hash Time-Locked Contracts (HTLCs). This includes generating success/timeout scripts, computing aggregate keys, creating virtual PSBTs for HTLCs, and generating Bitcoin control blocks for HTLC paths. OP_TRUE
Sweep Utilities: A new utility function,CreateOpTrueSweepVpkt
, has been introduced to create virtual PSBTs specifically for sweeping assets that are secured by a simpleOP_TRUE
Tapscript, simplifying certain asset recovery or transfer scenarios.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command>
or @gemini-code-assist <command>
. Below is a summary of the supported commands.
Feature | Command | Description |
---|---|---|
Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/
folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
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.
Code Review
This pull request adds significant functionality to the tapd
client for asset swaps. It introduces methods for funding, signing, and managing virtual PSBTs, handling proofs, and watching for send/receive events. New packages htlc
and tapkit
provide utilities for creating HTLC scripts and sweeping assets. The changes are extensive and well-structured. My review focuses on improving robustness, maintainability, and consistency. Key areas for improvement include handling of goroutine lifecycle, removing duplicated code, and addressing TODO
comments.
case <-receiveEventsClient.Context().Done(): | ||
panic(receiveEventsClient.Context().Err()) |
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.
Panicking within a goroutine will crash the entire application. The context being done is a normal part of the lifecycle (e.g., shutdown) and should be handled gracefully. Instead of panicking, the error should be sent to the errChan
and the goroutine should exit.
case <-receiveEventsClient.Context().Done(): | |
panic(receiveEventsClient.Context().Err()) | |
case <-receiveEventsClient.Context().Done(): | |
errChan <- receiveEventsClient.Context().Err() | |
return |
errChan := make(chan error, 1) | ||
|
||
go func() { | ||
for { |
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.
This goroutine's loop is missing a check for context cancellation. Unlike WaitForReceiveComplete
, it doesn't handle the case where the client context is closed. This could lead to a goroutine leak if the context is cancelled while the Recv()
call is blocking.
Please add a select
block to check for sendEventsClient.Context().Done()
similar to the one in WaitForReceiveComplete
to ensure graceful shutdown of the goroutine.
|
||
// GenTaprootAssetRootFromProof generates the taproot asset root from the proof | ||
// of the swap. | ||
func GenTaprootAssetRootFromProof(proof *proof.Proof) ([]byte, error) { |
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.
|
||
// GenTaprootAssetRootFromProof generates the taproot asset root from the proof | ||
// of the swap. | ||
func GenTaprootAssetRootFromProof(proof *proof.Proof) ([]byte, error) { |
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.
// TODO(bhandras): set this to the actual internal key derived | ||
// from the sender node, otherwise they'll lose the 1000 sats | ||
// of the tombstone output. |
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.
assets/client.go
Outdated
func handleReceiveEvent(event *taprpc.ReceiveEvent, | ||
resChan chan<- TapReceiveEvent) (bool, error) { | ||
|
||
fmt.Printf("!!! Received event: %v\n", event.Status) |
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.
This fmt.Printf
appears to be a debug statement. It should be removed or replaced with a proper logger call (e.g., using a log
package) before merging to avoid polluting production logs.
fmt.Printf("!!! Received event: %v\n", event.Status) | |
// fmt.Printf("!!! Received event: %v\n", event.Status) |
asset.NUMSPubKey, rootHash[:], | ||
) | ||
|
||
merkleRootHash := tree.RootNode.TapHash() |
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.
RawKey: keychain.KeyDescriptor{ | ||
PubKey: asset.NUMSPubKey, | ||
}, | ||
Tweak: merkleRootHash[:], |
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.
assets/htlc/swapkit.go
Outdated
ScriptKey: asset.NUMSScriptKey, | ||
}) | ||
pkt.Outputs = append(pkt.Outputs, &tappsbt.VOutput{ | ||
// todo(sputn1ck) assetversion |
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.
/* | ||
addressRecvVpkt, err := tappsbt.FromAddresses([]*address.Tap{addr}, 0) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
sweepVpkt.Outputs = addressRecvVpkt.Outputs | ||
*/ |
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.
e9141c9
to
d1e742b
Compare
With this commit we add high level helpers along with scripts to create asset HTLCs.
f3b944d
to
8970466
Compare
This commit enables package relayed HTLCs by making the CSV check in the success path optional.
This commit adds additional scaffolding to our tapd client, along with new high-level helpers in the assets package, which will be used later for swaps and deposits.
8970466
to
1bfa780
Compare
This commit adds methods to the tapd client for preparing, funding, signing, and publishing virtual PSBTs, managing asset balances, handling proofs, and watching for asset send/receive completion events. Also introduces utility methods for creating OP_TRUE sweep vpackets from asset proofs.
Build on top of: #979
Pull Request Checklist
release_notes.md
if your PR contains major features, breaking changes or bugfixes