Conversation
Summary of ChangesHello @GeorgeTsagk, 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 improves the trustless asset swap functionality by allowing the asset seller to receive change from their asset input. By leveraging an ANYONE_CAN_SPEND script key for the swapped assets and specific PSBT signing flags, the new mechanism offers greater flexibility and reduces on-chain fees compared to previous "whole coin" transfer requirements, making asset swaps more practical and efficient. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe 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 by creating a comment using either
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 Limitations & Feedback Gemini Code Assist 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. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces an enhanced trustless swap mechanism that allows using "dirty" inputs by leveraging an OP_TRUE script key for the swapped assets. This is a great improvement as it adds flexibility for the swap creator. The implementation is well-tested with a new integration test that covers the end-to-end flow. The code is clear and follows the project's style guide. I have one suggestion to improve the readability and robustness of the test code by simplifying how outputs are identified.
| var changeVOutIdx, swapVOutIdx int = -1, -1 | ||
| for i, out := range bobVPsbt.Outputs { | ||
| if out.Asset != nil && out.Asset.SplitCommitmentRoot != nil { | ||
| changeVOutIdx = i | ||
| } else if out.Asset != nil && | ||
| len(out.Asset.PrevWitnesses) > 0 && | ||
| out.Asset.PrevWitnesses[0].SplitCommitment != nil { | ||
|
|
||
| swapVOutIdx = i | ||
| } | ||
| } |
There was a problem hiding this comment.
The logic to identify the change and swap outputs can be simplified. Instead of inspecting the asset structure (SplitCommitmentRoot or SplitCommitment), you can use the Type field of the virtual output, similar to how it's done for Alice's side in lines 2711-2721. This would make the code more readable and less dependent on the internal structure of assets. I've also added assertions to ensure both outputs are found.
var changeVOutIdx, swapVOutIdx int = -1, -1
for i, out := range bobVPsbt.Outputs {
if out.Type == tappsbt.TypeSplitRoot {
changeVOutIdx = i
} else {
swapVOutIdx = i
}
}
require.NotEqual(t.t, -1, changeVOutIdx, "change output not found")
require.NotEqual(t.t, -1, swapVOutIdx, "swap output not found")References
- Readable code is the most important requirement for any commit created. The suggested change improves readability by using a more direct and less implementation-dependent way to identify outputs. (link)
Pull Request Test Coverage Report for Build 21709218905Details
💛 - Coveralls |
2 participants
Description
A while ago we introduced vPSBT level sighashes which allowed us to build a basic trustless swap construct:
#804
One limitation there is that the seller needs to create a clean input for the assets, which doesn't carry any passive assets back to them (no change, needs to spend all).
This PR introduces an itest where we use an ANYONE_CAN_SPEND (OP_TRUE) script key for the assets, which is pre-defined by Alice (the seller) when preparing the swap offer.
Alice then creates two outputs on the asset level:
As far as the input goes, she can bring in any "dirty" input that may carry more than what is involved in the swap:
What matters for the swap to be complete is for Bob to complete the BTC level transaction validity by bringing an extra btc input, which makes Alice whole at Output@0.
sequenceDiagram box Alice's Offer participant IN as Input<br/>1000 units + 330 sats participant O0 as Output 0<br/>400u + 69,420 sats participant O1 as Output 1<br/>600u + 330 sats end IN->>O0: Alice's change (protected) Note over IN,O0: SIGHASH_SINGLE|ANYONE_CAN_PAY Note over O1: OP_TRUE<br/>Finalized by BobProblems
Alice locks additional asset balance in Input@0, which she might want to use before the swap is completed by someone. This approach reduces on-chain fees (no need to prepare an explicit input for the swap), but also limits the usability of the passive funds in that input. E.g if Alice wanted to then create another swap offer with some of the passive assets in that first offer, she'd be locked while waiting for someone to redeem the first one. From this perspective, the previous approach with "clean" asset inputs offers way greater flexibility for the swap offer creator.
Closes #813, #857