Liquidity-pools: Add unitary tests #1879

lemunozm · 2024-06-19T06:49:11Z

Description

We didn't have UTs for the liquidity pool. Although the Integration Tests cover more or less the basics of liquidity-pools, having those checks there makes Integration Tests more boilerplate. We can simplify some tests if the basics are moved to the unitary tests.

Changes:

Some cleanings / simplifications
Added UT tests to check all errors when transferring
Removed liquidity::gateway tests, which are correctly checked in UTs.
Removed IT utils::democracy which are no longer needed (see comment below)
Removed liquidity_pools::transfer covered in UTs.

BLOCKED BY #1869

lemunozm · 2024-06-19T07:25:28Z

runtime/common/src/account_conversion.rs

-impl Convert<(Domain, [u8; 32]), AccountId32> for AccountConverter {
-	fn convert((domain, account): (Domain, [u8; 32])) -> AccountId32 {
-		match domain {
-			Domain::Centrifuge => AccountId32::new(account),
-			// EVM AccountId20 addresses are right-padded to 32 bytes
-			Domain::EVM(chain_id) => {
-				let mut bytes20 = [0; 20];
-				bytes20.copy_from_slice(&account[..20]);
-				Self::convert_evm_address(chain_id, bytes20)
-			}
-		}
-	}
-}
-


We can implement this converter as a composition of the above two converters. This is also valid for liquidity pools, and we do not need to pass the extra Convert as a trait to the Config.

lemunozm · 2024-06-19T07:25:57Z

runtime/common/src/account_conversion.rs

+	pub fn domain_account_to_account(domain: Domain, account_id: AccountId) -> AccountId {
+		let domain_address = Self::convert((domain, account_id.into()));
+		Self::convert(domain_address)
+	}


A utility is still here for testing or other stuff

lemunozm · 2024-06-19T07:42:10Z

runtime/common/src/account_conversion.rs

@@ -61,10 +61,15 @@ impl AccountConverter {
 		}
 	}

-	pub fn into_account_id<R: pallet_evm_chain_id::Config>(address: H160) -> AccountId {
+	pub fn evm_to_account<R: pallet_evm_chain_id::Config>(address: H160) -> AccountId {


Modified the name (open to other names that match better) for two reasons:

The into_* names are usually used when the own type is converted into something, not when the thing to convert is used as a parameter. The name into_account_id() was inherited from the name used by the trait AddressMapping, not chosen by us. Anyways, the into_account_id() is still accessible through the trait.

The new name follows the format: FROM_to_TO, as other methods:

location_to_account()

domain_account_to_account()

Nit - evm_address_to_account_id ^^

Like it more!

lemunozm · 2024-06-19T11:52:26Z

runtime/integration-tests/src/generic/utils/democracy.rs

@@ -1,276 +0,0 @@
-use std::ops::Add;


I've removed this file because:

No longer used in our gateway tests, which were correctly covered by its UTs.

It tests just democracy stuff which is not under our umbrella. We do not want to maintain this.

No longer used after OpenGov (?): feat: Open Gov for Development and Altair Runtimes #1828

Anyways, I am open to discussing this.

I agree with you!

cdamian · 2024-06-19T12:03:09Z

pallets/liquidity-pools/src/mock.rs

+		ForeignInvestment: cfg_mocks::foreign_investment::pallet,
+		Gateway: cfg_mocks::outbound_queue::pallet,
+		DomainAddressToAccountId: cfg_mocks::converter::pallet::<Instance1>,
+		DomainAccountToDomainAddress: cfg_mocks::converter::pallet::<Instance3>,


Why do we have to use Instance3 here? IIRC we used maximum 2 instances in tests.

Right! I had an Instance2 before. I will change it! Good eye 😆

cdamian

Ultra-clean, as always :D Thank you @lemunozm!

lemunozm · 2024-06-20T10:07:20Z

pallets/liquidity-pools/src/lib.rs

-			ensure!(
-				T::PoolInspect::tranche_exists(pool_id, tranche_id),
-				Error::<T>::TrancheNotFound
-			);
-


Already checked under derive_invest_id() to create the investment_id

lemunozm · 2024-06-20T10:08:55Z

pallets/liquidity-pools/src/lib.rs

@@ -1097,25 +1068,3 @@ pub mod pallet {
 		}
 	}
 }
-
-#[cfg(test)]
-mod tests {


moved to domain_address.rs tests

codecov · 2024-06-20T14:44:36Z

Codecov Report

Attention: Patch coverage is 30.00000% with 49 lines in your changes missing coverage. Please review.

Project coverage is 47.65%. Comparing base (ee8fc69) to head (c3a56ef).

Files	Patch %	Lines
libs/mocks/src/foreign_investment.rs	0.00%	25 Missing ⚠️
pallets/liquidity-pools/src/lib.rs	26.31%	14 Missing ⚠️
runtime/common/src/account_conversion.rs	0.00%	5 Missing ⚠️
pallets/liquidity-pools/src/inbound.rs	0.00%	3 Missing ⚠️
pallets/liquidity-pools/src/hooks.rs	0.00%	1 Missing ⚠️
runtime/common/src/gateway.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1879      +/-   ##
==========================================
+ Coverage   46.81%   47.65%   +0.83%     
==========================================
  Files         170      171       +1     
  Lines       13221    13233      +12     
==========================================
+ Hits         6190     6306     +116     
+ Misses       7031     6927     -104

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

wischli

Great improvement! Loving the clean distinction between all test scenarios instead of the previous huge blocks. I have two minor nits which can be ignored.

wischli · 2024-06-24T14:19:41Z

pallets/liquidity-pools/src/tests.rs

+			assert_ok!(LiquidityPools::transfer(
+				RuntimeOrigin::signed(ALICE),
+				CurrencyId::ForeignAsset(1),
+				EVM_ADDRESS,
+				AMOUNT
+			));


Nit: Please add the previously existing check that the balance of the sender and total issuance of the assed was reduced by AMOUNT.

Initially I removed that check because it's checking that orml_tokens is correctly working (which is out of scope from this UT test), but thinking again, it makes sense to check that actually this method calls correctly to orml_tokens, with the correct amount. So, will do 👍🏻

Very good point BTW

wischli · 2024-06-24T14:22:08Z

pallets/liquidity-pools/src/tests.rs

+			assert_ok!(LiquidityPools::transfer_tranche_tokens(
+				RuntimeOrigin::signed(ALICE),
+				POOL_ID,
+				TRANCHE_ID,
+				EVM_ADDRESS,
+				AMOUNT
+			),);


Nit: Same as above: Can we add a check which ensures the balance was altered correctly on the sender and the domain account?

Done! I've also simplified in the process some minor thing regarding this: ebdebbd

Generally, the Convert trait is quite obscure when you have different conversions, and in that case, we didn't need it.

The base branch was changed.

lemunozm · 2024-06-25T07:53:08Z

@wischli @wischli, ready for your re-approvals and get this in 🙏🏻

wischli

Thanks for adhering to my nits!

lemunozm added the I4-tests Test needs fixing or improving. label Jun 19, 2024

lemunozm self-assigned this Jun 19, 2024

lemunozm changed the base branch from main to it/simplify-xtransfer-tests June 19, 2024 06:49

lemunozm commented Jun 19, 2024

View reviewed changes

lemunozm marked this pull request as ready for review June 19, 2024 11:53

lemunozm requested review from cdamian, wischli and mustermeiszer as code owners June 19, 2024 11:53

cdamian reviewed Jun 19, 2024

View reviewed changes

cdamian previously approved these changes Jun 19, 2024

View reviewed changes

lemunozm commented Jun 20, 2024

View reviewed changes

lemunozm mentioned this pull request Jun 20, 2024

IT: Simplify liquidity_pool test module #1869

Merged

2 tasks

lemunozm force-pushed the lp/unitary-tests branch from a95397c to 91b79bb Compare June 20, 2024 13:26

lemunozm mentioned this pull request Jun 20, 2024

IT: Panic if event is not found in the expected blocks #1880

Merged

wischli previously approved these changes Jun 24, 2024

View reviewed changes

lemunozm added 11 commits June 24, 2024 21:06

prepare mock for UTs and simplify some minor things

0198a07

normal function instead of Convert trait

03f6fc7

rename into_account_id() to evm_to_account()

c635004

add UTs for transfer

8618a5c

simplify account conversion

d3349be

remove unused CurrencyIdOf

45d03d1

remove already covered tests

72c67ff

remove democracy utils

eabec57

apply Cosmin suggestions

adff114

add transfer_tranche_tokens tests

e37b1f9

minor clean

7a65074

lemunozm added 4 commits June 24, 2024 21:07

add add_pool tests

24ebf2e

add add_tranche tests

7d576fc

check balance post transfers

ebdebbd

fix lp tests

d35c365

lemunozm force-pushed the lp/unitary-tests branch from 83e175f to d2e04cc Compare June 24, 2024 19:13

lemunozm changed the base branch from it/simplify-xtransfer-tests to main June 24, 2024 19:14

fix imports after rebasing

f870660

lemunozm force-pushed the lp/unitary-tests branch from d2e04cc to f870660 Compare June 25, 2024 05:35

lemunozm enabled auto-merge (squash) June 25, 2024 05:35

fix taplo

c3a56ef

wischli approved these changes Jun 25, 2024

View reviewed changes

lemunozm merged commit 47384e9 into main Jun 25, 2024
10 of 12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Liquidity-pools: Add unitary tests #1879

Liquidity-pools: Add unitary tests #1879

lemunozm commented Jun 19, 2024 •

edited

Loading

lemunozm Jun 19, 2024 •

edited

Loading

lemunozm Jun 19, 2024

lemunozm Jun 19, 2024

cdamian Jun 19, 2024

lemunozm Jun 19, 2024

lemunozm Jun 19, 2024

wischli Jun 24, 2024

cdamian Jun 19, 2024

lemunozm Jun 19, 2024

cdamian left a comment

lemunozm Jun 20, 2024

lemunozm Jun 20, 2024

codecov bot commented Jun 20, 2024 •

edited

Loading

wischli left a comment

wischli Jun 24, 2024

lemunozm Jun 24, 2024

lemunozm Jun 24, 2024

wischli Jun 24, 2024

lemunozm Jun 24, 2024 •

edited

Loading

lemunozm commented Jun 25, 2024

wischli left a comment

Liquidity-pools: Add unitary tests #1879

Liquidity-pools: Add unitary tests #1879

Conversation

lemunozm commented Jun 19, 2024 • edited Loading

Description

Changes:

lemunozm Jun 19, 2024 • 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

cdamian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jun 20, 2024 • edited Loading

Codecov Report

wischli 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

lemunozm Jun 24, 2024 • edited Loading

Choose a reason for hiding this comment

lemunozm commented Jun 25, 2024

wischli left a comment

Choose a reason for hiding this comment

lemunozm commented Jun 19, 2024 •

edited

Loading

lemunozm Jun 19, 2024 •

edited

Loading

codecov bot commented Jun 20, 2024 •

edited

Loading

lemunozm Jun 24, 2024 •

edited

Loading