assets swap deposits [3/N]: extend swapserverrpc with asset deposit RPCs #981
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 extends the swapserverrpc with new RPCs to support asset deposits, laying the groundwork for asset-based swaps. It introduces comprehensive client-side utilities for managing Taproot Assets, including the creation and handling of HTLCs for atomic swaps, and general-purpose asset transaction preparation and execution.
Highlights
- New Asset Deposit RPCs: A new
AssetDepositServicehas been introduced inswapserverrpc, defining RPCs for initiating, managing, and completing asset deposits. This includesNewAssetDepositto create a deposit address,WithdrawAssetDepositsto retrieve asset keys, andPushAssetDepositHtlcSigs/PushAssetDepositKeysfor signature and key exchange in HTLC-based asset transfers. - Enhanced Taproot Assets Client Capabilities: The
TapdClientinassets/client.gohas been significantly extended to support new asset-related operations. This includes functions for funding and signing virtual PSBTs (FundAndSignVpacket), preparing and committing asset transactions (PrepareAndCommitVirtualPsbts), logging and publishing transfers (LogAndPublish), managing asset balances, deriving new asset keys, and importing asset proofs. It also adds new event subscription mechanisms for asset receives and sends. - HTLC Scripting and SwapKit for Assets: A new
assets/htlcpackage has been added, providing core components for constructing and managing Hashed Timelock Contracts (HTLCs) specifically for Taproot Asset swaps. This includes functions for generating Bitcoin scripts for HTLC success and timeout paths, computing aggregate keys, creating virtual PSBTs for asset HTLCs, and generating Bitcoin control blocks for HTLC spending paths. - General Taproot Asset Utilities: A new
assets/tapkit.gofile introduces general utility functions for Taproot Assets, such asCreateOpTrueSweepVpktfor creating virtual PSBTs to sweep assets withOP_TRUETapscripts, which is a common pattern for receiving assets into a wallet.
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.
Code Review
This pull request introduces significant new functionality for asset swaps and deposits, including new RPCs, a new htlc package, and extensions to the TapdClient. The changes are extensive. My review focuses on correctness, security, and maintainability. I've found some critical issues, such as a potential panic on context cancellation and a TODO indicating risk of fund loss. There are also several high-severity issues related to unhandled errors, potential panics from slice out-of-bounds, a goroutine leak, and a security concern with sending private keys over RPC. I've also included several medium-severity comments on maintainability and performance.
| for { | ||
| select { | ||
| case <-receiveEventsClient.Context().Done(): | ||
| panic(receiveEventsClient.Context().Err()) |
There was a problem hiding this comment.
A library function should not panic, especially on a context cancellation which is a normal way to signal termination. Panicking will crash the entire application. The goroutine should gracefully exit by sending the context error on the error channel.
errChan <- receiveEventsClient.Context().Err()
return| // 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.
This TODO comment indicates a risk of fund loss. This is a critical issue that should be addressed before merging. Using a NUMS key for the tombstone output means the funds sent to it will be unspendable.
| assetTxOut := &wire.TxOut{ | ||
| PkScript: sweepBtcPacket.Inputs[0].WitnessUtxo.PkScript, | ||
| Value: sweepBtcPacket.Inputs[0].WitnessUtxo.Value, | ||
| } | ||
| feeTxOut := &wire.TxOut{ | ||
| PkScript: sweepBtcPacket.Inputs[1].WitnessUtxo.PkScript, | ||
| Value: sweepBtcPacket.Inputs[1].WitnessUtxo.Value, | ||
| } |
There was a problem hiding this comment.
This code assumes that sweepBtcPacket.Inputs has at least two elements. Accessing sweepBtcPacket.Inputs[1] will cause a panic if there is only one input. It's safer to check the length of the slice before accessing elements by index. For example: if len(sweepBtcPacket.Inputs) < 2 { ... }.
assets/client.go
Outdated
| if changeKeyDesc != nil { | ||
| err = addP2WPKHOutputToPsbt( | ||
| btcPkt, *changeKeyDesc, btcutil.Amount(1), params, | ||
| ) | ||
| commitRequest.AnchorChangeOutput = | ||
| &assetwalletrpc.CommitVirtualPsbtsRequest_ExistingOutputIndex{ //nolint:lll | ||
| ExistingOutputIndex: 1, | ||
| } | ||
| } else { | ||
| commitRequest.AnchorChangeOutput = | ||
| &assetwalletrpc.CommitVirtualPsbtsRequest_Add{ | ||
| Add: true, | ||
| } | ||
| } |
There was a problem hiding this comment.
The else block is redundant. The commitRequest.AnchorChangeOutput field is already initialized with &assetwalletrpc.CommitVirtualPsbtsRequest_Add{Add: true} on line 438. You can remove this else block to simplify the code. Also, the error returned by addP2WPKHOutputToPsbt on line 453 is not being handled, which could lead to silent failures.
if changeKeyDesc != nil {
err = addP2WPKHOutputToPsbt(
btcPkt, *changeKeyDesc, btcutil.Amount(1), params,
)
if err != nil {
return nil, nil, nil, nil, err
}
commitRequest.AnchorChangeOutput =
&assetwalletrpc.CommitVirtualPsbtsRequest_ExistingOutputIndex{
ExistingOutputIndex: 1,
}
}| // PushAssetDepositKeysReq holds private keys of one or more deposits. | ||
| message PushAssetDepositKeysReq { | ||
| // deposit_keys is a map wich maps deposit_id to deposit internal private | ||
| // key. | ||
| map<string, bytes> deposit_keys = 1; | ||
| } |
There was a problem hiding this comment.
Sending private keys over an RPC interface is a significant security risk. Even if the connection is encrypted, it exposes sensitive material that should ideally never leave the client. Consider refactoring the protocol to send signatures instead of private keys. The client should perform the signing operation and send the resulting signature to the server.
Also, there is a typo in the comment: "wich" should be "which".
// PushAssetDepositKeysReq holds private keys of one or more deposits.
message PushAssetDepositKeysReq {
// deposit_keys is a map which maps deposit_id to deposit internal private
// key.
map<string, bytes> deposit_keys = 1;
}
|
|
||
| // 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.
This function GenTaprootAssetRootFromProof seems to be a duplicate of the one defined in assets/htlc/swapkit.go. To avoid code duplication, consider moving this function to a shared utility package or choosing one of the two implementations to be the canonical one and call it from the other location.
| /* | ||
| 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.
This block of code is commented out. It should be removed if it's no longer needed to keep the codebase clean.
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.
This appears to be a debug Printf statement. Please remove it before merging.
| // one ore more deposits to a zero fee HTLC. | ||
| rpc PushAssetDepositHtlcSigs (PushAssetDepositHtlcSigsReq) | ||
| returns (PushAssetDepositHtlcSigsRes); | ||
|
|
||
| // PushKeys pushes (ie reveals) the private keys of one ore more deposits to | ||
| // the server. |
There was a problem hiding this comment.
There are a couple of typos in the comments for this service:
- Line 23: "one ore more" -> "one or more"
- Line 27: "one ore more" -> "one or more"
These should be corrected for clarity in the generated documentation.
// one or more deposits to a zero fee HTLC.
rpc PushAssetDepositHtlcSigs (PushAssetDepositHtlcSigsReq)
returns (PushAssetDepositHtlcSigsRes);
// PushKeys pushes (ie reveals) the private keys of one or more deposits to
// the server.
| func (t *TapdClient) GetUnEncumberedAssetBalance(ctx context.Context, | ||
| assetID []byte) (uint64, error) { |
There was a problem hiding this comment.
This function iterates over all assets by calling ListAssets without a filter, and then filters them in memory. This can be inefficient if the node holds a large number of assets. If the ListAssets RPC supports filtering by asset ID, it would be more efficient to use that. If not, this is the best that can be done, but it's worth noting the potential performance issue.
bhandras
force-pushed
the
asset-swap-deposits-3
branch
2 times, most recently
from
25ab30b to
a5c7d14
Compare
With this commit we add high level helpers along with scripts to create asset HTLCs.
bhandras
force-pushed
the
asset-swap-deposits-3
branch
2 times, most recently
from
4a07fd8 to
e0abd45
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.
bhandras
force-pushed
the
asset-swap-deposits-3
branch
from
e0abd45 to
0fe5327
Compare
Currently on top of: #980 (can be separated).
Pull Request Checklist
release_notes.mdif your PR contains major features, breaking changes or bugfixes