✨ class hash whitelist #1550
✨ class hash whitelist #1550
Conversation
|
Can you rebase? Also, do we want this in the vanilla codebase? Or should we put it behind a flag? As not all chain will want it. It's our first "out of spec" feature |
There was a problem hiding this comment.
I am confused with the feature flag option. Technically, it feels correct to add it behind the feature flag but
- if we add more features like this eventually, we might end up with multiple flags A, B, C etc.
- the only way to ensure they are maintained is to add their test cases in the CI. this can eventually complicate things as maybe some feature works with flags A and B but not when you enable C. so do we test all combinations or do we test the trivial ones?
I am not sure what the pragmatic thing to do here is. What codebases would be good examples for this?
| @@ -157,6 +157,8 @@ pub mod pallet { | |||
| type ProtocolVersion: Get<u8>; | |||
| #[pallet::constant] | |||
| type ProgramHash: Get<Felt252Wrapper>; | |||
| #[pallet::constant] | |||
| type WhitelistedClassHashes: Get<Vec<CasmClassHash>>; | |||
There was a problem hiding this comment.
why do we've a pallet constant and a storage variable?
There was a problem hiding this comment.
should we also add a vector in the genesis config where the chain can set the class hashes on start?
There was a problem hiding this comment.
Hm yea we can probably do that, in this case I'll remove the constant then right ?
| @@ -552,6 +563,15 @@ pub mod pallet { | |||
| // This ensures that the function can only be called via unsigned transaction. | |||
| ensure_none(origin)?; | |||
|
|
|||
| // Check if class hash is whitelisted | |||
| let whitelisted_class_hashes = Self::whitelisted_class_hashes(); | |||
| if !whitelisted_class_hashes.is_empty() { | |||
There was a problem hiding this comment.
should we have a boolean variable for whitelist class hash enabled? i can think there would be cases where a chain starts with whitelisted but later opens it to everyone. i guess Pragma might want to do this as well right?
There was a problem hiding this comment.
Yes would make sense
Agreed I'm really not sure of the best approach here. For this small feature, let's put under a feature flag and add it in CI for now? We can think of a solution for next times |
|
There hasn't been any activity on this pull request recently, and in order to prioritize active work, it has been marked as stale. |
|
Should be easier to impl after #1590 |
Pull Request type
Please add the labels corresponding to the type of changes your PR introduces:
X Feature
What is the current behavior?
Resolves: #NA
What is the new behavior?
Introduces a new class hash whitelisting mechanism.
Adds 2 new extrinsics to whitelist/blacklist class hashes that can only be called by
root.The validation happens both in the mempool and at the
declareextrinsic level.If no class hashes have been whitelisted (by default), everything can be declared.
Does this introduce a breaking change?
No
Other information