fvm_dispatch improvements (#5)

* update gitignore

* Renaming and docstrings

* added a workspace file

* fvm_dispatch: handle hash collisions and invalid method names

* clippy fixes

* Update fvm_dispatch/src/message.rs

Co-authored-by: Andy Bright <[email protected]>

* Update fvm_dispatch/src/hash.rs

Co-authored-by: Andy Bright <[email protected]>

* add thiserror macro

* use thiserror for defining errors more concisely

* implement as_u32 with from_be_bytes

* fix failing unit tests

Co-authored-by: Andy Bright <[email protected]>

Author: alexytsu

.gitignore

@@ -8,3 +8,6 @@ Cargo.lock
 
 # These are backup files generated by rustfmt
 **/*.rs.bk
+
+# IDE specific user-config
+.vscode/

Cargo.toml

@@ -0,0 +1,5 @@
+[workspace]
+
+members = [
+    "fvm_dispatch",
+]

fvm_dispatch/Cargo.toml

@@ -8,4 +8,5 @@ edition = "2021"
 [dependencies]
 fvm_ipld_encoding = { version = "0.2.2" }
 fvm_sdk = { version = "1.0.0" }
-fvm_shared = { version = "0.8.0" }
+fvm_shared = { version = "0.8.0" }
+thiserror = { version = "1.0.31" }

fvm_dispatch/src/hash.rs

@@ -1,8 +1,11 @@
+use thiserror::Error;
+
 use fvm_sdk::crypto;
 
 /// Minimal interface for a hashing function
 ///
-/// Hasher::hash() must return a digest that is at least 4 bytes long so that it can be cast to a u32
+/// Hasher::hash() must return a digest that is at least 4 bytes long so that it can be cast to a
+/// u32
 pub trait Hasher {
     /// For an input of bytes return a digest that is at least 4 bytes long
     fn hash(&self, bytes: &[u8]) -> Vec<u8>;
@@ -18,45 +21,84 @@ impl Hasher for Blake2bSyscall {
     }
 }
 
+/// Uses an underlying hashing function (blake2b by convention) to generate method numbers from
+/// method names
 #[derive(Default)]
-pub struct MethodHasher<T: Hasher> {
+pub struct MethodResolver<T: Hasher> {
     hasher: T,
 }
 
-impl<T: Hasher> MethodHasher<T> {
+#[derive(Error, PartialEq, Debug)]
+pub enum MethodNameErr {
+    #[error("empty method name provided")]
+    EmptyString,
+    #[error("illegal symbol used in method name")]
+    IllegalSymbol,
+    #[error("unable to calculate method id, choose a another method name")]
+    IndeterminableId,
+}
+
+impl<T: Hasher> MethodResolver<T> {
     const CONSTRUCTOR_METHOD_NAME: &'static str = "Constructor";
     const CONSTRUCTOR_METHOD_NUMBER: u64 = 1_u64;
+    const RESERVED_METHOD_NUMBER: u64 = 0_u64;
+
+    /// Creates a MethodResolver with an instance of a hasher (blake2b by convention)
     pub fn new(hasher: T) -> Self {
         Self { hasher }
     }
 
-    pub fn method_number(&self, method_name: &str) -> u64 {
+    /// Generates a standard FRC-XXX compliant method number
+    ///
+    /// The method number is calculated as the first four bytes of `hash(method-name)`.
+    /// The name `Constructor` is always hashed to 1 and other method names that hash to
+    /// 0 or 1 are avoided via rejection sampling.
+    ///
+    ///
+    pub fn method_number(&self, method_name: &str) -> Result<u64, MethodNameErr> {
+        // TODO: sanitise method_name before checking (or reject invalid whitespace)
+        if method_name.contains('|') {
+            return Err(MethodNameErr::IllegalSymbol);
+        }
+
+        if method_name.is_empty() {
+            return Err(MethodNameErr::EmptyString);
+        }
+
         if method_name == Self::CONSTRUCTOR_METHOD_NAME {
-            Self::CONSTRUCTOR_METHOD_NUMBER
-        } else {
-            let digest = self.hasher.hash(method_name.as_bytes());
-            if digest.len() < 4 {
-                panic!("Invalid hasher used: digest must be at least 4 bytes long");
+            return Ok(Self::CONSTRUCTOR_METHOD_NUMBER);
+        }
+        let mut digest = self.hasher.hash(method_name.as_bytes());
+        while digest.len() >= 4 {
+            let method_id = as_u32(digest.as_slice()) as u64;
+            if method_id != Self::CONSTRUCTOR_METHOD_NUMBER
+                && method_id != Self::RESERVED_METHOD_NUMBER
+            {
+                return Ok(method_id);
+            } else {
+                digest.remove(0);
             }
-            as_u32(digest.as_slice()) as u64
         }
+        Err(MethodNameErr::IndeterminableId)
     }
 }
 
 /// Takes a byte array and interprets it as a u32 number
-/// Assumes little-endian order
-#[rustfmt::skip]
+///
+/// Using big-endian order interperets the first four bytes to an int.
+/// The slice passed to this must be at least length 4
 fn as_u32(bytes: &[u8]) -> u32 {
-    ((bytes[0] as u32) << (8 * 3)) +
-    ((bytes[1] as u32) << (8 * 2)) +
-    ((bytes[2] as u32) << (8 * 1)) +
-    (bytes[3] as u32)
+    u32::from_be_bytes(
+        bytes[0..4]
+            .try_into()
+            .expect("bytes was not at least length 4"),
+    )
 }
 
 #[cfg(test)]
 mod tests {
 
-    use super::{Blake2bSyscall, Hasher, MethodHasher};
+    use super::{Hasher, MethodNameErr, MethodResolver};
 
     #[derive(Clone, Copy)]
     struct FakeHasher {}
@@ -67,24 +109,56 @@ mod tests {
     }
 
     #[test]
-    #[allow(unused)]
-    fn compile() {
-        let method_hasher = MethodHasher::new(Blake2bSyscall {});
+    fn constructor_is_1() {
+        let method_hasher = MethodResolver::new(FakeHasher {});
+        assert_eq!(method_hasher.method_number("Constructor").unwrap(), 1);
     }
 
     #[test]
-    fn constructor_method_number() {
-        let method_hasher = MethodHasher::new(FakeHasher {});
-        assert_eq!(method_hasher.method_number("Constructor"), 1);
+    fn normal_method_is_hashed() {
+        let fake_hasher = FakeHasher {};
+        let method_hasher = MethodResolver::new(fake_hasher);
+        assert_eq!(
+            method_hasher.method_number("NormalMethod").unwrap(),
+            super::as_u32(&fake_hasher.hash(b"NormalMethod")) as u64
+        );
+
+        assert_eq!(
+            method_hasher.method_number("NormalMethod2").unwrap(),
+            super::as_u32(&fake_hasher.hash(b"NormalMethod2")) as u64
+        );
     }
 
     #[test]
-    fn normal_method_number() {
-        let fake_hasher = FakeHasher {};
-        let method_hasher = MethodHasher::new(fake_hasher);
+    fn disallows_invalid_method_names() {
+        let method_hasher = MethodResolver::new(FakeHasher {});
         assert_eq!(
-            method_hasher.method_number("NormalMethod"),
-            super::as_u32(&fake_hasher.hash(b"NormalMethod")) as u64
+            method_hasher.method_number("Invalid|Method").unwrap_err(),
+            MethodNameErr::IllegalSymbol
         );
+        assert_eq!(
+            method_hasher.method_number("").unwrap_err(),
+            MethodNameErr::EmptyString
+        );
+    }
+
+    #[test]
+    fn avoids_disallowed_method_numbers() {
+        let hasher = FakeHasher {};
+        let method_hasher = MethodResolver::new(hasher);
+
+        // This simulates a method name that would hash to 0
+        let contrived_0 = "\0\0\0\0MethodName";
+        let contrived_0_digest = hasher.hash(contrived_0.as_bytes());
+        assert_eq!(super::as_u32(&contrived_0_digest), 0);
+        // But the method number is not a collision
+        assert_ne!(method_hasher.method_number(contrived_0).unwrap(), 0);
+
+        // This simulates a method name that would hash to 1
+        let contrived_1 = "\0\0\0\x01MethodName";
+        let contrived_1_digest = hasher.hash(contrived_1.as_bytes());
+        assert_eq!(super::as_u32(&contrived_1_digest), 1);
+        // But the method number is not a collision
+        assert_ne!(method_hasher.method_number(contrived_1).unwrap(), 1);
     }
 }

fvm_dispatch/src/message.rs

@@ -1,29 +1,55 @@
-use crate::hash::{Hasher, MethodHasher};
+use thiserror::Error;
+
+use crate::hash::{Hasher, MethodNameErr, MethodResolver};
 
 use fvm_ipld_encoding::RawBytes;
-use fvm_sdk::{send, SyscallResult};
+use fvm_sdk::{send, sys::ErrorNumber};
 use fvm_shared::{address::Address, econ::TokenAmount, receipt::Receipt};
 
+/// Utility to invoke standard methods on deployed actors
 #[derive(Default)]
-pub struct MethodDispatcher<T: Hasher> {
-    method_hasher: MethodHasher<T>,
+pub struct MethodMessenger<T: Hasher> {
+    method_resolver: MethodResolver<T>,
+}
+
+#[derive(Error, PartialEq, Debug)]
+pub enum MethodMessengerError {
+    #[error("error when calculating method name: `{0}`")]
+    MethodName(MethodNameErr),
+    #[error("error sending message: `{0}`")]
+    Syscall(ErrorNumber),
+}
+
+impl From<ErrorNumber> for MethodMessengerError {
+    fn from(e: ErrorNumber) -> Self {
+        Self::Syscall(e)
+    }
+}
+
+impl From<MethodNameErr> for MethodMessengerError {
+    fn from(e: MethodNameErr) -> Self {
+        Self::MethodName(e)
+    }
 }
 
-impl<T: Hasher> MethodDispatcher<T> {
+impl<T: Hasher> MethodMessenger<T> {
+    /// Creates a new method messenger using a specified hashing function (blake2b by default)
     pub fn new(hasher: T) -> Self {
         Self {
-            method_hasher: MethodHasher::new(hasher),
+            method_resolver: MethodResolver::new(hasher),
         }
     }
 
+    /// Calls a method (by name) on a specified actor by constructing and publishing the underlying
+    /// on-chain Message
     pub fn call_method(
         &self,
         to: &Address,
         method: &str,
         params: RawBytes,
         value: TokenAmount,
-    ) -> SyscallResult<Receipt> {
-        let method = self.method_hasher.method_number(method);
-        send::send(to, method, params, value)
+    ) -> Result<Receipt, MethodMessengerError> {
+        let method = self.method_resolver.method_number(method)?;
+        send::send(to, method, params, value).map_err(MethodMessengerError::from)
     }
 }