-
Notifications
You must be signed in to change notification settings - Fork 9
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
Verify and claim swap if not verifiable in swap loop #681
base: main
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.
Looks good! Left just a comment about a potential efficiency issue.
Tested on the CLI by commenting out the regular claim paths and it worked well.
let tx_hex = self | ||
.liquid_chain_service | ||
.lock() | ||
.await | ||
.get_transaction_hex(&Txid::from_str(&tx_id)?) | ||
.await? | ||
.ok_or(anyhow!("Lockup tx not found for Receive swap {swap_id}"))? | ||
.serialize() | ||
.to_lower_hex_string(); | ||
self.verify_lockup_tx(receive_swap, &tx_id, &tx_hex, true) | ||
.await?; |
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.
Maybe I'm missing something, but this seems a bit inefficient. Are we fetching the tx hex just for the sake of being able to reuse the existing verify_tx method? If so, maybe we should keep in mind to optimize this later
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.
Not sure if there's any way to verify the tx without passing the whole hex string.
We need it to ensure ensure that the double hash of the encoded tx is equal to the provided tx id.
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.
Nice and simple, the BlockListener
comes in real handy here 👍
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.
Looks good.
async fn on_bitcoin_block(&self, _height: u32) {} | ||
|
||
async fn on_liquid_block(&self, height: u32) { | ||
if let Err(e) = self.claim_confirmed_lockups(height).await { |
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.
Don't we want to claim also unconfirmed that are in the zero-conf range?
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 claim should have happened for TransactionMempool
status already. There the tx is verified without the confirmation check.
Shall we add this logic here also to only verify confirmation on non zero-conf swaps?
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.
I was thinking perhaps the code that should claim on TransactionMempool status might also not find the transaction in the mempool and skip the claim. But this is ok I think to focus on confirmed ones as it is a fallback after all.
height | ||
); | ||
for swap in receive_swaps { | ||
if let Err(e) = self.claim_confirmed_lockup(&swap).await { |
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.
Do we want to skip these with claim_tx_id set here? Otherwise we will probably worst case log here an 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.
LGTM
async fn verify_lockup_tx( | ||
&self, | ||
receive_swap: &ReceiveSwap, | ||
swap_update_tx: &SwapUpdateTxDetails, | ||
tx_id: &str, | ||
tx_hex: &str, | ||
verify_confirmation: bool, | ||
) -> Result<()> { |
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.
Shouldn't we also verify the incoming amount here?
Verifies and claims any ongoing local receive swaps with a lockup tx but no claim tx.
Fixes #675