feat(tests): zcoin unit test to validate dex fee by dimxy · Pull Request #2460 · GLEECBTC/komodo-defi-framework

dimxy · 2025-05-20T14:04:02Z

Adds a framework for creating a zcoin unit tests to call and test Zcoin {} validation functions.
~~Also fixes a bug with address check in the validate_dex_fee_output fn (and adds a unit test for it).~~ Was fixed in another PR before this, this one adds the unit test for it.

PS: also removed 'multicore' feature from zcash_proofs for the wasm platform (as this caused errors in a wasm test)

…o avoid creating ZOMBIE.conf)

borngraced · 2025-05-21T14:31:53Z

mm2src/coins/z_coin/z_coin_native_tests.rs

 use crate::{DexFee, DexFeeBurnDestination};
 use mm2_number::MmNumber;
-
+dd


removed the file

shamardy

Thanks for the PR! It adds quite a lot of code just to test a very specific thing—maybe this zcoin unit testing framework can be used for other cases in the future, though. One concern: since the test code depends on ZCoin’s internal structure and initialization, we’ll have to change and maintain it whenever those internals change, which could make these tests fragile and harder to keep up-to-date. But I do get that the extra setup is necessary. Just out of curiosity, why didn't you cover this case in dockerized tests instead?

mm2src/coins/Cargo.toml

shamardy · 2025-05-22T21:58:40Z

mm2src/coins/z_coin.rs

+    #[cfg(all(test, not(target_arch = "wasm32")))]
+    UnitTests,


Can't we do the tests for wasm as well using cross_test

I tried to make it a cross_test and was getting a failure on wasm with an error in zcash_proofs/bellman::multicore mod:

Error { kind: Unsupported, message: "operation not supported on this platform" }

I managed to fix this error to by disabling the multicore feature in zcash_proofs for wasm dependencies (like it had been before the workspace dependencies PR #2449. See zcash_proofs in the main branch Cargo.toml ).
@borngraced you may want to disable multicore for zcash_proofs for wasm, as we now have zcash_proofs only with multicore in workspace deps.

PS However the wasm test still would not work as looks like mocktopus is not working there
(I even tried to make mocktopus as a release dependency but still no luck)

weird, even tho mocktopus supports WASM

@dimxy please could you be more specific on the issue you had using mocktopus in wasm?

mm2src/coins/z_coin.rs

mm2src/coins/z_coin/z_rpc.rs

shamardy · 2025-05-22T23:08:57Z

@dimxy please always check that CI passes before making a PR ready for review.

mm2src/coins/z_coin.rs

dimxy · 2025-05-27T15:40:07Z

Just out of curiosity, why didn't you cover this case in dockerized tests instead?

To add a test for this case I needed to mock dex and burn addresses. This is convenient to do in unit tests.
Since we have unit tests for other coins I thought it's a moment to add a similar framework for zcoin as well. Ofc the idea was to use it not only for this case but for others as well (like we do this for utxo for e.g.).

dimxy · 2025-05-27T15:57:38Z

@dimxy please always check that CI passes before making a PR ready for review.

Okay (I guess it was a temp runner failure when I last updated this PR)

* dev: fix(evm-api): find enabled erc20 token using platform ticker (#2445) chore(docs): add DeepWiki badge to README (#2463) chore(core): organize deps using workspace.dependencies (#2449) feat(db-arch): more dbdir to address_dir replacements (#2398)

borngraced

last notes from my side

borngraced · 2025-05-27T21:35:23Z

mm2src/coins/z_coin.rs

+        add_test_spend(
+            coin,
+            &mut tx_builder,
+            dex_params.1
+                + if let Some(ref burn_params) = burn_params {
+                    burn_params.1
+                } else {
+                    0
+                }
+                + u64::from(DEFAULT_FEE),
+        );


Suggested change

add_test_spend(

coin,

&mut tx_builder,

dex_params.1

+ if let Some(ref burn_params) = burn_params {

burn_params.1

} else {

0

}

+ u64::from(DEFAULT_FEE),

);

let amount = dex_params.1 + burn_params.map(|(_, v)| v).unwrap_or_default() + u64::from(DEFAULT_FEE);

add_test_spend(coin, &mut tx_builder, amount);

mm2src/coins/z_coin.rs

onur-ozkan

LGTM other than one note:

mm2src/coins/z_coin.rs

shamardy · 2025-05-28T08:45:03Z

@borngraced can you please check if your notes have been resolved and approve if so

borngraced · 2025-05-28T09:01:10Z

@borngraced can you please check if your notes have been resolved and approve if so

#2460 (comment)

shamardy · 2025-05-30T00:03:55Z

@dimxy test_validate_zcoin_dex_fee is currently failing in CI

dimxy · 2025-05-31T09:31:41Z

@dimxy test_validate_zcoin_dex_fee is currently failing in CI

Oh, so we did not download zcash params in the CI host OS. I have added code for this (for tests) ae28620

* dev: (30 commits) chore(core): replace hash_raw_entry with stable entry() API (GLEECBTC#2473) chore(core): adapt `MmError` and usages for compatibility with new rustc versions (GLEECBTC#2443) feat(wallet): add `delete_wallet` RPC (GLEECBTC#2497) chore(release): add changelog entries for v2.5.0-beta (GLEECBTC#2494) chore(release): bump kdf version to 2.5.0-beta (GLEECBTC#2492) feat(tests): zcoin unit test to validate dex fee (GLEECBTC#2460) fix(zcoin): correctly track unconfirmed z-coin notes (GLEECBTC#2331) improvement(orders): remove BTC specific volume from min_trading_vol logic (GLEECBTC#2483) feat(ibc-routing-part-2): supporting entire Cosmos network for swaps (GLEECBTC#2476) fix(startup): don't initialize WalletConnect during startup (GLEECBTC#2485) fix(dns): better ip resolution (GLEECBTC#2487) improvement(event-streaming): strong type streamer IDs (GLEECBTC#2441) bump timed-map to `1.4.1` (GLEECBTC#2481) improvement(RPC): unified interface for legacy and current RPC interfaces (GLEECBTC#2450) improvement(tendermint): `tendermint_tx_internal_id` helper (GLEECBTC#2438) feat(walletconnect): add WalletConnect v2 support for EVM and Cosmos (GLEECBTC#2223) feat(ibc-routing-part-1): supporting entire Cosmos network for swaps (GLEECBTC#2459) fix(test): fix HD Wallet message signing tests (GLEECBTC#2474) improvement(builds): enable static CRT linking for MSVC builds (GLEECBTC#2464) feat(wallet): implement HD multi-address support for message signing (GLEECBTC#2432) ... # Conflicts: # mm2src/coins/qrc20.rs # mm2src/mm2_main/src/lp_swap/maker_swap.rs

* dev: chore(core): replace hash_raw_entry with stable entry() API (#2473) chore(core): adapt `MmError` and usages for compatibility with new rustc versions (#2443) feat(wallet): add `delete_wallet` RPC (#2497) chore(release): add changelog entries for v2.5.0-beta (#2494) chore(release): bump kdf version to 2.5.0-beta (#2492) feat(tests): zcoin unit test to validate dex fee (#2460) # Conflicts: # mm2src/coins/qrc20.rs # mm2src/mm2_main/src/lp_swap/maker_swap.rs

dimxy added 2 commits May 20, 2025 18:51

fix address check in zcoin validate_dex_fee_output

5a841c6

add zcoin validate dex fee unit tests

518b7ae

dimxy added status: pending review priority: medium Moderately important tasks that should be completed but are not urgent. bug: swap improvement: tests labels May 20, 2025

dimxy mentioned this pull request May 20, 2025

fix(zcoin): correctly track unconfirmed z-coin notes #2331

Merged

dimxy added status: in progress and removed status: pending review labels May 20, 2025

use electrum client (instead of native) to mock in zcoin unit test (t…

820532e

…o avoid creating ZOMBIE.conf)

borngraced suggested changes May 21, 2025

View reviewed changes

remove z_coin_native_tests.rs (not used anymore)

fa7bf0d

dimxy added status: pending review and removed status: in progress labels May 22, 2025

shamardy reviewed May 22, 2025

View reviewed changes

borngraced reviewed May 26, 2025

View reviewed changes

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

onur-ozkan reviewed May 27, 2025

View reviewed changes

mm2src/coins/z_coin.rs Show resolved Hide resolved

dimxy added 7 commits May 27, 2025 21:13

move test deps to dev dependencies

d74cefe

fix after merge: remove unused db param in test zcoin init

bd45704

remove unused imports in test

d9a58e9

review (shamardy): restore SaplingSyncConnector visibility

d8d59fc

fix cfg test for ZcoinRpcMode::UnitTests

10caa50

fix zcoin test deps as workspace

196a597

borngraced suggested changes May 27, 2025

View reviewed changes

onur-ozkan previously approved these changes May 28, 2025

View reviewed changes

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

shamardy added the 2.5.0-beta label May 28, 2025

dimxy added 2 commits May 28, 2025 14:02

add cfg to disable wasm for zcoin unit tests

e0492b7

disable zcash_proof multicore feaure for wasm

62bcffe

dimxy dismissed onur-ozkan’s stale review via 62bcffe May 28, 2025 09:03

borngraced previously approved these changes May 28, 2025

View reviewed changes

add download zcash params for unit tests

ae28620

dimxy dismissed borngraced’s stale review via ae28620 May 31, 2025 08:42

dimxy and others added 2 commits May 31, 2025 14:49

refactor: move zcoin unit tests in separate file

fd4f2e2

Merge branch 'dev' into zcoin-dex-fee-unit-test

be3818d

shamardy approved these changes Jun 16, 2025

View reviewed changes

shamardy changed the title ~~feat(tests): zcoin unit test to validate dex fee, fix address check bug~~ feat(tests): zcoin unit test to validate dex fee Jun 16, 2025

shamardy merged commit dc39db7 into dev Jun 16, 2025
26 checks passed

shamardy deleted the zcoin-dex-fee-unit-test branch June 16, 2025 05:57

shamardy mentioned this pull request Jun 17, 2025

chore(release): v2.5.0-beta #2491

Merged

Conversation

dimxy commented May 20, 2025 • edited by shamardy Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

borngraced May 21, 2025

Choose a reason for hiding this comment

Uh oh!

dimxy May 27, 2025

Choose a reason for hiding this comment

Uh oh!

shamardy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

shamardy May 22, 2025

Choose a reason for hiding this comment

Uh oh!

dimxy May 27, 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!

borngraced May 28, 2025

Choose a reason for hiding this comment

Uh oh!

borngraced May 28, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

shamardy commented May 22, 2025

Uh oh!

Uh oh!

Uh oh!

dimxy commented May 27, 2025

Uh oh!

dimxy commented May 27, 2025

Uh oh!

borngraced left a comment

Choose a reason for hiding this comment

Uh oh!

borngraced May 27, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

onur-ozkan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

shamardy commented May 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

borngraced commented May 28, 2025

Uh oh!

shamardy commented May 30, 2025

Uh oh!

dimxy commented May 31, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dimxy commented May 20, 2025 •

edited by shamardy

Loading

dimxy May 27, 2025 •

edited

Loading

shamardy commented May 28, 2025 •

edited

Loading