Skip to content
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

Fix schedule fee #723

Merged
merged 8 commits into from
Feb 5, 2021
Merged

Fix schedule fee #723

merged 8 commits into from
Feb 5, 2021

Conversation

zjb0807
Copy link
Member

@zjb0807 zjb0807 commented Feb 4, 2021

Closes: #700

@zjb0807 zjb0807 requested a review from xlc February 4, 2021 09:33
@@ -25,6 +25,8 @@ jobs:
components: rustfmt
target: wasm32-unknown-unknown
default: true
- name: Checkout submodules
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we shouldn't need this

@@ -327,8 +376,27 @@ pub fn bob() -> H160 {
H160([0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 2])
}

pub const NETWORK_CONTRACT_INDEX: u64 = 2048;
pub fn evm_genesis() -> (BTreeMap<H160, module_evm::GenesisAccount<Balance, Nonce>>, u64) {
let contracts_json = &include_bytes!("../../../../../predeploy-contracts/resources/bytecodes.json")[..];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you have one more ../?

@@ -321,3 +323,38 @@ pub trait EVMStateRentTrait<AccountId, Balance> {
/// Transfer the maintainer of the contract address.
fn transfer_maintainer(from: AccountId, contract: H160, new_maintainer: H160) -> DispatchResult;
}

pub trait EnsureCanChargeFee<AccountId, Balance, NegativeImbalance> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EnsureCanChargeFee is not very accurate, maybe make it TransactionPayment instead or ChargeTransactionPayment

let refund_gas = gas_limit.saturating_sub(used_gas);
if !refund_gas.is_zero() {
T::ChargeTransactionPayment::refund_fee(&from_account, T::GasToWeight::convert(refund_gas), imbalance)
.map_err(|_| Error::<T>::ChargeFeeFailed)?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should just ignore the refund error here

) -> Result<(PalletBalanceOf<T>, Option<NegativeImbalanceOf<T>>), TransactionValidityError> {
let fee = Module::<T>::weight_to_fee(weight);
<T as Config>::Currency::unreserve(&who, fee);
Module::<T>::ensure_can_charge_fee(who, fee, WithdrawReasons::TRANSACTION_PAYMENT);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we want this again. The unreserve should make sure there is enough ACA and ensure_can_charge_fee could buy more ACA if the preferred token is not ACA

fn unreserve_and_charge_fee(
who: &T::AccountId,
weight: Weight,
) -> Result<(PalletBalanceOf<T>, Option<NegativeImbalanceOf<T>>), TransactionValidityError> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why Option<NegativeImbalanceOf<T>> not just NegativeImbalanceOf<T>?

let actual_payment = match <T as Config>::Currency::deposit_into_existing(&who, refund) {
Ok(refund_imbalance) => {
// The refund cannot be larger than the up front payed max weight.
// `PostDispatchInfo::calc_unspent` guards against such a case.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this comment is accurate?

@zjb0807 zjb0807 merged commit 69ddb4f into master Feb 5, 2021
@zjb0807 zjb0807 deleted the fix-schedule-fee branch February 5, 2021 03:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

EVM: Scheduler handle transaction fee
2 participants