IT: rewards to generic framework. Remove previous non-generic IT utilities #1805

lemunozm · 2024-04-11T08:31:23Z

Description

Fixes #1790

Changes and Descriptions

Block rewards tests ported to run under generic version.
- WARNING. block rewwards now uses Instance1 instead of Instance2 in development runtime in order to align all runtimes. This can imply some reset on dev/demo.
- Pending to fix an event issue with this test.
- Pending to test against altair and centrifuge runtimes.
Remove previous utilities no longer used (but It deserves to maintain this macro: IntegrationTest: generic tests with assert_events! macro support #1804).
Remove fast-runtime feature from IT (for now, it's not required).
Added Blocks::JumpByNumber to force the runtime to jump to a block in the future without passing by the previous blocks (can be used to speed up an integration-test)

codecov · 2024-04-11T09:06:38Z

Codecov Report

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

Project coverage is 48.65%. Comparing base (430fbe9) to head (77fa4fa).

Files	Patch %	Lines
runtime/altair/src/lib.rs	0.00%	2 Missing ⚠️
runtime/centrifuge/src/lib.rs	0.00%	2 Missing ⚠️
runtime/development/src/lib.rs	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1805      +/-   ##
==========================================
- Coverage   48.66%   48.65%   -0.01%     
==========================================
  Files         167      167              
  Lines       13318    13318              
==========================================
- Hits         6481     6480       -1     
- Misses       6837     6838       +1

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

lemunozm · 2024-04-11T10:15:08Z

@gpmayorga, have you checked what we need to change to avoid showing a failure in the codecov/patch job?

Maybe just return 0 after calling tarpaulin?

lemunozm · 2024-04-11T10:25:58Z

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

+crate::test_for_runtimes!(all, block_rewards_api);
+fn block_rewards_api<T: Runtime>() {


I think it would be really cool having this as a procedural macro:

#[test_for_runtimes(all)] fn block_rewards_api() { // ... }

I think it should not be complicated, and we can leave this much more legible

That would be awesome!

lemunozm · 2024-04-11T10:28:04Z

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

+		pallet_collator_selection::Pallet::<T>::register_as_candidate(
+			RawOrigin::Signed(collator.clone()).into(),
+		)
+		.unwrap();


I can't call this in altair and centrifuge because I get a ValidatorNotRegistered error.

@wischli, do you have any idea why this happens?

In terms of pallet_collator_selection::Config, Dev and Altair/Centrifuge differ in the ValidatorRegistration trait implementor: Dev uses Sessions whereas production uses the CollatorAllowlist. Therefore, dev only requires session keys to be set for those addresses, which are registered. Production however requires the accounts to be whitelisted via the pallet_collator_allowlist beforehand. This can done via the add extrinsic of that pallet. This also requires the address to have their session key set.

Thanks so much for this answer @wischli, it helps me a lot!

@wischli, do you think we should align this for all runtimes? Why we are not using collator allowlist for dev?

cc @mustermeiszer

Functionally, it does not make sense to me to add an extra gate to collators for our dev runtime IMO. I suppose the only achievement for us would be that this test works for all runtimes?

I definitely dont want to block your proposal, just questioning the effort.

Nono, it's not needed for the test; we can discriminate the behavior per runtime (at least it seems so by now). I asked because development is our way of proving stuff that later will go to centrifuge. So maybe it deserves that the demo also uses collator allowlist. Just a thought!

lemunozm · 2024-04-12T05:39:26Z

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

+
+	frame_system::Pallet::<T>::reset_events();
+
+	pallet_session::Pallet::<T>::rotate_session();


BTW, I've been playing with this and I've substituted pass_n(Period) to rotate_session(), which maps to the same behavior and allow us get rid of fast-runtime feature in integration-test and simplify a bit the test.

If you have some thoughts against it @wischli , I can revert this.

Thats a perfect replacement and upgrade! Thanks a lot.

lemunozm · 2024-04-12T05:43:01Z

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

+	/* TODO: pending to fix. Why try_into() fails getting the reward_event::GroupRewarded?
+
+	// The event exists in this list:
+	dbg!(frame_system::Pallet::<T>::events())
+
+	// But not in in this list (that is the implementation of find_event()),
+	// so try_into returns an Err for it.
+	dbg!(frame_system::Pallet::<T>::events()
+		.into_iter()
+		.rev()
+		.find_map(|record| record.event.try_into().ok())
+		.flatten());
+
+	// But later, if manually I create the event as follows:
+	let e = T::RuntimeEventExt::from(pallet_rewards::Event::<T, BlockRewards>::GroupRewarded {
+		group_id: 1,
+		amount: 2,
+	});
+
+	// And I call try_into(), it works.
+	let re: pallet_rewards::Event<T, BlockRewards> = e.try_into().ok().unwrap();
+	*/


99% of developers can not fix this nor saying what's happening here

cc: @cdamian @wischli @mustermeiszer

I don't have a solution at hand right now, would have to debug that myself. Can look into that next week, if desired.

Thanks William. My guess is that something weird is happening with the try_into when events contain instances. But I've checked all the places where the instance is set, and we always set the correct one for that pallet. 🤔

wischli

Thanks for slimming this down and making it much more readable! I hope one of us is the 1% dev, which can solve the mystery. I can't dive into this for now, but next week I would have capacity.

wischli · 2024-04-12T11:04:15Z

runtime/common/src/lib.rs

@@ -43,6 +43,11 @@ use sp_runtime::{
 };
 use sp_std::marker::PhantomData;

+pub mod instances {
+	/// The rewards associated to block rewards
+	pub type BlockRewards = pallet_rewards::Instance1;


Love it, thanks!

wischli · 2024-04-12T11:08:38Z

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

+		pallet_collator_selection::Pallet::<T>::register_as_candidate(
+			RawOrigin::Signed(collator.clone()).into(),
+		)
+		.unwrap();


In terms of pallet_collator_selection::Config, Dev and Altair/Centrifuge differ in the ValidatorRegistration trait implementor: Dev uses Sessions whereas production uses the CollatorAllowlist. Therefore, dev only requires session keys to be set for those addresses, which are registered. Production however requires the accounts to be whitelisted via the pallet_collator_allowlist beforehand. This can done via the add extrinsic of that pallet. This also requires the address to have their session key set.

wischli · 2024-04-12T11:11:20Z

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

+	/* TODO: pending to fix. Why try_into() fails getting the reward_event::GroupRewarded?
+
+	// The event exists in this list:
+	dbg!(frame_system::Pallet::<T>::events())
+
+	// But not in in this list (that is the implementation of find_event()),
+	// so try_into returns an Err for it.
+	dbg!(frame_system::Pallet::<T>::events()
+		.into_iter()
+		.rev()
+		.find_map(|record| record.event.try_into().ok())
+		.flatten());
+
+	// But later, if manually I create the event as follows:
+	let e = T::RuntimeEventExt::from(pallet_rewards::Event::<T, BlockRewards>::GroupRewarded {
+		group_id: 1,
+		amount: 2,
+	});
+
+	// And I call try_into(), it works.
+	let re: pallet_rewards::Event<T, BlockRewards> = e.try_into().ok().unwrap();
+	*/


I don't have a solution at hand right now, would have to debug that myself. Can look into that next week, if desired.

wischli · 2024-04-12T11:15:02Z

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

-///
-/// Accounts:
-/// * Keyring::Admin
-/// * Keyring::Alice
-/// * Keyring::Bob
-/// * Keyring::Ferdie
-/// * Keyring::Charlie
-/// * Keyring::Dave
-/// * Keyring::Eve
-/// * Keyring::TrancheInvestor(1)


I actually liked the comment. Of course we dont need a line for every single tranche investor but do you think we could bring back something like Admin, ... Eve, TranchInvestor(1), ..., TrancheInvestor(50)?

Sure, but I'm just curious, what add the comment that does not tell you the own implementation?

Fair point. I often leverage rustdocs from hovering over. In this case, it is crystal clear so lets keep as is!

lemunozm · 2024-04-18T10:55:41Z

Ok, now it works for all runtimes. Thanks for the help @wischli.

Could we consider this ready to be merged? and leave the 99% of developers can not fix this nor saying what's happening here issue as a tech-debt?

lemunozm · 2024-04-18T11:00:31Z

WARNING: after merging this block rewards now will use Instance1 instead of Instance2 in development runtime. This would imply we need to do a reset in dev/demo in the next RU for them.

wischli · 2024-04-18T12:56:44Z

WARNING: after merging this block rewards now will use Instance1 instead of Instance2 in development runtime. This would imply we need to do a reset in dev/demo in the next RU for them.

Its definitely fine to reset on Dev. Do you want to open another PR for the cleanup and reset or do it in this one? The reset might require to execute the pallet_block_rewards::migration::InitBlockRewards again.

lemunozm · 2024-04-18T14:26:04Z

I would say to open a new PR. Too many things in this one.

InitBlockRewards migration is done each time we reset?

wischli

LGTM!

mustermeiszer

Blind approval after review of others.

lemunozm · 2024-04-19T10:41:25Z

Thanks both!

lemunozm · 2024-04-19T10:43:06Z

I want to remove the generic folder because after this PR is no longer needed and reestructure it a bit now . But I will wait until you get this #1720 merged

wischli · 2024-04-19T11:44:20Z

I would say to open a new PR. Too many things in this one.

InitBlockRewards migration is done each time we reset?

I think it is because it attaches the staking currency to the group of the underlying pallet_rewards::<Instance>. I will handle this! I noticed Demo is super outdated with lots of pallet versions and I am working on fixing all of that right now.

lemunozm · 2024-04-19T12:18:29Z

Thanks William!

I was wondering... if we want to avoid resetting the storage in the demo to not lose everything set. Could we instead modify the politic always to reset demo but have a script that later it populates with some sane state for Apps (by adding a bunch of transactions). Just thinking out loud.

wischli · 2024-04-19T12:23:08Z

Thanks William!

I was wondering... if we want to avoid resetting the storage in the demo to not lose everything set. Could we instead modify the politic always to reset demo but have a script that later it populates with some sane state for Apps (by adding a bunch of transactions). Just thinking out loud.

Let's discuss this at the offsite. If, Dev should be reset but Demo needs to be stable because of Axelar. I think repopulating state is quite intense as it requires multiple pools to be spawned etc. But given the current state of Demo, we might have to do a reset. It has never been planned to use that chain as our main testnet such that migrations have been ignored throughout numerous Subtrate and business logic updates.

There are many many keys which cannot be decoded anymore..

lemunozm added the I4-tests Test needs fixing or improving. label Apr 11, 2024

lemunozm self-assigned this Apr 11, 2024

lemunozm requested review from mustermeiszer, wischli and cdamian as code owners April 11, 2024 08:31

lemunozm commented Apr 11, 2024

View reviewed changes

lemunozm force-pushed the it/rewards-to-generic branch from 22c05e4 to b3e0838 Compare April 12, 2024 05:31

lemunozm commented Apr 12, 2024

View reviewed changes

wischli reviewed Apr 12, 2024

View reviewed changes

lemunozm force-pushed the fix/block-rewards-ed branch from 26db2ff to 60c7b28 Compare April 16, 2024 09:37

lemunozm requested a review from hieronx as a code owner April 16, 2024 09:37

Base automatically changed from fix/block-rewards-ed to main April 16, 2024 18:29

lemunozm added 2 commits April 16, 2024 20:33

test working

0bd5bfe

clear previous testing framework

d478366

lemunozm force-pushed the it/rewards-to-generic branch from b3e0838 to d478366 Compare April 16, 2024 18:34

lemunozm mentioned this pull request Apr 17, 2024

IntegrationTests: Port non-generic tests into generic module #1790

Closed

lemunozm added 2 commits April 18, 2024 12:53

give support for altair and centrifuge

edaf53b

Merge remote-tracking branch 'origin/main' into it/rewards-to-generic

77fa4fa

lemunozm mentioned this pull request Apr 18, 2024

99% of developers can not fix this nor saying what's happening here #1817

Open

lemunozm enabled auto-merge (squash) April 18, 2024 14:27

wischli approved these changes Apr 18, 2024

View reviewed changes

lemunozm disabled auto-merge April 19, 2024 08:53

mustermeiszer approved these changes Apr 19, 2024

View reviewed changes

lemunozm merged commit 97cb823 into main Apr 19, 2024
10 of 12 checks passed

lemunozm deleted the it/rewards-to-generic branch April 19, 2024 10:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IT: rewards to generic framework. Remove previous non-generic IT utilities #1805

IT: rewards to generic framework. Remove previous non-generic IT utilities #1805

lemunozm commented Apr 11, 2024 •

edited

Loading

codecov bot commented Apr 11, 2024 •

edited

Loading

lemunozm commented Apr 11, 2024

lemunozm Apr 11, 2024 •

edited

Loading

cdamian Apr 11, 2024

lemunozm Apr 11, 2024

wischli Apr 12, 2024

lemunozm Apr 12, 2024

lemunozm Apr 18, 2024

wischli Apr 18, 2024

lemunozm Apr 18, 2024

lemunozm Apr 12, 2024 •

edited

Loading

wischli Apr 12, 2024

lemunozm Apr 12, 2024

wischli Apr 12, 2024

lemunozm Apr 12, 2024

wischli left a comment

wischli Apr 12, 2024

wischli Apr 12, 2024

wischli Apr 12, 2024

wischli Apr 12, 2024

lemunozm Apr 12, 2024

wischli Apr 12, 2024

lemunozm commented Apr 18, 2024

lemunozm commented Apr 18, 2024 •

edited

Loading

wischli commented Apr 18, 2024

lemunozm commented Apr 18, 2024 •

edited

Loading

wischli left a comment

mustermeiszer left a comment

lemunozm commented Apr 19, 2024

lemunozm commented Apr 19, 2024 •

edited

Loading

wischli commented Apr 19, 2024

lemunozm commented Apr 19, 2024

wischli commented Apr 19, 2024

		crate::test_for_runtimes!(all, block_rewards_api);
		fn block_rewards_api<T: Runtime>() {


		frame_system::Pallet::<T>::reset_events();

		pallet_session::Pallet::<T>::rotate_session();

IT: rewards to generic framework. Remove previous non-generic IT utilities #1805

IT: rewards to generic framework. Remove previous non-generic IT utilities #1805

Conversation

lemunozm commented Apr 11, 2024 • edited Loading

Description

Changes and Descriptions

codecov bot commented Apr 11, 2024 • edited Loading

Codecov Report

lemunozm commented Apr 11, 2024

lemunozm Apr 11, 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

lemunozm Apr 12, 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lemunozm commented Apr 18, 2024

lemunozm commented Apr 18, 2024 • edited Loading

wischli commented Apr 18, 2024

lemunozm commented Apr 18, 2024 • edited Loading

wischli left a comment

Choose a reason for hiding this comment

mustermeiszer left a comment

Choose a reason for hiding this comment

lemunozm commented Apr 19, 2024

lemunozm commented Apr 19, 2024 • edited Loading

wischli commented Apr 19, 2024

lemunozm commented Apr 19, 2024

wischli commented Apr 19, 2024

lemunozm commented Apr 11, 2024 •

edited

Loading

codecov bot commented Apr 11, 2024 •

edited

Loading

lemunozm Apr 11, 2024 •

edited

Loading

lemunozm Apr 12, 2024 •

edited

Loading

lemunozm commented Apr 18, 2024 •

edited

Loading

lemunozm commented Apr 18, 2024 •

edited

Loading

lemunozm commented Apr 19, 2024 •

edited

Loading