From ce64dbe33cd46c0120ea237d0975bcd434abdcff Mon Sep 17 00:00:00 2001 From: Hernando Castano Date: Wed, 1 Feb 2023 10:53:48 -0800 Subject: [PATCH] Remove `Default` implementation for `AccountId` (#1255) * Remove `Default` bound on `AccountId` type * Manually implement storage traits for AccountID Since it doesn't implement `Default` we can't use the macro implementations anymore. * Update `Call` builder to not use default AccountId * Use non-Default bound functions when reading AccountIds * Remove unused `get_property_inplace` method * Update `CallParams` to use `None` when no account ID is set * Update examples to explicitly use zero address * Remove unused environment function * Manually implement `Default` in DNS example * Fix spellcheck * Correctly encode the `callee` account ID when calling `pallet-contracts` * Update `CHANGELOG` * Make cross-contract callee non-optional (#1636) * Make cross-contract callee non-optional * clippy * Fmt * Fix * clippy * clippy * clippy * Add a similar method for `code_hash` * Fix doc tests * RustFmt * Rename top level methods to `call` and `delegate` * Fix some renames --------- Co-authored-by: Hernando Castano --------- Co-authored-by: Andrew Jones --- CHANGELOG.md | 4 + crates/env/src/call/call_builder.rs | 98 ++++++++----------- crates/env/src/engine/on_chain/impls.rs | 21 +--- crates/env/src/types.rs | 5 +- .../generator/as_dependency/call_builder.rs | 2 +- .../src/generator/trait_def/call_builder.rs | 2 +- crates/ink/src/env_access.rs | 6 +- crates/primitives/src/types.rs | 13 +-- examples/dns/lib.rs | 23 ++++- examples/erc1155/lib.rs | 15 ++- .../call-builder/lib.rs | 5 +- examples/multisig/lib.rs | 20 ++-- .../forward-calls/lib.rs | 10 +- 13 files changed, 101 insertions(+), 123 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1e54bc9fd5..64955cbe3e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,11 +6,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] - Rename `_checked` codegen call methods with `try_` ‒ [#1621](https://github.com/paritytech/ink/pull/1621) +- Remove `Default` implementation for AccountId ‒ [#1255](https://github.com/paritytech/ink/pull/1255) ### Breaking Changes 1. We've renamed some of the generated message methods on the `ContractRef` struct. They have been changed from `_checked` to `try_` ([#1621](https://github.com/paritytech/ink/pull/1621)) +1. We have removed the `Default` implementation for `AccountId`s. This is because of + security concerns around the use of the zero address which has a known private key in + the `sr25519` and `ed25519` curves ([#1255](https://github.com/paritytech/ink/pull/1255)). ## Version 4.0.0-beta.1 The coolest feature included in this release is the first first published version of diff --git a/crates/env/src/call/call_builder.rs b/crates/env/src/call/call_builder.rs index 3ec443a2d8..3ea6c065c9 100644 --- a/crates/env/src/call/call_builder.rs +++ b/crates/env/src/call/call_builder.rs @@ -28,7 +28,6 @@ use crate::{ Error, }; use core::marker::PhantomData; -use ink_primitives::Clear; use num_traits::Zero; /// The final parameters to the cross-contract call. @@ -203,11 +202,9 @@ where /// # type AccountId = ::AccountId; /// # type Balance = ::Balance; /// build_call::() -/// .call_type( -/// Call::new() -/// .callee(AccountId::from([0x42; 32])) -/// .gas_limit(5000) -/// .transferred_value(10)) +/// .call(AccountId::from([0x42; 32])) +/// .gas_limit(5000) +/// .transferred_value(10) /// .exec_input( /// ExecutionInput::new(Selector::new([0xDE, 0xAD, 0xBE, 0xEF])) /// .push_arg(42u8) @@ -239,9 +236,8 @@ where /// # }; /// # type AccountId = ::AccountId; /// let my_return_value: i32 = build_call::() -/// .call_type(Call::new() -/// .callee(AccountId::from([0x42; 32])) -/// .gas_limit(5000)) +/// .call_type(Call::new(AccountId::from([0x42; 32]))) +/// .gas_limit(5000) /// .transferred_value(10) /// .exec_input( /// ExecutionInput::new(Selector::new([0xDE, 0xAD, 0xBE, 0xEF])) @@ -268,8 +264,7 @@ where /// # use ink_primitives::Clear; /// # type AccountId = ::AccountId; /// let my_return_value: i32 = build_call::() -/// .call_type(DelegateCall::new() -/// .code_hash(::Hash::CLEAR_HASH)) +/// .delegate(::Hash::CLEAR_HASH) /// .exec_input( /// ExecutionInput::new(Selector::new([0xDE, 0xAD, 0xBE, 0xEF])) /// .push_arg(42u8) @@ -304,12 +299,9 @@ where /// # type AccountId = ::AccountId; /// # type Balance = ::Balance; /// let call_result = build_call::() -/// .call_type( -/// Call::new() -/// .callee(AccountId::from([0x42; 32])) -/// .gas_limit(5000) -/// .transferred_value(10), -/// ) +/// .call(AccountId::from([0x42; 32])) +/// .gas_limit(5000) +/// .transferred_value(10) /// .try_invoke() /// .expect("Got an error from the Contract's pallet."); /// @@ -346,36 +338,21 @@ pub struct Call { transferred_value: E::Balance, } -impl Default for Call { - fn default() -> Self { - Call { - callee: Default::default(), +impl Call { + /// Returns a clean builder for [`Call`]. + pub fn new(callee: E::AccountId) -> Self { + Self { + callee, gas_limit: Default::default(), transferred_value: E::Balance::zero(), } } } -impl Call { - /// Returns a clean builder for [`Call`]. - pub fn new() -> Self { - Default::default() - } -} - impl Call where E: Environment, { - /// Sets the `callee` for the current cross-contract call. - pub fn callee(self, callee: E::AccountId) -> Self { - Call { - callee, - gas_limit: self.gas_limit, - transferred_value: self.transferred_value, - } - } - /// Sets the `gas_limit` for the current cross-contract call. pub fn gas_limit(self, gas_limit: Gas) -> Self { Call { @@ -402,16 +379,8 @@ pub struct DelegateCall { impl DelegateCall { /// Returns a clean builder for [`DelegateCall`] - pub const fn new() -> Self { - DelegateCall { - code_hash: E::Hash::CLEAR_HASH, - } - } -} - -impl Default for DelegateCall { - fn default() -> Self { - Self::new() + pub const fn new(code_hash: E::Hash) -> Self { + DelegateCall { code_hash } } } @@ -519,19 +488,17 @@ where } } -impl CallBuilder>, Args, RetType> +impl CallBuilder, Args, RetType> where E: Environment, { - /// Sets the `callee` for the current cross-contract call. - pub fn callee(self, callee: E::AccountId) -> Self { - let call_type = self.call_type.value(); + /// Prepares the `CallBuilder` for a cross-contract [`Call`]. + pub fn call( + self, + callee: E::AccountId, + ) -> CallBuilder>, Args, RetType> { CallBuilder { - call_type: Set(Call { - callee, - gas_limit: call_type.gas_limit, - transferred_value: call_type.transferred_value, - }), + call_type: Set(Call::new(callee)), call_flags: self.call_flags, exec_input: self.exec_input, return_type: self.return_type, @@ -539,6 +506,25 @@ where } } + /// Prepares the `CallBuilder` for a cross-contract [`DelegateCall`]. + pub fn delegate( + self, + code_hash: E::Hash, + ) -> CallBuilder>, Args, RetType> { + CallBuilder { + call_type: Set(DelegateCall::new(code_hash)), + call_flags: self.call_flags, + exec_input: self.exec_input, + return_type: self.return_type, + _phantom: Default::default(), + } + } +} + +impl CallBuilder>, Args, RetType> +where + E: Environment, +{ /// Sets the `gas_limit` for the current cross-contract call. pub fn gas_limit(self, gas_limit: Gas) -> Self { let call_type = self.call_type.value(); diff --git a/crates/env/src/engine/on_chain/impls.rs b/crates/env/src/engine/on_chain/impls.rs index fec97b6ddf..ae9fa3fdb1 100644 --- a/crates/env/src/engine/on_chain/impls.rs +++ b/crates/env/src/engine/on_chain/impls.rs @@ -182,21 +182,6 @@ impl EnvInstance { ScopedBuffer::from(&mut self.buffer[..]) } - /// Returns the contract property value into the given result buffer. - /// - /// # Note - /// - /// This skips the potentially costly decoding step that is often equivalent to a `memcpy`. - #[inline(always)] - fn get_property_inplace(&mut self, ext_fn: fn(output: &mut &mut [u8])) -> T - where - T: Default + AsMut<[u8]>, - { - let mut result = T::default(); - ext_fn(&mut result.as_mut()); - result - } - /// Returns the contract property value from its little-endian representation. /// /// # Note @@ -372,7 +357,8 @@ impl EnvBackend for EnvInstance { impl TypedEnvBackend for EnvInstance { fn caller(&mut self) -> E::AccountId { - self.get_property_inplace::(ext::caller) + self.get_property::(ext::caller) + .expect("The executed contract must have a caller with a valid account id.") } fn transferred_value(&mut self) -> E::Balance { @@ -388,7 +374,8 @@ impl TypedEnvBackend for EnvInstance { } fn account_id(&mut self) -> E::AccountId { - self.get_property_inplace::(ext::address) + self.get_property::(ext::address) + .expect("A contract being executed must have a valid account id.") } fn balance(&mut self) -> E::Balance { diff --git a/crates/env/src/types.rs b/crates/env/src/types.rs index fcdd4793a9..9f5fa22075 100644 --- a/crates/env/src/types.rs +++ b/crates/env/src/types.rs @@ -101,7 +101,7 @@ pub trait Environment { /// The value must match the maximum number of supported event topics of the used runtime. const MAX_EVENT_TOPICS: usize; - /// The address type. + /// The account id type. type AccountId: 'static + scale::Codec + Clone @@ -109,8 +109,7 @@ pub trait Environment { + Eq + Ord + AsRef<[u8]> - + AsMut<[u8]> - + Default; + + AsMut<[u8]>; /// The type of balances. type Balance: 'static diff --git a/crates/ink/codegen/src/generator/as_dependency/call_builder.rs b/crates/ink/codegen/src/generator/as_dependency/call_builder.rs index 06ce61e442..fe5f09feaf 100644 --- a/crates/ink/codegen/src/generator/as_dependency/call_builder.rs +++ b/crates/ink/codegen/src/generator/as_dependency/call_builder.rs @@ -391,7 +391,7 @@ impl CallBuilder<'_> { #( , #input_bindings : #input_types )* ) -> #output_type { ::ink::env::call::build_call::() - .call_type(::ink::env::call::Call::new().callee(::ink::ToAccountId::to_account_id(self))) + .call(::ink::ToAccountId::to_account_id(self)) .exec_input( ::ink::env::call::ExecutionInput::new( ::ink::env::call::Selector::new([ #( #selector_bytes ),* ]) diff --git a/crates/ink/codegen/src/generator/trait_def/call_builder.rs b/crates/ink/codegen/src/generator/trait_def/call_builder.rs index 34221367b3..69db77bb43 100644 --- a/crates/ink/codegen/src/generator/trait_def/call_builder.rs +++ b/crates/ink/codegen/src/generator/trait_def/call_builder.rs @@ -319,7 +319,7 @@ impl CallBuilder<'_> { #( , #input_bindings : #input_types )* ) -> Self::#output_ident { ::ink::env::call::build_call::() - .call_type(::ink::env::call::Call::new().callee(::ink::ToAccountId::to_account_id(self))) + .call(::ink::ToAccountId::to_account_id(self)) .exec_input( ::ink::env::call::ExecutionInput::new( ::ink::env::call::Selector::new([ #( #selector_bytes ),* ]) diff --git a/crates/ink/src/env_access.rs b/crates/ink/src/env_access.rs index 89cbcf8255..f54660cd42 100644 --- a/crates/ink/src/env_access.rs +++ b/crates/ink/src/env_access.rs @@ -524,8 +524,7 @@ where /// pub fn invoke_contract(&self) -> i32 { /// let call_params = build_call::() /// .call_type( - /// Call::new() - /// .callee(AccountId::from([0x42; 32])) + /// Call::new(AccountId::from([0x42; 32])) /// .gas_limit(5000) /// .transferred_value(10)) /// .exec_input( @@ -588,8 +587,7 @@ where /// pub fn invoke_contract_delegate(&self) -> i32 { /// let call_params = build_call::() /// .call_type( - /// DelegateCall::new() - /// .code_hash(::Hash::CLEAR_HASH)) + /// DelegateCall::new(::Hash::CLEAR_HASH)) /// .exec_input( /// ExecutionInput::new(Selector::new([0xCA, 0xFE, 0xBA, 0xBE])) /// .push_arg(42u8) diff --git a/crates/primitives/src/types.rs b/crates/primitives/src/types.rs index 6b86068e6b..bd369290a8 100644 --- a/crates/primitives/src/types.rs +++ b/crates/primitives/src/types.rs @@ -28,18 +28,7 @@ use scale_info::TypeInfo; /// This is a mirror of the `AccountId` type used in the default configuration /// of PALLET contracts. #[derive( - Debug, - Copy, - Clone, - PartialEq, - Eq, - Ord, - PartialOrd, - Hash, - Encode, - Decode, - From, - Default, + Debug, Copy, Clone, PartialEq, Eq, Ord, PartialOrd, Hash, Encode, Decode, From, )] #[cfg_attr(feature = "std", derive(TypeInfo))] pub struct AccountId([u8; 32]); diff --git a/examples/dns/lib.rs b/examples/dns/lib.rs index aea2ceece7..a78eb8cb26 100644 --- a/examples/dns/lib.rs +++ b/examples/dns/lib.rs @@ -53,7 +53,6 @@ mod dns { /// to facilitate transfers, voting and DApp-related operations instead /// of resorting to long IP addresses that are hard to remember. #[ink(storage)] - #[derive(Default)] pub struct DomainNameService { /// A hashmap to store all name to addresses mapping. name_to_address: Mapping, @@ -63,6 +62,21 @@ mod dns { default_address: AccountId, } + impl Default for DomainNameService { + fn default() -> Self { + let mut name_to_address = Mapping::new(); + name_to_address.insert(Hash::default(), &zero_address()); + let mut name_to_owner = Mapping::new(); + name_to_owner.insert(Hash::default(), &zero_address()); + + Self { + name_to_address, + name_to_owner, + default_address: zero_address(), + } + } + } + /// Errors that can occur upon calling this contract. #[derive(Debug, PartialEq, Eq, scale::Encode, scale::Decode)] #[cfg_attr(feature = "std", derive(::scale_info::TypeInfo))] @@ -165,6 +179,13 @@ mod dns { } } + /// Helper for referencing the zero address (`0x00`). Note that in practice this address should + /// not be treated in any special way (such as a default placeholder) since it has a known + /// private key. + fn zero_address() -> AccountId { + [0u8; 32].into() + } + #[cfg(test)] mod tests { use super::*; diff --git a/examples/erc1155/lib.rs b/examples/erc1155/lib.rs index 196eb66cf5..ed49e73bcc 100644 --- a/examples/erc1155/lib.rs +++ b/examples/erc1155/lib.rs @@ -358,7 +358,6 @@ mod erc1155 { { use ink::env::call::{ build_call, - Call, ExecutionInput, Selector, }; @@ -366,7 +365,8 @@ mod erc1155 { // If our recipient is a smart contract we need to see if they accept or // reject this transfer. If they reject it we need to revert the call. let result = build_call::() - .call_type(Call::new().callee(to).gas_limit(5000)) + .call(to) + .gas_limit(5000) .exec_input( ExecutionInput::new(Selector::new(ON_ERC_1155_RECEIVED_SELECTOR)) .push_arg(caller) @@ -432,7 +432,7 @@ mod erc1155 { ensure!(self.is_approved_for_all(from, caller), Error::NotApproved); } - ensure!(to != AccountId::default(), Error::ZeroAddressTransfer); + ensure!(to != zero_address(), Error::ZeroAddressTransfer); let balance = self.balance_of(from, token_id); ensure!(balance >= value, Error::InsufficientBalance); @@ -457,7 +457,7 @@ mod erc1155 { ensure!(self.is_approved_for_all(from, caller), Error::NotApproved); } - ensure!(to != AccountId::default(), Error::ZeroAddressTransfer); + ensure!(to != zero_address(), Error::ZeroAddressTransfer); ensure!(!token_ids.is_empty(), Error::BatchTransferMismatch); ensure!( token_ids.len() == values.len(), @@ -581,6 +581,13 @@ mod erc1155 { } } + /// Helper for referencing the zero address (`0x00`). Note that in practice this address should + /// not be treated in any special way (such as a default placeholder) since it has a known + /// private key. + fn zero_address() -> AccountId { + [0u8; 32].into() + } + #[cfg(test)] mod tests { /// Imports all the definitions from the outer scope so we can use them here. diff --git a/examples/lang-err-integration-tests/call-builder/lib.rs b/examples/lang-err-integration-tests/call-builder/lib.rs index 1d143fadec..092ad1d035 100755 --- a/examples/lang-err-integration-tests/call-builder/lib.rs +++ b/examples/lang-err-integration-tests/call-builder/lib.rs @@ -21,7 +21,6 @@ mod call_builder { use ink::env::{ call::{ build_call, - Call, ExecutionInput, Selector, }, @@ -52,7 +51,7 @@ mod call_builder { selector: [u8; 4], ) -> Option { let result = build_call::() - .call_type(Call::new().callee(address)) + .call(address) .exec_input(ExecutionInput::new(Selector::new(selector))) .returns::<()>() .try_invoke() @@ -79,7 +78,7 @@ mod call_builder { use ink::env::call::build_call; build_call::() - .call_type(Call::new().callee(address)) + .call(address) .exec_input(ExecutionInput::new(Selector::new(selector))) .returns::<()>() .invoke() diff --git a/examples/multisig/lib.rs b/examples/multisig/lib.rs index 250db845d3..b5452c7b1c 100755 --- a/examples/multisig/lib.rs +++ b/examples/multisig/lib.rs @@ -67,7 +67,6 @@ mod multisig { env::{ call::{ build_call, - Call, ExecutionInput, }, CallFlags, @@ -310,7 +309,6 @@ mod multisig { /// use ink::env::{ /// call::{ /// utils::ArgumentList, - /// Call, /// CallParams, /// ExecutionInput, /// Selector, @@ -536,12 +534,9 @@ mod multisig { let t = self.take_transaction(trans_id).expect(WRONG_TRANSACTION_ID); assert!(self.env().transferred_value() == t.transferred_value); let result = build_call::<::Env>() - .call_type( - Call::new() - .callee(t.callee) - .gas_limit(t.gas_limit) - .transferred_value(t.transferred_value), - ) + .call(t.callee) + .gas_limit(t.gas_limit) + .transferred_value(t.transferred_value) .call_flags(CallFlags::default().set_allow_reentry(t.allow_reentry)) .exec_input( ExecutionInput::new(t.selector.into()).push_arg(CallInput(&t.input)), @@ -574,12 +569,9 @@ mod multisig { self.ensure_confirmed(trans_id); let t = self.take_transaction(trans_id).expect(WRONG_TRANSACTION_ID); let result = build_call::<::Env>() - .call_type( - Call::new() - .callee(t.callee) - .gas_limit(t.gas_limit) - .transferred_value(t.transferred_value), - ) + .call(t.callee) + .gas_limit(t.gas_limit) + .transferred_value(t.transferred_value) .call_flags(CallFlags::default().set_allow_reentry(t.allow_reentry)) .exec_input( ExecutionInput::new(t.selector.into()).push_arg(CallInput(&t.input)), diff --git a/examples/upgradeable-contracts/forward-calls/lib.rs b/examples/upgradeable-contracts/forward-calls/lib.rs index 51c4144cc0..d65c511168 100644 --- a/examples/upgradeable-contracts/forward-calls/lib.rs +++ b/examples/upgradeable-contracts/forward-calls/lib.rs @@ -17,7 +17,6 @@ #[ink::contract] pub mod proxy { - use ink::env::call::Call; /// A simple proxy contract. #[ink(storage)] @@ -70,12 +69,9 @@ pub mod proxy { #[ink(message, payable, selector = _)] pub fn forward(&self) -> u32 { ink::env::call::build_call::() - .call_type( - Call::new() - .callee(self.forward_to) - .transferred_value(self.env().transferred_value()) - .gas_limit(0), - ) + .call(self.forward_to) + .transferred_value(self.env().transferred_value()) + .gas_limit(0) .call_flags( ink::env::CallFlags::default() .set_forward_input(true)