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 <hernando@hcastano.com>

---------

Co-authored-by: Andrew Jones <ascjones@gmail.com>

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

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 = <DefaultEnvironment as Environment>::AccountId;
 /// # type Balance = <DefaultEnvironment as Environment>::Balance;
 /// build_call::<DefaultEnvironment>()
-///     .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 = <DefaultEnvironment as Environment>::AccountId;
 /// let my_return_value: i32 = build_call::<DefaultEnvironment>()
-///     .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 = <DefaultEnvironment as Environment>::AccountId;
 /// let my_return_value: i32 = build_call::<DefaultEnvironment>()
-///     .call_type(DelegateCall::new()
-///                 .code_hash(<DefaultEnvironment as Environment>::Hash::CLEAR_HASH))
+///     .delegate(<DefaultEnvironment as Environment>::Hash::CLEAR_HASH)
 ///     .exec_input(
 ///         ExecutionInput::new(Selector::new([0xDE, 0xAD, 0xBE, 0xEF]))
 ///             .push_arg(42u8)
@@ -304,12 +299,9 @@ where
 /// # type AccountId = <DefaultEnvironment as Environment>::AccountId;
 /// # type Balance = <DefaultEnvironment as Environment>::Balance;
 /// let call_result = build_call::<DefaultEnvironment>()
-///     .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<E: Environment> {
     transferred_value: E::Balance,
 }
 
-impl<E: Environment> Default for Call<E> {
-    fn default() -> Self {
-        Call {
-            callee: Default::default(),
+impl<E: Environment> Call<E> {
+    /// 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<E: Environment> Call<E> {
-    /// Returns a clean builder for [`Call`].
-    pub fn new() -> Self {
-        Default::default()
-    }
-}
-
 impl<E> Call<E>
 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<E: Environment> {
 
 impl<E: Environment> DelegateCall<E> {
     /// Returns a clean builder for [`DelegateCall`]
-    pub const fn new() -> Self {
-        DelegateCall {
-            code_hash: E::Hash::CLEAR_HASH,
-        }
-    }
-}
-
-impl<E: Environment> Default for DelegateCall<E> {
-    fn default() -> Self {
-        Self::new()
+    pub const fn new(code_hash: E::Hash) -> Self {
+        DelegateCall { code_hash }
     }
 }
 
@@ -519,26 +488,43 @@ where
     }
 }
 
-impl<E, Args, RetType> CallBuilder<E, Set<Call<E>>, Args, RetType>
+impl<E, CallType, Args, RetType> CallBuilder<E, Unset<CallType>, 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<E, Set<Call<E>>, 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,
             _phantom: Default::default(),
         }
     }
 
+    /// Prepares the `CallBuilder` for a cross-contract [`DelegateCall`].
+    pub fn delegate(
+        self,
+        code_hash: E::Hash,
+    ) -> CallBuilder<E, Set<DelegateCall<E>>, 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<E, Args, RetType> CallBuilder<E, Set<Call<E>>, 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();

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<T>(&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<E: Environment>(&mut self) -> E::AccountId {
-        self.get_property_inplace::<E::AccountId>(ext::caller)
+        self.get_property::<E::AccountId>(ext::caller)
+            .expect("The executed contract must have a caller with a valid account id.")
     }
 
     fn transferred_value<E: Environment>(&mut self) -> E::Balance {
@@ -388,7 +374,8 @@ impl TypedEnvBackend for EnvInstance {
     }
 
     fn account_id<E: Environment>(&mut self) -> E::AccountId {
-        self.get_property_inplace::<E::AccountId>(ext::address)
+        self.get_property::<E::AccountId>(ext::address)
+            .expect("A contract being executed must have a valid account id.")
     }
 
     fn balance<E: Environment>(&mut self) -> E::Balance {

crates/env/src/types.rs

@@ -101,16 +101,15 @@ 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
         + PartialEq
         + Eq
         + Ord
         + AsRef<[u8]>
-        + AsMut<[u8]>
-        + Default;
+        + AsMut<[u8]>;
 
     /// The type of balances.
     type Balance: 'static

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::<Environment>()
-                    .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 ),* ])

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::<Self::Env>()
-                    .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 ),* ])

crates/ink/src/env_access.rs

@@ -524,8 +524,7 @@ where
     /// pub fn invoke_contract(&self) -> i32 {
     ///     let call_params = build_call::<DefaultEnvironment>()
     ///             .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::<DefaultEnvironment>()
     ///             .call_type(
-    ///                 DelegateCall::new()
-    ///                  .code_hash(<DefaultEnvironment as ink::env::Environment>::Hash::CLEAR_HASH))
+    ///                 DelegateCall::new(<DefaultEnvironment as ink::env::Environment>::Hash::CLEAR_HASH))
     ///             .exec_input(
     ///                 ExecutionInput::new(Selector::new([0xCA, 0xFE, 0xBA, 0xBE]))
     ///                  .push_arg(42u8)

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]);

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<Hash, AccountId>,
@@ -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::*;