-
Notifications
You must be signed in to change notification settings - Fork 520
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
Allow for precompiles to have arbitrary addresses, potentially more than one. #1016
base: master
Are you sure you want to change the base?
Changes from 1 commit
71e440d
584f5a2
5683cdb
fa9a744
84f21ae
eea6f64
428320f
88dd7b4
074f5bc
093d3a9
0961b8a
e13ce6c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -467,6 +467,7 @@ pub mod pallet { | |
#[cfg_attr(feature = "std", derive(Default))] | ||
pub struct GenesisConfig { | ||
pub accounts: std::collections::BTreeMap<H160, GenesisAccount>, | ||
pub precompiles: Vec<(Vec<u8>, H160)>, | ||
} | ||
|
||
#[pallet::genesis_build] | ||
|
@@ -497,6 +498,10 @@ pub mod pallet { | |
<AccountStorages<T>>::insert(address, index, value); | ||
} | ||
} | ||
|
||
for (label, address) in &self.precompiles { | ||
Pallet::<T>::add_precompile(label, address); | ||
} | ||
} | ||
} | ||
|
||
|
@@ -508,6 +513,17 @@ pub mod pallet { | |
#[pallet::getter(fn account_storages)] | ||
pub type AccountStorages<T: Config> = | ||
StorageDoubleMap<_, Blake2_128Concat, H160, Blake2_128Concat, H256, H256, ValueQuery>; | ||
|
||
/// Allows for precompiles to have arbitrary addresses, potentially more than one. | ||
/// `k1`: precompile label, e.g.: `b"Sha3FIPS256".to_vec()` | ||
/// `k2`: precompile address | ||
/// | ||
/// Please note that adding a new precompile label here is not enough to guarantee its execution | ||
/// It is also required to list the new label on the implementation of `PrecompileSet::execute()` | ||
#[pallet::storage] | ||
#[pallet::getter(fn precompiles)] | ||
pub type Precompiles<T: Config> = | ||
StorageDoubleMap<_, Blake2_128Concat, Vec<u8>, Blake2_128Concat, H160, (), OptionQuery>; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If this pr is finally acceptable, consider changing the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm why do we need double storage map here? I think just a storage map mapping There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @sorpaas I switched to But then I was thinking: wouldn't it be useful to use
where that would avoid the need for using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That can be useful, but I don't see why we need double storage map. We can have 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 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. cool, I'll refactor the changes into a separate There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do we leave There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I did some experiments and it turns out having an on-chain
even though I can read It seems the only available approach is using Moonbeam's There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can modify that trait to take on But if you want, let's do that in a separate PR. This There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. |
||
} | ||
|
||
/// Type alias for currency balance. | ||
|
@@ -671,6 +687,16 @@ impl<T: Config> GasWeightMapping for FixedGasWeightMapping<T> { | |
static LONDON_CONFIG: EvmConfig = EvmConfig::london(); | ||
|
||
impl<T: Config> Pallet<T> { | ||
/// Add a precompile to storage | ||
pub fn add_precompile(label: &Vec<u8>, address: &H160) { | ||
bernardoaraujor marked this conversation as resolved.
Show resolved
Hide resolved
|
||
Precompiles::<T>::set(label, address, Some(())); | ||
} | ||
|
||
/// Remove a precompile from storage | ||
pub fn remove_precompile(label: &Vec<u8>, address: &H160) { | ||
Precompiles::<T>::remove(label, address); | ||
} | ||
|
||
/// Check whether an account is empty. | ||
pub fn is_account_empty(address: &H160) -> bool { | ||
let (account, _) = Self::account_basic(address); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think mostly something like ERC20/asset. You can have lots of contracts of the same code.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 usespallet-assets
to manage the tokens. In this case, different precompile addresses (that are associated with theFungibles
label) will map to differentAssetId
s.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.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 wayFrontierPrecompiles<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.