contracts: Fix some minor bugs around instantiation (#8879)

* Fix output of wrongly outputted error

The "Tombstoned a contract that is below the subsistence threshold: {:?}" was
triggered when too few balance was provided. It was a false alarm.

* Fix return of wrong code_len

* Split up `NotCallable` into more fine grained errors

* Fix typos in docs

Co-authored-by: Keith Yeung <[email protected]>

* RentNotPayed -> RentNotPaid

* Fix typo: payed -> paid

It is OK to change the in-storage field name because:

1. The SCALE encoding is not based on names only on position.
2. The struct is not public (only to the crate).

Co-authored-by: Keith Yeung <[email protected]>

frame/contracts/CHANGELOG.md

@@ -49,7 +49,7 @@ This version constitutes the first release that brings any stability guarantees
 [#8014](https://github.com/paritytech/substrate/pull/8014)
 
 - Charge rent for code stored on the chain in addition to the already existing
-rent that is payed for data storage.
+rent that is paid for data storage.
 [#7935](https://github.com/paritytech/substrate/pull/7935)
 
 - Allow the runtime to configure per storage item costs in addition

frame/contracts/src/benchmarking/mod.rs

@@ -409,7 +409,7 @@ benchmarks! {
 
 	// We benchmark the costs for sucessfully evicting an empty contract.
 	// The actual costs are depending on how many storage items the evicted contract
-	// does have. However, those costs are not to be payed by the sender but
+	// does have. However, those costs are not to be paid by the sender but
 	// will be distributed over multiple blocks using a scheduler. Otherwise there is
 	// no incentive to remove large contracts when the removal is more expensive than
 	// the reward for removing them.
@@ -435,7 +435,7 @@ benchmarks! {
 		instance.ensure_tombstone()?;
 
 		// the caller should get the reward for being a good snitch
-		// this is capped by the maximum amount of rent payed. So we only now that it should
+		// this is capped by the maximum amount of rent paid. So we only now that it should
 		// have increased by at most the surcharge reward.
 		assert!(
 			T::Currency::free_balance(&instance.caller) >

frame/contracts/src/exec.rs

@@ -686,8 +686,11 @@ where
 					contract
 				} else {
 					<ContractInfoOf<T>>::get(&dest)
-						.and_then(|contract| contract.get_alive())
-						.ok_or((Error::<T>::NotCallable.into(), 0))?
+						.ok_or((<Error<T>>::ContractNotFound.into(), 0))
+						.and_then(|contract|
+							contract.get_alive()
+								.ok_or((<Error<T>>::ContractIsTombstone.into(), 0))
+						)?
 				};
 
 				let executable = E::from_storage(contract.code_hash, schedule, gas_meter)
@@ -701,7 +704,7 @@ where
 				let contract = Rent::<T, E>
 					::charge(&dest, contract, executable.occupied_storage())
 					.map_err(|e| (e.into(), executable.code_len()))?
-					.ok_or((Error::<T>::NotCallable.into(), executable.code_len()))?;
+					.ok_or((Error::<T>::RentNotPaid.into(), executable.code_len()))?;
 				(dest, contract, executable, ExportedFunction::Call)
 			}
 			FrameArgs::Instantiate{sender, trie_seed, executable, salt} => {
@@ -791,7 +794,7 @@ where
 			let code_len = executable.code_len();
 
 			// Every call or instantiate also optionally transferres balance.
-			self.initial_transfer().map_err(|e| (ExecError::from(e), 0))?;
+			self.initial_transfer().map_err(|e| (ExecError::from(e), code_len))?;
 
 			// Call into the wasm blob.
 			let output = executable.execute(
@@ -954,12 +957,23 @@ where
 
 	// The transfer as performed by a call or instantiate.
 	fn initial_transfer(&self) -> DispatchResult {
+		let frame = self.top_frame();
+		let value = frame.value_transferred;
+		let subsistence_threshold = <Contracts<T>>::subsistence_threshold();
+
+		// If the value transferred to a new contract is less than the subsistence threshold
+		// we can error out early. This avoids executing the constructor in cases where
+		// we already know that the contract has too little balance.
+		if frame.entry_point == ExportedFunction::Constructor && value < subsistence_threshold {
+			return Err(<Error<T>>::NewContractNotFunded.into());
+		}
+
 		Self::transfer(
 			self.caller_is_origin(),
 			false,
 			self.caller(),
-			&self.top_frame().account_id,
-			self.top_frame().value_transferred,
+			&frame.account_id,
+			value,
 		)
 	}
 
@@ -2005,7 +2019,7 @@ mod tests {
 					ctx.ext.instantiate(
 						0,
 						dummy_ch,
-						15u64,
+						Contracts::<Test>::subsistence_threshold(),
 						vec![],
 						&[],
 					),
@@ -2286,7 +2300,7 @@ mod tests {
 		let code = MockLoader::insert(Constructor, |ctx, _| {
 			assert_matches!(
 				ctx.ext.call(0, ctx.ext.address().clone(), 0, vec![]),
-				Err((ExecError{error, ..}, _)) if error == <Error<Test>>::NotCallable.into()
+				Err((ExecError{error, ..}, _)) if error == <Error<Test>>::ContractNotFound.into()
 			);
 			exec_success()
 		});

frame/contracts/src/lib.rs

@@ -388,7 +388,7 @@ pub mod pallet {
 		/// producer fails to do so, a regular users will be allowed to claim the reward.
 		///
 		/// In case of a successful eviction no fees are charged from the sender. However, the
-		/// reward is capped by the total amount of rent that was payed by the contract while
+		/// reward is capped by the total amount of rent that was paid by the contract while
 		/// it was alive.
 		///
 		/// If contract is not evicted as a result of this call, [`Error::ContractNotEvictable`]
@@ -421,10 +421,10 @@ pub mod pallet {
 
 			// If poking the contract has lead to eviction of the contract, give out the rewards.
 			match Rent::<T, PrefabWasmModule<T>>::try_eviction(&dest, handicap)? {
-				(Some(rent_payed), code_len) => {
+				(Some(rent_paid), code_len) => {
 					T::Currency::deposit_into_existing(
 						&rewarded,
-						T::SurchargeReward::get().min(rent_payed),
+						T::SurchargeReward::get().min(rent_paid),
 					)
 					.map(|_| PostDispatchInfo {
 						actual_weight: Some(T::WeightInfo::claim_surcharge(code_len / 1024)),
@@ -535,9 +535,20 @@ pub mod pallet {
 		/// Performing a call was denied because the calling depth reached the limit
 		/// of what is specified in the schedule.
 		MaxCallDepthReached,
-		/// The contract that was called is either no contract at all (a plain account)
-		/// or is a tombstone.
-		NotCallable,
+		/// No contract was found at the specified address.
+		ContractNotFound,
+		/// A tombstone exist at the specified address.
+		///
+		/// Tombstone cannot be called. Anyone can use `seal_restore_to` in order to revive
+		/// the contract, though.
+		ContractIsTombstone,
+		/// The called contract does not have enough balance to pay for its storage.
+		///
+		/// The contract ran out of balance and is therefore eligible for eviction into a
+		/// tombstone. Anyone can evict the contract by submitting a `claim_surcharge`
+		/// extrinsic. Alternatively, a plain balance transfer can be used in order to
+		/// increase the contracts funds so that it can be called again.
+		RentNotPaid,
 		/// The code supplied to `instantiate_with_code` exceeds the limit specified in the
 		/// current schedule.
 		CodeTooLarge,

frame/contracts/src/rent.rs

@@ -96,7 +96,7 @@ where
 	/// Process a report that a contract under the given address should be evicted.
 	///
 	/// Enact the eviction right away if the contract should be evicted and return the amount
-	/// of rent that the contract payed over its lifetime.
+	/// of rent that the contract paid over its lifetime.
 	/// Otherwise, **do nothing** and return None.
 	///
 	/// The `handicap` parameter gives a way to check the rent to a moment in the past instead
@@ -130,15 +130,15 @@ where
 		match verdict {
 			Verdict::Evict { ref amount } => {
 				// The outstanding `amount` is withdrawn inside `enact_verdict`.
-				let rent_payed = amount
+				let rent_paid = amount
 					.as_ref()
 					.map(|a| a.peek())
 					.unwrap_or_else(|| <BalanceOf<T>>::zero())
-					.saturating_add(contract.rent_payed);
+					.saturating_add(contract.rent_paid);
 				Self::enact_verdict(
 					account, contract, current_block_number, verdict, Some(module),
 				)?;
-				Ok((Some(rent_payed), code_len))
+				Ok((Some(rent_paid), code_len))
 			}
 			_ => Ok((None, code_len)),
 		}
@@ -297,7 +297,7 @@ where
 		<ContractInfoOf<T>>::insert(&dest, ContractInfo::Alive(AliveContractInfo::<T> {
 			code_hash,
 			rent_allowance,
-			rent_payed: <BalanceOf<T>>::zero(),
+			rent_paid: <BalanceOf<T>>::zero(),
 			deduct_block: current_block,
 			last_write,
 			.. origin_contract
@@ -544,7 +544,7 @@ where
 				let contract = ContractInfo::Alive(AliveContractInfo::<T> {
 					rent_allowance: alive_contract_info.rent_allowance - amount.peek(),
 					deduct_block: current_block_number,
-					rent_payed: alive_contract_info.rent_payed.saturating_add(amount.peek()),
+					rent_paid: alive_contract_info.rent_paid.saturating_add(amount.peek()),
 					..alive_contract_info
 				});
 				<ContractInfoOf<T>>::insert(account, &contract);

frame/contracts/src/storage.rs

@@ -97,11 +97,11 @@ pub struct RawAliveContractInfo<CodeHash, Balance, BlockNumber> {
 	pub code_hash: CodeHash,
 	/// Pay rent at most up to this value.
 	pub rent_allowance: Balance,
-	/// The amount of rent that was payed by the contract over its whole lifetime.
+	/// The amount of rent that was paid by the contract over its whole lifetime.
 	///
 	/// A restored contract starts with a value of zero just like a new contract.
-	pub rent_payed: Balance,
-	/// Last block rent has been payed.
+	pub rent_paid: Balance,
+	/// Last block rent has been paid.
 	pub deduct_block: BlockNumber,
 	/// Last block child storage has been written.
 	pub last_write: Option<BlockNumber>,
@@ -243,7 +243,7 @@ where
 				// charge rent for it during instantiation.
 				<frame_system::Pallet<T>>::block_number().saturating_sub(1u32.into()),
 			rent_allowance: <BalanceOf<T>>::max_value(),
-			rent_payed: <BalanceOf<T>>::zero(),
+			rent_paid: <BalanceOf<T>>::zero(),
 			pair_count: 0,
 			last_write: None,
 			_reserved: None,

frame/contracts/src/tests.rs

@@ -366,7 +366,7 @@ fn calling_plain_account_fails() {
 			Contracts::call(Origin::signed(ALICE), BOB, 0, GAS_LIMIT, Vec::new()),
 			Err(
 				DispatchErrorWithPostInfo {
-					error: Error::<Test>::NotCallable.into(),
+					error: Error::<Test>::ContractNotFound.into(),
 					post_info: PostDispatchInfo {
 						actual_weight: Some(base_cost),
 						pays_fee: Default::default(),
@@ -396,7 +396,7 @@ fn account_removal_does_not_remove_storage() {
 				deduct_block: System::block_number(),
 				code_hash: H256::repeat_byte(1),
 				rent_allowance: 40,
-				rent_payed: 0,
+				rent_paid: 0,
 				last_write: None,
 				_reserved: None,
 			});
@@ -412,7 +412,7 @@ fn account_removal_does_not_remove_storage() {
 				deduct_block: System::block_number(),
 				code_hash: H256::repeat_byte(2),
 				rent_allowance: 40,
-				rent_payed: 0,
+				rent_paid: 0,
 				last_write: None,
 				_reserved: None,
 			});
@@ -1088,15 +1088,15 @@ fn call_removed_contract() {
 			// Calling contract should deny access because rent cannot be paid.
 			assert_err_ignore_postinfo!(
 				Contracts::call(Origin::signed(ALICE), addr.clone(), 0, GAS_LIMIT, call::null()),
-				Error::<Test>::NotCallable
+				Error::<Test>::RentNotPaid,
 			);
 			// No event is generated because the contract is not actually removed.
 			assert_eq!(System::events(), vec![]);
 
 			// Subsequent contract calls should also fail.
 			assert_err_ignore_postinfo!(
 				Contracts::call(Origin::signed(ALICE), addr.clone(), 0, GAS_LIMIT, call::null()),
-				Error::<Test>::NotCallable
+				Error::<Test>::RentNotPaid,
 			);
 
 			// A snitch can now remove the contract
@@ -1321,7 +1321,7 @@ fn restoration(
 				Contracts::call(
 					Origin::signed(ALICE), addr_bob.clone(), 0, GAS_LIMIT, call::null()
 				),
-				Error::<Test>::NotCallable
+				Error::<Test>::RentNotPaid,
 			);
 			assert!(System::events().is_empty());
 			assert!(ContractInfoOf::<Test>::get(&addr_bob).unwrap().get_alive().is_some());
@@ -2669,11 +2669,11 @@ fn surcharge_reward_is_capped() {
 		let balance = Balances::free_balance(&ALICE);
 		let reward = <Test as Config>::SurchargeReward::get();
 
-		// some rent should have payed due to instantiation
-		assert_ne!(contract.rent_payed, 0);
+		// some rent should have paid due to instantiation
+		assert_ne!(contract.rent_paid, 0);
 
 		// the reward should be parameterized sufficiently high to make this test useful
-		assert!(reward > contract.rent_payed);
+		assert!(reward > contract.rent_paid);
 
 		// make contract eligible for eviction
 		initialize_block(40);
@@ -2682,13 +2682,13 @@ fn surcharge_reward_is_capped() {
 		assert_ok!(Contracts::claim_surcharge(Origin::none(), addr.clone(), Some(ALICE)));
 
 		// this reward does not take into account the last rent payment collected during eviction
-		let capped_reward = reward.min(contract.rent_payed);
+		let capped_reward = reward.min(contract.rent_paid);
 
 		// this is smaller than the actual reward because it does not take into account the
 		// rent collected during eviction
 		assert!(Balances::free_balance(&ALICE) > balance + capped_reward);
 
-		// the full reward is not payed out because of the cap introduced by rent_payed
+		// the full reward is not paid out because of the cap introduced by rent_paid
 		assert!(Balances::free_balance(&ALICE) < balance + reward);
 	});
 }

frame/contracts/src/wasm/runtime.rs

@@ -601,14 +601,16 @@ where
 		let transfer_failed = Error::<E::T>::TransferFailed.into();
 		let not_funded = Error::<E::T>::NewContractNotFunded.into();
 		let no_code = Error::<E::T>::CodeNotFound.into();
-		let invalid_contract = Error::<E::T>::NotCallable.into();
+		let not_found = Error::<E::T>::ContractNotFound.into();
+		let is_tombstone = Error::<E::T>::ContractIsTombstone.into();
+		let rent_not_paid = Error::<E::T>::RentNotPaid.into();
 
 		match from {
 			x if x == below_sub => Ok(BelowSubsistenceThreshold),
 			x if x == transfer_failed => Ok(TransferFailed),
 			x if x == not_funded => Ok(NewContractNotFunded),
 			x if x == no_code => Ok(CodeNotFound),
-			x if x == invalid_contract => Ok(NotCallable),
+			x if (x == not_found || x == is_tombstone || x == rent_not_paid) => Ok(NotCallable),
 			err => Err(err)
 		}
 	}