IT: Simplify liquidity_pool test module #1869

lemunozm · 2024-06-11T15:32:40Z

Description

There are a lot of overlapping and boilerplate in the current liquidity-pool tests.

Minor IT utilities improvements & simplifications.
Added an approx() to check approximation amounts.
Give more "multi-runtime" support in several tests
Extracted and refactored some modules from liquidity_pools.rs into their own files, trying to avoid as much testing overlapping as possible:
- Module transfer into xcm_transfer.rs file:
  - para -> para
  - relay -> para -> relay
- Module asset_registry into assets.rs file.
- Module currency_id_convert into currency_conversion.rs file.
- Module restricted_tokens into the already existent restricted_tokens.rs file.
Remove fudge from proxy tests.
PARA_ID and SIBLING_ID no longer tied to FudgeHandle. They are gloval values that can be read in any place without using T::FudgeHandle

I do not touch any tests related to liquidity pools itself. I want to give them support for altair and centrifuge and remove the Fudge requirement in another PR. Also, I think there is a lot of overlap with the new lp module, so maybe some of them can just be removed.

Next in line (future PRs):

Move gateway tests to its own module tested for all runtimes: Liquidity-pools: Add unitary tests #1879
Move routers tests to its own module tested for all runtimes.
liquidity pool tests without fudge, and tested for all runtimes.

lemunozm · 2024-06-11T15:38:51Z

runtime/integration-tests/src/generic/cases/xtransfers.rs

+
+		assert_eq!(
+			orml_tokens::Pallet::<T>::free_balance(curr.id(), &Keyring::Alice.id()),
+			curr.val(INITIAL) - curr.val(TRANSFER)


FYI: the val() method will create the balance with the correct decimals based on the currency it is. We no longer need usd6()/usd12()/usd18() methods which leads to errors mixing different amounts.

lemunozm · 2024-06-12T15:57:57Z

runtime/integration-tests/src/utils/mod.rs

+
+		assert_ne!(1000u128, 996.approx(0.01));
+		assert_ne!(1000u128, 1004.approx(-0.01));
+		assert_ne!(1000u128, 1500.approx(0.1));


Real example: https://github.com/centrifuge/centrifuge-chain/pull/1869/files#diff-01902c6719103239e2fbaf51a564691265934b0a32d12105674d56aa465a3783R193

The value 0.01 means that both values are equal if the right-hand side is as much as 1% higher than the other one. -0.01 means lower than.

…-tests

lemunozm · 2024-06-13T14:20:20Z

There are a lot of git diffs in liquidity_pools.rs file. But 99% of the changes is just removing the modules transfer, asset_registry, currency_id_convert, and restricted_transfer

lemunozm · 2024-06-13T14:22:38Z

runtime/integration-tests/src/generic/cases/currency_conversions.rs

+		assert_eq!(
+			CurrencyIdConvert::<T>::convert(Location::new(0, Here)),
+			Some(curr.id()),
+		);
+
+		assert_eq!(CurrencyIdConvert::<T>::convert(curr.id()), None);


BTW, here I would expect a symmetric behavior, but it isn't.

The implementation only checks the transferability when converting from location -> currency ref. Should we add the same filter for currency -> location ?

Honestly not sure if we want this to be symmetric. AFAICT, we could switch to that based on my below observations.

If we decide on that, it blocks converting any local currency locations into their corresponding currency id. One implication is that those currencies cannot be used for paying XCM transaction fees. On mainnet, CFG is configured as XCM transferable.

It would be interesting to see the failing tests when switching to symmetric behaviour. Depending on the failing tests, this could be a good indicator on whether we require location-to-currency location for local currencies as part of XCM for instance.

AFAICS, the CurrencyIdConvert is used to determine and withdraw cross-chain fee amounts in the configured fee currency in three different pallets:

As orml_xtokens::CurrencyIdConvert to derive the asset origination location

As pallet_xcm_transactor::CurrencyIdLocation to withdraw the fee amount of the configured fee currency

In staging_xcm_executor::Config::AssetTransactor = FungiblesTransactor as Matcher AT of our FungiblesAdaptor impl to withdraw the XCM fee in the configured fee currencies

Curious about your thoughts @mustermeiszer

codecov · 2024-06-13T15:42:55Z

Codecov Report

Attention: Patch coverage is 0% with 10 lines in your changes missing coverage. Please review.

Project coverage is 46.84%. Comparing base (737278c) to head (3991dea).

Files	Patch %	Lines
libs/types/src/tokens.rs	0.00%	6 Missing ⚠️
runtime/common/src/xcm.rs	0.00%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1869      +/-   ##
==========================================
- Coverage   46.87%   46.84%   -0.03%     
==========================================
  Files         170      170              
  Lines       13223    13221       -2     
==========================================
- Hits         6198     6194       -4     
- Misses       7025     7027       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

lemunozm · 2024-06-17T06:51:24Z

libs/types/src/tokens.rs

+	/// Fees will be charged using `FixedRateOfFungible`.
+	#[cfg(feature = "std")]
+	pub fn xcm_default() -> Self {
+		Self::Xcm(XcmMetadata {
+			fee_per_second: None,
+		})
+	}
+
+	/// Fees will be charged using `AssetRegistryTrader`.
+	/// If value is 0, no fees will be charged.
+	#[cfg(feature = "std")]
+	pub fn xcm_with_fees(value: Balance) -> Self {
+		Self::Xcm(XcmMetadata {
+			fee_per_second: Some(value),
+		})
+	}


Tech-debt to create some tests to be able to compute the exact fee charged in the above two cases. Using FixedRateOfFungible and using AssetRegistryTrader

…-tests

wischli

Super organized and clean, love it! Two minor nits, which can be ignored and one comment from my side.

libs/primitives/src/lib.rs

wischli · 2024-06-24T11:19:12Z

runtime/integration-tests/src/generic/cases/currency_conversions.rs

+		assert_eq!(
+			CurrencyIdConvert::<T>::convert(Location::new(0, Here)),
+			Some(curr.id()),
+		);
+
+		assert_eq!(CurrencyIdConvert::<T>::convert(curr.id()), None);


Honestly not sure if we want this to be symmetric. AFAICT, we could switch to that based on my below observations.

If we decide on that, it blocks converting any local currency locations into their corresponding currency id. One implication is that those currencies cannot be used for paying XCM transaction fees. On mainnet, CFG is configured as XCM transferable.

It would be interesting to see the failing tests when switching to symmetric behaviour. Depending on the failing tests, this could be a good indicator on whether we require location-to-currency location for local currencies as part of XCM for instance.

AFAICS, the CurrencyIdConvert is used to determine and withdraw cross-chain fee amounts in the configured fee currency in three different pallets:

As orml_xtokens::CurrencyIdConvert to derive the asset origination location

As pallet_xcm_transactor::CurrencyIdLocation to withdraw the fee amount of the configured fee currency

In staging_xcm_executor::Config::AssetTransactor = FungiblesTransactor as Matcher AT of our FungiblesAdaptor impl to withdraw the XCM fee in the configured fee currencies

Curious about your thoughts @mustermeiszer

runtime/integration-tests/src/generic/cases/xcm_transfers.rs

lemunozm · 2024-06-24T18:11:22Z

@wischli @cdamian can you re-approve this? 🙏🏻

basic xtransfer tests

ea08cec

lemunozm added I4-tests Test needs fixing or improving. crcl-protocol Circle protocol related. labels Jun 11, 2024

lemunozm self-assigned this Jun 11, 2024

lemunozm force-pushed the it/simplify-xtransfer-tests branch 3 times, most recently from 8ccf242 to 650e96e Compare June 11, 2024 15:41

parachain transfer tested

0bc96f6

lemunozm force-pushed the it/simplify-xtransfer-tests branch from 650e96e to 0bc96f6 Compare June 11, 2024 15:43

lemunozm added 2 commits June 11, 2024 19:50

fix tests

e9174c9

transfer relay tests

57908e9

lemunozm requested a review from cdamian June 12, 2024 15:54

lemunozm commented Jun 12, 2024

View reviewed changes

lemunozm added 2 commits June 13, 2024 08:43

polish and minor cleans

1770f11

fixes

5b41f5e

lemunozm force-pushed the it/simplify-xtransfer-tests branch from 02ca1ad to 5b41f5e Compare June 13, 2024 09:16

lemunozm added 3 commits June 13, 2024 12:28

Merge remote-tracking branch 'origin/main' into it/simplify-xtransfer…

cdf8837

…-tests

assets tests

8dd05e3

currency conversion tests

bf338c8

lemunozm commented Jun 13, 2024

View reviewed changes

lemunozm marked this pull request as ready for review June 13, 2024 14:25

fix cargo fmt

126eeb5

lemunozm added 3 commits June 14, 2024 10:20

fix tests

891bfe7

reduce xcm restricted_transfer tests

d781110

rename file

eeb1dbf

lemunozm changed the title ~~IT: Simplify cross transfer tests~~ IT: Simplify liquidity_pool test module Jun 14, 2024

lemunozm added 2 commits June 14, 2024 14:19

restricted ethereum tests simplified and moved to restricted

bdee3ed

cleaning warnings

0c3bb43

removed re-added tranche tokens tests

5a832ad

lemunozm force-pushed the it/simplify-xtransfer-tests branch from f285d2e to 5a832ad Compare June 17, 2024 06:49

lemunozm commented Jun 17, 2024

View reviewed changes

remove fudge from proxy tests

de3760a

lemunozm force-pushed the it/simplify-xtransfer-tests branch from ec2c1e5 to de3760a Compare June 17, 2024 07:53

lemunozm added 3 commits June 17, 2024 15:23

Merge remote-tracking branch 'origin/main' into it/simplify-xtransfer…

1b79822

…-tests

minor cleans

abace1f

fix warnings

a2f7119

lemunozm requested review from mustermeiszer and wischli June 18, 2024 08:18

lemunozm mentioned this pull request Jun 19, 2024

Liquidity-pools: Add unitary tests #1879

Merged

decouple PARA_ID and SIBLING_ID from FudgeHandle

d24c686

cdamian previously approved these changes Jun 21, 2024

View reviewed changes

lemunozm mentioned this pull request Jun 24, 2024

IT: more support for router tests #1885

Merged

lemunozm added 3 commits June 24, 2024 09:04

Merge remote-tracking branch 'origin/main' into it/simplify-xtransfer…

415a158

…-tests

revert liquidity-pool changes

cce79b6

Merge branch 'main' into it/simplify-xtransfer-tests

7f79140

lemunozm dismissed cdamian’s stale review via 7f79140 June 24, 2024 10:27

wischli previously approved these changes Jun 24, 2024

View reviewed changes

William suggestions

3991dea

lemunozm dismissed wischli’s stale review via 3991dea June 24, 2024 13:51

lemunozm enabled auto-merge (squash) June 24, 2024 13:53

lemunozm requested review from cdamian and wischli June 24, 2024 16:37

cdamian approved these changes Jun 24, 2024

View reviewed changes

lemunozm merged commit ee8fc69 into main Jun 24, 2024
11 of 12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IT: Simplify liquidity_pool test module #1869

IT: Simplify liquidity_pool test module #1869

lemunozm commented Jun 11, 2024 •

edited

Loading

lemunozm Jun 11, 2024

lemunozm Jun 12, 2024

lemunozm Jun 13, 2024

lemunozm commented Jun 13, 2024 •

edited

Loading

lemunozm Jun 13, 2024

wischli Jun 24, 2024

codecov bot commented Jun 13, 2024 •

edited

Loading

lemunozm Jun 17, 2024

wischli left a comment

wischli Jun 24, 2024

lemunozm commented Jun 24, 2024

IT: Simplify liquidity_pool test module #1869

IT: Simplify liquidity_pool test module #1869

Conversation

lemunozm commented Jun 11, 2024 • edited Loading

Description

Next in line (future PRs):

lemunozm Jun 11, 2024

Choose a reason for hiding this comment

lemunozm Jun 12, 2024

Choose a reason for hiding this comment

lemunozm Jun 13, 2024

Choose a reason for hiding this comment

lemunozm commented Jun 13, 2024 • edited Loading

lemunozm Jun 13, 2024

Choose a reason for hiding this comment

wischli Jun 24, 2024

Choose a reason for hiding this comment

codecov bot commented Jun 13, 2024 • edited Loading

Codecov Report

lemunozm Jun 17, 2024

Choose a reason for hiding this comment

wischli left a comment

Choose a reason for hiding this comment

wischli Jun 24, 2024

Choose a reason for hiding this comment

lemunozm commented Jun 24, 2024

lemunozm commented Jun 11, 2024 •

edited

Loading

lemunozm commented Jun 13, 2024 •

edited

Loading

codecov bot commented Jun 13, 2024 •

edited

Loading