Location conversion tests for relay and parachains #487

x3c41a · 2024-10-17T16:41:22Z

Added location convertion tests.
Polkadot sdk PR
Fixes #488

…matter

acatangiu

The conversions are the same for all system parachains, so this test could be a macro to dedup the code, but I don't know where we would put that macro or if it's worth the effort. I'm fine with the duplicated code too.

Please see the other comments.

acatangiu · 2024-10-21T13:55:27Z

relay/kusama/tests/location_conversion.rs

+		// DescribeTerminus
+		TestCase {
+			description: "DescribeTerminus Parent",
+			location: Location::new(1, Here),
+			expected_account_id_str: "5GyWtDJP7qaipWRGr4KJ6VUDxRXf4jDnPW6KPTeCekHfqZkD",
+		},
+		TestCase {
+			description: "DescribeTerminus Sibling",
+			location: Location::new(1, [Parachain(1111)]),
+			expected_account_id_str: "5EC5GfEFm9XEBYjXzxb1VseMHsG2VhPeGTGWF9H8tYZnGsSk",
+		},
+		// DescribePalletTerminal
+		TestCase {
+			description: "DescribePalletTerminal Parent",
+			location: Location::new(1, [PalletInstance(50)]),
+			expected_account_id_str: "5CnwemvaAXkWFVwibiCvf2EjqwiqBi29S5cLLydZLEaEw6jZ",
+		},
+		TestCase {
+			description: "DescribePalletTerminal Sibling",
+			location: Location::new(1, [Parachain(1111), PalletInstance(50)]),
+			expected_account_id_str: "5GFBgPjpEQPdaxEnFirUoa51u5erVx84twYxJVuBRAT2UP2g",
+		},
+		// DescribeAccountId32Terminal
+		TestCase {
+			description: "DescribeAccountId32Terminal Parent",
+			location: Location::new(
+				1,
+				[AccountId32 { network: None, id: AccountId::from(ALICE).into() }],
+			),
+			expected_account_id_str: "5DN5SGsuUG7PAqFL47J9meViwdnk9AdeSWKFkcHC45hEzVz4",
+		},
+		TestCase {
+			description: "DescribeAccountId32Terminal Sibling",
+			location: Location::new(
+				1,
+				[
+					Parachain(1111),
+					Junction::AccountId32 { network: None, id: AccountId::from(ALICE).into() },
+				],
+			),
+			expected_account_id_str: "5DGRXLYwWGce7wvm14vX1Ms4Vf118FSWQbJkyQigY2pfm6bg",
+		},
+		// DescribeAccountKey20Terminal
+		TestCase {
+			description: "DescribeAccountKey20Terminal Parent",
+			location: Location::new(1, [AccountKey20 { network: None, key: [0u8; 20] }]),
+			expected_account_id_str: "5F5Ec11567pa919wJkX6VHtv2ZXS5W698YCW35EdEbrg14cg",
+		},
+		TestCase {
+			description: "DescribeAccountKey20Terminal Sibling",
+			location: Location::new(
+				1,
+				[Parachain(1111), AccountKey20 { network: None, key: [0u8; 20] }],
+			),
+			expected_account_id_str: "5CB2FbUds2qvcJNhDiTbRZwiS3trAy6ydFGMSVutmYijpPAg",
+		},
+		// DescribeTreasuryVoiceTerminal
+		TestCase {
+			description: "DescribeTreasuryVoiceTerminal Parent",
+			location: Location::new(1, [Plurality { id: BodyId::Treasury, part: BodyPart::Voice }]),
+			expected_account_id_str: "5CUjnE2vgcUCuhxPwFoQ5r7p1DkhujgvMNDHaF2bLqRp4D5F",
+		},
+		TestCase {
+			description: "DescribeTreasuryVoiceTerminal Sibling",
+			location: Location::new(
+				1,
+				[Parachain(1111), Plurality { id: BodyId::Treasury, part: BodyPart::Voice }],
+			),
+			expected_account_id_str: "5G6TDwaVgbWmhqRUKjBhRRnH4ry9L9cjRymUEmiRsLbSE4gB",
+		},
+		// DescribeBodyTerminal
+		TestCase {
+			description: "DescribeBodyTerminal Parent",
+			location: Location::new(1, [Plurality { id: BodyId::Unit, part: BodyPart::Voice }]),
+			expected_account_id_str: "5EBRMTBkDisEXsaN283SRbzx9Xf2PXwUxxFCJohSGo4jYe6B",
+		},
+		TestCase {
+			description: "DescribeBodyTerminal Sibling",
+			location: Location::new(
+				1,
+				[Parachain(1111), Plurality { id: BodyId::Unit, part: BodyPart::Voice }],
+			),
+			expected_account_id_str: "5DBoExvojy8tYnHgLL97phNH975CyT45PWTZEeGoBZfAyRMH",
+		},


Parent and Sibling (parachain) locations do not make sense for a Relay chain.

All of these should be replaced with Child (parachain) locations.
(DescribeTerminus Child, DescribePalletTerminal Child, etc)

acatangiu · 2024-10-21T13:55:46Z

relay/polkadot/tests/location_conversion.rs

+		// DescribeTerminus
+		TestCase {
+			description: "DescribeTerminus Parent",
+			location: Location::new(1, Here),
+			expected_account_id_str: "5GyWtDJP7qaipWRGr4KJ6VUDxRXf4jDnPW6KPTeCekHfqZkD",
+		},
+		TestCase {
+			description: "DescribeTerminus Sibling",
+			location: Location::new(1, [Parachain(1111)]),
+			expected_account_id_str: "5EC5GfEFm9XEBYjXzxb1VseMHsG2VhPeGTGWF9H8tYZnGsSk",
+		},
+		// DescribePalletTerminal
+		TestCase {
+			description: "DescribePalletTerminal Parent",
+			location: Location::new(1, [PalletInstance(50)]),
+			expected_account_id_str: "5CnwemvaAXkWFVwibiCvf2EjqwiqBi29S5cLLydZLEaEw6jZ",
+		},
+		TestCase {
+			description: "DescribePalletTerminal Sibling",
+			location: Location::new(1, [Parachain(1111), PalletInstance(50)]),
+			expected_account_id_str: "5GFBgPjpEQPdaxEnFirUoa51u5erVx84twYxJVuBRAT2UP2g",
+		},
+		// DescribeAccountId32Terminal
+		TestCase {
+			description: "DescribeAccountId32Terminal Parent",
+			location: Location::new(
+				1,
+				[AccountId32 { network: None, id: AccountId::from(ALICE).into() }],
+			),
+			expected_account_id_str: "5DN5SGsuUG7PAqFL47J9meViwdnk9AdeSWKFkcHC45hEzVz4",
+		},
+		TestCase {
+			description: "DescribeAccountId32Terminal Sibling",
+			location: Location::new(
+				1,
+				[
+					Parachain(1111),
+					Junction::AccountId32 { network: None, id: AccountId::from(ALICE).into() },
+				],
+			),
+			expected_account_id_str: "5DGRXLYwWGce7wvm14vX1Ms4Vf118FSWQbJkyQigY2pfm6bg",
+		},
+		// DescribeAccountKey20Terminal
+		TestCase {
+			description: "DescribeAccountKey20Terminal Parent",
+			location: Location::new(1, [AccountKey20 { network: None, key: [0u8; 20] }]),
+			expected_account_id_str: "5F5Ec11567pa919wJkX6VHtv2ZXS5W698YCW35EdEbrg14cg",
+		},
+		TestCase {
+			description: "DescribeAccountKey20Terminal Sibling",
+			location: Location::new(
+				1,
+				[Parachain(1111), AccountKey20 { network: None, key: [0u8; 20] }],
+			),
+			expected_account_id_str: "5CB2FbUds2qvcJNhDiTbRZwiS3trAy6ydFGMSVutmYijpPAg",
+		},
+		// DescribeTreasuryVoiceTerminal
+		TestCase {
+			description: "DescribeTreasuryVoiceTerminal Parent",
+			location: Location::new(1, [Plurality { id: BodyId::Treasury, part: BodyPart::Voice }]),
+			expected_account_id_str: "5CUjnE2vgcUCuhxPwFoQ5r7p1DkhujgvMNDHaF2bLqRp4D5F",
+		},
+		TestCase {
+			description: "DescribeTreasuryVoiceTerminal Sibling",
+			location: Location::new(
+				1,
+				[Parachain(1111), Plurality { id: BodyId::Treasury, part: BodyPart::Voice }],
+			),
+			expected_account_id_str: "5G6TDwaVgbWmhqRUKjBhRRnH4ry9L9cjRymUEmiRsLbSE4gB",
+		},
+		// DescribeBodyTerminal
+		TestCase {
+			description: "DescribeBodyTerminal Parent",
+			location: Location::new(1, [Plurality { id: BodyId::Unit, part: BodyPart::Voice }]),
+			expected_account_id_str: "5EBRMTBkDisEXsaN283SRbzx9Xf2PXwUxxFCJohSGo4jYe6B",
+		},
+		TestCase {
+			description: "DescribeBodyTerminal Sibling",
+			location: Location::new(
+				1,
+				[Parachain(1111), Plurality { id: BodyId::Unit, part: BodyPart::Voice }],
+			),
+			expected_account_id_str: "5DBoExvojy8tYnHgLL97phNH975CyT45PWTZEeGoBZfAyRMH",
+		},


Parent and Sibling (parachain) locations do not make sense for a Relay chain.

All of these should be replaced with Child (parachain) locations.
(DescribeTerminus Child, DescribePalletTerminal Child, etc)

system-parachains/asset-hubs/asset-hub-kusama/tests/tests.rs

acatangiu · 2024-10-21T14:01:15Z

system-parachains/asset-hubs/asset-hub-kusama/tests/tests.rs

+		// DescribeAccountKey20Terminal
+		TestCase {
+			description: "DescribeAccountKey20Terminal Parent",
+			location: Location::new(1, [AccountKey20 { network: None, key: [0u8; 20] }]),


Same for these AccountKey20s:

Suggested change

location: Location::new(1, [AccountKey20 { network: None, key: [0u8; 20] }]),

location: Location::new(1, [bob_20.clone()]),

with

let bob_20 = AccountKey20 { network: None, key: [123u8; 20] }

(notice I also suggested using non-zero keys)

☑️
Out of curiosity, what edge-case you want to avoid by eliminating zero keys?

x3c41a · 2024-10-23T10:53:34Z

bot fmt

x3c41a · 2024-10-23T10:54:50Z

The conversions are the same for all system parachains, so this test could be a macro to dedup the code, but I don't know where we would put that macro or if it's worth the effort. I'm fine with the duplicated code too.

Please see the other comments.

I don't feel like writing macros just yet, still want to get used to Rust.

…implement Copy trait

github-actions · 2024-10-25T12:45:24Z

Review required! Latest push from author must always be reviewed

CHANGELOG.md

ggwpez · 2024-11-25T14:02:45Z

system-parachains/people/people-polkadot/src/tests.rs

+		expected_account_id_str: &'static str,
+	}
+
+	let test_cases = vec![


Why are the test-cases not defined in a shared crate? They seem to be copy&pasted?

There is a subtle difference among these tests -- DescribeAccountId32Terminal is different in many of the cases.

If I were to refactor the tests I'd rather rewrite those into macros, as Adrian suggested a few comments before. I'll leave it as a follow-up for myself once I fully onboard.

Unless you're talking about people folder specifically, though I would not do it for a single folder...

ggwpez · 2024-11-28T13:35:11Z

/merge

fellowship-merge-bot · 2024-11-28T13:37:02Z

Enabled auto-merge in Pull Request

Available commands

/merge: Enables auto-merge for Pull Request
/merge cancel: Cancels auto-merge for Pull Request
/merge help: Shows this menu

For more information see the documentation

x3c41a added 3 commits October 17, 2024 16:27

added location conversion to relays and parachains

01ffdc2

added test macro and formatted the crate

2e0bc1d

added missing test macro and formatted test files

aa53f0e

x3c41a marked this pull request as draft October 17, 2024 17:03

x3c41a added 3 commits October 17, 2024 20:25

reverted unintended fmt changes caused by wrong version of nighly for…

438c935

…matter

Merge branch 'main' into loc_conv_test

9149704

removed an extra space

b94f2a2

x3c41a marked this pull request as ready for review October 17, 2024 20:36

x3c41a marked this pull request as draft October 17, 2024 20:36

x3c41a changed the title ~~Loc conv test~~ Location conversion tests for relay and parachains Oct 18, 2024

x3c41a added 2 commits October 18, 2024 11:59

modified changelog

2c0661a

Merge branch 'main' into loc_conv_test

ccb601d

x3c41a marked this pull request as ready for review October 18, 2024 09:47

acatangiu suggested changes Oct 21, 2024

View reviewed changes

x3c41a added 4 commits October 23, 2024 13:40

changed relay conversion tests

e381820

refactored system-parachain tests

ce255ad

removed extra namespace xcm::prelude::

331567a

Merge branch 'main' into loc_conv_test

e8c9813

x3c41a requested a review from acatangiu October 23, 2024 11:11

x3c41a marked this pull request as draft October 23, 2024 11:37

x3c41a added 2 commits October 23, 2024 14:53

formatted the change

ef05ebb

clippy: removed AccountId32 and AccountKey20 cloning since they both …

61c44d6

…implement Copy trait

x3c41a marked this pull request as ready for review October 23, 2024 15:06

acatangiu approved these changes Oct 24, 2024

View reviewed changes

franciscoaguirre approved these changes Oct 24, 2024

View reviewed changes

resolved conflicts

4acdf4f

github-actions bot requested review from acatangiu and franciscoaguirre October 25, 2024 12:45

acatangiu approved these changes Oct 25, 2024

View reviewed changes

franciscoaguirre approved these changes Oct 25, 2024

View reviewed changes

Merge branch 'main' into loc_conv_test

65649f2

github-actions bot requested review from acatangiu and franciscoaguirre November 25, 2024 06:16

acatangiu approved these changes Nov 25, 2024

View reviewed changes

bkontur reviewed Nov 25, 2024

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

moved changelog under *Unreleased* section

ef90a33

franciscoaguirre approved these changes Nov 25, 2024

View reviewed changes

ggwpez reviewed Nov 25, 2024

View reviewed changes

x3c41a requested a review from ggwpez November 25, 2024 15:25

ggwpez approved these changes Nov 28, 2024

View reviewed changes

Merge branch 'main' into loc_conv_test

1eaadce

fellowship-merge-bot bot enabled auto-merge (squash) November 28, 2024 13:37

github-actions bot requested review from acatangiu, franciscoaguirre and ggwpez November 28, 2024 13:37

acatangiu approved these changes Nov 28, 2024

View reviewed changes

ggwpez approved these changes Nov 28, 2024

View reviewed changes

fellowship-merge-bot bot merged commit db4bb53 into polkadot-fellows:main Nov 28, 2024
49 of 50 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Location conversion tests for relay and parachains #487

Location conversion tests for relay and parachains #487

x3c41a commented Oct 17, 2024 •

edited

Loading

acatangiu left a comment

acatangiu Oct 21, 2024

x3c41a Oct 23, 2024

acatangiu Oct 21, 2024

x3c41a Oct 23, 2024

acatangiu Oct 21, 2024

x3c41a Oct 23, 2024

x3c41a commented Oct 23, 2024

x3c41a commented Oct 23, 2024

github-actions bot commented Oct 25, 2024

ggwpez Nov 25, 2024

x3c41a Nov 25, 2024

x3c41a Nov 25, 2024

ggwpez commented Nov 28, 2024

fellowship-merge-bot bot commented Nov 28, 2024

	location: Location::new(1, [AccountKey20 { network: None, key: [0u8; 20] }]),
	location: Location::new(1, [bob_20.clone()]),

Location conversion tests for relay and parachains #487

Location conversion tests for relay and parachains #487

Conversation

x3c41a commented Oct 17, 2024 • edited Loading

acatangiu 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

x3c41a commented Oct 23, 2024

x3c41a commented Oct 23, 2024

github-actions bot commented Oct 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ggwpez commented Nov 28, 2024

fellowship-merge-bot bot commented Nov 28, 2024

x3c41a commented Oct 17, 2024 •

edited

Loading