test: Solana's delta-based-ingress electoral System #5445

syan095 · 2024-11-25T08:15:15Z

Pull Request

Checklist

Please conduct a thorough self-review before opening the PR.

I am confident that the code works.
I have written sufficient tests.
I have written and tested required migrations.
I have updated documentation where appropriate.

Summary

Added unit tests for the delta based ingress electoral system.
Improved the mock system for the tests.
Fixed a bug where on revert, on_finalize will cause underflow panic
Log a warning on ingress reverts

state-chain/pallets/cf-elections/src/electoral_systems/mocks/access.rs

codecov · 2024-11-25T08:27:55Z

Codecov Report

Attention: Patch coverage is 97.18310% with 2 lines in your changes missing coverage. Please review.

Project coverage is 71%. Comparing base (461e23e) to head (ff2e596).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...lectoral_systems/blockchain/delta_based_ingress.rs	0%	0 Missing and 1 partial ⚠️
...cf-elections/src/electoral_systems/mocks/access.rs	98%	1 Missing ⚠️

Additional details and impacted files

@@          Coverage Diff          @@
##            main   #5445   +/-   ##
=====================================
- Coverage     72%     71%   -0%     
=====================================
  Files        494     494           
  Lines      87706   87627   -79     
  Branches   87706   87627   -79     
=====================================
- Hits       62712   62652   -60     
+ Misses     22383   22364   -19     
  Partials    2611    2611

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

state-chain/pallets/cf-elections/src/electoral_systems/tests/delta_based_ingress.rs

state-chain/pallets/cf-elections/src/electoral_systems/blockchain/delta_based_ingress.rs

state-chain/pallets/cf-elections/src/electoral_systems/mocks/access.rs

state-chain/pallets/cf-elections/src/electoral_systems/tests/delta_based_ingress.rs

kylezs · 2024-11-27T10:14:54Z

state-chain/pallets/cf-elections/src/electoral_systems/blockchain/delta_based_ingress.rs

-								details.asset,
-								ingress_total.amount - previous_amount,
-							);
+							log::warn!("Deposit channels on Solana chain has reverted! Account: {:?}, Asset: {:?}, amount: {:?}", account, details.asset, previous_amount - ingress_total.amount);


I think it's better to provide the previous and ingress total amounts separately here, easier to debug, we can do the subtraction ourselves if required

state-chain/pallets/cf-elections/src/electoral_systems/mocks/access.rs

kylezs · 2024-11-27T10:39:37Z

state-chain/pallets/cf-elections/src/electoral_systems/tests/delta_based_ingress.rs

+					(1u32, Asset::Sol, 1_000u64),
+					(2u32, Asset::SolUsdc, 2_000u64),


Could we change these values on the channel_state_final slightly? at the moment it looks like it's just duplicating them, which isn't actually the case, it's dependent on the channel_state_final, which by coincidence makes it look duplicated - just makes it easier to tell that the test is water-tight

kylezs · 2024-11-27T10:43:48Z

state-chain/pallets/cf-elections/src/electoral_systems/tests/delta_based_ingress.rs

+			most_recent: None,
+			new: channel_state_ingressed().to_state(),
+		})
+		.test_on_finalize(&699, |_| (), vec![Check::assert_unchanged()])


this is quite confusing on first read, 699 is a magic value here. ideally it's related to the local test state. So we can say "channel.block_number - 1", that way it's clear exactly what the test is trying to convey

This could probably be made a lot easier by having the utility functions a bit flatter, i.e. rather than on the DepositChannels struct. So for example,
We define at the top of the test:

let first_channel = DepositChannel { ... }; let initial_channels = vec![first_channel, ...]; // then here we can do: .force_consensus_update(ConsensusStatus::Gained { most_recent: None, new: channels_to_state(initial_channels), }) // NB: Now there's no magic number, it's clear what case we're testing .test_on_finalize(&first_channel.block_number - 1)

So we have a flat channels_to_state which allows us to more easily construct the test conditions locally, so the assertions aren't dependent on global state.

The same applies to the other tests too, for things like expected ingress value, they look like magic numbers.

state-chain/pallets/cf-elections/src/electoral_systems/blockchain/delta_based_ingress.rs

state-chain/pallets/cf-elections/src/electoral_systems/tests/delta_based_ingress.rs

kylezs

Nice tests 🚀 thanks for knocking this one off 🫡

kylezs · 2024-11-29T08:58:02Z

state-chain/pallets/cf-elections/src/electoral_systems/tests/delta_based_ingress.rs

+	let deposit_channel_pending_updated_block = DepositChannel {
+		account: 1u32,
+		asset: Asset::Sol,
+		total_ingressed: 1_000u64,
+		block_number: 499u64,
+		close_block: 1_000u64,
+	};
+	let deposit_channel_pending_updated_amount = DepositChannel {
+		account: 1u32,
+		asset: Asset::Sol,
+		total_ingressed: 999u64,
+		block_number: 500u64,
+		close_block: 1_000u64,
+	};


nit: would switch the order of these two definitions so it matches the test flow

kylezs · 2024-11-29T09:05:08Z

state-chain/pallets/cf-elections/src/electoral_systems/tests/delta_based_ingress.rs

@@ -221,6 +221,9 @@ register_checks! {
 		ended_at_state(_pre, post, election_state: BTreeMap<AccountId, ChannelTotalIngressedFor<MockIngressSink>>) {
 			assert_eq!(*post.election_state.get(post.election_identifiers[0].unique_monotonic()).unwrap(), election_state, "Expected election state incorrect.");
 		},
+		ended_at_empty_state(_pre, post) {


👍 nice additional safety check

Improved the mock system for the tests. Fixed a bug where on revert, on_finalize will cause underflow panic Deposit an event on ingress reverts

Improved test and the use of Checks Added tests for recycled deposit channels.

…ption. `get` still returns Option. `set` still takes Option, removes the value if None is passed in. State map now returns decoded state map instead of raw encoded. Adjusted function names accordingly.

syan095 requested review from dandanlen and kylezs November 25, 2024 08:15

kylezs reviewed Nov 25, 2024

View reviewed changes

state-chain/pallets/cf-elections/src/electoral_systems/mocks/access.rs Show resolved Hide resolved

kylezs reviewed Nov 25, 2024

View reviewed changes

state-chain/pallets/cf-elections/src/electoral_systems/tests/delta_based_ingress.rs Outdated Show resolved Hide resolved

kylezs reviewed Nov 25, 2024

View reviewed changes

syan095 changed the title ~~WIP: test: Solana's delta-based-ingress electoral System~~ test: Solana's delta-based-ingress electoral System Nov 27, 2024

syan095 marked this pull request as ready for review November 27, 2024 09:23

syan095 requested a review from kylezs November 27, 2024 09:24

kylezs reviewed Nov 27, 2024

View reviewed changes

syan095 requested a review from kylezs November 28, 2024 20:08

kylezs approved these changes Nov 29, 2024

View reviewed changes

syan095 added 7 commits December 2, 2024 13:34

Added unit tests for the delta based ingress electoral system.

baae552

Improved the mock system for the tests. Fixed a bug where on revert, on_finalize will cause underflow panic Deposit an event on ingress reverts

Addressed some PR comments

de3d93b

Improved test and the use of Checks Added tests for recycled deposit channels.

Changed the state map so it stores Vec<u8> for the value instead of O…

b5645dd

…ption. `get` still returns Option. `set` still takes Option, removes the value if None is passed in. State map now returns decoded state map instead of raw encoded. Adjusted function names accordingly.

Addressed some PR comments.

a44e4c4

Added new test for pending ingresses.

6e1dfb3

Fixed unit tests

c3c8ea4

Minor adjustment to address PR review comment

ff2e596

syan095 force-pushed the test/delta-based-ingress branch from bd973e5 to ff2e596 Compare December 2, 2024 00:36

syan095 enabled auto-merge December 2, 2024 00:39

syan095 added this pull request to the merge queue Dec 2, 2024

github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Dec 2, 2024

syan095 added this pull request to the merge queue Dec 2, 2024

Merged via the queue into main with commit d12200a Dec 2, 2024
28 of 49 checks passed

syan095 deleted the test/delta-based-ingress branch December 2, 2024 05:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: Solana's delta-based-ingress electoral System #5445

test: Solana's delta-based-ingress electoral System #5445

syan095 commented Nov 25, 2024 •

edited

Loading

codecov bot commented Nov 25, 2024 •

edited

Loading

kylezs Nov 27, 2024

kylezs Nov 27, 2024

syan095 Nov 28, 2024

kylezs Nov 27, 2024

kylezs Nov 27, 2024

kylezs left a comment

kylezs Nov 29, 2024

kylezs Nov 29, 2024

		(1u32, Asset::Sol, 1_000u64),
		(2u32, Asset::SolUsdc, 2_000u64),

test: Solana's delta-based-ingress electoral System #5445

test: Solana's delta-based-ingress electoral System #5445

Conversation

syan095 commented Nov 25, 2024 • edited Loading

Pull Request

Checklist

Summary

codecov bot commented Nov 25, 2024 • edited Loading

Codecov Report

kylezs Nov 27, 2024

Choose a reason for hiding this comment

kylezs Nov 27, 2024

Choose a reason for hiding this comment

syan095 Nov 28, 2024

Choose a reason for hiding this comment

kylezs Nov 27, 2024

Choose a reason for hiding this comment

kylezs Nov 27, 2024

Choose a reason for hiding this comment

kylezs left a comment

Choose a reason for hiding this comment

kylezs Nov 29, 2024

Choose a reason for hiding this comment

kylezs Nov 29, 2024

Choose a reason for hiding this comment

syan095 commented Nov 25, 2024 •

edited

Loading

codecov bot commented Nov 25, 2024 •

edited

Loading