From caa6efa5ec15ba65c4d81bca47ca5a3b71fd3387 Mon Sep 17 00:00:00 2001
From: Hero Bird <robin.freyler@gmail.com>
Date: Tue, 21 Jan 2020 13:06:54 +0100
Subject: [PATCH] contracts: New contract events + unconfusions (#4685)
* contracts: during execution -> contract trapped during execution
This message confused many people so we are improving it to make clear what happened.
* contracts: rename Event::Contract -> Event::ContractExecution
* contracts: fix tests after ContractExecution renaming
* contracts: Add Evicted and Restored events
* fix doc comment
* wrap to not go over (soft) 100 column line limit
* add event deposit for eventual eviction upon pay_rent
* contracts: adjust tests for the new events
* emit Evicted event immediately and add tombstone flag bool
---
substrate/frame/contracts/src/exec.rs | 2 +-
substrate/frame/contracts/src/lib.rs | 30 +++++-
substrate/frame/contracts/src/rent.rs | 6 +-
substrate/frame/contracts/src/tests.rs | 98 ++++++++++++++++++-
substrate/frame/contracts/src/wasm/mod.rs | 6 +-
substrate/frame/contracts/src/wasm/runtime.rs | 2 +-
6 files changed, 131 insertions(+), 13 deletions(-)
diff --git a/substrate/frame/contracts/src/exec.rs b/substrate/frame/contracts/src/exec.rs
index 2c771731350..9d786c320b5 100644
--- a/substrate/frame/contracts/src/exec.rs
+++ b/substrate/frame/contracts/src/exec.rs
@@ -782,7 +782,7 @@ where
fn deposit_event(&mut self, topics: Vec<T::Hash>, data: Vec<u8>) {
self.ctx.deferred.push(DeferredAction::DepositEvent {
topics,
- event: RawEvent::Contract(self.ctx.self_account.clone(), data),
+ event: RawEvent::ContractExecution(self.ctx.self_account.clone(), data),
});
}
diff --git a/substrate/frame/contracts/src/lib.rs b/substrate/frame/contracts/src/lib.rs
index df8da886602..40ce86518a5 100644
--- a/substrate/frame/contracts/src/lib.rs
+++ b/substrate/frame/contracts/src/lib.rs
@@ -786,7 +786,12 @@ impl<T: Trait> Module<T> {
rent_allowance,
delta,
} => {
- let _result = Self::restore_to(donor, dest, code_hash, rent_allowance, delta);
+ let result = Self::restore_to(
+ donor.clone(), dest.clone(), code_hash.clone(), rent_allowance.clone(), delta
+ );
+ Self::deposit_event(
+ RawEvent::Restored(donor, dest, code_hash, rent_allowance, result.is_ok())
+ );
}
}
});
@@ -896,6 +901,25 @@ decl_event! {
/// Contract deployed by address at the specified address.
Instantiated(AccountId, AccountId),
+ /// Contract has been evicted and is now in tombstone state.
+ ///
+ /// # Params
+ ///
+ /// - `contract`: `AccountId`: The account ID of the evicted contract.
+ /// - `tombstone`: `bool`: True if the evicted contract left behind a tombstone.
+ Evicted(AccountId, bool),
+
+ /// Restoration for a contract has been initiated.
+ ///
+ /// # Params
+ ///
+ /// - `donor`: `AccountId`: Account ID of the restoring contract
+ /// - `dest`: `AccountId`: Account ID of the restored contract
+ /// - `code_hash`: `Hash`: Code hash of the restored contract
+ /// - `rent_allowance: `Balance`: Rent allowance of the restored contract
+ /// - `success`: `bool`: True if the restoration was successful
+ Restored(AccountId, AccountId, Hash, Balance, bool),
+
/// Code with the specified hash has been stored.
CodeStored(Hash),
@@ -906,8 +930,8 @@ decl_event! {
/// successful execution or not.
Dispatched(AccountId, bool),
- /// An event from contract of account.
- Contract(AccountId, Vec<u8>),
+ /// An event deposited upon execution of a contract from the account.
+ ContractExecution(AccountId, Vec<u8>),
}
}
diff --git a/substrate/frame/contracts/src/rent.rs b/substrate/frame/contracts/src/rent.rs
index 59c8b02d199..508511da4cb 100644
--- a/substrate/frame/contracts/src/rent.rs
+++ b/substrate/frame/contracts/src/rent.rs
@@ -14,7 +14,8 @@
// You should have received a copy of the GNU General Public License
// along with Substrate. If not, see <http://www.gnu.org/licenses/>.
-use crate::{BalanceOf, ContractInfo, ContractInfoOf, TombstoneContractInfo, Trait, AliveContractInfo};
+use crate::{Module, RawEvent, BalanceOf, ContractInfo, ContractInfoOf, TombstoneContractInfo,
+ Trait, AliveContractInfo};
use sp_runtime::traits::{Bounded, CheckedDiv, CheckedMul, Saturating, Zero,
SaturatedConversion};
use frame_support::traits::{Currency, ExistenceRequirement, Get, WithdrawReason, OnUnbalanced};
@@ -101,6 +102,7 @@ fn try_evict_or_and_pay_rent<T: Trait>(
// The contract cannot afford to leave a tombstone, so remove the contract info altogether.
<ContractInfoOf<T>>::remove(account);
child::kill_storage(&contract.trie_id, contract.child_trie_unique_id());
+ <Module<T>>::deposit_event(RawEvent::Evicted(account.clone(), false));
return (RentOutcome::Evicted, None);
}
@@ -160,6 +162,8 @@ fn try_evict_or_and_pay_rent<T: Trait>(
child::kill_storage(&contract.trie_id, contract.child_trie_unique_id());
+ <Module<T>>::deposit_event(RawEvent::Evicted(account.clone(), true));
+
return (RentOutcome::Evicted, Some(tombstone_info));
}
diff --git a/substrate/frame/contracts/src/tests.rs b/substrate/frame/contracts/src/tests.rs
index 91679a9216a..9a2ef36bb86 100644
--- a/substrate/frame/contracts/src/tests.rs
+++ b/substrate/frame/contracts/src/tests.rs
@@ -453,7 +453,7 @@ fn instantiate_and_call_and_deposit_event() {
},
EventRecord {
phase: Phase::ApplyExtrinsic(0),
- event: MetaEvent::contract(RawEvent::Contract(BOB, vec![1, 2, 3, 4])),
+ event: MetaEvent::contract(RawEvent::ContractExecution(BOB, vec![1, 2, 3, 4])),
topics: vec![],
},
EventRecord {
@@ -650,7 +650,7 @@ fn dispatch_call_not_dispatched_after_top_level_transaction_failure() {
100_000,
vec![],
),
- "during execution"
+ "contract trapped during execution"
);
assert_eq!(System::events(), vec![
EventRecord {
@@ -1139,8 +1139,16 @@ fn call_removed_contract() {
Contract::call(Origin::signed(ALICE), BOB, 0, 100_000, call::null()),
"contract has been evicted"
);
+ // Calling a contract that is about to evict shall emit an event.
+ assert_eq!(System::events(), vec![
+ EventRecord {
+ phase: Phase::ApplyExtrinsic(0),
+ event: MetaEvent::contract(RawEvent::Evicted(BOB, true)),
+ topics: vec![],
+ },
+ ]);
- // Subsequent contract calls should also fail.
+ // Subsequent contract calls should also fail.
assert_err!(
Contract::call(Origin::signed(ALICE), BOB, 0, 100_000, call::null()),
"contract has been evicted"
@@ -1367,6 +1375,9 @@ fn restoration(test_different_storage: bool, test_restore_to_with_dirty_storage:
// Advance 4 blocks, to the 5th.
initialize_block(5);
+ /// Preserve `BOB`'s code hash for later introspection.
+ let bob_code_hash = ContractInfoOf::<Test>::get(BOB).unwrap()
+ .get_alive().unwrap().code_hash;
// Call `BOB`, which makes it pay rent. Since the rent allowance is set to 0
// we expect that it will get removed leaving tombstone.
assert_err!(
@@ -1374,6 +1385,15 @@ fn restoration(test_different_storage: bool, test_restore_to_with_dirty_storage:
"contract has been evicted"
);
assert!(ContractInfoOf::<Test>::get(BOB).unwrap().get_tombstone().is_some());
+ assert_eq!(System::events(), vec![
+ EventRecord {
+ phase: Phase::ApplyExtrinsic(0),
+ event: MetaEvent::contract(
+ RawEvent::Evicted(BOB.clone(), true)
+ ),
+ topics: vec![],
+ },
+ ]);
/// Create another account with the address `DJANGO` with `CODE_RESTORATION`.
///
@@ -1416,6 +1436,60 @@ fn restoration(test_different_storage: bool, test_restore_to_with_dirty_storage:
assert_eq!(django_contract.storage_size, 16);
assert_eq!(django_contract.trie_id, django_trie_id);
assert_eq!(django_contract.deduct_block, System::block_number());
+ match (test_different_storage, test_restore_to_with_dirty_storage) {
+ (true, false) => {
+ assert_eq!(System::events(), vec![
+ EventRecord {
+ phase: Phase::ApplyExtrinsic(0),
+ event: MetaEvent::contract(
+ RawEvent::Restored(DJANGO, BOB, bob_code_hash, 50, false)
+ ),
+ topics: vec![],
+ },
+ ]);
+ }
+ (_, true) => {
+ assert_eq!(System::events(), vec![
+ EventRecord {
+ phase: Phase::ApplyExtrinsic(0),
+ event: MetaEvent::contract(RawEvent::Evicted(BOB, true)),
+ topics: vec![],
+ },
+ EventRecord {
+ phase: Phase::ApplyExtrinsic(0),
+ event: MetaEvent::balances(pallet_balances::RawEvent::NewAccount(CHARLIE, 1_000_000)),
+ topics: vec![],
+ },
+ EventRecord {
+ phase: Phase::ApplyExtrinsic(0),
+ event: MetaEvent::balances(pallet_balances::RawEvent::NewAccount(DJANGO, 30_000)),
+ topics: vec![],
+ },
+ EventRecord {
+ phase: Phase::ApplyExtrinsic(0),
+ event: MetaEvent::contract(RawEvent::Transfer(CHARLIE, DJANGO, 30_000)),
+ topics: vec![],
+ },
+ EventRecord {
+ phase: Phase::ApplyExtrinsic(0),
+ event: MetaEvent::contract(RawEvent::Instantiated(CHARLIE, DJANGO)),
+ topics: vec![],
+ },
+ EventRecord {
+ phase: Phase::ApplyExtrinsic(0),
+ event: MetaEvent::contract(RawEvent::Restored(
+ DJANGO,
+ BOB,
+ bob_code_hash,
+ 50,
+ false,
+ )),
+ topics: vec![],
+ },
+ ]);
+ }
+ _ => unreachable!(),
+ }
} else {
// Here we expect that the restoration is succeeded. Check that the restoration
// contract `DJANGO` ceased to exist and that `BOB` returned back.
@@ -1427,6 +1501,20 @@ fn restoration(test_different_storage: bool, test_restore_to_with_dirty_storage:
assert_eq!(bob_contract.trie_id, django_trie_id);
assert_eq!(bob_contract.deduct_block, System::block_number());
assert!(ContractInfoOf::<Test>::get(DJANGO).is_none());
+ assert_eq!(System::events(), vec![
+ EventRecord {
+ phase: Phase::ApplyExtrinsic(0),
+ event: MetaEvent::balances(balances::RawEvent::ReapedAccount(DJANGO, 0)),
+ topics: vec![],
+ },
+ EventRecord {
+ phase: Phase::ApplyExtrinsic(0),
+ event: MetaEvent::contract(
+ RawEvent::Restored(DJANGO, BOB, bob_contract.code_hash, 50, true)
+ ),
+ topics: vec![],
+ },
+ ]);
}
});
}
@@ -1533,7 +1621,7 @@ fn storage_max_value_limit() {
100_000,
Encode::encode(&(self::MaxValueSize::get() + 1)),
),
- "during execution"
+ "contract trapped during execution"
);
});
}
@@ -2056,7 +2144,7 @@ fn cannot_self_destruct_while_live() {
100_000,
vec![0],
),
- "during execution"
+ "contract trapped during execution"
);
// Check that BOB is still alive.
diff --git a/substrate/frame/contracts/src/wasm/mod.rs b/substrate/frame/contracts/src/wasm/mod.rs
index 8e02eb482bb..60402cf3a09 100644
--- a/substrate/frame/contracts/src/wasm/mod.rs
+++ b/substrate/frame/contracts/src/wasm/mod.rs
@@ -1430,7 +1430,9 @@ mod tests {
MockExt::default(),
&mut gas_meter
),
- Err(ExecError { reason: DispatchError::Other("during execution"), buffer: _ })
+ Err(ExecError {
+ reason: DispatchError::Other("contract trapped during execution"), buffer: _
+ })
);
}
@@ -1472,7 +1474,7 @@ mod tests {
MockExt::default(),
&mut gas_meter
),
- Err(ExecError { reason: DispatchError::Other("during execution"), buffer: _ })
+ Err(ExecError { reason: DispatchError::Other("contract trapped during execution"), buffer: _ })
);
}
diff --git a/substrate/frame/contracts/src/wasm/runtime.rs b/substrate/frame/contracts/src/wasm/runtime.rs
index 81b0809f82b..75751b6d359 100644
--- a/substrate/frame/contracts/src/wasm/runtime.rs
+++ b/substrate/frame/contracts/src/wasm/runtime.rs
@@ -109,7 +109,7 @@ pub(crate) fn to_execution_result<E: Ext>(
Err(ExecError { reason: "validation error".into(), buffer: runtime.scratch_buf }),
// Any other kind of a trap should result in a failure.
Err(sp_sandbox::Error::Execution) | Err(sp_sandbox::Error::OutOfBounds) =>
- Err(ExecError { reason: "during execution".into(), buffer: runtime.scratch_buf }),
+ Err(ExecError { reason: "contract trapped during execution".into(), buffer: runtime.scratch_buf }),
}
}
--
GitLab