-
Notifications
You must be signed in to change notification settings - Fork 19
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
Chain swaps #51
Chain swaps #51
Conversation
Massive! Thanks @dangeross ! Will review over the weekend. |
@dangeross couldnt' find tests for this. Attempting to put one together now. |
Thanks @i5hi, let me know if I can help at all. It would be cool if @michael1011 could confirm the boltz states used in the chain swaps. |
@dangeross When creating a ChainSwap |
Additionally, its a bit tricky for the user of the library to know who part of the
There is a claim and lockup - |
The lockup side is BtcSwapScript, the claim side is LBtcSwapScript |
Could work, maybe also having two wrapper functions, one for claim and one for lockup. E.g. |
If it helps @i5hi, here is our current usage. I think having 2 functions is a better approach, then passing the |
@dangeross The sign_claim methods now use a new cooperative
The updated tests in liquid/bitcoin_v2.rs have been updated as follows:
This would fail as I went through the breez code base to understand how to use this properly. Can you confirm that this would be correct usage?
|
I've noticed this actually won't be an issue with LN swaps
So i will not change the the LN swap tests. Let me know if the previous comment is the correct flow for chain swaps. |
@i5hi the |
And the |
And when refunding they would be from the claim_tx? |
Yes
They are only needed for the claim flow |
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.
Great stuff! Nice to see that you managed to make it work even with our lackluster documentation (coming soon 🤞)
Left some minor comments. Some test cases would be nice too
@@ -888,8 +1015,9 @@ impl BtcSwapTxV2 { | |||
/// Multiply the size by the fee_rate to get the absolute fees. | |||
pub fn size(&self, keys: &Keypair, preimage: &Preimage) -> Result<usize, Error> { | |||
let dummy_abs_fee = 5_000; | |||
// Can only calculate non-coperative claims |
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.
In case there is a need for calculating those, schnorr signatures are always 64 bytes. Could be stubbed
src/swaps/boltz.rs
Outdated
"transaction.server.confirmed".to_string() | ||
} | ||
ChainSwapStates::TransactionClaimed => "transaction.claimed".to_string(), | ||
ChainSwapStates::TransactionClaimPending => "transaction.claim.pending".to_string(), |
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.
transaction.claim.pending
is for submarine swaps, when we paid the invoice but wait before broadcasting the claim transaction in case you happen to come online to do a key path spend with us
In chain swaps you either:
- key path spend with us and both claim transactions, ours and yours, are broadcasted
- you enforce via script path and we do the same thing
So, that status doesn't need to be here
SwapType::Chain => { | ||
boltz_client | ||
.get_chain_txs(swap_id)? | ||
.user_lock | ||
.ok_or(Error::Protocol( | ||
"No user_lock transaction for Chain Swap available".to_string(), | ||
))? | ||
.transaction | ||
.hex | ||
} | ||
SwapType::ReverseSubmarine => boltz_client.get_reverse_tx(swap_id)?.hex, | ||
SwapType::Submarine => boltz_client.get_submarine_tx(swap_id)?.hex, |
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 is getting a little confusing. For submarine and chain swaps this would fetch the lockup transaction of the user. For reverse swaps the one of the server. How about returning a struct with optional user
and server
transaction?
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.
Updated the function name to fetch_lockup_utxo_boltz
does that make it more clear?
Added chain_swaps.rs to tests
Minor fixes
Co-authored-by: michael1011 <[email protected]>
Co-authored-by: michael1011 <[email protected]>
Co-authored-by: michael1011 <[email protected]>
Co-authored-by: michael1011 <[email protected]>
Co-authored-by: michael1011 <[email protected]>
Damn! I merged michael's update to the function name and it broke the test because its usage needs to be updated. Sorry guys! I will just merge and fix it on trunk immediately. Can I get an ACK on doing it this way? Or @dangeross would you be able to fix it from your branch? Or recommend another strategy? |
ACK @i5hi. Feel free to commit to the PR |
Patched on trunk! |
This PR adds Chain Swaps. It should include:
Tested: