ETH/ERC20/UTXO swaps with watcher rewards#1750
Conversation
…h-utxo-erc20-swap-watchers
…h-utxo-erc20-swap-watchers
…h-utxo-erc20-swap-watchers
borngraced
left a comment
There was a problem hiding this comment.
Great work!
Just a suggestion for now...
shamardy
left a comment
There was a problem hiding this comment.
Amazing work as always! Only 2 comments after a quick first review, will dig deeper into the code in upcoming reviews :)
onur-ozkan
left a comment
There was a problem hiding this comment.
great work!
First review iteration:
mm2src/coins/utxo/utxo_common.rs
Outdated
| T: MarketCoinOps + UtxoCommonOps, | ||
| { | ||
| let decimals = coin.as_ref().decimals; | ||
| println!("**dex_fee_amount: {}", dex_fee_amount); |
There was a problem hiding this comment.
Let's use one of the log macros here
There was a problem hiding this comment.
I was using this while debugging, will remove it
mm2src/coins/eth.rs
Outdated
| pub const PAYMENT_STATE_UNINITIALIZED: u8 = 0; | ||
| pub const PAYMENT_STATE_SENT: u8 = 1; | ||
| const _PAYMENT_STATE_SPENT: u8 = 2; | ||
| const PAYMENT_STATE_SPENT: u8 = 2; | ||
| const _PAYMENT_STATE_REFUNDED: u8 = 3; |
There was a problem hiding this comment.
It would be nice to bundle these in an enum, if you want to leave it as it is for now, please leave a TODO on top of them with referencing this comment.
mm2src/coins/eth.rs
Outdated
| match args.watcher_reward { | ||
| Some(reward) => Ok(Some(reward.to_string().into_bytes())), | ||
| None => Ok(None), | ||
| } |
There was a problem hiding this comment.
| match args.watcher_reward { | |
| Some(reward) => Ok(Some(reward.to_string().into_bytes())), | |
| None => Ok(None), | |
| } | |
| Ok(args.watcher_reward.map(|r| r.to_string().into_bytes())) |
mm2src/coins/eth.rs
Outdated
| _args: PaymentInstructionArgs, | ||
| ) -> Result<PaymentInstructions, MmError<ValidateInstructionsErr>> { | ||
| MmError::err(ValidateInstructionsErr::UnsupportedCoin(self.ticker().to_string())) | ||
| let watcher_reward = BigDecimal::from_str(&String::from_utf8_lossy(instructions)) |
There was a problem hiding this comment.
is from_utf8_lossy safe to be used here?
mm2src/coins/utxo/utxo_common.rs
Outdated
|
|
||
| if let Some(watcher_reward) = watcher_reward { | ||
| let watcher_reward = sat_from_big_decimal(&watcher_reward.amount, coin.as_ref().decimals)?; | ||
| let min_expected_amount = (watcher_reward / 100) * 95 + amount; |
There was a problem hiding this comment.
I think we should encapsulate this kind of calculations within the functions for readability and maintainability reasons.
|
As discussed some weeks ago, please add a check,msg.value == 0, here: No obvious exploit, but this prevents unintended behavior of adding ETH to the contract. I'll have further review in the coming week. |
…h-utxo-erc20-swap-watchers
|
We're storing data indefinitely for all swaps. We can get rid of both the ReceiverSpent and the SenderRefunded states and instead do |
@Alrighttt I made the function non-payable instead. Now it's not possible to send any value to it. |
mm2src/coins/Cargo.toml
Outdated
| tendermint-rpc = { version = "=0.23.7", default-features = false } | ||
| tiny-bip39 = "0.8.0" | ||
| uuid = { version = "1.2.2", features = ["fast-rng", "serde", "v4"] } | ||
| wasm-timer = "0.2.4" |
There was a problem hiding this comment.
This should go under dev-dependencies.
|
What is the purpose of |
If a payment contains reward, this parameter is used to specify where the reward is going to go after the other party spends this payment. If the target is |
I prepared documentation for enabling the watchtower functionality and setting the test parameters. You can find it here: |
shamardy
left a comment
There was a problem hiding this comment.
Thank you for the amazing work! Just left some non-blocker comments and some questions/considerations for next PRs.
| { | ||
| other_coin_amount.checked_div(coin_amount) | ||
| } else { | ||
| get_base_price_in_rel(Some(coin.ticker().to_string()), Some("ETH".to_string())).await |
There was a problem hiding this comment.
Just a consideration for next PR/s, do you think takers should validate the amount returned from the price service on their side in case it gives a bad price which leads to paying a very high reward amount?
The reward_amount in the end shouldn't be more than a certain constant amount or a certain percentage of the taker payment whichever is smaller.
There was a problem hiding this comment.
It makes a lot of sense. I added this to the TODO list here
| } | ||
| } | ||
|
|
||
| // TODO: Validate also maker payment |
There was a problem hiding this comment.
Does the watcher also validate the reward amount? I can't find the place this is done, maybe I missed it.
There was a problem hiding this comment.
Yes it does for the ETH transactions, I moved that logic inside the watcher_validate_taker_payment method to get rid of the taker_coin.is_eth.
For UTXO it doesn't because the transaction will contain the trade amount + watcher reward, and the watcher doesn't know about the trade amount. But some validation can still be implemented by deriving the trade amount from the taker fee. I added this to the TODO list.
|
@ozkanonur can you please check if all your comments are resolved? |
…h-utxo-erc20-swap-watchers
PoC implementation to extend the ETH watcher reward functionality to ETH/UTXO and ERC20/UTXO swaps. Additionally, this PR improves the reward protocol to allow only the takers to pay for the watcher service that they use. The problem is that if the taker wishes to use the watcher service, the watcher can either refund the taker payment (sent by the taker), or spend the maker payment (sent by the maker). During the swap, it is not known which of these payments the watcher is going to spend. Previous solution to this problem was to make both the taker and maker to include watcher rewards of equal amounts in their corresponding payments, which would later be exchanged if the swap is completed successfully, i.e. taker's reward goes to the maker and maker's reward goes to the taker. This solution was not ideal because there is little reason for the makers to include extra fees in their payments for a service that is used by the taker, even if they're going to take back the fee later. It would be better if the watcher service used by the taker is invisible to the maker. This is achieved by defining different targets for the reward while calling the payment functions of the swap contract. The target of the reward can be
Contract,PaymentSenderorPaymentSpenderand it depends on the coin protocols of taker and maker. The updated contract can be found here.ETH/ERC20 - ERC20/ETH
If both taker and maker uses EVM coins, the taker sends the reward (as ETH) to the
Contracttarget with the taker payment. Later, this reward that is locked in the contract can be taken either by spending the maker payment or refunding the taker payment. The receiver of the reward is whoever spends/refunds the payments, i.e. taker takes back its own reward if it doesn't use watchers.ETH taker / UTXO maker - ERC20 taker / UTXO maker
If the taker coin is EVM and the maker coin is Non-EVM, watcher only spends gas if it refunds the taker payment, spending the Non-EVM maker payment won't have a cost. In this case, the taker includes in the taker payment a reward (as ETH) with the target
PaymentSender. This is a refund-only reward that can only be claimed with the refund function of the contract. Again, the receiver of the reward is the one who's calling the refund function, either the taker himself or a watcher. If the taker payment is spent successfully by the maker, the reward automatically goes back to the taker.UTXO taker / ETH maker - UTXO taker / ERC20 maker
Unfortunately, for this case the maker is involved in the process which was avoided in the other two cases. The reason is that since the taker is Non-EVM, it can't send the reward (that will be required to spend the maker payment) to the contract together with the taker payment. It would be possible to add a new function to the swap contract just for this purpose, but sending a transaction just for the reward would be unnecessarily expensive. For this reason, the old solution is still used in this case: both the taker and the maker include some additional reward (in their own coins) in their respective payments. If the watcher spends the maker payment, it receives the reward either in ETH or ERC20. If the swap is completed successfully without watcher involvement, the taker will have maker's reward and the maker will have taker's reward. The reward that is sent by the maker has the target
PaymentSpenderand it goes to whoever spends this payment. The advantage of this approach is that it makes it possible to buy ETH/ERC20 with a Non-EVM coin without having any ETH.Further information on enabling a watchtower node can be found here:
https://docs.google.com/document/d/1YHtsIFGkO2itPlFt72uH0oB0v_Puwtt3Cmjd6BF-fTM/edit#