diff --git a/frame/contracts/common/src/lib.rs b/frame/contracts/common/src/lib.rs index d8408f77e06e2..b5873869495e9 100644 --- a/frame/contracts/common/src/lib.rs +++ b/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 { Existing(Hash), } +impl>, Hash> From for Code { + 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))] diff --git a/frame/contracts/src/exec.rs b/frame/contracts/src/exec.rs index 916c2d3df84f5..bd54c7a08c636 100644 --- a/frame/contracts/src/exec.rs +++ b/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])); }); } diff --git a/frame/contracts/src/lib.rs b/frame/contracts/src/lib.rs index 0b8786fa704a3..b604c9618c6ae 100644 --- a/frame/contracts/src/lib.rs +++ b/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(>::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(>::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(>::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. diff --git a/frame/contracts/src/tests.rs b/frame/contracts/src/tests.rs index 1078a7e73352d..e7745e1e5f7ff 100644 --- a/frame/contracts/src/tests.rs +++ b/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() { >::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() { >::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::("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![], + ), + >::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![], + ), + >::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!(!>::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()), + >::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); + }); +} diff --git a/frame/contracts/src/wasm/mod.rs b/frame/contracts/src/wasm/mod.rs index 01e220d410fa4..913894e8152db 100644 --- a/frame/contracts/src/wasm/mod.rs +++ b/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#"