Emit ContractReverted error when revert flag is set (paritytech#10481)

* Emit `ContractReverted` error when revert flag is set

* `is_success()` -> `did_revert()`

frame/contracts/common/src/lib.rs

@@ -131,9 +131,9 @@ pub struct ExecReturnValue {
 }
 
 impl ExecReturnValue {
-	/// We understand the absense of a revert flag as success.
-	pub fn is_success(&self) -> bool {
-		!self.flags.contains(ReturnFlags::REVERT)
+	/// The contract did revert all storage changes.
+	pub fn did_revert(&self) -> bool {
+		self.flags.contains(ReturnFlags::REVERT)
 	}
 }
 
@@ -170,6 +170,12 @@ pub enum Code<Hash> {
 	Existing(Hash),
 }
 
+impl<T: Into<Vec<u8>>, Hash> From<T> for Code<Hash> {
+	fn from(from: T) -> Self {
+		Code::Upload(Bytes(from.into()))
+	}
+}
+
 /// The amount of balance that was either charged or refunded in order to pay for storage.
 #[derive(Eq, PartialEq, Ord, PartialOrd, Encode, Decode, RuntimeDebug, Clone)]
 #[cfg_attr(feature = "std", derive(Serialize, Deserialize))]

frame/contracts/src/exec.rs

@@ -686,7 +686,7 @@ where
 				.map_err(|e| ExecError { error: e.error, origin: ErrorOrigin::Callee })?;
 
 			// Additional work needs to be performed in case of an instantiation.
-			if output.is_success() && entry_point == ExportedFunction::Constructor {
+			if !output.did_revert() && entry_point == ExportedFunction::Constructor {
 				let frame = self.top_frame();
 
 				// It is not allowed to terminate a contract inside its constructor.
@@ -713,7 +713,7 @@ where
 		let (success, output) = with_transaction(|| {
 			let output = do_transaction();
 			match &output {
-				Ok(result) if result.is_success() => TransactionOutcome::Commit((true, output)),
+				Ok(result) if !result.did_revert() => TransactionOutcome::Commit((true, output)),
 				_ => TransactionOutcome::Rollback((false, output)),
 			}
 		});
@@ -1352,7 +1352,7 @@ mod tests {
 			)
 			.unwrap();
 
-			assert!(!output.is_success());
+			assert!(output.did_revert());
 			assert_eq!(get_balance(&origin), 100);
 			assert_eq!(get_balance(&dest), balance);
 		});
@@ -1403,7 +1403,7 @@ mod tests {
 			);
 
 			let output = result.unwrap();
-			assert!(output.is_success());
+			assert!(!output.did_revert());
 			assert_eq!(output.data, Bytes(vec![1, 2, 3, 4]));
 		});
 	}
@@ -1435,7 +1435,7 @@ mod tests {
 			);
 
 			let output = result.unwrap();
-			assert!(!output.is_success());
+			assert!(output.did_revert());
 			assert_eq!(output.data, Bytes(vec![1, 2, 3, 4]));
 		});
 	}

frame/contracts/src/lib.rs

@@ -289,7 +289,7 @@ pub mod pallet {
 		) -> DispatchResultWithPostInfo {
 			let origin = ensure_signed(origin)?;
 			let dest = T::Lookup::lookup(dest)?;
-			let output = Self::internal_call(
+			let mut output = Self::internal_call(
 				origin,
 				dest,
 				value,
@@ -298,6 +298,11 @@ pub mod pallet {
 				data,
 				None,
 			);
+			if let Ok(retval) = &output.result {
+				if retval.did_revert() {
+					output.result = Err(<Error<T>>::ContractReverted.into());
+				}
+			}
 			output.gas_meter.into_dispatch_result(output.result, T::WeightInfo::call())
 		}
 
@@ -346,7 +351,7 @@ pub mod pallet {
 			let origin = ensure_signed(origin)?;
 			let code_len = code.len() as u32;
 			let salt_len = salt.len() as u32;
-			let output = Self::internal_instantiate(
+			let mut output = Self::internal_instantiate(
 				origin,
 				value,
 				gas_limit,
@@ -356,6 +361,11 @@ pub mod pallet {
 				salt,
 				None,
 			);
+			if let Ok(retval) = &output.result {
+				if retval.1.did_revert() {
+					output.result = Err(<Error<T>>::ContractReverted.into());
+				}
+			}
 			output.gas_meter.into_dispatch_result(
 				output.result.map(|(_address, result)| result),
 				T::WeightInfo::instantiate_with_code(code_len / 1024, salt_len / 1024),
@@ -381,7 +391,7 @@ pub mod pallet {
 		) -> DispatchResultWithPostInfo {
 			let origin = ensure_signed(origin)?;
 			let salt_len = salt.len() as u32;
-			let output = Self::internal_instantiate(
+			let mut output = Self::internal_instantiate(
 				origin,
 				value,
 				gas_limit,
@@ -391,6 +401,11 @@ pub mod pallet {
 				salt,
 				None,
 			);
+			if let Ok(retval) = &output.result {
+				if retval.1.did_revert() {
+					output.result = Err(<Error<T>>::ContractReverted.into());
+				}
+			}
 			output.gas_meter.into_dispatch_result(
 				output.result.map(|(_address, output)| output),
 				T::WeightInfo::instantiate(salt_len / 1024),
@@ -540,6 +555,11 @@ pub mod pallet {
 		StorageDepositLimitExhausted,
 		/// Code removal was denied because the code is still in use by at least one contract.
 		CodeInUse,
+		/// The contract ran to completion but decided to revert its storage changes.
+		/// Please note that this error is only returned from extrinsics. When called directly
+		/// or via RPC an `Ok` will be returned. In this case the caller needs to inspect the flags
+		/// to determine whether a reversion has taken place.
+		ContractReverted,
 	}
 
 	/// A mapping from an original code hash to the original code, untouched by instrumentation.

frame/contracts/src/tests.rs

@@ -24,7 +24,7 @@ use crate::{
 	storage::Storage,
 	wasm::{PrefabWasmModule, ReturnCode as RuntimeReturnCode},
 	weights::WeightInfo,
-	BalanceOf, CodeStorage, Config, ContractInfoOf, Error, Pallet, Schedule,
+	BalanceOf, Code, CodeStorage, Config, ContractInfoOf, Error, Pallet, Schedule,
 };
 use assert_matches::assert_matches;
 use codec::Encode;
@@ -1096,7 +1096,7 @@ fn crypto_hashes() {
 				<Pallet<Test>>::bare_call(ALICE, addr.clone(), 0, GAS_LIMIT, None, params, false)
 					.result
 					.unwrap();
-			assert!(result.is_success());
+			assert!(!result.did_revert());
 			let expected = hash_fn(input.as_ref());
 			assert_eq!(&result.data[..*expected_size], &*expected);
 		}
@@ -1881,11 +1881,11 @@ fn reinstrument_does_charge() {
 
 		let result0 =
 			Contracts::bare_call(ALICE, addr.clone(), 0, GAS_LIMIT, None, zero.clone(), false);
-		assert!(result0.result.unwrap().is_success());
+		assert!(!result0.result.unwrap().did_revert());
 
 		let result1 =
 			Contracts::bare_call(ALICE, addr.clone(), 0, GAS_LIMIT, None, zero.clone(), false);
-		assert!(result1.result.unwrap().is_success());
+		assert!(!result1.result.unwrap().did_revert());
 
 		// They should match because both where called with the same schedule.
 		assert_eq!(result0.gas_consumed, result1.gas_consumed);
@@ -1899,7 +1899,7 @@ fn reinstrument_does_charge() {
 		// This call should trigger reinstrumentation
 		let result2 =
 			Contracts::bare_call(ALICE, addr.clone(), 0, GAS_LIMIT, None, zero.clone(), false);
-		assert!(result2.result.unwrap().is_success());
+		assert!(!result2.result.unwrap().did_revert());
 		assert!(result2.gas_consumed > result1.gas_consumed);
 		assert_eq!(
 			result2.gas_consumed,
@@ -2162,7 +2162,7 @@ fn ecdsa_recover() {
 			<Pallet<Test>>::bare_call(ALICE, addr.clone(), 0, GAS_LIMIT, None, params, false)
 				.result
 				.unwrap();
-		assert!(result.is_success());
+		assert!(!result.did_revert());
 		assert_eq!(result.data.as_ref(), &EXPECTED_COMPRESSED_PUBLIC_KEY);
 	})
 }
@@ -2808,3 +2808,94 @@ fn call_after_killed_account_needs_funding() {
 		);
 	});
 }
+
+#[test]
+fn contract_reverted() {
+	let (wasm, code_hash) = compile_module::<Test>("return_with_data").unwrap();
+
+	ExtBuilder::default().existential_deposit(100).build().execute_with(|| {
+		let _ = Balances::deposit_creating(&ALICE, 1_000_000);
+		let flags = ReturnFlags::REVERT;
+		let buffer = [4u8, 8, 15, 16, 23, 42];
+		let input = (flags.bits(), buffer).encode();
+
+		// We just upload the code for later use
+		assert_ok!(Contracts::upload_code(Origin::signed(ALICE), wasm.clone(), None));
+
+		// Calling extrinsic: revert leads to an error
+		assert_err_ignore_postinfo!(
+			Contracts::instantiate(
+				Origin::signed(ALICE),
+				0,
+				GAS_LIMIT,
+				None,
+				code_hash,
+				input.clone(),
+				vec![],
+			),
+			<Error<Test>>::ContractReverted,
+		);
+
+		// Calling extrinsic: revert leads to an error
+		assert_err_ignore_postinfo!(
+			Contracts::instantiate_with_code(
+				Origin::signed(ALICE),
+				0,
+				GAS_LIMIT,
+				None,
+				wasm,
+				input.clone(),
+				vec![],
+			),
+			<Error<Test>>::ContractReverted,
+		);
+
+		// Calling directly: revert leads to success but the flags indicate the error
+		// This is just a different way of transporting the error that allows the read out
+		// the `data` which is only there on success. Obviously, the contract isn't
+		// instantiated.
+		let result = Contracts::bare_instantiate(
+			ALICE,
+			0,
+			GAS_LIMIT,
+			None,
+			Code::Existing(code_hash),
+			input.clone(),
+			vec![],
+			false,
+		)
+		.result
+		.unwrap();
+		assert_eq!(result.result.flags, flags);
+		assert_eq!(result.result.data.0, buffer);
+		assert!(!<ContractInfoOf<Test>>::contains_key(result.account_id));
+
+		// Pass empty flags and therefore successfully instantiate the contract for later use.
+		let addr = Contracts::bare_instantiate(
+			ALICE,
+			0,
+			GAS_LIMIT,
+			None,
+			Code::Existing(code_hash),
+			ReturnFlags::empty().bits().encode(),
+			vec![],
+			false,
+		)
+		.result
+		.unwrap()
+		.account_id;
+
+		// Calling extrinsic: revert leads to an error
+		assert_err_ignore_postinfo!(
+			Contracts::call(Origin::signed(ALICE), addr.clone(), 0, GAS_LIMIT, None, input.clone()),
+			<Error<Test>>::ContractReverted,
+		);
+
+		// Calling directly: revert leads to success but the flags indicate the error
+		let result = Contracts::bare_call(ALICE, addr.clone(), 0, GAS_LIMIT, None, input, false)
+			.result
+			.unwrap();
+		assert_eq!(result.flags, flags);
+		assert_eq!(result.data.0, buffer);
+	});
+}

frame/contracts/src/wasm/mod.rs

@@ -1765,7 +1765,7 @@ mod tests {
 				data: Bytes(hex!("445566778899").to_vec()),
 			}
 		);
-		assert!(output.is_success());
+		assert!(!output.did_revert());
 	}
 
 	#[test]
@@ -1781,7 +1781,7 @@ mod tests {
 				data: Bytes(hex!("5566778899").to_vec()),
 			}
 		);
-		assert!(!output.is_success());
+		assert!(output.did_revert());
 	}
 
 	const CODE_OUT_OF_BOUNDS_ACCESS: &str = r#"