Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pallet prefix runtime test #573

Merged
merged 6 commits into from
Jul 6, 2021
Merged

Conversation

4meta5
Copy link
Contributor

@4meta5 4meta5 commented Jul 4, 2021

Adds a test to tell us if the pallet prefixes for each runtime ever change.

the tested pallet prefix is used to determine the storage key for all storage items in the pallet

The test verifies that the String passed in as an arg matches the pallet_prefix() method on the implementation of StorageInstance for all storage items in the given pallet (here). The method pallet_prefix() (from the StorageInstance trait) is used in the calculation of the unique storage key in the implementation of StorageValue (and all the other storage item types in their respective implementatons).

Comment on lines 81 to 94
let prefix = |pallet_name, storage_name| {
let mut res = [0u8; 32];
res[0..16].copy_from_slice(&Twox128::hash(pallet_name));
res[16..32].copy_from_slice(&Twox128::hash(storage_name));
res
};
assert_eq!(
<moonbase_runtime::Sudo as StorageInfoTrait>::storage_info(),
vec![StorageInfo {
prefix: prefix(b"Sudo", b"Key"),
max_values: Some(1),
max_size: Some(20),
}]
);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately some pallet instances do not implement StorageInfoTrait so I cannot do this test for all pallets. I can remove it if we're satisfied with the above tests.

It still may be worth having some test like this for all storage items in all pallets to ensure the entire storage key does not change with any runtime changes. The tests further above only test the consistency of the pallet prefix part of the storage key.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I like better the test where it queries from a storage item, as it is closer to the actual usage done by the runtime.
But both are good

Comment on lines 81 to 94
let prefix = |pallet_name, storage_name| {
let mut res = [0u8; 32];
res[0..16].copy_from_slice(&Twox128::hash(pallet_name));
res[16..32].copy_from_slice(&Twox128::hash(storage_name));
res
};
assert_eq!(
<moonbase_runtime::Sudo as StorageInfoTrait>::storage_info(),
vec![StorageInfo {
prefix: prefix(b"Sudo", b"Key"),
max_values: Some(1),
max_size: Some(20),
}]
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I like better the test where it queries from a storage item, as it is closer to the actual usage done by the runtime.
But both are good

@4meta5
Copy link
Contributor Author

4meta5 commented Jul 5, 2021

@thiolliere do you know of a more direct way to check that the pallet prefix part of the storage keys match our expectations? The goal is a test that tells us if/when the pallet prefix changes in a way that changes the storage keys.

I'd prefer to use the storage_info() method on the StorageInfoTrait, but it doesn't find the implementation for a few pallets.

error[E0277]: the trait bound `frame_system::Pallet<moonbase_runtime::Runtime>: StorageInfoTrait` is not satisfied
  --> runtime/moonbase/tests/integration_test.rs:96:3
   |
96 |         <moonbase_runtime::System as StorageInfoTrait>::storage_info(),
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `StorageInfoTrait` is not implemented for `frame_system::Pallet<moonbase_runtime::Runtime>`
   |
   = note: required by `storage_info`

error: aborting due to previous error

@gui1117
Copy link

gui1117 commented Jul 5, 2021

@thiolliere do you know of a more direct way to check that the pallet prefix part of the storage keys match our expectations? The goal is a test that tells us if/when the pallet prefix changes in a way that changes the storage keys.

I'd prefer to use the storage_info() method on the StorageInfoTrait, but it doesn't find the implementation for a few pallets.

error[E0277]: the trait bound `frame_system::Pallet<moonbase_runtime::Runtime>: StorageInfoTrait` is not satisfied
  --> runtime/moonbase/tests/integration_test.rs:96:3
   |
96 |         <moonbase_runtime::System as StorageInfoTrait>::storage_info(),
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `StorageInfoTrait` is not implemented for `frame_system::Pallet<moonbase_runtime::Runtime>`
   |
   = note: required by `storage_info`

error: aborting due to previous error

The trait StorageInfoTrait was implemented only when the attribute generate_storage_info was set. But with paritytech/substrate#9246 you should be able to query it

@4meta5
Copy link
Contributor Author

4meta5 commented Jul 5, 2021

Thank you! I'll use PartialStorageInfo once we update our substrate deps to include that commit.

@gui1117
Copy link

gui1117 commented Jul 5, 2021

Thank you! I'll use PartialStorageInfo once we update our substrate deps to include that commit.

in the commit, it is StorageInfoTrait which is implemented for all pallets struct. So you can call <moonbase_runtime::System as StorageInfoTrait>::storage_info()

@4meta5 4meta5 merged commit 18ffdfc into master Jul 6, 2021
@4meta5 4meta5 deleted the amar-verify-storage-pallet-prefixes branch July 6, 2021 10: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.

3 participants