feat(utxo-swap): add utxo burn output for non-kmd taker fee #2112

dimxy · 2024-05-06T18:34:05Z

Adds a burn output sending 25% of the taker utxo DEX fee to a dedicated pre-burn address. Funds collected on the pre-burn address will be traded for KMD to burn them (thus additionally burning KMD supply).
This PR partially closes #2010

In this PR:

split dex fee for non-kmd utxo coins (apart from zcoin) and added an output to the pre-burn account
old-style non-split dex fee non-kmd txns are also allowed in validation code (until all taker nodes upgraded) - this feature is removed in favour of version-based burning activation
refactored dex fee pubkey in params: instead of passing dex fee and burn pubkeys in function params new methods dex_pubkey() and burn_pubkey() were added to the SwapOps trait
add version to maker/taker negotiation message and activate burning for the new version
mocktopus was made optional dependency and activated only for development builds (as its doc suggests).

NOTE: As mocktopus now is marked 'optional = true' in coins Cargo.toml and activated from the mm2_main crate by adding features = ["mocktopus"] in [dev-dependencies] section, you also need to mark your mockable code, called from other crates, this way: #[cfg_attr(feature = "mocktopus", mockable)], otherwise mocks won't work (see samples in code)

TODO:

fix burn pubkey and burn zaddr
Add private burn output for zcoin
disable non-split non-kmd dex fee (no burn output) validation when all taker nodes upgrade to new dex fee splitting
do not pay dex fee if taker is the dex pubkey (for non-privacy utxo)

* dev: docs(README): remove outdated information from the README (#2097) fix(sia): fix sia compilation after hd wallet PR merge (#2103) feat(hd_wallet): utxo and evm hd wallet and trezor (#1962) feat(sia): initial Sia integration (#2086) fix(BCH): deserialize BCH header that uses KAWPOW version correctly (#2099) fix(eth_tests): remove ETH_DEV_NODE from tests (#2101)

fix is_kmd in TestCoin

* dev: feat(tendermint): pubkey-only activation and unsigned tx (#2088) fix(tests): set txfee for some tbtc tests (#2116) fix(eth): remove my_address from sign_and_send_transaction_with_keypair (#2115) fix(utxo-swap): apply events occurred while taker down (#2114) refactor(memory): memory usage improvements (#2098) feat(app-dir): implement root application dir `.kdf` (#2102) fix tendermint fee calculation (#2106) update dockerfile (#2104)

* dev: feat(ETH): eip1559 gas fee estimator and rpcs (#2051) fix(p2pk-tests): fix p2pk tests post merge (#2119) fix(p2pk): show and spend P2PK balance (#2053) fix(swap): use tmp file for swap and order files (#2118)

fix zhtlc send and spend tests

add should_burn_dex_fee method to indicate which coin has burn output

* dev: fix(indexeddb): window usage in worker env (#2131) feat(tx-history): handle encoded transaction values (#2133) fix(core): tendermint withdraws on hd accounts (#2130) fix(core): improve validation rules for table names (#2123) fix(test): improve log wait condition to fix taker restart test (#2125)

…er test add timeout in wait for fee in qrc20 docker test

… docker test failed due to different dex fee values if trait default impl was used)

laruh

Great progress! Here is the first review iteration

mm2src/coins/lp_coins.rs

use same ovk for dex fee and burn outputs

add sanity check for dex_fee in calc_burn_amount_for_op_return

… not deprecated, we need support both V2 and V3)

* dev: chore(mm2_main): replace lib.rs by mm2.rs as the root lib (#2178)

…ssage; add tests for old non-versioned maker/taker

…remove mocks not working)

dimxy · 2024-08-14T15:38:06Z

In the recent 16 commits I added a 'version' field to negotiation protocol. This is done in the current PR to activate burning of non-kmd part of dex fee (a new tx output to the pre-burn account) by node's supported protocol version.
However I guess version is a useful feature for other future protocol changes (so this PR closes this issue #640 too).

How negotiation with version is working, also to provide compatibility with old nodes (see MakerSwap::negotiate and TakerSwap::negotiate fn):

Now new Maker sends both versioned and old non-versioned negotiation message to the Taker.
If Taker is new:

New Taker receives the versioned message first and responses with the versioned negotiation reply (and ignores the old negotiation message). I estimate this ordering (versioned message is sent first, non-versioned is second) should work.
New Maker receives the versioned reply and both new Taker and new Maker now know version of each other (now it is 1) and use the new DexFee (with the burning output).

If Taker is old:

it ignores the versioned message and responses with old reply.
New Maker receives the old reply and now knows the Taker is old (its version is 0) - both use old DexFee
Also, if Maker is old and Taker is new - new Taker would respond with old non-versioned reply and so on.

When all nodes upgrade we can remove old non-versioned negotiation protocol support.

I added two cargo features to allow to emulate old node behaviour (sending always non-versioned negotiation request or reply) for use with docker tests, like test_trade_base_rel_mycoin_mycoin1_coins

I think we should add the similar version field to the TPU protocol too.

…in coins code)

…Msg enum changed back for compatibility with old nodes)

shamardy

Amazing work! Next review iteration!

The below tests are failing and you need to fix them

test utxo::utxo_tests::test_validate_old_fee_tx ... FAILED
test lp_swap::recreate_swap_data::tests::test_recreate_maker_swap ... FAILED
test lp_swap::recreate_swap_data::tests::test_recreate_maker_swap_maker_payment_wait_confirm_failed ... FAILED
test lp_swap::recreate_swap_data::tests::test_recreate_taker_swap ... FAILED
test lp_swap::recreate_swap_data::tests::test_recreate_taker_swap_taker_payment_wait_confirm_failed ... FAILED

Please also add To Test to the opening PR comment #2112 (comment) for QA to know what to test after the PR is merged.

P.S. we still need to add protocol version to v2 swaps, and burning for EVM TPU. We can add them in another PR but these are required for TPU release c.c. @laruh

shamardy · 2024-08-23T00:50:37Z

mm2src/coins/lp_coins.rs

@@ -230,6 +233,7 @@ pub mod coin_balance;
 use coin_balance::{AddressBalanceStatus, HDAddressBalance, HDWalletBalanceOps};

 pub mod lp_price;
+pub mod swap_features;


Shouldn't this be part of mm2src/mm2_main/src/lp_swap/ instead?

I did like that initially but had to put it into the coins mod (mm2_main depends on it) as the coins code need to use it too

shamardy · 2024-08-23T00:52:43Z

mm2src/coins/lp_coins.rs

+/// Default swap protocol version before version field added to NegotiationDataMsg
+pub const LEGACY_PROTOCOL_VERSION: u16 = 0;
+
+/// Current swap protocol version
+pub const SWAP_PROTOCOL_VERSION: u16 = 1;
+
+/// Minimal supported swap protocol version implemented by remote peer
+pub const MIN_SWAP_PROTOCOL_VERSION: u16 = LEGACY_PROTOCOL_VERSION;
+
+// TODO: add version field to the SWAP V2 negotiation protocol


Same thing for these constants.

same as above

shamardy · 2024-08-23T01:01:07Z

mm2src/coins/lp_coins.rs

+/// Current swap protocol version
+pub const SWAP_PROTOCOL_VERSION: u16 = 1;
+
+/// Minimal supported swap protocol version implemented by remote peer


The version is not implemented by the remote peer, how about
/// Minimal swap protocol version required by remote peer to swap with local peer running SWAP_PROTOCOL_VERSION

I meant with with const MIN_SWAP_PROTOCOL_VERSION we will reject too old nodes (with their version less than MIN_SWAP_PROTOCOL_VERSION)

shamardy · 2024-08-23T01:13:25Z

mm2src/coins/lp_coins.rs

+                burn_amount,
+                burn_destination: DexFeeBurnDestination::KmdOpReturn,
+            };
+        } else if taker_coin.should_burn_dex_fee() && burn_active {


This is interesting since we might add more coins with should_burn_dex_fee being true in the future so the check for burn_active and should_burn_dex_fee will not be enough for backward compatibility and we will add another bool, I guess we will make should_burn_dex_fee result depend on swap protocol version so do we need to pass burn_active or swap protocol version instead? I guess this can be handled in the future anyways even the swap feature name

komodo-defi-framework/mm2src/coins/swap_features.rs

Line 5 in 44a2586

SendToPreBurnAccount,

can be changed in the future as the naming is local and not part of the p2p messages.

shamardy · 2024-08-23T01:17:23Z

mm2src/coins/swap_features.rs

+#[derive(PartialEq)]
+pub enum SwapFeature {
+    SendToPreBurnAccount,
+}
+
+impl SwapFeature {
+    // add new features to activate
+    const SWAP_FEATURE_ACTIVATION: &[(u16, SwapFeature)] = &[(1, SwapFeature::SendToPreBurnAccount)];
+
+    pub fn is_active(feature: SwapFeature, version: u16) -> bool {
+        if let Some(found) = Self::SWAP_FEATURE_ACTIVATION.iter().find(|fv| fv.1 == feature) {
+            return version >= found.0;
+        }
+        false
+    }
+}


#[derive(PartialEq)] pub enum SwapFeature { SendToPreBurnAccount = 1, } impl SwapFeature { pub fn is_active(feature: SwapFeature, version: u16) -> bool { version >= feature as u16 } }

Much simpler this way

But we may have multiple features activated with the same version
(actually I did part similar to one in zebra codebase)

shamardy · 2024-08-23T03:13:19Z

mm2src/mm2_main/src/lp_swap.rs

 /// Do not add refund fee to calculate fee needed only to make a successful swap
 pub(crate) const NO_REFUND_FEE: bool = false;

+/// Sending part of dex fee to the pre-burn account is active
+pub const PRE_BURN_ACCOUNT_ACTIVE: bool = true;


Let's move all constants to the top.

fixed in 74d8ea3

shamardy · 2024-08-23T03:41:32Z

mm2src/mm2_main/src/lp_swap/maker_swap.rs

+        if remote_version < MIN_SWAP_PROTOCOL_VERSION {
+            self.broadcast_negotiated_false();
+            return Ok((Some(MakerSwapCommand::Finish), vec![MakerSwapEvent::NegotiateFailed(
+                ERRL!("Taker protocol version {} too old", remote_version).into(),


It would be good to also show the current supported version in this error.

fixed in 1def60d

shamardy · 2024-08-23T03:52:43Z

mm2src/mm2_main/src/lp_swap/taker_swap.rs

+        #[allow(clippy::absurd_extreme_comparisons)]
+        if remote_version < MIN_SWAP_PROTOCOL_VERSION {
+            return Ok((Some(TakerSwapCommand::Finish), vec![TakerSwapEvent::NegotiateFailed(
+                ERRL!("Maker protocol version {} too old", remote_version).into(),


Same here for adding taker version in the error.

fixed in 1def60d

shamardy · 2024-08-23T03:55:07Z

mm2src/mm2_main/src/lp_swap/taker_swap.rs

+            .r()
+            .data
+            .maker_version
+            .ok_or("No swap protocol version".to_owned())?;


I don't think we should return string errors from swap logic / commands

But all functions in this src return errors as String (what, I agree, is not good but maybe it's a task for a dedicated PR to refactor code)

shamardy · 2024-08-23T03:58:37Z

mm2src/mm2_main/tests/docker_tests/docker_tests_inner.rs

@@ -3890,6 +3890,7 @@ fn test_eth_swap_negotiation_fails_maker_no_fallback() {
 #[test]
 fn test_trade_base_rel_eth_erc20_coins() { trade_base_rel(("ETH", "ERC20DEV")); }

+// run this test also with features "test-use-old-maker" or "test-use-old-taker" to emulate non-versioned nodes


I guess this is a todo, right?

removed (made dedicated tests instead)

… fee not stored but recalculated now)

…est DexFee::NoFee) fix tests for versioned negotiation to use env var instead of feature

…wap v2

* dev: chore(RPCs): rename `get_peers_info` RPC to `get_directly_connected_peers` (#2195) chore(WASM-builds): remove `wasm-opt` overriding (#2200) fix(coins): add p2p feature to mm2_net dependency (#2210) chore(test): turn on debug assertion (#2204) feat(sia): extract sia lib to external repo (#2167) feat(eth-swap): eth tpu v2 methods, eth docker test enhancements (#2169) fix(cors): allow OPTIONS request to KDF server (#2191) docs(README): update commit badges to use dev branch (#2193) use default value for `komodo_proxy` (#2192) feat(cosmos): komodo-defi-proxy support (#2173)

dimxy added 9 commits April 27, 2024 14:33

make mockable optional

f3625e5

add support for utxo coin burn account to dex fee

d3f843d

remove left over dup dex_fee_rate()

7894f7d

fix coins crate mocktopus dependency for mm2_main crate tests

2984c67

fix dex fee changed due to new burn output in couple of lp_swap tests

128c4b4

fix is_kmd in TestCoin

fix swap v2 test non-kmd burn output

23f2a08

fix fmt

2c810b0

remove comment

af45b71

dimxy changed the title ~~Add utxo burn output for non-kmd taker fee~~ feat(utxo-swap): add utxo burn output for non-kmd taker fee May 6, 2024

dimxy added in progress Changes will be made from the author under review and removed in progress Changes will be made from the author labels May 6, 2024

dimxy assigned shamardy May 19, 2024

Merge branch 'dev' into burn-dex-fee-for-non-kmd

0f34d2e

* dev: feat(ETH): eip1559 gas fee estimator and rpcs (#2051) fix(p2pk-tests): fix p2pk tests post merge (#2119) fix(p2pk): show and spend P2PK balance (#2053) fix(swap): use tmp file for swap and order files (#2118)

dimxy added in progress Changes will be made from the author and removed under review labels Jun 9, 2024

dimxy added 9 commits June 14, 2024 20:54

fix zombie chain scan block num

d53dc37

fix zhtlc send and spend tests

add burn output for privacy coin,

81263e0

add should_burn_dex_fee method to indicate which coin has burn output

use custom log! instead debug! (as env_logger disabled) in qrc20 dock…

9331e40

…er test add timeout in wait for fee in qrc20 docker test

fix to make should_burn_dex_fee explicitly in each coin (to fix qrc20…

b343275

… docker test failed due to different dex fee values if trait default impl was used)

restore original test log message

587b1ae

add test for std dex fee for zcoin

58d84cf

remove unused cache dir var in z_coin init block

1243935

fix clippy

bc00ee8

laruh requested changes Jun 18, 2024

View reviewed changes

dimxy added 2 commits June 19, 2024 13:05

fix burn addr in ZOutput

4d59545

use same ovk for dex fee and burn outputs

refactor dex fee code;

dc9e710

add sanity check for dex_fee in calc_burn_amount_for_op_return

dimxy added 10 commits August 12, 2024 22:29

added NegotiationDataMsgVersion, removed NegotiationDataV4 (bcz V2 is…

22b5c42

… not deprecated, we need support both V2 and V3)

fix activation ver

9951626

Merge branch 'dev' into burn-dex-fee-for-non-kmd

d72a313

* dev: chore(mm2_main): replace lib.rs by mm2.rs as the root lib (#2178)

fix after merge

a63c3e0

remove relaxed dex fee validation (in favour versioned check)

983d8ef

add mocktopus to mm2_main

849b190

fix version in recreate swap

8e2fdaf

fix node version determination: try versioned then old negotiation me…

b844318

…ssage; add tests for old non-versioned maker/taker

add test feature to emulate old non-versioned negotiation msgs (also …

ddb4863

…remove mocks not working)

fix fmt

9a34268

fix swap tx spend size

f8b543b

shamardy mentioned this pull request Aug 16, 2024

feat(eth-swap): eth tpu v2 methods, eth docker test enhancements #2169

Merged

13 tasks

dimxy added 5 commits August 17, 2024 11:39

move version activation swap_feature mod to coins (to allow using it …

44a2586

…in coins code)

add delay between broadcasts of new and old messages

e6fe67f

fix old (non-versioned) taker and maker emulation (for tests)

b20e679

fix swap proto version: use new topic for versioned negotiation (Swap…

e930f58

…Msg enum changed back for compatibility with old nodes)

fix comment

80d34f6

shamardy reviewed Aug 23, 2024

View reviewed changes

dimxy added 8 commits August 24, 2024 14:53

add DexFee::NoFee option support for utxo if taker is dex pubkey (dex…

5cc210e

… fee not stored but recalculated now)

add tests for swap v1 mycoin to mycoin with dex pubkey as Alice (to t…

69cbfa4

…est DexFee::NoFee) fix tests for versioned negotiation to use env var instead of feature

fix tests to validate fee and recreate swaps

248536a

fix DexFee::NoFee taker payment spending outputs for swaps V2

b0fabcf

fix tests for no dex fee for swaps v1, add tests for no dex fee for s…

f6cc684

…wap v2

fix TODO for delay in broadcast msgs

7053e2e

fixes for review notes

1def60d

fixes for review notes

74d8ea3

dimxy added under review and removed in progress Changes will be made from the author labels Aug 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(utxo-swap): add utxo burn output for non-kmd taker fee #2112

feat(utxo-swap): add utxo burn output for non-kmd taker fee #2112

dimxy commented May 6, 2024 •

edited

Loading

laruh left a comment

dimxy commented Aug 14, 2024 •

edited

Loading

shamardy left a comment •

edited

Loading

shamardy Aug 23, 2024

dimxy Aug 26, 2024

shamardy Aug 23, 2024

dimxy Aug 26, 2024

shamardy Aug 23, 2024

dimxy Aug 26, 2024

shamardy Aug 23, 2024

shamardy Aug 23, 2024

dimxy Aug 26, 2024

shamardy Aug 23, 2024

dimxy Aug 26, 2024

shamardy Aug 23, 2024

dimxy Aug 26, 2024

shamardy Aug 23, 2024

dimxy Aug 26, 2024

shamardy Aug 23, 2024

dimxy Aug 26, 2024

shamardy Aug 23, 2024

dimxy Aug 26, 2024

feat(utxo-swap): add utxo burn output for non-kmd taker fee #2112

Are you sure you want to change the base?

feat(utxo-swap): add utxo burn output for non-kmd taker fee #2112

Conversation

dimxy commented May 6, 2024 • edited Loading

laruh left a comment

Choose a reason for hiding this comment

dimxy commented Aug 14, 2024 • edited Loading

shamardy left a comment • edited Loading

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

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

dimxy commented May 6, 2024 •

edited

Loading

dimxy commented Aug 14, 2024 •

edited

Loading

shamardy left a comment •

edited

Loading