[r2r] Watcher rewards for ETH swaps #1658

caglaryucekaya · 2023-02-15T11:16:43Z

Adds watcher reward functionality for ETH-ETH swaps. The problem is that in order to spend the maker payment or refund the taker payment, watchers have to spend some gas fee. With this PR, both taker and maker add a small amount of watcher reward to their corresponding payments to compensate for this cost (plus a slight profit). This reward goes to whoever spends/refunds the payments: if watcher spends/refunds it goes to the watcher, if taker/maker complete the swap themselves they take the reward back. In case of refund, taker's reward goes back to the taker and maker's reward goes back to the maker. If the taker/maker spend each other's payments, then it is simply an exchange of rewards of same value between them. The reward will be activated if either the taker or the maker coin is ETH and both of them agree to use watchers. If one of the coins is ETH, watchers will only accept payments with rewards. The updated contract with the rewards can be found here:

https://github.com/caglaryucekaya/etomic-swap/blob/ethereum-swap-watchers/contracts/EtomicSwap.sol

…h-swap-watchers

onur-ozkan

Great work 🔥

First notes from my partial review:

onur-ozkan · 2023-02-16T12:05:53Z

mm2src/coins/lp_coins.rs

@@ -626,6 +633,16 @@ pub struct ValidateFeeArgs<'a> {
    pub uuid: &'a [u8],
 }

+#[derive(Clone, Debug)]


redundant traits

Suggested change

#[derive(Clone, Debug)]

onur-ozkan · 2023-02-16T12:09:24Z

mm2src/coins/lp_coins.rs

+        secret_hash: &[u8],
+        spend_tx: &[u8],
+        watcher_reward: bool,
+    ) -> Result<Vec<u8>, String>;


We started migrating from ERR(string errors) couple months ago. I think we should stop adding String as an error type to functions.

I didn't add this, it was this way already. There are many functions like this with different implementations for all coins. We can open a refactoring issue for this and assign it to someone in next sprints since it will take some time.

onur-ozkan · 2023-02-16T12:24:58Z

mm2src/mm2_main/src/lp_swap.rs

+// TODO: This can be done in a better way
+pub async fn watcher_reward_from_gas(
+    coin: &MmCoinEnum,
+    other_coin: &MmCoinEnum,
+    gas: u64,
+    watcher_reward: bool,
+) -> Result<Option<u64>, String> {
+    if !watcher_reward {
+        Ok(None)
+    } else if let MmCoinEnum::EthCoin(coin) = &coin {
+        let gas_price = try_s!(coin.get_gas_price().compat().await).as_u64();
+        Ok(Some(gas * gas_price))
+    } else if let MmCoinEnum::EthCoin(coin) = &other_coin {
+        let gas_price = try_s!(coin.get_gas_price().compat().await).as_u64();
+        Ok(Some(gas * gas_price))
+    } else {
+        return Err(ERRL!("At least one coin must be ETH to use watcher reward"));
+    }
+}


Suggested change

// TODO: This can be done in a better way

pub async fn watcher_reward_from_gas(

coin: &MmCoinEnum,

other_coin: &MmCoinEnum,

gas: u64,

watcher_reward: bool,

) -> Result<Option<u64>, String> {

if !watcher_reward {

Ok(None)

} else if let MmCoinEnum::EthCoin(coin) = &coin {

let gas_price = try_s!(coin.get_gas_price().compat().await).as_u64();

Ok(Some(gas * gas_price))

} else if let MmCoinEnum::EthCoin(coin) = &other_coin {

let gas_price = try_s!(coin.get_gas_price().compat().await).as_u64();

Ok(Some(gas * gas_price))

} else {

return Err(ERRL!("At least one coin must be ETH to use watcher reward"));

}

}

pub async fn watcher_reward_from_gas(

coin: &MmCoinEnum,

other_coin: &MmCoinEnum,

gas: u64,

watcher_reward: bool,

) -> Result<Option<u64>, String> {

if !watcher_reward {

return Ok(None);

}

match (coin, other_coin) {

(MmCoinEnum::EthCoin(coin), _) | (_, MmCoinEnum::EthCoin(coin)) => {

let gas_price = try_s!(coin.get_gas_price().compat().await).as_u64();

Ok(Some(gas * gas_price))

},

_ => Err(ERRL!("At least one coin must be ETH to use watcher reward")),

}

}

Nice, this is better

onur-ozkan · 2023-02-16T12:35:10Z

mm2src/mm2_main/src/lp_swap.rs

+pub async fn watcher_reward_amount(
+    coin: &MmCoinEnum,
+    other_coin: &MmCoinEnum,
+    watcher_reward: bool,
+) -> Result<Option<u64>, String> {
+    const WATCHER_REWARD_GAS: u64 = 100_000;
+    watcher_reward_from_gas(coin, other_coin, WATCHER_REWARD_GAS, watcher_reward).await
+}
+
+pub async fn min_watcher_reward(
+    coin: &MmCoinEnum,
+    other_coin: &MmCoinEnum,
+    watcher_reward: bool,
+) -> Result<Option<u64>, String> {
+    const MIN_WATCHER_REWARD_GAS: u64 = 70_000;
+    watcher_reward_from_gas(coin, other_coin, MIN_WATCHER_REWARD_GAS, watcher_reward).await
+}


Wouldn't be better checking watcher_reward in maker/taker_swap.rs and avoid awaiting these(watcher_reward_from_gas, min_watcher_reward, watcher_reward_amount) async functions? This way we can avoid unnecessary context switches, thread yielding, etc. I think it matters for low power devices like mobile phones.

Good point. I was trying to avoid repeating this block every time we call these functions:

let reward = if watcher_reward { watcher_reward_amount(taker_coin, maker_coin) } else { None }

Can you think of a better way for this?

You can use then_some on watcher_reward.

let reward = watcher_reward.then_some(watcher_reward_amount().await?);

That's what I tried first but then_some evaluates the passed function whether the bool is true or false

Then your suggestion makes more sense

onur-ozkan

Last review iteration:

onur-ozkan · 2023-02-21T09:24:27Z

mm2src/mm2_bitcoin/chain/src/output.txt

+
+running 1 test
+**is_zcash: true
+test block_header::tests::test_rick_v1_block_header_des ... ok
+
+test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 45 filtered out; finished in 0.00s
+


What is this for?

It's a log file that I accidentally pushed 😅

onur-ozkan · 2023-02-21T09:31:02Z

mm2src/coins/eth.rs

+        false
+        //self.contract_supports_watchers


Why it's hardcoded as false?

The ETH watcher rewards aren't complete yet, and watchers shouldn't be used until it's ready

onur-ozkan · 2023-02-21T09:35:56Z

mm2src/coins/eth.rs

+                let address_input = get_function_input_data(&decoded_input, function, 0)
+                    .map_to_mm(ValidatePaymentError::TxDeserializationError)?;
+                let value_input = get_function_input_data(&decoded_input, function, 1)
+                    .map_to_mm(ValidatePaymentError::TxDeserializationError)?;
+
+                if address_input != Token::Address(fee_addr) {
+                    return MmError::err(ValidatePaymentError::WrongPaymentTx(format!(
+                        "{}: ERC20 Fee tx was sent to wrong address {:?}, expected {:?}",
+                        INVALID_RECEIVER_ERR_LOG, address_input, fee_addr
+                    )));
+                }


address_input validation should be right after address_input computation.

onur-ozkan · 2023-02-21T09:40:37Z

mm2src/coins/eth.rs

+                    if let Some(min_reward) = input.min_watcher_reward {
+                        if tx_from_rpc.value.as_u64() < min_reward {
+                            return MmError::err(ValidatePaymentError::WrongPaymentTx(format!(
+                                "Payment tx value arg {} is less than the minimum expected watcher reward {}",
+                                tx_from_rpc.value, min_reward
+                            )));
+                        }
+                    }


Can this check move right before or after the if tx_from_rpc.to != Some(expected_swap_contract_address) check?

…h-swap-watchers

shamardy

Great work! first review iteration.

mm2src/coins/eth/v2_activation.rs

mm2src/mm2_main/src/lp_swap/maker_swap.rs

shamardy · 2023-02-22T20:31:45Z

mm2src/mm2_main/src/lp_swap.rs

+    InvalidCoinType(String),
+}
+
+// This needs discussion. We need a way to determine the watcher reward amount, and a way to validate it at watcher side so


I have a question regarding if 2 watchers try to spend the same payment with one of those transactions failing, will the watcher pay some gas for the failed transaction/contract call? If so, we need to think of a way to avoid this in the next PRs.

Good point, this is one thing we need to think carefully and test thoroughly since it can cost ETH to watchers. This and similar problems can also be exploited by attackers to cause damage to the watchers. The contract function to spend the maker payment first checks if the payment is already spent at the first line, with a require statement. As far as I understand, require statement isn't supposed to consume any gas if it fails, but I wrote an integration test case where two watchers spend the same maker payment and the result is that the second watcher also spends some gas. I'll research/experiment with this further in the next sprint to understand it better. If it's inevitable to spend gas for failed payments, than we need to find a way to stop the watchers from ever attempting. One solution could be to use some randomized delay before the watchers spend any payments to give some space for other watchers, then check if the payment is already spent before spending it themselves.

I have a fear that there might be no way to avoid this but to subscribe with only one watcher for every EVM related swap, but this has its own problems too if this watcher is the maker.

…h-swap-watchers

cipig · 2023-02-23T16:33:08Z

I have 2 questions:
Can the watchers decide which coins/chains they support and which not?
Can the watcher configure min_volumes?

Problem i see is that users could not set min_volume for their ERC20 swaps and if they fail and watcher claims the payment, it could well be that it's not worth it, like if the exchanged tokens are worth 2 USD, but txfees to claim those tokens is 10 USD.
atm, ERC20 swaps cost 15 USD, see https://etherscan.io/gastracker ("Uniswap V3: Swap" has ~ the same cost as ADEX swaps).

shamardy

🔥

caglaryucekaya · 2023-02-24T12:08:34Z

@cipig

Can the watchers decide which coins/chains they support and which not?
Can the watcher configure min_volumes?

Currently there's no such configuration, but yes I should add something about it in the next PRs. I think it should not be the watchers but users who should decide for which coins they want watcher support, right? And is it necessary to configure it coin by coin, or is a configuration like "use_watcher_rewards" enough? If set false, this can disable watcher support for all ETH-based coins. And for the min_volume, it is again the user who should decide I think. As long as they know the reward amount, it should be fine. But we could also add some rules to the watchers like "the trade amount must always be larger than the reward".

onur-ozkan

thank you for the fixes

cipig · 2023-02-24T13:51:59Z

Currently there's no such configuration, but yes I should add something about it in the next PRs. I think it should not be the watchers but users who should decide for which coins they want watcher support, right? And is it necessary to configure it coin by coin, or is a configuration like "use_watcher_rewards" enough? If set false, this can disable watcher support for all ETH-based coins. And for the min_volume, it is again the user who should decide I think. As long as they know the reward amount, it should be fine. But we could also add some rules to the watchers like "the trade amount must always be larger than the reward".

Yes, indeed, min_volume is and should be set by the users who post the orders. I was just thinking that it would make no sense for a watcher to claim payments if the txfees are higher than the claimed amount. But i see that it will not be easy to configure/implement that on watcher side, since he would need to know how much those claimed tokens are worth, so he needs to know their price, which would make the entire thing pretty complex.
I wonder though how we could make sure that watchers are not operating at a loss. Since this problem is mainly related to ERC20 (and maybe some other EVM chains like BEP20) i thought that a watcher could configure to not get involved with those chains at all. Or is the user paying for all the involved txfees?

We should continue this discussion somewhere else though, since it's out of the scope of this PR. Sorry that i started it here in the first place.

…h-swap-watchers

caglaryucekaya · 2023-02-27T15:55:41Z

We should continue this discussion somewhere else though, since it's out of the scope of this PR. Sorry that i started it here in the first place.

Okay, I will open some discussions about watchers on Mattermost soon, including this one.

…h-swap-watchers

shamardy

Re-approve

caglaryucekaya added 15 commits January 20, 2023 16:52

add watcher_validate_taker_payment unit tests for eth, erc20 and utxo

384b143

reuse ethereum validate_fee method for watcher validations

0855843

add contract support parameter to eth coin activation parameters

fe4d410

fix lock_duration overflow bug by adding validation

966bdd3

use the refactored etomic swap contract

36ce113

fix a watcher bug in wait_for_htlc_tx_spend and add a test case

19c22a8

change swap contract address and abi

c4dea58

remove the contract_supports_watchers method

5d1d029

add watcher_reward to negotiaton data

79366f9

add watcher reward functionality

4b8c6f5

Merge branch 'dev' of github.com:KomodoPlatform/atomicDEX-API into et…

3ceae5f

…h-swap-watchers

fix merge conflicts

4ca9d6e

remove log file

dc73825

remove watcher reward related parts from negotiation data

2958563

disable the watcher rewards

601c2a8

caglaryucekaya changed the title ~~[wip] Watcher rewards for ETH swaps~~ [r2r] Watcher rewards for ETH swaps Feb 15, 2023

caglaryucekaya requested review from onur-ozkan, shamardy and ca333 February 15, 2023 13:37

Merge branch 'dev' of github.com:KomodoPlatform/atomicDEX-API into et…

38f9be0

…h-swap-watchers

onur-ozkan requested changes Feb 16, 2023

View reviewed changes

caglaryucekaya added 2 commits February 16, 2023 20:40

small fixes

bb4ecbb

disable watcher rewards

b287ba4

onur-ozkan requested changes Feb 21, 2023

View reviewed changes

caglaryucekaya added 3 commits February 22, 2023 16:24

remove log file

69f1f0f

minor fixes

a0ed8f1

Merge branch 'dev' of github.com:KomodoPlatform/atomicDEX-API into et…

0d7e859

…h-swap-watchers

shamardy requested changes Feb 22, 2023

View reviewed changes

ca333 added the under review label Feb 23, 2023

caglaryucekaya changed the title ~~[r2r] Watcher rewards for ETH swaps~~ [wip] Watcher rewards for ETH swaps Feb 23, 2023

caglaryucekaya added 4 commits February 23, 2023 13:44

add test case for multiple watchers spending the same payment

5df9858

use serde default for contract_supports_watchers field

c52c2ca

use multiple attempts to get gas price for watcher rewards

efbfdc2

Merge branch 'dev' of github.com:KomodoPlatform/atomicDEX-API into et…

8502034

…h-swap-watchers

caglaryucekaya changed the title ~~[wip] Watcher rewards for ETH swaps~~ [r2r] Watcher rewards for ETH swaps Feb 23, 2023

shamardy previously approved these changes Feb 23, 2023

View reviewed changes

onur-ozkan previously approved these changes Feb 24, 2023

View reviewed changes

Merge branch 'dev' of github.com:KomodoPlatform/atomicDEX-API into et…

3578aa4

…h-swap-watchers

caglaryucekaya dismissed stale reviews from onur-ozkan and shamardy via 3578aa4 February 27, 2023 15:50

Merge branch 'dev' of github.com:KomodoPlatform/atomicDEX-API into et…

5ea8f3c

…h-swap-watchers

shamardy approved these changes Mar 7, 2023

View reviewed changes

shamardy merged commit 32d2ed5 into dev Mar 7, 2023

shamardy deleted the eth-swap-watchers branch March 7, 2023 15:29

shamardy mentioned this pull request Mar 10, 2023

[r2r] wait for EVM approval transaction confirmation #1706

Merged

ca333 mentioned this pull request Mar 27, 2023

[release] 1.0.1-beta #1721

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[r2r] Watcher rewards for ETH swaps #1658

[r2r] Watcher rewards for ETH swaps #1658

caglaryucekaya commented Feb 15, 2023 •

edited

Loading

onur-ozkan left a comment

onur-ozkan Feb 16, 2023

onur-ozkan Feb 16, 2023

caglaryucekaya Feb 16, 2023

onur-ozkan Feb 16, 2023

caglaryucekaya Feb 16, 2023

onur-ozkan Feb 16, 2023

caglaryucekaya Feb 16, 2023

onur-ozkan Feb 16, 2023

caglaryucekaya Feb 16, 2023

onur-ozkan Feb 16, 2023

onur-ozkan left a comment

onur-ozkan Feb 21, 2023

caglaryucekaya Feb 22, 2023

onur-ozkan Feb 21, 2023

caglaryucekaya Feb 22, 2023

onur-ozkan Feb 21, 2023

onur-ozkan Feb 21, 2023

shamardy left a comment

shamardy Feb 22, 2023

caglaryucekaya Feb 23, 2023

shamardy Feb 23, 2023

cipig commented Feb 23, 2023

shamardy left a comment

caglaryucekaya commented Feb 24, 2023

onur-ozkan left a comment

cipig commented Feb 24, 2023

caglaryucekaya commented Feb 27, 2023

shamardy left a comment

[r2r] Watcher rewards for ETH swaps #1658

[r2r] Watcher rewards for ETH swaps #1658

Conversation

caglaryucekaya commented Feb 15, 2023 • edited Loading

onur-ozkan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

onur-ozkan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shamardy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cipig commented Feb 23, 2023

shamardy left a comment

Choose a reason for hiding this comment

caglaryucekaya commented Feb 24, 2023

onur-ozkan left a comment

Choose a reason for hiding this comment

cipig commented Feb 24, 2023

caglaryucekaya commented Feb 27, 2023

shamardy left a comment

Choose a reason for hiding this comment

caglaryucekaya commented Feb 15, 2023 •

edited

Loading