Allow for precompiles to have arbitrary addresses, potentially more than one.

[bernardoaraujor]

The main modification is the addition of a new StorageDoubleMap into pallet-evm:

edit: this PR adds a new pallet-evm-precompile, which allows using Precompiles with arbitrary addresses, potentially more than one.

A StorageMap keeps track of the Precompiles on-chain, where:

• key: H160
• value: PrecompileLabel

https://github.com/paritytech/frontier/blob/e13ce6c611771f5b436d1e3cff89c013853f4b71/frame/evm-precompile/src/lib.rs#L147-L150

A PrecompileLabel determines which functionality the Precompile has. It is declared as a BoundedVec<u8, ConstU32<32>>, which means the user is free to choose a label (e.g.: b"Sha3FIPS512") that's up-to 32 bytes long:
https://github.com/paritytech/frontier/blob/e13ce6c611771f5b436d1e3cff89c013853f4b71/frame/evm-precompile/src/lib.rs#L46

OnChainPrecompiles implements the PrecompileSet trait, where the Precompile addresses are routed to the appropriate Precompile::execute implementation according to the on-chan mapping:
https://github.com/paritytech/frontier/blob/e13ce6c611771f5b436d1e3cff89c013853f4b71/frame/evm-precompile/src/lib.rs#L72-L107

[boundless-forest]

In which cases do you use the multiple addresses for a particular precompile? If the address of the a precompile changes, is it enough to just update the address is enough?

[sorpaas]

I think mostly something like ERC20/asset. You can have lots of contracts of the same code.

If the address of the a precompile changes, is it enough to just update the address is enough?

No, because in this case the address will have their associated storages, which differentiate them.

And this is supposed not to happen -- a precompile's label is to remain static.

[bernardoaraujor]

In which cases do you use the multiple addresses for a particular precompile?

This change is in the context of a new feature that will potentially be upstreamed later. We want to have specific precompiles that use traits like frame_support::traits::tokens::fungibles::*. For example, an ERC20-compatible precompile that uses pallet-assets to manage the tokens. In this case, different precompile addresses (that are associated with the Fungibles label) will map to different AssetIds.

But for most use-cases where a single precompile address is sufficient, this won't be really useful. In those cases, just associate a single address (potentially hardcoded) to the precompile label and that's it.

If the address of the a precompile changes, is it enough to just update the address is enough?

I'm not sure I fully understand the question. With this implementation, you can't really "change" the address of a precompile. You can add a new precompile address with the same label, which would now make two instances of the same precompile. You can later remove the first address. Whatever dApp that expects the old precompile address would stop working at this point.

[bernardoaraujor]

Another point worth mentioning is that using this feature (on-chain precompile addresses) is totally optional.

You could still implement your own PrecompileSet::execute() so that it only executes precompiles with hardcoded addresses, the same way FrontierPrecompiles<R> does in its current form:
https://github.com/paritytech/frontier/blob/58c568964dc32dd96153b7d113ac2df58d27de2b/template/runtime/src/precompiles.rs#L30-L47

A hybrid solution mixing both approaches (hardcoded + on-chain precompile addresses) is also possible.

The only breaking changes that this PR imposes is in case some runtime eventually decides to opt-in to on-chain precompile addresses via pallet-evm-precompile, a storage migration is necessary to populate storage with the previously hardcoded addresses.

[boundless-forest]

If this pr is finally acceptable, consider changing the () to bool?
This way, you can choose to enable/disable one of the precompile's addresses.

[sorpaas]

Hmm why do we need double storage map here? I think just a storage map mapping H160 to label would be enough.

[bernardoaraujor]

@sorpaas I switched to StorageMap.

But then I was thinking: wouldn't it be useful to use StorageDoubleMap and have an index for each H160 address?

• k1: H160
• k2: PrecompileLabel
• v: PrecompileIndex

where PrecompileIndex is a Config type, with same traits as AssetId

that would avoid the need for using #[precompiles::precompile_set] & #[precompile::discriminant] to distinguish between precompiles within the same label (which btw is Moonbeam's approach for their assets-erc20 precompiles cc @nanocryk )

[sorpaas]

That can be useful, but I don't see why we need double storage map. We can have H160 -> (PrecompileLabel, PrecompileIndex) map, and that should be sufficient. Instead of PrecompileIndex, we should allow arbitrary data to be passed to the executor with the type controllable by the config.

I honestly start to think that all this probably belongs to a new pallet. Apart from all the scaffolding, it should be a trivial change. We have a new pallet pallet-evm-precompile and move the following to it: a) this precompile storage map, b) the add/remove precompile dispatchable. Then we implement PrecompileSet on Pallet struct. Any users who have imported the new pallet can then set the PrecompileSet config in pallet-evm to this new pallet.

The advantage of this is that anyone who wants not to use precompile label can stop worrying about it, and don't need to care about an unused precompile storage map in the code.

[bernardoaraujor]

cool, I'll refactor the changes into a separate pallet-evm-precompile

[bernardoaraujor]

do we leave template using the old FrontierPrecompiles as PrecompileSet, or the new one (pallet-evm-precompile)?

[bernardoaraujor]

I did some experiments and it turns out having an on-chain PrecompileIndex is not really useful.

impl<R> PrecompileSet for OnChainPrecompiles<R>
where
	R: pallet::Config + pallet_evm::Config,
{
	fn execute(&self, handle: &mut impl PrecompileHandle) -> Option<PrecompileResult> {
		match handle.code_address() {
			// Ethereum precompiles :
			a if &Precompiles::<R>::get(a).0[..] == b"PrecompileX" => {
				let precompile_index = Precompiles::<R>::get(a).1; // this is useless
				Some(PrecompileX::execute(handle))
			}
...

even though I can read precompile_index, I have no means to pass it to PrecompileX::execute(handle).

It seems the only available approach is using Moonbeam's #[precompiles::precompile_set] & #[precompile::discriminant].

[sorpaas]

We can modify that trait to take on precompile_index though.

But if you want, let's do that in a separate PR. This precompile_index is best added as a separate storage item (because large storage, which can happen with arbitrary data, takes longer to retrieve).

[sorpaas]

do we leave template using the old FrontierPrecompiles as PrecompileSet, or the new one (pallet-evm-precompile)?

Please make the template use the new one. One of the use for the template is to test our all pallets within this project, so it should include as many pallets as possible.

[crystalin]

@tgmichel or @nanocryk can you get a look at it too please ?

[sorpaas]

You can directly impl<T: Config> PrecompileSet for Pallet<T>. That will allow the interface to be much nicer.

To do that you need to create another trait whose purpose is to map labels to executions, something like this:

trait PrecompileLabelSet {
  fn execute(&self, label: &PrecompileLabel, handle: &mut impl PrecompileHandle) -> Option<PrecompileResult>;
}

Then add that to Config.

[bernardoaraujor]

pallet-evm has the following types in its Config trait:

type PrecompilesType: PrecompileSet;
type PrecompilesValue: Get<Self::PrecompilesType>;

as pointed out by @kianenigma , it seems that impl<T: Config> PrecompileSet for Pallet<T> is not doable, because pallet structs are not meant to be instantiated: https://substrate.stackexchange.com/questions/7579/how-to-implement-the-get-trait-for-a-pallett?noredirect=1#comment7602_7579

[sorpaas]

I'd still prefer that we use the pallet name directly instead of creating another custom type, to keep things consistent everywhere.

To do this we need to remove the self attribute from PrecompileSet. This is trivial on type level:

trait Precompiles {
   fn execute(arg1, arg2) -> res;
}

trait PrecompileSet {
  fn execute(&self, arg1, arg2) -> res;
}

impl<T: PrecompileSet> Precompiles for Get<T> {
  fn execute(arg1, arg2) -> res {
    T::get().execute(arg1, arg2)
  }
}

Then on config:

type Precompiles: Precompiles;

However, on code level we uses in a lot of places actual references of precompiles, which makes things slightly complicated.

evm and pallet-evm had different design goals. In evm we'd actually want the reference thing because precompile set can be dynamic. And people should be able to change the set without recompiling their code. In pallet-evm, this is not needed at all because all the storage accesses (which is the only thing that makes things dynamic) is entirely static.

We probably have other type workarounds, so unless you have better ideas, please temporarily do not work on this until we figure something out (or decide to use the old wrapper type method).

[nanocryk]

I would likely discourage to store the mapping in Substrate storage unless you want to be able to change the addresses without runtime upgrades. You do a lot of storage reads which have non negligible costs.

If you don't need to change addresses outside of runtime upgrades, you should make your own enum representing the precompiles with a method fn from_address(address: H160) -> Option<Self>.

If you need this storage I would advice renaming it to pallet-evm-precompile-label, as the pallet itself only provide labels that are then used inside the PrecompileSet impl, while the current naming could make think that this is THE way to manage precompile in a runtime.

[sorpaas]

@nanocryk The case is when you have millions of precompiles around and when most of them are of the same code (think ERC20). It's impossible to keep everything in memory.

That's also why this is designed as a separate pallet, because not everyone needs them.