Contracts: Remove topics for internal events (paritytech#4510)

PolkadotDom · May 17, 2024 · a90d324 · a90d324
1 parent 2e36f57
commit a90d324
Show file tree

Hide file tree

Showing 6 changed files with 107 additions and 118 deletions.
diff --git a/prdoc/pr_4510.prdoc b/prdoc/pr_4510.prdoc
@@ -0,0 +1,13 @@
+title: "[Contracts] Remove internal topic index"
+
+doc:
+  - audience: Runtime Dev
+    description: |
+      This PR removes topics from internal events emitted by pallet_contracts. It does not touch the `deposit_event` host function used by 
+      smart contracts that can still include topics.
+      Event topics incurs significant Storage costs, and are only used by light clients to index events and avoid downloading the entire block.
+      They are not used by Dapp or Indexers that download the whole block anyway.
+
+crates:
+  - name: pallet-contracts
+    bump: patch
diff --git a/substrate/frame/contracts/src/exec.rs b/substrate/frame/contracts/src/exec.rs
@@ -46,7 +46,7 @@ use sp_core::{
 };
 use sp_io::{crypto::secp256k1_ecdsa_recover_compressed, hashing::blake2_256};
 use sp_runtime::{
-	traits::{Convert, Dispatchable, Hash, Zero},
+	traits::{Convert, Dispatchable, Zero},
 	DispatchError,
 };
 use sp_std::{fmt::Debug, marker::PhantomData, mem, prelude::*, vec::Vec};
@@ -983,16 +983,16 @@ where
 					let caller = self.caller().account_id()?.clone();
 
 					// Deposit an instantiation event.
-					Contracts::<T>::deposit_event(
-						vec![T::Hashing::hash_of(&caller), T::Hashing::hash_of(account_id)],
-						Event::Instantiated { deployer: caller, contract: account_id.clone() },
-					);
+					Contracts::<T>::deposit_event(Event::Instantiated {
+						deployer: caller,
+						contract: account_id.clone(),
+					});
 				},
 				(ExportedFunction::Call, Some(code_hash)) => {
-					Contracts::<T>::deposit_event(
-						vec![T::Hashing::hash_of(account_id), T::Hashing::hash_of(&code_hash)],
-						Event::DelegateCalled { contract: account_id.clone(), code_hash },
-					);
+					Contracts::<T>::deposit_event(Event::DelegateCalled {
+						contract: account_id.clone(),
+						code_hash,
+					});
 				},
 				(ExportedFunction::Call, None) => {
 					// If a special limit was set for the sub-call, we enforce it here.
@@ -1002,10 +1002,10 @@ where
 					frame.nested_storage.enforce_subcall_limit(contract)?;
 
 					let caller = self.caller();
-					Contracts::<T>::deposit_event(
-						vec![T::Hashing::hash_of(&caller), T::Hashing::hash_of(&account_id)],
-						Event::Called { caller: caller.clone(), contract: account_id.clone() },
-					);
+					Contracts::<T>::deposit_event(Event::Called {
+						caller: caller.clone(),
+						contract: account_id.clone(),
+					});
 				},
 			}
 
@@ -1324,13 +1324,10 @@ where
 				.charge_deposit(frame.account_id.clone(), StorageDeposit::Refund(*deposit));
 		}
 
-		Contracts::<T>::deposit_event(
-			vec![T::Hashing::hash_of(&frame.account_id), T::Hashing::hash_of(&beneficiary)],
-			Event::Terminated {
-				contract: frame.account_id.clone(),
-				beneficiary: beneficiary.clone(),
-			},
-		);
+		Contracts::<T>::deposit_event(Event::Terminated {
+			contract: frame.account_id.clone(),
+			beneficiary: beneficiary.clone(),
+		});
 		Ok(())
 	}
 
@@ -1422,7 +1419,7 @@ where
 	}
 
 	fn deposit_event(&mut self, topics: Vec<T::Hash>, data: Vec<u8>) {
-		Contracts::<Self::T>::deposit_event(
+		Contracts::<Self::T>::deposit_indexed_event(
 			topics,
 			Event::ContractEmitted { contract: self.top_frame().account_id.clone(), data },
 		);
@@ -1527,14 +1524,11 @@ where
 
 		Self::increment_refcount(hash)?;
 		Self::decrement_refcount(prev_hash);
-		Contracts::<Self::T>::deposit_event(
-			vec![T::Hashing::hash_of(&frame.account_id), hash, prev_hash],
-			Event::ContractCodeUpdated {
-				contract: frame.account_id.clone(),
-				new_code_hash: hash,
-				old_code_hash: prev_hash,
-			},
-		);
+		Contracts::<Self::T>::deposit_event(Event::ContractCodeUpdated {
+			contract: frame.account_id.clone(),
+			new_code_hash: hash,
+			old_code_hash: prev_hash,
+		});
 		Ok(())
 	}
 
@@ -1639,7 +1633,7 @@ mod tests {
 		exec::ExportedFunction::*,
 		gas::GasMeter,
 		tests::{
-			test_utils::{get_balance, hash, place_contract, set_balance},
+			test_utils::{get_balance, place_contract, set_balance},
 			ExtBuilder, RuntimeCall, RuntimeEvent as MetaEvent, Test, TestFilter, ALICE, BOB,
 			CHARLIE, GAS_LIMIT,
 		},
@@ -3164,7 +3158,7 @@ mod tests {
 							caller: Origin::from_account_id(ALICE),
 							contract: BOB,
 						}),
-						topics: vec![hash(&Origin::<Test>::from_account_id(ALICE)), hash(&BOB)],
+						topics: vec![],
 					},
 				]
 			);
@@ -3264,7 +3258,7 @@ mod tests {
 							caller: Origin::from_account_id(ALICE),
 							contract: BOB,
 						}),
-						topics: vec![hash(&Origin::<Test>::from_account_id(ALICE)), hash(&BOB)],
+						topics: vec![],
 					},
 				]
 			);

diff --git a/substrate/frame/contracts/src/lib.rs b/substrate/frame/contracts/src/lib.rs
@@ -135,7 +135,7 @@ use frame_system::{
 use scale_info::TypeInfo;
 use smallvec::Array;
 use sp_runtime::{
-	traits::{Convert, Dispatchable, Hash, Saturating, StaticLookup, Zero},
+	traits::{Convert, Dispatchable, Saturating, StaticLookup, Zero},
 	DispatchError, RuntimeDebug,
 };
 use sp_std::{fmt::Debug, prelude::*};
@@ -833,14 +833,11 @@ pub mod pallet {
 				};
 				<ExecStack<T, WasmBlob<T>>>::increment_refcount(code_hash)?;
 				<ExecStack<T, WasmBlob<T>>>::decrement_refcount(contract.code_hash);
-				Self::deposit_event(
-					vec![T::Hashing::hash_of(&dest), code_hash, contract.code_hash],
-					Event::ContractCodeUpdated {
-						contract: dest.clone(),
-						new_code_hash: code_hash,
-						old_code_hash: contract.code_hash,
-					},
-				);
+				Self::deposit_event(Event::ContractCodeUpdated {
+					contract: dest.clone(),
+					new_code_hash: code_hash,
+					old_code_hash: contract.code_hash,
+				});
 				contract.code_hash = code_hash;
 				Ok(())
 			})
@@ -1827,8 +1824,13 @@ impl<T: Config> Pallet<T> {
 		Ok(())
 	}
 
-	/// Deposit a pallet contracts event. Handles the conversion to the overarching event type.
-	fn deposit_event(topics: Vec<T::Hash>, event: Event<T>) {
+	/// Deposit a pallet contracts event.
+	fn deposit_event(event: Event<T>) {
+		<frame_system::Pallet<T>>::deposit_event(<T as Config>::RuntimeEvent::from(event))
+	}
+
+	/// Deposit a pallet contracts indexed event.
+	fn deposit_indexed_event(topics: Vec<T::Hash>, event: Event<T>) {
 		<frame_system::Pallet<T>>::deposit_event_indexed(
 			&topics,
 			<T as Config>::RuntimeEvent::from(event).into(),

diff --git a/substrate/frame/contracts/src/storage/meter.rs b/substrate/frame/contracts/src/storage/meter.rs
@@ -34,10 +34,10 @@ use frame_support::{
 	DefaultNoBound, RuntimeDebugNoBound,
 };
 use sp_runtime::{
-	traits::{Hash as HashT, Saturating, Zero},
+	traits::{Saturating, Zero},
 	DispatchError, FixedPointNumber, FixedU128,
 };
-use sp_std::{fmt::Debug, marker::PhantomData, vec, vec::Vec};
+use sp_std::{fmt::Debug, marker::PhantomData, vec::Vec};
 
 /// Deposit that uses the native fungible's balance type.
 pub type DepositOf<T> = Deposit<BalanceOf<T>>;
@@ -551,14 +551,11 @@ impl<T: Config> Ext<T> for ReservingExt {
 					Fortitude::Polite,
 				)?;
 
-				Pallet::<T>::deposit_event(
-					vec![T::Hashing::hash_of(&origin), T::Hashing::hash_of(&contract)],
-					Event::StorageDepositTransferredAndHeld {
-						from: origin.clone(),
-						to: contract.clone(),
-						amount: *amount,
-					},
-				);
+				Pallet::<T>::deposit_event(Event::StorageDepositTransferredAndHeld {
+					from: origin.clone(),
+					to: contract.clone(),
+					amount: *amount,
+				});
 			},
 			Deposit::Refund(amount) => {
 				let transferred = T::Currency::transfer_on_hold(
@@ -571,14 +568,11 @@ impl<T: Config> Ext<T> for ReservingExt {
 					Fortitude::Polite,
 				)?;
 
-				Pallet::<T>::deposit_event(
-					vec![T::Hashing::hash_of(&contract), T::Hashing::hash_of(&origin)],
-					Event::StorageDepositTransferredAndReleased {
-						from: contract.clone(),
-						to: origin.clone(),
-						amount: transferred,
-					},
-				);
+				Pallet::<T>::deposit_event(Event::StorageDepositTransferredAndReleased {
+					from: contract.clone(),
+					to: origin.clone(),
+					amount: transferred,
+				});
 
 				if transferred < *amount {
 					// This should never happen, if it does it means that there is a bug in the