Skip to content

Conversation

@karolk91
Copy link
Contributor

@karolk91 karolk91 commented Jun 24, 2025

This PR introduces test case scenarios to verify if parachain is able to process authorize_upgrade call as if it was send via governance chain (a Relay Chain for purpose of this PR)

These test cases were introduced to polkadot-sdk and suggested via comment that it would be nice to port, so this is just integration of existing testnets cases for production networks

This PR also introduces integration tests covering all system chains that simulates real network upgrade using whitelisting (via Collectives chain in case of Polkadot)

Test Coverage

Chain Unit Tests Integration Tests
Polkadot Asset Hub
Polkadot Coretime
Polkadot Collectives
Polkadot Bridge Hub
Polkadot People
Kusama Asset Hub
Kusama Coretime
Kusama Bridge Hub
Kusama People
Kusama Encointer Lack of mocked network definition here
Kusama Relay Not applicable
Polkadot Relay Not applicable
Kusama Glutton Not supported: BasicParachainRuntime not implemented Lack of mocked network definition here

Follow-up:

For AHM, we will add also the tests for OpenGov on AHs:
#776
#626

@karolk91 karolk91 marked this pull request as ready for review June 25, 2025 08:32
pub UniversalLocation: InteriorLocation =
[GlobalConsensus(RelayNetwork::get()), Parachain(ParachainInfo::parachain_id().into())].into();
pub StakingPot: AccountId = CollatorSelection::account_id();
pub const GovernanceLocation: Location = Location::parent();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@karolk91 just thinking, can we just define this in the test file directly, when it is not used here? I think later we will have this constant here: https://github.com/polkadot-fellows/runtimes/blob/main/system-parachains/constants/src/kusama.rs#L153 right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@karolk91 just thinking, can we just define this in the test file directly, when it is not used here? I think later we will have this constant here: https://github.com/polkadot-fellows/runtimes/blob/main/system-parachains/constants/src/kusama.rs#L153 right?

I should probably just introduce this common constant as part of this PR

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have introduced GovernanceLocation to common package in b5a8792. I have dropped const modifier because in future I expect it to point to AssetHubLocation and we would need to drop const anyway

@bkontur, I have followed the convention that GovernanceLocation is re-exported via xcm_config for every chain. In case of Encointer its only used for testing, do you think its worth breaking the convention for this single occurence and import straight in tests instead of via xcm_config ?

Copy link
Contributor

@bkontur bkontur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@karolk91 Good job! This PR captures the current state as-is with tests and will be useful when moving OpenGov to the AssetHubs - #626 and #776

pub UniversalLocation: InteriorLocation =
[GlobalConsensus(RelayNetwork::get()), Parachain(ParachainInfo::parachain_id().into())].into();
pub StakingPot: AccountId = CollatorSelection::account_id();
pub const GovernanceLocation: Location = Location::parent();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@github-actions github-actions bot requested review from acatangiu and bkontur June 25, 2025 19:14
@github-actions
Copy link

Review required! Latest push from author must always be reviewed

Copy link
Contributor

@bkontur bkontur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@karolk91 please, add to the desc, that we are adding also governance integration emulated tests, which simulates the real usage how we upgrade live chains and

Follow-up section:

For AHM, we will add also the tests for OpenGov on AHs:
#776
#626

@karolk91 karolk91 changed the title System chains governace authorizes upgrade unit tests System chains governace authorizes upgrade tests Jun 26, 2025
Co-authored-by: Branislav Kontur <[email protected]>
@github-actions github-actions bot requested a review from bkontur June 26, 2025 12:38
);

// whitelist
Kusama::execute_with(|| {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@karolk91 Karol, we forgot to add and use here macro_rules! assert_whitelisted { :), please

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@github-actions github-actions bot requested a review from bkontur June 26, 2025 20:12
@github-actions github-actions bot requested a review from bkontur July 3, 2025 08:10
Copy link
Member

@pandres95 pandres95 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, looks good. Just a minor nit, but can be handled on a further PR.

[[package]]
name = "emulated-integration-tests-common"
version = "20.1.0"
version = "21.0.0"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Open question: Wouldn't it make sense to upgrade the entire set of dependencies to the latest patch version, now that we're making changes on Cargo.lock?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Open question: Wouldn't it make sense to upgrade the entire set of dependencies to the latest patch version, now that we're making changes on Cargo.lock?

Well, in the past we did both - PR with patch upgrade and also PR with just few patch deps upgrade - I think both ways are ok. We can do follow-up to bring the full stable2503-7

frame-election-provider-support = { version = "40.1.1", default-features = false }
frame-executive = { version = "40.0.1", default-features = false }
frame-support = { version = "40.1.0", default-features = false }
frame-system = { version = "40.1.0", default-features = false }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think it'd definitely be better if we update other related deps in the PSDK, to prevent having multiple versions of the crates pinned to multiple dependants. This tends to be messy sometimes,

@bkontur
Copy link
Contributor

bkontur commented Jul 11, 2025

/merge

@fellowship-merge-bot fellowship-merge-bot bot merged commit 089c589 into polkadot-fellows:main Jul 11, 2025
66 checks passed
@fellowship-merge-bot
Copy link
Contributor

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

@bkontur bkontur deleted the kk-governance-tests-main branch July 11, 2025 06:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants