Permissioned contract deployment

[Szegoo]

Closes: #3196

[athei]

This PR does not check the InstantiateOrigin when a contract instantiates another contract. Is this intended? No matter what the answer this has to be documented on the Config knob.

[Szegoo]

This PR does not check the InstantiateOrigin when a contract instantiates another contract. Is this intended?

Yes, this was intended, as I think it should probably be up to the contract logic to regulate other contract instantiation.

[athei]

Tests are insufficient. The instantiation extrinsics are not tested. You also need to define a special implementation for EnsureOrigin where you can change what it does dynamically. Have a look how we do that with parameter_types! for our TestExtension. This will allow you to change what it does for your tests while leave it as-is for the existing tests.

[Szegoo]

The instantiation extrinsics are not tested.

It was actually tested, the instantiation origin was configured to allow any signed origin to deploy, and there was already a root_cannot_instantiate test.

Based on your suggestion, I added more tests by dynamically restricting upload and instantiation, so it is better tested now.

[pgherveou]

@Szegoo seeing your PR a bit late commented here
#3196 (comment)

Haven't look at the code here yet, but from the description it seems that this is something that can be done already. Can you double check and tell me what you think

[Szegoo]

This is more configurable. dispatch_as requires the origin to be root, while here the upload and instantiate origin could be set to anything(e.g. a collective).

[paritytech-cicd-pr]

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-clippy
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5431858

[athei]

dispatch_as requires the origin to be root, while here the upload and instantiate origin could be set to anything(e.g. a collective).

Are you sure this is true? You should be able to check the origin in the filter. Zeitgeist is not doing this but you could do that, right?

[Szegoo]

Are you sure this is true?

I was referring to:

polkadot-sdk/substrate/frame/utility/src/lib.rs

Line 390 in ec30d2f

ensure_root(origin)?;

Not so sure about checking the origin in the filter though. I will look into that.

[athei]

Yes this is understood. Probably should have quoted this:

This is more configurable.

Because you don't need to use dispatch_as as Zeitgeist does I think. You should be able to check the origin in the filter.

[pgherveou]

dispatch_as requires the origin to be root, while here the upload and instantiate origin could be set to anything(e.g. a collective).

Are you sure this is true? You should be able to check the origin in the filter. Zeitgeist is not doing this but you could do that, right?

I don't think you can the filter only give you access to the RuntimeCall

[pgherveou]

polkadot-sdk/substrate/frame/system/src/lib.rs

Lines 444 to 455 in c34ad77

	/// The basic call filter to use in Origin. All origins are built with this filter as base,
	/// except Root.
	///
	/// This works as a filter for each incoming call. The call needs to pass this filter in
	/// order to dispatch. Otherwise it will be rejected with `CallFiltered`. This can be
	/// bypassed via `dispatch_bypass_filter` which should only be accessible by root. The
	/// filter can be composed of sub-filters by nesting for example
	/// [`frame_support::traits::InsideBoth`], [`frame_support::traits::TheseExcept`] or
	/// [`frame_support::traits::EverythingBut`] et al. The default would be
	/// [`frame_support::traits::Everything`].
	#[pallet::no_default_bounds]
	type BaseCallFilter: Contains<Self::RuntimeCall>;

This is used by the construct_runtime here

polkadot-sdk/substrate/frame/support/procedural/src/construct_runtime/expand/origin.rs

Lines 164 to 170 in c34ad77

	fn filter_call(&self, call: &Self::Call) -> bool {
	match self.caller {
	// Root bypasses all filters
	OriginCaller::system(#system_path::Origin::<#runtime>::Root) => true,
	_ => (self.filter)(call),
	}
	}

[athei]

Yes you are right. The filter does not allow access to the origin.