Ensure topics are unique (use-ink#594)

* Ensure topics are unique * Remove unavailable method * Fix salt appending in Wasm * Remove salt param * Add missing assignment * Update crates/lang/src/events.rs Co-authored-by: Hero Bird <[email protected]> * Add suggestions * Remove always inline * Fix event hashing in examples * Make nightly clippy happy * Remove unnecessary return param * Do not use internal type for hash calculation * Hide internal type from docs * Expose `PrefixedValue` from ink_env::topics * Apply cargo fmt * Apply cargo fmt * Apply comments * Apply comments * Apply comments Co-authored-by: Hero Bird <[email protected]>
edenworkroom · Dec 2, 2020 · 64eec2c · 64eec2c
1 parent 8e8fe09
commit 64eec2c
Show file tree

Hide file tree

Showing 6 changed files with 221 additions and 10 deletions.
diff --git a/crates/env/src/engine/off_chain/db/events.rs b/crates/env/src/engine/off_chain/db/events.rs
@@ -57,6 +57,10 @@ where
             result.as_mut()[0..copy_len].copy_from_slice(&hash_output[0..copy_len]);
         }
         let off_hash = OffHash::new(&result);
+        debug_assert!(
+            !self.topics.contains(&off_hash),
+            "duplicate topic hash discovered!"
+        );
         self.topics.push(off_hash);
     }
 

diff --git a/crates/env/src/topics.rs b/crates/env/src/topics.rs
@@ -201,3 +201,36 @@ pub trait Topics {
         E: Environment,
         B: TopicsBuilderBackend<E>;
 }
+
+/// For each topic a hash is generated. This hash must be unique
+/// for a field and its value. The `prefix` is concatenated
+/// with the `value` and this result is then hashed.
+/// The `prefix` is typically set to the path a field has in
+/// an event struct + the identifier of the event struct.
+///
+/// For example, in the case of our Erc20 example contract the
+/// prefix `Erc20::Transfer::from` is concatenated with the
+/// field value of `from` and then hashed.
+/// In this example `Erc20` would be the contract identified,
+/// `Transfer` the event identifier, and `from` the field identifier.
+#[doc(hidden)]
+pub struct PrefixedValue<'a, 'b, T> {
+    pub prefix: &'a [u8],
+    pub value: &'b T,
+}
+
+impl<X> scale::Encode for PrefixedValue<'_, '_, X>
+where
+    X: scale::Encode,
+{
+    #[inline]
+    fn size_hint(&self) -> usize {
+        self.prefix.size_hint() + self.value.size_hint()
+    }
+
+    #[inline]
+    fn encode_to<T: scale::Output>(&self, dest: &mut T) {
+        self.prefix.encode_to(dest);
+        self.value.encode_to(dest);
+    }
+}
diff --git a/crates/lang/codegen/src/generator/events.rs b/crates/lang/codegen/src/generator/events.rs
@@ -237,15 +237,23 @@ impl<'a> Events<'a> {
                         .map(quote::ToTokens::into_token_stream)
                         .unwrap_or_else(|| quote_spanned!(span => #n));
                     let field_type = topic_field.ty();
+                    let signature = syn::LitByteStr::new(
+                        format!("{}::{}::{}", contract_ident, event_ident,
+                            field_ident
+                        ).as_bytes(), span);
                     quote_spanned!(span =>
-                        .push_topic::<#field_type>(&self.#field_ident)
+                        .push_topic::<::ink_env::topics::PrefixedValue<#field_type>>(
+                            &::ink_env::topics::PrefixedValue { value: &self.#field_ident, prefix: #signature }
+                        )
                     )
                 });
             // Only include topic for event signature in case of non-anonymous event.
             let event_signature_topic = match event.anonymous {
                 true => None,
                 false => Some(quote_spanned!(span=>
-                    .push_topic::<[u8; #len_event_signature]>(#event_signature)
+                    .push_topic::<::ink_env::topics::PrefixedValue<[u8; #len_event_signature]>>(
+                        &::ink_env::topics::PrefixedValue { value: #event_signature, prefix: b"" }
+                    )
                 ))
             };
             // Anonymous events require 1 fewer topics since they do not include their signature.

diff --git a/crates/lang/macro/tests/unique_topics.rs b/crates/lang/macro/tests/unique_topics.rs
@@ -0,0 +1,98 @@
+// Copyright 2018-2020 Parity Technologies (UK) Ltd.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#![cfg_attr(not(feature = "std"), no_std)]
+
+use ink_lang as ink;
+
+#[ink::contract]
+mod my_contract {
+    #[ink(storage)]
+    pub struct MyContract {}
+
+    /// Exemplary event
+    #[ink(event)]
+    pub struct MyEvent {
+        #[ink(topic)]
+        v0: Option<AccountId>,
+        #[ink(topic)]
+        v1: Balance,
+        #[ink(topic)]
+        v2: bool,
+        #[ink(topic)]
+        v3: bool,
+    }
+
+    impl MyContract {
+        /// Creates a new `MyContract` instance.
+        #[ink(constructor)]
+        pub fn new() -> Self {
+            MyContract {}
+        }
+
+        /// Emits a `MyEvent`.
+        #[ink(message)]
+        pub fn emit_my_event(&self) {
+            Self::env().emit_event(MyEvent {
+                v0: None,
+                v1: 0,
+                v2: false,
+                v3: false,
+            });
+        }
+    }
+
+    #[cfg(test)]
+    mod tests {
+        use super::*;
+        use ink_env::test::EmittedEvent;
+        use ink_lang as ink;
+
+        #[ink::test]
+        fn event_must_have_unique_topics() {
+            // given
+            let my_contract = MyContract::new();
+
+            // when
+            MyContract::emit_my_event(&my_contract);
+
+            // then
+            // all topics must be unique
+            let emitted_events =
+                ink_env::test::recorded_events().collect::<Vec<EmittedEvent>>();
+            let mut encoded_topics: std::vec::Vec<&[u8]> = emitted_events[0]
+                .topics
+                .iter()
+                .map(|topic| topic.encoded_bytes().expect("encoded bytes must exist"))
+                .collect();
+            assert!(!has_duplicates(&mut encoded_topics));
+        }
+    }
+
+    /// Finds duplicates in a given vector.
+    ///
+    /// This function has complexity of `O(n * log n)` and no additional memory
+    /// is required, although the order of items is not preserved.
+    fn has_duplicates<T: PartialEq + AsRef<[u8]>>(items: &mut Vec<T>) -> bool {
+        // Sort the vector
+        items.sort_by(|a, b| Ord::cmp(a.as_ref(), b.as_ref()));
+        // And then find any two consecutive equal elements.
+        items.windows(2).any(|w| {
+            match w {
+                &[ref a, ref b] => a == b,
+                _ => false,
+            }
+        })
+    }
+}
diff --git a/examples/erc20/lib.rs b/examples/erc20/lib.rs
@@ -260,10 +260,22 @@ mod erc20 {
                 result
             }
             let expected_topics = vec![
-                encoded_into_hash(b"Erc20::Transfer"),
-                encoded_into_hash(&expected_from),
-                encoded_into_hash(&expected_to),
-                encoded_into_hash(&expected_value),
+                encoded_into_hash(&PrefixedValue {
+                    value: b"Erc20::Transfer",
+                    prefix: b"",
+                }),
+                encoded_into_hash(&PrefixedValue {
+                    prefix: b"Erc20::Transfer::from",
+                    value: &expected_from,
+                }),
+                encoded_into_hash(&PrefixedValue {
+                    prefix: b"Erc20::Transfer::to",
+                    value: &expected_to,
+                }),
+                encoded_into_hash(&PrefixedValue {
+                    prefix: b"Erc20::Transfer::value",
+                    value: &expected_value,
+                }),
             ];
             for (n, (actual_topic, expected_topic)) in
                 event.topics.iter().zip(expected_topics).enumerate()
@@ -519,4 +531,26 @@ mod erc20 {
             assert_eq!(emitted_events_before.len(), emitted_events_after.len());
         }
     }
+
+    /// For calculating the event topic hash.
+    struct PrefixedValue<'a, 'b, T> {
+        pub prefix: &'a [u8],
+        pub value: &'b T,
+    }
+
+    impl<X> scale::Encode for PrefixedValue<'_, '_, X>
+    where
+        X: scale::Encode,
+    {
+        #[inline]
+        fn size_hint(&self) -> usize {
+            self.prefix.size_hint() + self.value.size_hint()
+        }
+
+        #[inline]
+        fn encode_to<T: scale::Output>(&self, dest: &mut T) {
+            self.prefix.encode_to(dest);
+            self.value.encode_to(dest);
+        }
+    }
 }
diff --git a/examples/trait-erc20/lib.rs b/examples/trait-erc20/lib.rs
@@ -308,10 +308,22 @@ mod erc20 {
                 result
             }
             let expected_topics = vec![
-                encoded_into_hash(b"Erc20::Transfer"),
-                encoded_into_hash(&expected_from),
-                encoded_into_hash(&expected_to),
-                encoded_into_hash(&expected_value),
+                encoded_into_hash(&PrefixedValue {
+                    prefix: b"",
+                    value: b"Erc20::Transfer",
+                }),
+                encoded_into_hash(&PrefixedValue {
+                    prefix: b"Erc20::Transfer::from",
+                    value: &expected_from,
+                }),
+                encoded_into_hash(&PrefixedValue {
+                    prefix: b"Erc20::Transfer::to",
+                    value: &expected_to,
+                }),
+                encoded_into_hash(&PrefixedValue {
+                    prefix: b"Erc20::Transfer::value",
+                    value: &expected_value,
+                }),
             ];
             for (n, (actual_topic, expected_topic)) in
                 event.topics.iter().zip(expected_topics).enumerate()
@@ -571,4 +583,26 @@ mod erc20 {
             assert_eq!(emitted_events_before.len(), emitted_events_after.len());
         }
     }
+
+    /// For calculating the event topic hash.
+    struct PrefixedValue<'a, 'b, T> {
+        pub prefix: &'a [u8],
+        pub value: &'b T,
+    }
+
+    impl<X> scale::Encode for PrefixedValue<'_, '_, X>
+    where
+        X: scale::Encode,
+    {
+        #[inline]
+        fn size_hint(&self) -> usize {
+            self.prefix.size_hint() + self.value.size_hint()
+        }
+
+        #[inline]
+        fn encode_to<T: scale::Output>(&self, dest: &mut T) {
+            self.prefix.encode_to(dest);
+            self.value.encode_to(dest);
+        }
+    }
 }