Move pallet-revive precompile to its own crate

[xlc]

Mono repo makes it so easy to introduce poorly considered dependency. There is no automation to check this so code reviewers have to be more strict during code review.

Currently pallet-xcm and pallet-assets both depends on pallet-revive just because the precompile lives in those crates. Obviously the majority of the user of those crates never going to touch pallet-revive but have to suffer all the downsides of including it (include two VMs to the dependencies).

We could feature flag it but feature flag is often (ab)used too much.

Related: #9425

[bkchr]

The precompiles should not be part of the pallet crates. They should be put into their own crates.

CC @athei

[athei]

I agree. Needing to pull in pallet-revive when you don't have to is annoying because it increases build times.

Are we sure we want a separate pallet vs. feature flag? I dislike feature flags like the next guy but it could be annoying to export APIs from the pallet just for the sake of using it in the precompile crate.

I don't really have a strong opinion on this.

As of right now its only pallet-asset (@pgherveou) and pallet-xcm (@franciscoaguirre) that would need to be refactored. I am gonna make sure that new pre-compiles will be their own pallet.

[bkchr]

Are we sure we want a separate pallet vs. feature flag?

I assume we will hit the point in which we will need multiple pallets for one set of precompiles? Then we would need to go the separate crate way any way. (I would not create an extra pallet) Maybe for now just having one extra crate for all the pre-compiles could also be solution.

[athei]

Maybe for now just having one extra crate for all the pre-compiles could also be solution.

But this would then need feature flag or you start in pulling pallets you don't need. Okay, so far its only super essentially pallets but it might change.

I vote for one pallet per pre-compile. As soon as you move the pre-compile out of the pallet crate you already need to export everything it needs from the pallet.

(I would not create an extra pallet)

You kinda need a pallet to have benchmarks. And the pre-compile does also read data from the pallet which does not have pre-existing benchmarks.

The idea was to have the pre-compile as close to the pallet as possible because it has to be stable. Keeping the code closer together makes it easier to reason about. But its just a minor argument.

[pandres95]

Question: is this thread somehow related to this issue I'm experiencing when upgrading to stable2506?

https://github.com/virto-network/frame-contrib/runs/48614629875

Basically, it tries to cargo check on pallet-revive (which is a dependency of pallet-assets), but instead ends up raising this build error, even though that a) pallet-assets is not a dependency on any crates within, but a dev-dependency on some, and b) pallet-revive is not a dependency on any:

error[E0046]: not all trait items implemented, missing: `unchecked_into_checked_i_know_what_i_am_doing`
   --> /home/runner/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/pallet-revive-0.7.0/src/evm/runtime.rs:123:1
    |
123 | / impl<LookupSource, Signature, E, Lookup> Checkable<Lookup>
124 | |     for UncheckedExtrinsic<LookupSource, Signature, E>
125 | | where
126 | |     E: EthExtra,
...   |
140 | |             Checked = CheckedExtrinsic<AccountIdOf<E::Config>, CallOf<E::Config>, E::Extension>,
141 | |         >,
    | |__________^ missing `unchecked_into_checked_i_know_what_i_am_doing` in implementation
    |
    = help: implement the missing item: `fn unchecked_into_checked_i_know_what_i_am_doing(self, _: &Lookup) -> std::result::Result<<Self as Checkable<Lookup>>::Checked, TransactionValidityError> { todo!() }`

[athei]

What happened here is that for some reason the try-truntime feature got enabled for pallet-revive but not enabled for sp-runtime.

Why that happens need some debugging. Maybe some feature is not forwarded where it should be in some of your manifests?

[pandres95]

I've already reviewed my workspace using zepter. Not really sure that's the case to us. 🤔

Oh, also forgot to mention: the issue just happens when doing cargo check --workspace --all-features --frozen. Individual projects' checks (like cargo check -p fc-pallet-pass --all-features --frozen), as well as the checks in the runtime that actually uses these pallets don't raise this error.