-
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.
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.
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
lib/core/src/receive_swap.rs
Outdated
.map(|o| o.value)?; | ||
let expected_lockup_amount_sat = | ||
receive_swap.receiver_amount_sat + receive_swap.claim_fees_sat; | ||
if expected_lockup_amount_sat != lockup_amount_sat { |
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.
Should be more tolerant here: expected_lockup_amount_sat <= lockup_amount_sat
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.
Fixed a675bf3
lib/core/src/recover/model.rs
Outdated
Ok(ReceiveSwapImmutableData { | ||
swap_id, | ||
swap_timestamp: swap.created_at, | ||
timeout_block_height: create_response.timeout_block_height, | ||
blinding_key, |
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 for the scope of this PR of course, just adding a note here that perhaps we consider to set the swap object itself instead of keep adding fields.
lib/core/src/recover/recoverer.rs
Outdated
"Failed to verify lockup amount for Receive Swap {}: {} sat vs {} sat", | ||
swap_id, expected_lockup_amount_sat, lockup_amount_sat | ||
); | ||
recovered_data.lockup_tx_id = None; |
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 you think we can simplify the recoverer by not making it responsible for validating the lockup amount?
If we prevent claiming the wrong amount isn't that enough?
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 swap/payment would be set to Pending if we don't. Especially for non-local 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.
Right but since this is an edge case (where the swOpers cheats) doesn't it worth the simplicity? Worst case the payment stays pending untill expiration.
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.
Simplified 0cb9ca1
c377e36
to
0cb9ca1
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.
LGTM
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.
Did you mean to commit this file?
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.
Nope 😅
This PR:
Fixes #675
Testing Notes: