-
Notifications
You must be signed in to change notification settings - Fork 123
assets swap deposits [5/N]: add asset deposit manager #983
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 introduces a comprehensive client-side asset deposit management system, which is a crucial component for enabling asset swaps. It encompasses the core logic for creating, tracking, withdrawing, and spending Taproot Asset deposits, along with the necessary persistence layer and RPC interfaces for both the client and the swap server. This is a significant step towards enabling more complex asset-based financial operations within the system.
Highlights
- New Asset Deposit Module: A comprehensive
assets/deposit
package has been introduced, encapsulating the core logic for managing the lifecycle of asset deposits. This includes defining various deposit states (e.g., Initiated, Pending, Confirmed, Expired, Withdrawn, Spent, Swept), managing associated cryptographic keys and expiry conditions, and orchestrating state transitions. - Enhanced Taproot Assets (tapd) and LND Integration: The
TapdClient
has been significantly extended to interact withtapd
's asset wallet RPCs, enabling advanced functionalities such as funding and signing virtual PSBTs, publishing asset transactions, querying asset balances, deriving new keys, and importing/managing asset proofs. The deposit manager also deeply integrates with LND's wallet, signer, and chain notification services for Bitcoin-level operations. - Persistent Storage and Database Migrations: A new SQL database schema has been implemented to provide persistent storage for asset deposit information. This includes details like deposit ID, asset ID, amount, associated keys, confirmation height, and sweep addresses. Corresponding
sqlc
generated code and migration scripts (000015_asset_deposits.up.sql
,000015_asset_deposits.down.sql
) are included to manage the database structure. - Client and Server RPC Interfaces: New gRPC services and protobuf definitions have been added for both the client (
looprpc/client_asset_deposit.proto
) and the swap server (swapserverrpc/asset_deposit.proto
). These interfaces facilitate key operations such as creating new deposits, listing existing ones, initiating withdrawals, revealing keys for cooperative spending, and co-signing HTLCs for asset transfers. - Automated Deposit Lifecycle Management: The
Manager
component within theassets/deposit
package is responsible for automating the entire deposit lifecycle. It handles the initial funding, monitors for on-chain confirmations, manages expiry conditions by initiating timeout sweeps, and facilitates cooperative withdrawals or spending of assets into HTLCs, ensuring robust and reliable operation. - New CLI Commands for Asset Deposits: New command-line interface (CLI) commands have been added to the
loop
tool, providing users with direct interaction capabilities for the new asset deposit functionality. These commands includeloop asset deposits new
,list
,withdraw
,testkeyreveal
, andtestcosign
.
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. ↩
bfab03a
to
4d5f4f2
Compare
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 introduces a comprehensive asset deposit manager, which is a significant feature addition. The changes include the client-side manager, a database store, a client sub-server, and the asset deposit protocol. The implementation covers creating, withdrawing, and spending asset deposits.
My review has identified several critical and high-severity issues that should be addressed before merging. These include a potential panic in an event handling goroutine, incorrect business logic in API handlers, incomplete database migrations, and unhandled errors that could lead to panics. I've also pointed out some medium-severity issues like leftover debug code and misleading comments. Additionally, the go.mod
file contains local replace
directives that must be removed.
Overall, this is a substantial piece of work. Addressing the feedback will improve the robustness and maintainability of this new functionality.
select { | ||
case <-receiveEventsClient.Context().Done(): | ||
panic(receiveEventsClient.Context().Err()) | ||
default: | ||
} |
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.
The use of panic
here can bring down the entire application if the context is canceled. Instead of panicking, you should gracefully handle the context cancellation by sending the error on errChan
and exiting the goroutine. The Recv()
call on a gRPC stream is blocking and will return an error when the context is canceled, so you can rely on that error to detect context cancellation, making this select
block unnecessary.
case <-receiveEventsClient.Context().Done():
errChan <- receiveEventsClient.Context().Err()
return
if err != nil { | ||
log.Errorf("Unable to prepare and commit virtual psbt: %v", | ||
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.
The error from s.tapdClient.PrepareAndCommitVirtualPsbts
is logged but not returned. The function continues execution with a potentially nil
timeoutSweepBtcPkt
, which will likely cause a panic in subsequent operations. The error should be handled and returned immediately.
if err != nil { | |
log.Errorf("Unable to prepare and commit virtual psbt: %v", | |
err) | |
} | |
if err != nil { | |
log.Errorf("Unable to prepare and commit virtual psbt: %v", | |
err) | |
return nil, err | |
} |
default: | ||
return fmt.Errorf("unimplemented deposit state "+ | ||
"update: %v", d.State) | ||
} |
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.
The switch
statement in UpdateDeposit
only handles StateConfirmed
and StateExpired
. Other states like StateWithdrawn
, StateSwept
, and StateKeyRevealed
will fall into the default
case and return an "unimplemented" error. This will prevent these state transitions from being persisted and break the deposit lifecycle management.
You should add cases for all other valid state transitions or remove the default error to allow the state update to proceed for states that don't require special column updates.
if in.MinConfs < in.MaxConfs { | ||
return nil, status.Error(codes.InvalidArgument, | ||
"max_confs must be greater than or equal to min_confs") | ||
} |
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.
The validation logic for min_confs
and max_confs
is incorrect. The protobuf definition for ListAssetDepositsRequest
specifies that a max_confs
of 0 means no maximum. The current logic in.MinConfs < in.MaxConfs
is flawed because if max_confs
is 0, this will always be true for any min_confs > 0
, which is not the intended behavior.
The check should be if in.MaxConfs > 0 && in.MinConfs > in.MaxConfs
.
if in.MinConfs < in.MaxConfs { | |
return nil, status.Error(codes.InvalidArgument, | |
"max_confs must be greater than or equal to min_confs") | |
} | |
if in.MaxConfs > 0 && in.MinConfs > in.MaxConfs { |
go.mod
Outdated
replace github.com/btcsuite/btcwallet => github.com/bhandras/btcwallet v0.11.1-0.20250507171803-0de1c46b1cfc | ||
|
||
replace github.com/btcsuite/btcd => github.com/bhandras/btcd v0.22.0-beta.0.20250507171227-f18160c86e92 | ||
|
||
replace github.com/lightninglabs/loop/swapserverrpc => ./swapserverrpc | ||
|
||
replace github.com/lightninglabs/loop/looprpc => ./looprpc | ||
|
||
replace github.com/lightningnetwork/lnd => ../lnd | ||
|
||
replace github.com/lightningnetwork/lnd/sqldb => ../lnd/sqldb | ||
|
||
replace github.com/lightninglabs/lndclient => ../lndclient | ||
|
||
replace github.com/lightninglabs/taproot-assets => ../taproot-assets | ||
|
||
replace github.com/lightninglabs/taproot-assets/taprpc => ../taproot-assets/taprpc |
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 go.mod
file contains several replace
directives that point to local paths or forked repositories (e.g., ../lnd
, github.com/bhandras/btcwallet
). These are typically used for local development and must be removed before merging to ensure that CI and other developers use the correct upstream dependencies.
// handleDepositStateUpdate updates the deposit state in the store and | ||
// notifies all subscribers of the deposit state change. | ||
func (m *Manager) handleDepositExpired(ctx context.Context, d *Deposit) 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.
This comment seems to be a copy-paste from handleDepositStateUpdate
and is incorrect for this function. It should be updated to describe what handleDepositExpired
does.
// handleDepositStateUpdate updates the deposit state in the store and | |
// notifies all subscribers of the deposit state change. | |
func (m *Manager) handleDepositExpired(ctx context.Context, d *Deposit) error { | |
// handleDepositExpired is called when a deposit expires. It updates the | |
// deposit state and generates a sweep address. |
fmt.Printf("!!! client partial sig: %x\n", clientPartialSig) | ||
fmt.Printf("!!! client nonce: %x\n", session.PublicNonce) |
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.
|
||
// IsMatchingAddr checks if the given address is a matching deposit address for | ||
// the deposit kit. It checks that the address has the same internal key, script | ||
// key and sibling preimage as the deposit address. Note that this function does | ||
// not check the amount of the address. | ||
func (d *Kit) IsMatchingAddr(addr string) (bool, 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.
go func() { | ||
http.Handle("/", http.RedirectHandler( | ||
"/debug/pprof", http.StatusSeeOther, | ||
)) | ||
|
||
listenAddr := fmt.Sprintf(":%d", 4321) | ||
infof("Starting profile server at %s", listenAddr) | ||
fmt.Println(http.ListenAndServe(listenAddr, nil)) // nolint: gosec | ||
}() |
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.
The pprof server is started on a hardcoded port 4321
. This should be made configurable and ideally disabled by default in production environments for security reasons. Additionally, the error from http.ListenAndServe
is printed to stdout via fmt.Println
but is not otherwise handled. It should be logged using the logger for better visibility and consistency.
ORDER BY update_timestamp DESC | ||
LIMIT 1 | ||
) | ||
AND u.update_state IN (0, 1, 2, 3, 4, 5, 6); |
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.
The GetActiveAssetDeposits
query filters for states IN (0, 1, 2, 3, 4, 5, 6)
. This excludes StateKeyRevealed
(state 7). A deposit with a revealed key is still active while waiting for the server to spend it into an HTLC. It should likely be considered an active state. Please verify if this is the intended behavior. If StateKeyRevealed
should be considered active, it needs to be added to the list of states.
AND u.update_state IN (0, 1, 2, 3, 4, 5, 6, 7);
bbde3c7
to
39e7af6
Compare
39e7af6
to
385c962
Compare
With this commit we add high level helpers along with scripts to create asset HTLCs.
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.
This commit adds a high level deposit Kit type which includes the necessary functions to create and spend deposits to HTLC or through success, timeout or cooperative MuSig2 sweeps.
This commit adds protos for the loopd asset deposit subserver RPCs.
This commit adds a placeholder asset deposit subserver along with the corresponding CLI commands to the Loop daemon and CLI.
This commit adds a sweeper tailored for asset deposits, capable of sweeping a TAP deposit (a Kit) using either the timeout path or a revealed co-signer key. This lays the groundwork for implementing client-specific sweeping logic.
This commit extends the deposit package with the `Deposit` type and a bare-bones `Manager`, along with the structural definition of the `SQLStore` which together implement the basic structure, run loop and storage for the deposit manager. It is not yet functional and serves as a foundation for future commits that will gradually extend its functionality.
This commit extends the asset deposit manager along with the underlying sql store, adding functionality to create and fund new asset deposits.
This commit adds the necessary changes to the asset deposit manager, the underlying sql store and the asset deposit subserver to be able to list asset deposits.
With this commit, asset deposit expiry is checked on each new block. When a deposit expires, a timeout sweep is published. The deposit state is updated both when the sweep is first published and again upon its confirmation.
This commit adds deposit recovery to the deposit manager's startup process. All deposits that have not yet been spent by the server or swept by the client are recovered from the store.
This commit implements the necessary plumbing to enable deposit withdrawal. Withdrawing reveals the server's internal key for the deposit, allowing the client to sweep it independently.
This commit implements the full cooperative deposit withdrawal flow. The client first fetches keys for any pending withdrawals, then publishes sweep transactions using the revealed key to sign the deposit sweep. Once the sweep confirms, the deposit’s state is updated in the deposit store.
With this commit, the client can now reveal the keys of asset deposits to the server. This allows the server to sweep the deposits as needed, without requiring further interaction with the client.
This commit extends the deposit manager and sweeper to support generating a zero-fee HTLC transaction that spends a selected deposit. Once the HTLC is prepared, it is partially signed by the client and can be handed to the server as a safety measure before using the deposit in a trustless swap. This typically occurs after the client has accepted the swap payment. If the client becomes unresponsive during the swap process, the server can use the zero-fee HTLC as part of a recovery package.
385c962
to
24a9d0d
Compare
This PR implements the client side asset deposit manager, store, client subserver and the asset deposit protocol for creating, withdrawing and spending asset deposits.
Pull Request Checklist
release_notes.md
if your PR contains major features, breaking changes or bugfixes