Contracts: runtime_call and storage_deposit (#13990)

* wip

* add comments

* fix comment

* comments

* comments

* PR comment

* field orders

* Update frame/contracts/src/tests.rs

* Update frame/contracts/fixtures/call_runtime_and_call.wat

Co-authored-by: Sasha Gryaznov <hi@agryaznov.com>

* Apply suggestions from code review

Co-authored-by: Sasha Gryaznov <hi@agryaznov.com>

* Apply suggestions from code review

Co-authored-by: Sasha Gryaznov <hi@agryaznov.com>

* Update frame/contracts/src/tests.rs

Co-authored-by: Sasha Gryaznov <hi@agryaznov.com>

* Validate fees of failed call

* Update frame/contracts/src/tests.rs

* Update frame/contracts/src/tests.rs

* Update frame/contracts/src/tests.rs

* bubble up refund error

* rename fixture file

---------

Co-authored-by: Sasha Gryaznov <hi@agryaznov.com>
Co-authored-by: parity-processbot <>
This commit is contained in:
PG Herveou
2023-04-29 18:07:55 +02:00
committed by GitHub
parent 1eb2e4f390
commit 4df004d0d4
6 changed files with 189 additions and 67 deletions
+22 -64
View File
@@ -19,7 +19,7 @@
use crate::{
storage::{ContractInfo, DepositAccount},
BalanceOf, Config, Error, Inspect, Pallet, System, LOG_TARGET,
BalanceOf, Config, Error, Inspect, Pallet, System,
};
use codec::Encode;
use frame_support::{
@@ -85,7 +85,7 @@ pub trait Ext<T: Config> {
deposit_account: &DepositAccount<T>,
amount: &DepositOf<T>,
terminated: bool,
);
) -> Result<(), DispatchError>;
}
/// This [`Ext`] is used for actual on-chain execution when balance needs to be charged.
@@ -366,14 +366,14 @@ where
///
/// This drops the root meter in order to make sure it is only called when the whole
/// execution did finish.
pub fn into_deposit(self, origin: &T::AccountId) -> DepositOf<T> {
pub fn try_into_deposit(self, origin: &T::AccountId) -> Result<DepositOf<T>, DispatchError> {
for charge in self.charges.iter().filter(|c| matches!(c.amount, Deposit::Refund(_))) {
E::charge(origin, &charge.deposit_account, &charge.amount, charge.terminated);
E::charge(origin, &charge.deposit_account, &charge.amount, charge.terminated)?;
}
for charge in self.charges.iter().filter(|c| matches!(c.amount, Deposit::Charge(_))) {
E::charge(origin, &charge.deposit_account, &charge.amount, charge.terminated);
E::charge(origin, &charge.deposit_account, &charge.amount, charge.terminated)?;
}
self.total_deposit
Ok(self.total_deposit)
}
}
@@ -428,7 +428,8 @@ where
info.deposit_account(),
&deposit.saturating_sub(&Deposit::Charge(ed)),
false,
);
)?;
System::<T>::inc_consumers(info.deposit_account())?;
// We also need to make sure that the contract's account itself exists.
@@ -514,71 +515,27 @@ impl<T: Config> Ext<T> for ReservingExt {
deposit_account: &DepositAccount<T>,
amount: &DepositOf<T>,
terminated: bool,
) {
// There is nothing we can do when this fails as this constitutes a bug in the runtime.
// We need to settle for emitting an error log in this case.
//
// # Note
//
// This is infallible because it is called in a part of the execution where we cannot
// simply roll back. It might make sense to do some refactoring to move the deposit
// collection to the fallible part of execution.
) -> Result<(), DispatchError> {
match amount {
Deposit::Charge(amount) => {
// This will never fail because a deposit account is required to exist
// at all times. The pallet enforces this invariant by holding a consumer reference
// on the deposit account as long as the contract exists.
//
// The sender always has enough balance because we checked that it had enough
// balance when instantiating the storage meter. There is no way for the sender
// which is a plain account to send away this balance in the meantime.
let result = T::Currency::transfer(
origin,
deposit_account,
*amount,
ExistenceRequirement::KeepAlive,
);
if let Err(err) = result {
log::error!(
target: LOG_TARGET,
"Failed to transfer storage deposit {:?} from origin {:?} to deposit account {:?}: {:?}",
amount, origin, deposit_account, err,
);
if cfg!(debug_assertions) {
panic!("Unable to collect storage deposit. This is a bug.");
}
}
},
// The receiver always exists because the initial value transfer from the
// origin to the contract has a keep alive existence requirement. When taking a deposit
// we make sure to leave at least the ed in the free balance.
//
// The sender always has enough balance because we track it in the `ContractInfo` and
// never send more back than we have. No one has access to the deposit account. Hence no
// other interaction with this account takes place.
Deposit::Charge(amount) => T::Currency::transfer(
origin,
deposit_account,
*amount,
ExistenceRequirement::KeepAlive,
),
Deposit::Refund(amount) => {
if terminated {
System::<T>::dec_consumers(&deposit_account);
}
let result = T::Currency::transfer(
T::Currency::transfer(
deposit_account,
origin,
*amount,
// We can safely use `AllowDeath` because our own consumer prevents an removal.
ExistenceRequirement::AllowDeath,
);
if matches!(result, Err(_)) {
log::error!(
target: LOG_TARGET,
"Failed to refund storage deposit {:?} from deposit account {:?} to origin {:?}: {:?}",
amount, deposit_account, origin, result,
);
if cfg!(debug_assertions) {
panic!("Unable to refund storage deposit. This is a bug.");
}
}
)
},
};
}
}
}
@@ -651,7 +608,7 @@ mod tests {
contract: &DepositAccount<Test>,
amount: &DepositOf<Test>,
terminated: bool,
) {
) -> Result<(), DispatchError> {
TestExtTestValue::mutate(|ext| {
ext.charges.push(Charge {
origin: origin.clone(),
@@ -660,6 +617,7 @@ mod tests {
terminated,
})
});
Ok(())
}
}
@@ -757,7 +715,7 @@ mod tests {
nested0.enforce_limit(Some(&mut nested0_info)).unwrap();
meter.absorb(nested0, DepositAccount(BOB), Some(&mut nested0_info));
meter.into_deposit(&ALICE);
meter.try_into_deposit(&ALICE).unwrap();
assert_eq!(nested0_info.extra_deposit(), 112);
assert_eq!(nested1_info.extra_deposit(), 110);
@@ -817,7 +775,7 @@ mod tests {
nested0.absorb(nested1, DepositAccount(CHARLIE), None);
meter.absorb(nested0, DepositAccount(BOB), None);
meter.into_deposit(&ALICE);
meter.try_into_deposit(&ALICE).unwrap();
assert_eq!(
TestExtTestValue::get(),