Precompile for balances ERC-20 #1731
Conversation
lemunozm
added
P5-soon
lemunozm
requested review from
mustermeiszer,
hieronx and
wischli
as code owners
| pub fn to_evm_address(account: &AccountId) -> H160 { | ||
| // EVM AccountId20 addresses are right-padded to 32 bytes | ||
| let mut bytes20 = [0; 20]; | ||
| let account_32: [u8; 32] = account.clone().into(); | ||
| bytes20.copy_from_slice(&account_32); | ||
| H160::from(bytes20) | ||
| } | ||
|
|
There was a problem hiding this comment.
Why the new method? Doesn't our current converter already provide means to go back to H160. I am not the biggest fan of having this here if this is a duplicate and creates a second source of truth for conversions.
There was a problem hiding this comment.
Not sure exactly why, but PrecompileSetBuilder::user_addresses() returns an AccountId instead of H160 (internally, it transforms H160 into AccountId).
So, we need this method to go back from AccountId to H160 in chain_spec.rs.
I really think it is taking more responsibility than it should, but I have no choice right now with the API they offer. Maybe I can open an issue regarding this in Moonbeam.
There was a problem hiding this comment.
Doesn't our current converter already provide means to go back to H160
Not for this direction, only for H160 -> AccountId
There was a problem hiding this comment.
In the router we are doing this conversion:
/// NOTE - the sender account ID provided here will be converted to an EVM
/// address via truncating. When the call is processed by the underlying EVM
/// pallet, this EVM address will be converted back into a substrate account
/// which will be charged for the transaction. This converted substrate
/// account is not the same as the original account.
pub fn do_send(&self, sender: T::AccountId, msg: Vec<u8>) -> DispatchResultWithPostInfo {
let sender_evm_address = H160::from_slice(&sender.as_ref()[0..20]);and here we are doing it also. I would like to have the runtime_common::account_conversion::AccountConverter to implement a trait Convert<AccountId32, H160> and then use this converter as an associated type in theses pallets. Otherwise, we have multiple locations that do conversion.
No blocker. Just do not wanna create a foot-gun...
There was a problem hiding this comment.
Ok, I guess H160::from_slice(&sender.as_ref()[0..20]) should be extracted to a converter.
There was a problem hiding this comment.
I've been juggling traits in this commit: 0fb227c, and I've got to remove the conversion, so there is no issue (at least by now).
|
I think I've solved all comments @mustermeiszer |
| # Recomended command: | ||
| # cargo install taplo-cli --locked | ||
| cargo install --git=https://github.com/tamasfe/taplo.git taplo-cli | ||
| cargo install taplo-cli --locked |
There was a problem hiding this comment.
Required to fix taplo CI
I could also log the address in the utility method, but the method is infallible and very simple to work as expected in all cases. |
|
@mustermeiszer, merged but still open to add a log if after what I say you still consider we need it |
|
Fine for me if you are confident it works! |
Description
Fixes #1726
Changes and Descriptions
precompile-utils, which is more type safety.balances-erc20forbid-evm-reentrancyfeatureDelegateCallsecurity issue