Skip to content

pallet evm review #6

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.

Sign up for GitHub

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

Open
xlc wants to merge 1 commit into
base: master
Choose a base branch
from
Open

pallet evm review #6

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions frame/evm/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,6 +299,7 @@ decl_module! {

fn deposit_event() = default;

// why do we need this? people can always use `call` to do the transfer
/// Withdraw balance from EVM into currency/balances module.
#[weight = 0]
fn withdraw(origin, address: H160, value: BalanceOf<T>) {
Expand All @@ -314,6 +315,8 @@ decl_module! {
}

/// Issue an EVM call operation. This is similar to a message call transaction in Ethereum.
// why gas_price * gas_limit? why how much people paying matters here?
// This should be T::ConvertGasToWeight::convert(gas_limit)
#[weight = (*gas_price).saturated_into::<Weight>().saturating_mul(*gas_limit as Weight)]
fn call(
origin,
Expand Down Expand Up @@ -345,11 +348,13 @@ decl_module! {
},
}

// should refund unused gas as weight
Ok(Pays::No.into())
}

/// Issue an EVM create operation. This is similar to a contract creation transaction in
/// Ethereum.
// Why do we need three different calls? Can we just have an enum to indicate the action like pallet-ethereum does?
#[weight = (*gas_price).saturated_into::<Weight>().saturating_mul(*gas_limit as Weight)]
fn create(
origin,
Expand Down Expand Up @@ -432,16 +437,21 @@ impl<T: Trait> Module<T> {
if current.nonce < new.nonce {
// ASSUME: in one single EVM transaction, the nonce will not increase more than
// `u128::max_value()`.
// Account nonce is u32 for Polkadot, very unlikely to be u128 for most of the chains
// But I agree we should be able to assume it should never overflow to whatever the nonce type is
// This is super inefficient
for _ in 0..(new.nonce - current.nonce).low_u128() {
frame_system::Module::<T>::inc_account_nonce(&account_id);
}
}

if current.balance > new.balance {
let diff = current.balance - new.balance;
// why low_u128 is needed? unique_saturated_into should be enough.
T::Currency::slash(&account_id, diff.low_u128().unique_saturated_into());
} else if current.balance < new.balance {
let diff = new.balance - current.balance;
// ditto
T::Currency::deposit_creating(&account_id, diff.low_u128().unique_saturated_into());
}
}
Expand All @@ -451,8 +461,8 @@ impl<T: Trait> Module<T> {
let account = Self::account_basic(address);
let code_len = AccountCodes::decode_len(address).unwrap_or(0);

account.nonce == U256::zero() &&
account.balance == U256::zero() &&
account.nonce.is_zero() &&
account.is_zero() &&
code_len == 0
}

Expand All @@ -471,6 +481,8 @@ impl<T: Trait> Module<T> {
let balance = T::Currency::free_balance(&account_id);

Account {
// this doesn't feel right that we need to convert into u128 and then into u256
// what happen if the Balance type is U256?
nonce: U256::from(UniqueSaturatedInto::<u128>::unique_saturated_into(nonce)),
balance: U256::from(UniqueSaturatedInto::<u128>::unique_saturated_into(balance)),
}
Expand Down