diff --git a/prdoc/pr_8718.prdoc b/prdoc/pr_8718.prdoc new file mode 100644 index 0000000000000..b80f86bcc6a60 --- /dev/null +++ b/prdoc/pr_8718.prdoc @@ -0,0 +1,7 @@ +title: Record ed as part of the storage deposit +doc: +- audience: Runtime Dev + description: Fixes https://github.com/paritytech/contract-issues/issues/38 +crates: +- name: pallet-revive + bump: patch diff --git a/substrate/frame/revive/src/exec.rs b/substrate/frame/revive/src/exec.rs index 7797b7b2e475b..ad67ec9681100 100644 --- a/substrate/frame/revive/src/exec.rs +++ b/substrate/frame/revive/src/exec.rs @@ -761,14 +761,14 @@ where pub fn run_call( origin: Origin, dest: H160, - gas_meter: &'a mut GasMeter, - storage_meter: &'a mut storage::meter::Meter, + gas_meter: &mut GasMeter, + storage_meter: &mut storage::meter::Meter, value: U256, input_data: Vec, skip_transfer: bool, ) -> ExecResult { let dest = T::AddressMapper::to_account_id(&dest); - if let Some((mut stack, executable)) = Self::new( + if let Some((mut stack, executable)) = Stack::<'_, T, E>::new( FrameArgs::Call { dest: dest.clone(), cached_info: None, delegated_call: None }, origin.clone(), gas_meter, @@ -778,7 +778,7 @@ where )? { stack.run(executable, input_data).map(|_| stack.first_frame.last_frame_output) } else { - let result = Self::transfer_from_origin(&origin, &origin, &dest, value); + let result = Self::transfer_from_origin(&origin, &origin, &dest, value, storage_meter); if_tracing(|t| { t.enter_child_span( origin.account_id().map(T::AddressMapper::to_address).unwrap_or_default(), @@ -807,15 +807,15 @@ where pub fn run_instantiate( origin: T::AccountId, executable: E, - gas_meter: &'a mut GasMeter, - storage_meter: &'a mut storage::meter::Meter, + gas_meter: &mut GasMeter, + storage_meter: &mut storage::meter::Meter, value: U256, input_data: Vec, salt: Option<&[u8; 32]>, skip_transfer: bool, nonce_already_incremented: NonceAlreadyIncremented, ) -> Result<(H160, ExecReturnValue), ExecError> { - let (mut stack, executable) = Self::new( + let (mut stack, executable) = Stack::<'_, T, E>::new( FrameArgs::Instantiate { sender: origin.clone(), executable, @@ -1165,6 +1165,7 @@ where &caller, account_id, frame.value_transferred, + &mut frame.nested_storage, )?; } @@ -1365,11 +1366,12 @@ where /// not exist, the transfer does fail and nothing will be sent to `to` if either `origin` can /// not provide the ED or transferring `value` from `from` to `to` fails. /// Note: This will also fail if `origin` is root. - fn transfer( + fn transfer( origin: &Origin, from: &T::AccountId, to: &T::AccountId, value: U256, + storage_meter: &mut storage::meter::GenericMeter, ) -> ExecResult { let value = crate::Pallet::::convert_evm_to_native(value, ConversionPrecision::Exact)?; if value.is_zero() { @@ -1391,18 +1393,24 @@ where T::Currency::transfer(from, to, value, Preservation::Preserve) .map_err(|_| Error::::TransferFailed.into()) }) { - Ok(_) => TransactionOutcome::Commit(Ok(Default::default())), + Ok(_) => { + // ed is taken from the transaction signer so it should be + // limited by the storage deposit + storage_meter.record_charge(&StorageDeposit::Charge(ed)); + TransactionOutcome::Commit(Ok(Default::default())) + }, Err(err) => TransactionOutcome::Rollback(Err(err)), } }) } /// Same as `transfer` but `from` is an `Origin`. - fn transfer_from_origin( + fn transfer_from_origin( origin: &Origin, from: &Origin, to: &T::AccountId, value: U256, + storage_meter: &mut storage::meter::GenericMeter, ) -> ExecResult { // If the from address is root there is no account to transfer from, and therefore we can't // take any `value` other than 0. @@ -1411,7 +1419,7 @@ where Origin::Root if value.is_zero() => return Ok(Default::default()), Origin::Root => return Err(DispatchError::RootNotAllowed.into()), }; - Self::transfer(origin, from, to, value) + Self::transfer(origin, from, to, value, storage_meter) } /// Reference to the current (top) frame. @@ -1764,11 +1772,14 @@ where } else if is_read_only { Err(Error::::StateChangeDenied.into()) } else { + let account_id = self.account_id().clone(); + let frame = top_frame_mut!(self); Self::transfer_from_origin( &self.origin, - &Origin::from_account_id(self.account_id().clone()), + &Origin::from_account_id(account_id), &dest, value, + &mut frame.nested_storage, ) }; diff --git a/substrate/frame/revive/src/exec/tests.rs b/substrate/frame/revive/src/exec/tests.rs index ef2b165fd337b..2ea91aeb594e4 100644 --- a/substrate/frame/revive/src/exec/tests.rs +++ b/substrate/frame/revive/src/exec/tests.rs @@ -235,12 +235,19 @@ fn transfer_works() { set_balance(&ALICE, 100); set_balance(&BOB, 0); + let value = 55; let origin = Origin::from_account_id(ALICE); - MockStack::transfer(&origin, &ALICE, &BOB, 55u64.into()).unwrap(); + let mut storage_meter = storage::meter::Meter::new(u64::MAX); + MockStack::transfer(&origin, &ALICE, &BOB, value.into(), &mut storage_meter).unwrap(); let min_balance = ::Currency::minimum_balance(); - assert_eq!(get_balance(&ALICE), 45 - min_balance); - assert_eq!(get_balance(&BOB), 55 + min_balance); + assert!(min_balance > 0); + assert_eq!(get_balance(&ALICE), 100 - value - min_balance); + assert_eq!(get_balance(&BOB), min_balance + value); + assert_eq!( + storage_meter.try_into_deposit(&Origin::from_account_id(ALICE), false).unwrap(), + StorageDeposit::Charge(min_balance) + ); }); } @@ -252,6 +259,7 @@ fn transfer_to_nonexistent_account_works() { ExtBuilder::default().build().execute_with(|| { let ed = ::Currency::minimum_balance(); let value = 1024; + let mut storage_meter = storage::meter::Meter::new(u64::MAX); // Transfers to nonexistant accounts should work set_balance(&ALICE, ed * 2); @@ -261,7 +269,8 @@ fn transfer_to_nonexistent_account_works() { &Origin::from_account_id(ALICE), &BOB, &CHARLIE, - value.into() + value.into(), + &mut storage_meter, )); assert_eq!(get_balance(&ALICE), ed); assert_eq!(get_balance(&BOB), ed); @@ -271,7 +280,13 @@ fn transfer_to_nonexistent_account_works() { set_balance(&ALICE, ed); set_balance(&BOB, ed + value); assert_err!( - MockStack::transfer(&Origin::from_account_id(ALICE), &BOB, &DJANGO, value.into()), + MockStack::transfer( + &Origin::from_account_id(ALICE), + &BOB, + &DJANGO, + value.into(), + &mut storage_meter + ), >::StorageDepositNotEnoughFunds, ); @@ -279,7 +294,13 @@ fn transfer_to_nonexistent_account_works() { set_balance(&ALICE, ed * 2); set_balance(&BOB, value); assert_err!( - MockStack::transfer(&Origin::from_account_id(ALICE), &BOB, &EVE, value.into()), + MockStack::transfer( + &Origin::from_account_id(ALICE), + &BOB, + &EVE, + value.into(), + &mut storage_meter + ), >::TransferFailed ); // The ED transfer would work. But it should only be executed with the actual transfer @@ -444,9 +465,15 @@ fn balance_too_low() { let ed = ::Currency::minimum_balance(); set_balance(&ALICE, ed * 2); set_balance(&from, ed + 99); + let mut storage_meter = storage::meter::Meter::new(u64::MAX); - let result = - MockStack::transfer(&Origin::from_account_id(ALICE), &from, &dest, 100u64.into()); + let result = MockStack::transfer( + &Origin::from_account_id(ALICE), + &from, + &dest, + 100u64.into(), + &mut storage_meter, + ); assert_eq!(result, Err(Error::::TransferFailed.into())); assert_eq!(get_balance(&ALICE), ed * 2); diff --git a/substrate/frame/revive/src/storage/meter.rs b/substrate/frame/revive/src/storage/meter.rs index edb463ee3a131..6a4738a6c1df8 100644 --- a/substrate/frame/revive/src/storage/meter.rs +++ b/substrate/frame/revive/src/storage/meter.rs @@ -304,6 +304,14 @@ where } } + /// Record a charge that has taken place externally. + /// + /// This will not perform a charge. It just records it to reflect it in the + /// total amount of storage required for a transaction. + pub fn record_charge(&mut self, amount: &DepositOf) { + self.total_deposit = self.total_deposit.saturating_add(&amount); + } + /// The amount of balance that is still available from the original `limit`. fn available(&self) -> BalanceOf { self.total_deposit.available(&self.limit) @@ -397,14 +405,6 @@ impl> RawMeter { self.charges.push(Charge { contract, amount, state: ContractState::Alive }); } - /// Record a charge that has taken place externally. - /// - /// This will not perform a charge. It just records it to reflect it in the - /// total amount of storage required for a transaction. - pub fn record_charge(&mut self, amount: &DepositOf) { - self.total_deposit = self.total_deposit.saturating_add(&amount); - } - /// Call to tell the meter that the currently executing contract was terminated. /// /// This will manipulate the meter so that all storage deposit accumulated in