ETH/ERC20/UTXO swaps with watcher rewards #1750

caglaryucekaya · 2023-04-10T11:08:01Z

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, PaymentSender or PaymentSpender and 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 Contract target 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 PaymentSpender and 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#

…h-utxo-erc20-swap-watchers

borngraced

Great work!

Just a suggestion for now...

mm2src/mm2_main/src/lp_swap.rs

shamardy

Amazing work as always! Only 2 comments after a quick first review, will dig deeper into the code in upcoming reviews :)

mm2src/mm2_main/src/lp_swap/maker_swap.rs

mm2src/mm2_main/src/lp_swap.rs

onur-ozkan

great work!

First review iteration:

onur-ozkan · 2023-04-12T10:16:01Z

mm2src/coins/utxo/utxo_common.rs

@@ -3443,6 +3451,7 @@ where
    T: MarketCoinOps + UtxoCommonOps,
 {
    let decimals = coin.as_ref().decimals;
+    println!("**dex_fee_amount: {}", dex_fee_amount);


Let's use one of the log macros here

I was using this while debugging, will remove it

onur-ozkan · 2023-04-12T10:24:14Z

mm2src/coins/eth.rs

 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;


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.

onur-ozkan · 2023-04-12T10:30:26Z

mm2src/coins/eth.rs

+        match args.watcher_reward {
+            Some(reward) => Ok(Some(reward.to_string().into_bytes())),
+            None => Ok(None),
+        }


Suggested change

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()))

onur-ozkan · 2023-04-12T10:30:58Z

mm2src/coins/eth.rs

    ) -> Result<PaymentInstructions, MmError<ValidateInstructionsErr>> {
-        MmError::err(ValidateInstructionsErr::UnsupportedCoin(self.ticker().to_string()))
+        let watcher_reward = BigDecimal::from_str(&String::from_utf8_lossy(instructions))


is from_utf8_lossy safe to be used here?

onur-ozkan · 2023-04-12T10:59:57Z

mm2src/coins/utxo/utxo_common.rs

+
+        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;


I think we should encapsulate this kind of calculations within the functions for readability and maintainability reasons.

Alrighttt · 2023-04-12T18:08:20Z

As discussed some weeks ago, please add a check,msg.value == 0, here:
https://github.com/caglaryucekaya/etomic-swap/blob/b4879c0c047eb95570d7bbd06d87723779ce4b6f/contracts/EtomicSwap.sol#L97

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

Alrighttt · 2023-04-13T00:49:23Z

We're storing data indefinitely for all swaps. We can get rid of both the ReceiverSpent and the SenderRefunded states and instead do delete payments[_id]. This will give gas refund and reduce transaction fees.

…h-utxo-erc20-swap-watchers

caglaryucekaya · 2023-05-05T09:15:20Z

As discussed some weeks ago, please add a check,msg.value == 0, here: https://github.com/caglaryucekaya/etomic-swap/blob/b4879c0c047eb95570d7bbd06d87723779ce4b6f/contracts/EtomicSwap.sol#L97

No obvious exploit, but this prevents unintended behavior of adding ETH to the contract.

I'll have further review in the coming week.

@Alrighttt I made the function non-payable instead. Now it's not possible to send any value to it.

Alrighttt · 2023-05-05T11:27:16Z

mm2src/coins/Cargo.toml

@@ -95,6 +96,7 @@ utxo_signer = { path = "utxo_signer" }
 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"


This should go under dev-dependencies.

Alrighttt · 2023-05-05T11:28:20Z

What is the purpose of Contract within RewardTargetOnSpend enum?

caglaryucekaya · 2023-05-05T12:57:09Z

What is the purpose of Contract within RewardTargetOnSpend enum?

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 PaymentSender, then the reward goes back to the sender of the payment. If it is PaymentSpender, it goes to whoever spends the payment. If it is Contract, then the reward stays in the contract after the payment is spent. The purpose of this is to allow the taker to lock the reward in the contract, which can then be claimed when the maker payment is spent. Here's how it will work: taker sends the taker payment with the RewardTargetOnSpend::Contract parameter. The maker spends the taker payment, and since the reward target is set to contract, it continues to stay in the contract. When the watcher spends the maker payment, this reward goes to the watcher from the contract. I'm preparing a documentation with the explanations of these new contract functions and their parameters.

caglaryucekaya · 2023-05-05T16:27:54Z

Please document how to enable watcher node functionality on a node.

I prepared documentation for enabling the watchtower functionality and setting the test parameters. You can find it here:
https://docs.google.com/document/d/1YHtsIFGkO2itPlFt72uH0oB0v_Puwtt3Cmjd6BF-fTM/edit?usp=sharing. Note that the watchtowers for ETH coins are not yet ready for manual testing.

shamardy

Thank you for the amazing work! Just left some non-blocker comments and some questions/considerations for next PRs.

mm2src/mm2_test_helpers/src/for_tests.rs

mm2src/mm2_number/src/lib.rs

shamardy · 2023-05-05T19:40:29Z

mm2src/coins/utxo/utxo_common.rs

+            {
+                other_coin_amount.checked_div(coin_amount)
+            } else {
+                get_base_price_in_rel(Some(coin.ticker().to_string()), Some("ETH".to_string())).await


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.

It makes a lot of sense. I added this to the TODO list here

shamardy · 2023-05-05T20:00:52Z

mm2src/mm2_main/src/lp_swap/swap_watcher.rs

@@ -193,6 +192,7 @@ impl State for ValidateTakerFee {
    }
 }

+// TODO: Validate also maker payment


Does the watcher also validate the reward amount? I can't find the place this is done, maybe I missed it.

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.

https://github.com/KomodoPlatform/atomicDEX-API/blob/fad0116935cde05648d9ccfca025b37c368eef50/mm2src/coins/eth.rs#L1527-L1531

https://github.com/KomodoPlatform/atomicDEX-API/blob/fad0116935cde05648d9ccfca025b37c368eef50/mm2src/coins/eth.rs#L1597-L1604

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.

…h-utxo-erc20-swap-watchers

shamardy · 2023-05-09T18:28:28Z

@ozkanonur can you please check if all your comments are resolved?

…h-utxo-erc20-swap-watchers

onur-ozkan

LGTM

caglaryucekaya added 8 commits March 29, 2023 23:47

add watcher reward functionality for eth-erc20-utxo swaps

f091e0c

refactor watcher tests and add tests for eth-erc20-utxo swaps

0e82bb3

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

5e4af1b

…h-utxo-erc20-swap-watchers

fix merge problems

c4b78e3

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

0311858

…h-utxo-erc20-swap-watchers

update swap contract abi

4722c72

use improved contract to allow only the taker to pay watcher rewards

b47f95e

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

35680b4

…h-utxo-erc20-swap-watchers

caglaryucekaya requested review from shamardy, onur-ozkan, borngraced and laruh April 10, 2023 11:08

add changes to the CHANGELOG

e3998b5

caglaryucekaya added the under review label Apr 10, 2023

borngraced reviewed Apr 10, 2023

View reviewed changes

mm2src/mm2_main/src/lp_swap.rs Outdated Show resolved Hide resolved

caglaryucekaya requested a review from Alrighttt April 11, 2023 12:28

shamardy reviewed Apr 11, 2023

View reviewed changes

mm2src/mm2_main/src/lp_swap/maker_swap.rs Outdated Show resolved Hide resolved

mm2src/mm2_main/src/lp_swap.rs Outdated Show resolved Hide resolved

onur-ozkan requested changes Apr 12, 2023

View reviewed changes

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

b8f3493

…h-utxo-erc20-swap-watchers

caglaryucekaya added 8 commits April 14, 2023 01:01

fix a problem with utxo swap watcher integration tests

23ff820

fix repeatable future usage

1d55164

fix a bug in swap watcher docker tests

5ed3fd7

use cfg attribute for test specific code

791b7d9

remove println

be7f8e7

use enum for ETH payment states

df30685

fix formatting

cfd79f7

replace match statement with map

c7fa92e

caglaryucekaya added 4 commits May 4, 2023 01:03

minor fixes

2403012

implement checked_div method for BigDecimal and replace divs with it

6c4c65b

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

6e1de72

…h-utxo-erc20-swap-watchers

implement CheckedDiv for MmNumber and use it in get_cex_rates

3fc4f55

caglaryucekaya added under review and removed in progress Changes will be made from the author labels May 4, 2023

Alrighttt reviewed May 5, 2023

View reviewed changes

move the wasm-timer import to dev-dependencies in coins toml

a5fee0a

shamardy mentioned this pull request May 5, 2023

[release] v1.0.4-beta #1804

Merged

shamardy added the 1.0.4-beta label May 5, 2023

rename an environment variable used for tests

96a4cb0

shamardy previously approved these changes May 5, 2023

View reviewed changes

use helper functions to get time

fad0116

caglaryucekaya dismissed shamardy’s stale review via fad0116 May 5, 2023 21:38

caglaryucekaya mentioned this pull request May 5, 2023

Implement swap watcher nodes. #1431

Open

caglaryucekaya added 4 commits May 6, 2023 11:38

remove derivation path from eth sepolia configuration

96f1fc7

rename the local big_decimal module

8ae9597

fix wasm issues

e437d75

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

94f91e7

…h-utxo-erc20-swap-watchers

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

8331f4d

…h-utxo-erc20-swap-watchers

onur-ozkan approved these changes May 9, 2023

View reviewed changes

shamardy merged commit a1fc8f7 into dev May 9, 2023

shamardy deleted the eth-utxo-erc20-swap-watchers branch May 9, 2023 21:25

shamardy mentioned this pull request May 12, 2023

Hotfix: lightning swaps maker payment instructions #1824

Merged

shamardy mentioned this pull request Feb 1, 2024

feat(trading-proto-upgrade): locked amounts, kmd burn and other impl #2046

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ETH/ERC20/UTXO swaps with watcher rewards #1750

ETH/ERC20/UTXO swaps with watcher rewards #1750

caglaryucekaya commented Apr 10, 2023 •

edited

Loading

borngraced left a comment

shamardy left a comment

onur-ozkan left a comment

onur-ozkan Apr 12, 2023

caglaryucekaya Apr 12, 2023

onur-ozkan Apr 12, 2023

onur-ozkan Apr 12, 2023

onur-ozkan Apr 12, 2023

onur-ozkan Apr 12, 2023

Alrighttt commented Apr 12, 2023

Alrighttt commented Apr 13, 2023

caglaryucekaya commented May 5, 2023

Alrighttt May 5, 2023

caglaryucekaya May 5, 2023

Alrighttt commented May 5, 2023

caglaryucekaya commented May 5, 2023 •

edited

Loading

caglaryucekaya commented May 5, 2023

shamardy left a comment

shamardy May 5, 2023

caglaryucekaya May 5, 2023

shamardy May 5, 2023

caglaryucekaya May 5, 2023 •

edited

Loading

shamardy commented May 9, 2023

onur-ozkan left a comment

ETH/ERC20/UTXO swaps with watcher rewards #1750

ETH/ERC20/UTXO swaps with watcher rewards #1750

Conversation

caglaryucekaya commented Apr 10, 2023 • edited Loading

ETH/ERC20 - ERC20/ETH

ETH taker / UTXO maker - ERC20 taker / UTXO maker

UTXO taker / ETH maker - UTXO taker / ERC20 maker

borngraced left a comment

Choose a reason for hiding this comment

shamardy left a 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

Alrighttt commented Apr 12, 2023

Alrighttt commented Apr 13, 2023

caglaryucekaya commented May 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Alrighttt commented May 5, 2023

caglaryucekaya commented May 5, 2023 • edited Loading

caglaryucekaya commented May 5, 2023

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

caglaryucekaya May 5, 2023 • edited Loading

Choose a reason for hiding this comment

shamardy commented May 9, 2023

onur-ozkan left a comment

Choose a reason for hiding this comment

caglaryucekaya commented Apr 10, 2023 •

edited

Loading

caglaryucekaya commented May 5, 2023 •

edited

Loading

caglaryucekaya May 5, 2023 •

edited

Loading