feat(tpu): implement 0 dexfee for kmd trading pairs by borngraced · Pull Request #2323 · GLEECBTC/komodo-defi-framework

borngraced · 2025-01-16T14:52:38Z

onur-ozkan · 2025-02-05T11:54:21Z

mm2src/coins/eth.rs

+        let fee_coin = match &self.coin_type {
+            EthCoinType::Eth => self.ticker.to_owned(),
+            EthCoinType::Erc20 { platform, .. } => platform.to_owned(),
+            EthCoinType::Nft { .. } => return MmError::err(TradePreimageError::NftProtocolNotSupported),
+        };


You can use platform_ticker instead. I am not sure if you intentionally drop the nft support, but if we want to do that we need to add a if check before using platform_ticker.

onur-ozkan · 2025-02-05T11:56:28Z

mm2src/coins/lp_coins.rs

+    /// Check and return true if DexFee is not required to trade otherwise return false.
+    pub fn no_fee(&self) -> bool { matches!(self, Self::Zero) }


Suggested change

/// Check and return true if DexFee is not required to trade otherwise return false.

pub fn no_fee(&self) -> bool { matches!(self, Self::Zero) }

/// Check and return true if DexFee is not required to trade otherwise return false.

pub fn zero_fee(&self) -> bool { matches!(self, Self::Zero) }

onur-ozkan

Previous notes are also still valid

onur-ozkan · 2025-02-18T05:46:33Z

mm2src/coins/utxo/slp.rs

    ) -> TradePreimageResult<TradeFee> {
+        if dex_fee_amount.no_fee() {
+            return Ok(TradeFee {
+                coin: self.platform_coin.ticker().into(),


nit:

Suggested change

coin: self.platform_coin.ticker().into(),

coin: self.platform_coin.ticker().to_owned(),

mm2src/mm2_main/src/lp_swap/maker_swap_v2.rs

onur-ozkan · 2025-02-18T06:04:04Z

mm2src/mm2_main/src/lp_swap/maker_swap.rs

        };
        swap_events.push(MakerSwapEvent::MakerPaymentInstructionsReceived(instructions));

+        let taker_amount = MmNumber::from(self.taker_amount.clone());


MmNumber::from only takes a reference inside the function. You can implement:

impl From<&BigDecimal> for MmNumber { fn from(n: &BigDecimal) -> MmNumber { from_dec_to_ratio(n).into() } }

to avoid unnecessary cloning in cases like this.

yeah, we have such issue
#2338

@borngraced could you please introduce From<&BigDecimal> for MmNumber and start using it for your changes. after pr merge someone can take refactoring as a separate task.

laruh · 2025-02-19T05:47:40Z

@borngraced could you tell why status is blocked? or was it misclick?

onur-ozkan

Just a couple of non-blocker tiny nits, thanks for the fixes!

LGTM

onur-ozkan · 2025-02-19T07:35:17Z

mm2src/coins/lp_coins.rs

        }
    }
+
+    /// Check and return true if DexFee is not required to trade otherwise return false.


tiny nit:

Suggested change

/// Check and return true if DexFee is not required to trade otherwise return false.

/// Whether this is DexFee::Zero.

onur-ozkan · 2025-02-19T07:37:19Z

mm2src/mm2_main/src/lp_swap/maker_swap_v2.rs

+        let is_kmd = "KMD" == recreate_ctx.maker_coin.ticker() || "KMD" == recreate_ctx.taker_coin.ticker();
+        let dex_fee = if is_kmd {
+            DexFee::Zero


Suggested change

let is_kmd = "KMD" == recreate_ctx.maker_coin.ticker() || "KMD" == recreate_ctx.taker_coin.ticker();

let dex_fee = if is_kmd {

DexFee::Zero

let dex_fee = if "KMD" == recreate_ctx.maker_coin.ticker() || "KMD" == recreate_ctx.taker_coin.ticker() {

DexFee::Zero

onur-ozkan · 2025-02-19T07:38:18Z

mm2src/mm2_main/src/lp_swap/taker_swap_v2.rs

+        let is_kmd = "KMD" == recreate_ctx.maker_coin.ticker() || "KMD" == recreate_ctx.taker_coin.ticker();
+        let dex_fee = if is_kmd {
+            DexFee::Zero


Suggested change

let is_kmd = "KMD" == recreate_ctx.maker_coin.ticker() || "KMD" == recreate_ctx.taker_coin.ticker();

let dex_fee = if is_kmd {

DexFee::Zero

let dex_fee = if "KMD" == recreate_ctx.maker_coin.ticker() || "KMD" == recreate_ctx.taker_coin.ticker() {

DexFee::Zero

borngraced · 2025-02-19T09:37:39Z

@borngraced could you tell why status is blocked? or was it misclick?

this #2112 seems related and not sure which one will be merge first as both have some similar changes

laruh

LGTM!

laruh

Thanks, next iteration
also please re check zero fee leftovers in legacy functionality

mm2src/coins/qrc20.rs

mm2src/coins/lp_coins.rs

mm2src/coins/tendermint/tendermint_coin.rs

mm2src/mm2_main/src/lp_swap.rs

laruh · 2025-03-05T08:56:44Z

mm2src/coins/utxo/utxo_common.rs

    let sig_hash_type = match args.dex_fee {
        DexFee::Standard(_) => SIGHASH_SINGLE,
-        DexFee::WithBurn { .. } => SIGHASH_ALL,
+        DexFee::WithBurn { .. } | DexFee::Zero => SIGHASH_ALL,


Could you clarify why we should use SIGHASH_ALL for zero fee?

SIGHASH_ALL: Signs all inputs and outputs, preventing any changes after signing
SIGHASH_SINGLE: Only signs the input and the corresponding output at the same index, allowing other outputs to be modified.

I guess using either for zero fees won't make a significant diff. cc @dimxy

ok I was just curious will it be the difference for zero fee, as for Standard dex fee we use SIGHASH_SINGLE

Yes, we should use SIGHASH_ALL wherever possible.
SIGHASH_SINGLE could be used only in a special case (DexFee::Standard) when maker can add an output to the spending taker payment transaction (with only a single dexfee output signed by the taker with the SIGHASH_SINGLE flag), without the other party signature.
(this is how the SIGHASH_SINGLE feature works)

laruh

Almost done!
pls update SEPOLIA_TAKER_SWAP_V2 address to this one 0x3B19873b81a6B426c8B2323955215F7e89CfF33F

https://github.com/KomodoPlatform/komodo-defi-framework/blob/8ba9aaba47b56b990d132df37d3abbc74aa94cd6/mm2src/mm2_main/tests/docker_tests/docker_tests_common.rs#L1592

above SEPOLIA_TAKER_SWAP_V2 you can leave comment that this version was deployed
https://github.com/KomodoPlatform/etomic-swap/blob/bbdeb369eab0753454dae29640adaa4fdba221ba/contracts/EtomicSwapTakerV2.sol

and in taker_send_approve_and_spend_erc20 test change dex fee to &DexFee::Zero
https://github.com/KomodoPlatform/komodo-defi-framework/blob/8ba9aaba47b56b990d132df37d3abbc74aa94cd6/mm2src/mm2_main/tests/docker_tests/eth_docker_tests.rs#L1806

laruh

Thanks LGTM!

shamardy · 2025-03-06T13:52:57Z

Blocked as this requires merging #2112 first

dimxy

Approving.
This PR is very close to #2112.
#2112 is larger in size and changes how DexFee is created (in a dedicated function) so it will affect this PR code.
I think it's easier to merge this PR first and then incorporate the changes into #2112.

(Maybe it's good to add a full swap test for zero dexfee though)

# Conflicts: # mm2src/coins/lp_coins.rs # mm2src/coins/utxo/utxo_common.rs # mm2src/mm2_main/src/lp_swap.rs # mm2src/mm2_main/src/lp_swap/maker_swap.rs # mm2src/mm2_main/src/lp_swap/maker_swap_v2.rs # mm2src/mm2_main/src/lp_swap/taker_swap_v2.rs # mm2src/mm2_main/tests/docker_tests/swap_watcher_tests.rs

shamardy

LGTM! I have 2 questions about what appears to be unneeded changes.

mm2src/mm2_main/tests/docker_tests/eth_docker_tests.rs

mm2src/mm2_main/tests/docker_tests/docker_tests_common.rs

* dev: improvement(best-orders): return an rpc error when we can't find best orders (#2318) feat(utxo): support FIRO Spark verbose tx feat(ARRR): dockerize zombie/pirate tests (#2374) improvement(event-streaming): move UnknownClient error to trace level (#2401) feat(tpu): implement 0 dexfee for kmd trading pairs (#2323) feat(db-arch): ctx functions and use of global db (#2378) feat(swap): add utxo/cosmos/ARRR pre-burn address output (#2112)

* lr-swap-wip: (45 commits) review (mariocynicys): fix iterators zipping, refactor 1inch url builder, add docs to cross prices data, remove extra coin decimals check added doc comments for LrData struct error msg improved fix tx value eth conversion eliminate from_api_error fn fix src_decimals var name improve bad api TokenInfo error messages improvement(best-orders): return an rpc error when we can't find best orders (GLEECBTC#2318) feat(utxo): support FIRO Spark verbose tx feat(ARRR): dockerize zombie/pirate tests (GLEECBTC#2374) improvement(event-streaming): move UnknownClient error to trace level (GLEECBTC#2401) feat(tpu): implement 0 dexfee for kmd trading pairs (GLEECBTC#2323) feat(db-arch): ctx functions and use of global db (GLEECBTC#2378) feat(swap): add utxo/cosmos/ARRR pre-burn address output (GLEECBTC#2112) review (laruh): rename fn review (laruh): add fn to get contracts from LrData add TODO fix find best lr swap behaviour: skip lr provider error results (to use successful ones) refactor 1inch url builder fix 1inch result conversion test ...

borngraced added 6 commits January 14, 2025 06:09

impl zero dex fee for KMD

9059f93

fix swap unit test

450ced6

fix docker test and dissallow sending dex fee

0cc613a

fix gen_taker_payment_spend_preimage

49f08ac

check for zero dex fee in other protocol

1a8539a

eth and tendermint

dc45a35

borngraced added the status: in progress label Jan 16, 2025

borngraced self-assigned this Jan 16, 2025

borngraced changed the title ~~feat:(swap) implement 0 dexfee for kmd trading pairs~~ feat(swap) implement 0 dexfee for kmd trading pairs Jan 16, 2025

check for KMD ticker in swap_v2

f62a0de

borngraced changed the title ~~feat(swap) implement 0 dexfee for kmd trading pairs~~ feat(swap): implement 0 dexfee for kmd trading pairs Jan 16, 2025

borngraced added 2 commits January 20, 2025 15:55

save dev state

2d6ae32

update taker fee validated log info

a6cb780

onur-ozkan reviewed Feb 5, 2025

View reviewed changes

shamardy added the 2.5.0-beta label Feb 17, 2025

shamardy requested review from laruh and onur-ozkan February 17, 2025 10:08

onur-ozkan reviewed Feb 18, 2025

View reviewed changes

borngraced added the status: blocked label Feb 18, 2025

borngraced added 2 commits February 18, 2025 14:01

fix review notes, add MmNumber from ref BigDecimal

06ccb20

delegate Zero dex fee check to taker_swap

76cd46e

onur-ozkan previously approved these changes Feb 19, 2025

View reviewed changes

fix nits

4358025

borngraced dismissed onur-ozkan’s stale review via 4358025 February 19, 2025 10:03

laruh previously approved these changes Feb 19, 2025

View reviewed changes

borngraced removed status: blocked status: in progress labels Feb 19, 2025

borngraced added 3 commits March 3, 2025 19:25

revert taker_swap

a50238f

nits

d2e766e

revert dex_fee_amount

da5fc4e

laruh suggested changes Mar 4, 2025

View reviewed changes

mm2src/coins/qrc20.rs Outdated Show resolved Hide resolved

mm2src/coins/lp_coins.rs Outdated Show resolved Hide resolved

mm2src/coins/tendermint/tendermint_coin.rs Outdated Show resolved Hide resolved

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

fix review notes

8ba9aab

laruh reviewed Mar 5, 2025

View reviewed changes

laruh suggested changes Mar 6, 2025

View reviewed changes

update test constant and taker_send_approve_and_spennd_erc20 test

dd33004

laruh previously approved these changes Mar 6, 2025

View reviewed changes

shamardy added status: blocked and removed status: in progress labels Mar 6, 2025

shamardy requested review from dimxy and shamardy March 17, 2025 11:19

dimxy previously approved these changes Mar 17, 2025

View reviewed changes

borngraced added 2 commits March 24, 2025 09:05

Merge branch 'dev' into 0-kmd-dex-fee

84e5b7f

borngraced dismissed stale reviews from dimxy and laruh via 0d3be66 March 25, 2025 11:32

borngraced added 2 commits March 25, 2025 12:38

post merge fix

c714e4e

NoFee for swap with KMD coins

1afe061

shamardy added status: pending review and removed status: blocked labels Mar 25, 2025

shamardy approved these changes Mar 28, 2025

View reviewed changes

mm2src/mm2_main/tests/docker_tests/eth_docker_tests.rs Show resolved Hide resolved

mm2src/mm2_main/tests/docker_tests/docker_tests_common.rs Show resolved Hide resolved

shamardy changed the title ~~feat(swap): implement 0 dexfee for kmd trading pairs~~ feat(tpu): implement 0 dexfee for kmd trading pairs Mar 28, 2025

shamardy merged commit 9c6f1ee into dev Mar 28, 2025
26 of 27 checks passed

shamardy deleted the 0-kmd-dex-fee branch March 28, 2025 22:31

shamardy mentioned this pull request Apr 16, 2025

chore(release): v2.4.0-beta #2345

Merged

		/// Check and return true if DexFee is not required to trade otherwise return false.
		pub fn no_fee(&self) -> bool { matches!(self, Self::Zero) }

	coin: self.platform_coin.ticker().into(),
	coin: self.platform_coin.ticker().to_owned(),

	/// Check and return true if DexFee is not required to trade otherwise return false.
	/// Whether this is DexFee::Zero.

Conversation

borngraced commented Jan 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

onur-ozkan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laruh commented Feb 19, 2025

Uh oh!

onur-ozkan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

borngraced commented Feb 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laruh left a comment

Choose a reason for hiding this comment

Uh oh!

laruh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dimxy Mar 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laruh left a comment

Choose a reason for hiding this comment

Uh oh!

laruh left a comment

Choose a reason for hiding this comment

Uh oh!

shamardy commented Mar 6, 2025

Uh oh!

dimxy left a comment

Choose a reason for hiding this comment

Uh oh!

shamardy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

borngraced commented Jan 16, 2025 •

edited

Loading

borngraced commented Feb 19, 2025 •

edited

Loading

dimxy Mar 6, 2025 •

edited

Loading