[r2r] wait for EVM approval transaction confirmation #1706
Conversation
…before sending the swap payment
shamardy
changed the title
|
listed a new token and tried to sell it to my maker, but the swap failed: weird thing is that the TakerFeeSent was fine https://polygonscan.com/tx/0x2f61c485b3376b69999205c0829b4728fd86dc4f5e2cff77f589d90efd74d6ac and it's the same new token (REQ-PLG20) TakerPaymentSent tx failed though: https://polygonscan.com/tx/0x8f0ef8a9c40ff4a0d634f6f8b14bba69833121d0a04596df85f39765e71ef039 i also can't see the approve tx on https://polygonscan.com/address/0x15c577f4b9fbe66735a9bff9b0b5d3ea2854bfaa |
That is normal behaviour, taker fee is sent to the fee address straight away, no contract calls are required.
I see the new problem now, it's related to this recently merged PR #1658 (3 days ago) where this line was added https://github.com/KomodoPlatform/atomicDEX-API/blob/6671a2381da9e5cdcb3812e53e341e29636bf1ff/mm2src/coins/eth.rs#L2887 This causes the skipping of the approval transaction if there are no watchers. Will fix it. |
|
@cipig can you please retest after the latest commit? |
mm2src/coins/eth.rs
Outdated
| U256::from(ETH_GAS), | ||
| .and_then(move |approved| { | ||
| // make sure the approve tx is confirmed before sending the htlc tx | ||
| arc.wait_for_confirmations( |
There was a problem hiding this comment.
I decided to wait for confirmation of the approve transaction, alternatively, we can loop over the allowance fn and check until the amount is allowed. What do you think is the better solution @caglaryucekaya? checking allowance fn might be a cheaper call but I am not sure.
There was a problem hiding this comment.
https://www.quicknode.com/docs/ethereum/api_credits
Quicknode has an API credit measure here for each method based on its complexity. wait_for_confirmations requires two calls at each iteration of the loop: eth_getTransactionReceipt (2 credits) and eth_blockNumber (1 credit). Allowance query requires one call eth_call which is 2 credits. So allowance call is less credits in total according to this list, and one call is better than having two separate calls in general. I think it's worth changing it to a loop over the allowance function.
There was a problem hiding this comment.
Thanks for the detailed research/explanation for this. Will fix it :)
|
updated and tried again, but it failed too: the approve tx is now there: https://polygonscan.com/tx/0x68c3d603050560ab34b10a843b035088e0f3449daed8dde2e06f8cdb802843b0 |
yes, I messed up the wait for confirmation timestamp. Will fix it, hope it's the final fix :) |
no problem, i am here and will try again :-) |
Yes, the wait for confirmation of the approval transaction failed / didn't really happen. |
Hope it's fixed in the latest commit |
I also fixed the error message to be more verbose. |
|
hmmm, tried the same swap, takerpayment was ok (though since the approve already happened during last try, it is not surprising) but now it failed on maker side, maker can't spend that takerpayment: not sure if this is related to this changes, error message is the same as in #1224 console on maker does not show anything useful: |
|
maybe worth doing this change too: #1630 (comment) |
|
next problem is that the swap failed on the maker, but not yet on taker: |
|
after waiting for more than 5h, i killed ADEX Desktop and after restart i was refunded right away |
|
sent REQ-PLG20 manually to the maker and started a swap... worked fine mm2 waited for the approve, payment was confirmed 5 blocks later... also fine
taker not failing swap although maker failed it and locktime expired a long time ago persists, which is very dangerous since the other side (maker) could easily rob taker if he knows how |
Agree, I should work on it this week on another PR :)
Is this the same swap here #1706 (comment) but from the taker's side? If so, I will investigate all these problems this week and try to solve one by one starting by this #1630 (comment) |
|
@caglaryucekaya can you please take a look at this PR? |
Nevermind, just saw your comment here #1706 (comment). I should fix this first before you start the review process. |
mm2src/coins/eth.rs
Outdated
| ])); | ||
| let value = U256::from(args.watcher_reward.unwrap_or(0)); | ||
| // wait for the approval tx confirmation only half the time required for the actual htlc tx confirmation | ||
| let wait_for_approval_confirmation_until = args.wait_for_confirmation_until / 2; |
There was a problem hiding this comment.
@caglaryucekaya Do you think the wait period should be reduced for the approval confirmation? I don't want the check for confirmation to fail quickly if there is no new blocks for some time but if there is a problem with the approval transaction we want it to fail quickly, so that is the trade off between longer vs shorter wait time.
There was a problem hiding this comment.
We could put some random limit on it, but I think we can leave it this way. If the transaction is sent successfully with enough gas and correct addresses it shouldn't fail. Are there any other reasons why it can fail?
There was a problem hiding this comment.
but I think we can leave it this way
Yeah, I think so too.
Are there any other reasons why it can fail?
There is a more general problem with block reorganization specially with polygon, but this should be solved using a higher value for required confs for both taker and maker and if the approve transaction disappears, dependant transactions will fail too, so I see no problem with the current approach.
mm2src/coins/eth.rs
Outdated
| U256::from(ETH_GAS), | ||
| .and_then(move |approved| { | ||
| // make sure the approve tx is confirmed before sending the htlc tx | ||
| arc.wait_for_confirmations( |
There was a problem hiding this comment.
Thanks for the detailed research/explanation for this. Will fix it :)
Yes, it was that swap. |
|
@caglaryucekaya fixed this #1706 (comment) in the latest commit. This is ready for review now :) |
* add change logs for PRs merged to dev only
* remove {version} - {date} and add dev branch instead
* add ibc transfer change logs
* add lightning PR #1655 to change logs
* add changelog for #1706
* add changelog for #1694, #1665
---------
Reviewed-by: laruh, caglaryucekaya <caglaryucekaya@gmail.com>
Fixes #1689