Review expect/unwrap - Take 2 (#162)

c410-f3r · web-flow · commit c519c1a54acc · 2020-05-27T15:51:14.000+02:00
diff --git a/examples/ffi/c++/main.cpp b/examples/ffi/c++/main.cpp
@@ -17,7 +17,7 @@ int main(int argc, char *argv[]) {
         std::cerr << filecoin_signer_error_message(error);
     }
     else {
-        char *private_key = filecoin_signer_extended_key_private_key(extended_key);
+        char *private_key = filecoin_signer_extended_key_private_key(extended_key, error);
         assert(strcmp(private_key, "f15716d3b003b304b8055d9cc62e6b9c869d56cc930c3858d4d7c31f5f53f14a") == 0);
         filecoin_signer_string_free(private_key);
     }
diff --git a/examples/ffi/c/main.c b/examples/ffi/c/main.c
@@ -4,6 +4,20 @@
 
 #include "filecoin_signer_ffi.h"
 
+void free_resources(ExtendedKey *extended_key, ExternError *error) {
+    filecoin_signer_extended_key_free(extended_key);
+    filecoin_signer_error_free(error);
+}
+
+void manage_error(ExtendedKey *extended_key, ExternError *error) {
+    if (filecoin_signer_error_code(error) == 0) {
+        return;
+    }
+    fprintf(stderr, "%s\n", filecoin_signer_error_message(error));
+    free_resources(extended_key, error);
+    exit(EXIT_FAILURE);
+}
+
 int main(int argc, char *argv[]) {
     ExternError *error = filecoin_signer_error_new();
     ExtendedKey *extended_key = filecoin_signer_key_derive(
@@ -12,16 +26,10 @@ int main(int argc, char *argv[]) {
         "",
         error
     );
-
-    if (filecoin_signer_error_code(error) != 0) {
-        fprintf(stderr, "%s\n", filecoin_signer_error_message(error));
-    }
-    else {
-        char *private_key = filecoin_signer_extended_key_private_key(extended_key);
-        assert(strcmp(private_key, "f15716d3b003b304b8055d9cc62e6b9c869d56cc930c3858d4d7c31f5f53f14a") == 0);
-        filecoin_signer_string_free(private_key);
-    }
-
-    filecoin_signer_extended_key_free(extended_key);
-    filecoin_signer_error_free(error);
-}
+    manage_error(extended_key, error);
+    char *private_key = filecoin_signer_extended_key_private_key(extended_key, error);
+    manage_error(extended_key, error);
+    assert(strcmp(private_key, "f15716d3b003b304b8055d9cc62e6b9c869d56cc930c3858d4d7c31f5f53f14a") == 0);
+    filecoin_signer_string_free(private_key);
+    free_resources(extended_key, error);
+}
diff --git a/examples/ffi/flutter/lib/main.dart b/examples/ffi/flutter/lib/main.dart
@@ -26,7 +26,7 @@ class _MyAppState extends State<MyApp> {
       stderr.write(Filecoin.errorMessage(error));
     }
     else {
-      var privateKeyPtr = Filecoin.extendedKeyPrivateKey(extendedKey);
+      var privateKeyPtr = Filecoin.extendedKeyPrivateKey(extendedKey, error);
       privateKey = Utf8.fromUtf8(privateKeyPtr);
       assert(privateKey == 'f15716d3b003b304b8055d9cc62e6b9c869d56cc930c3858d4d7c31f5f53f14a');
       Filecoin.stringFree(privateKeyPtr);
diff --git a/examples/ffi/go/main.go b/examples/ffi/go/main.go
@@ -21,7 +21,7 @@ func main() {
 		err := C.filecoin_signer_error_message(error);
 		fmt.Fprintln(os.Stderr, C.GoString(err))
 	} else {
-		private_key := C.filecoin_signer_extended_key_private_key(extended_key);
+		private_key := C.filecoin_signer_extended_key_private_key(extended_key, error);
 		if C.GoString(private_key) != "f15716d3b003b304b8055d9cc62e6b9c869d56cc930c3858d4d7c31f5f53f14a" {
 			panic("Bad key");
 		}
diff --git a/examples/ffi/java/Main.java b/examples/ffi/java/Main.java
@@ -14,7 +14,7 @@ public static void main(String[] args) {
             System.err.println(FilecoinSigner.errorMessage(error));
         }
         else {
-            String privateKey = FilecoinSigner.extendedKeyPrivateKey(extendedKey);
+            String privateKey = FilecoinSigner.extendedKeyPrivateKey(extendedKey, error);
             assert privateKey.equals("f15716d3b003b304b8055d9cc62e6b9c869d56cc930c3858d4d7c31f5f53f14a");
         }
 
diff --git a/examples/ffi/kotlin/Main.kt b/examples/ffi/kotlin/Main.kt
@@ -13,7 +13,7 @@ fun main() {
         System.err.println(FilecoinSigner.errorMessage(error))
     }
     else {
-        val privateKey = FilecoinSigner.extendedKeyPrivateKey(extendedKey);
+        val privateKey = FilecoinSigner.extendedKeyPrivateKey(extendedKey, error);
         assert(privateKey == "f15716d3b003b304b8055d9cc62e6b9c869d56cc930c3858d4d7c31f5f53f14a")
     }
 
diff --git a/examples/ffi/objective-c/main.m b/examples/ffi/objective-c/main.m
@@ -17,7 +17,7 @@ int main(int argc, char *argv[]) {
         fprintf(stderr, "%s\n", filecoin_signer_error_message(error));
     }
     else {
-        char *private_key = filecoin_signer_extended_key_private_key(extended_key);
+        char *private_key = filecoin_signer_extended_key_private_key(extended_key, error);
         assert([[NSString stringWithUTF8String:private_key] isEqualToString:@"f15716d3b003b304b8055d9cc62e6b9c869d56cc930c3858d4d7c31f5f53f14a"]);
         filecoin_signer_string_free(private_key);
     }
diff --git a/examples/ffi/swift/main.swift b/examples/ffi/swift/main.swift
@@ -11,7 +11,7 @@ if (filecoin_signer_error_code(error) != 0) {
     fputs(err, stderr)
 }
 else {
-    let private_key = filecoin_signer_extended_key_private_key(extended_key);
+    let private_key = filecoin_signer_extended_key_private_key(extended_key, error);
     assert(String(cString: private_key!) == "f15716d3b003b304b8055d9cc62e6b9c869d56cc930c3858d4d7c31f5f53f14a");
     filecoin_signer_string_free(private_key);
 }
diff --git a/ledger/src/lib.rs b/ledger/src/lib.rs
@@ -15,6 +15,15 @@
 ********************************************************************************/
 //! Support library for Filecoin Ledger Nano S/X apps
 
+#![cfg_attr(
+    not(test),
+    deny(
+        clippy::option_unwrap_used,
+        clippy::option_expect_used,
+        clippy::result_unwrap_used,
+        clippy::result_expect_used,
+    )
+)]
 #![deny(warnings, trivial_casts, trivial_numeric_casts)]
 #![deny(unused_import_braces, unused_qualifications)]
 #![deny(missing_docs)]
diff --git a/service/src/lib.rs b/service/src/lib.rs
@@ -6,6 +6,15 @@
 
 // Tip: Deny warnings with `RUSTFLAGS="-D warnings"` environment variable in CI
 
+#![cfg_attr(
+    not(test),
+    deny(
+        clippy::option_unwrap_used,
+        clippy::option_expect_used,
+        clippy::result_unwrap_used,
+        clippy::result_expect_used,
+    )
+)]
 #![forbid(unsafe_code)]
 #![warn(
     missing_docs,
diff --git a/service/src/service/cache.rs b/service/src/service/cache.rs
@@ -1,3 +1,4 @@
+use crate::service::error::ServiceError;
 use lazy_static::lazy_static;
 use lru::LruCache;
 use std::sync::Mutex;
@@ -6,21 +7,30 @@ lazy_static! {
     static ref NONCE_CACHE: Mutex<LruCache::<String, u64>> = Mutex::new(LruCache::new(100));
 }
 
-pub fn cache_get_nonce(addr: &str) -> Option<u64> {
+pub fn cache_get_nonce(addr: &str) -> Result<u64, ServiceError> {
     // Retrieve from cache
-    let mut cache = NONCE_CACHE.lock().expect("mutex lock failed");
+    let mut cache = NONCE_CACHE
+        .lock()
+        .map_err(|e| ServiceError::ErrorStr(e.to_string()))?;
     let nonce = cache.get(&addr.to_owned());
-    nonce.copied().or_else(|| None)
+    nonce
+        .copied()
+        .ok_or_else(|| ServiceError::ErrorStr("Couldn't get cached value".to_string()))
 }
 
-pub fn cache_put_nonce(addr: &str, nonce: u64) {
-    let mut cache = NONCE_CACHE.lock().expect("mutex lock failed");
+pub fn cache_put_nonce(addr: &str, nonce: u64) -> Result<(), ServiceError> {
+    let mut cache = NONCE_CACHE
+        .lock()
+        .map_err(|e| ServiceError::ErrorStr(e.to_string()))?;
     cache.put(addr.to_owned(), nonce);
+    Ok(())
 }
 
-pub fn cache_len() -> usize {
-    let cache = NONCE_CACHE.lock().expect("mutex lock failed");
-    cache.len()
+pub fn cache_len() -> Result<usize, ServiceError> {
+    let cache = NONCE_CACHE
+        .lock()
+        .map_err(|e| ServiceError::ErrorStr(e.to_string()))?;
+    Ok(cache.len())
 }
 
 #[cfg(test)]
@@ -30,19 +40,19 @@ mod tests {
     #[test]
     fn cache_put_get() {
         let not_found = cache_get_nonce("unknown");
-        assert!(not_found.is_none());
-        assert_eq!(cache_len(), 0);
+        assert!(not_found.is_err());
+        assert_eq!(cache_len().unwrap(), 0);
 
-        cache_put_nonce("address1", 123);
-        cache_put_nonce("address2", 456);
-        assert_eq!(cache_len(), 2);
+        cache_put_nonce("address1", 123).unwrap();
+        cache_put_nonce("address2", 456).unwrap();
+        assert_eq!(cache_len().unwrap(), 2);
 
         let found1 = cache_get_nonce("address1");
-        assert!(found1.is_some());
+        assert!(found1.is_ok());
         assert_eq!(found1.unwrap(), 123);
 
         let found2 = cache_get_nonce("address2");
-        assert!(found2.is_some());
+        assert!(found2.is_ok());
         assert_eq!(found2.unwrap(), 456);
     }
 }
diff --git a/service/src/service/error.rs b/service/src/service/error.rs
@@ -1,5 +1,4 @@
 use filecoin_signer::error::SignerError;
-use serde_json::error::Error;
 use thiserror::Error;
 
 /// RemoteNode Error
@@ -19,8 +18,6 @@ pub enum RemoteNode {
 /// Signer Error
 #[derive(Error, Debug)]
 pub enum ServiceError {
-    #[error("This is not yet implemented")]
-    NotImplemented,
     #[error("The network information provided in the tx doesn't match the node network.")]
     WrongNetwork,
     /// JSONRPC error
@@ -43,5 +40,8 @@ pub enum ServiceError {
     HexDecode(#[from] hex::FromHexError),
     /// Serde Json Error
     #[error("Serde JSON | {0}")]
-    SerdeError(#[from] Error),
+    SerdeError(#[from] serde_json::error::Error),
+    /// Generic string error
+    #[error("Error | {0}")]
+    ErrorStr(String),
 }
diff --git a/signer-ffi/flutter/filecoin/lib/filecoin.dart b/signer-ffi/flutter/filecoin/lib/filecoin.dart
@@ -24,13 +24,13 @@ class Filecoin {
       .lookup<NativeFunction<Void Function(Pointer)>>("filecoin_signer_error_free")
       .asFunction();
 
-  static final Pointer<Utf8> Function(Pointer) extendedKeyPrivateKey =
+  static final Pointer<Utf8> Function(Pointer, Pointer) extendedKeyPrivateKey =
     filecoin
-      .lookup<NativeFunction<Pointer<Utf8> Function(Pointer)>>("filecoin_signer_extended_key_private_key")
+      .lookup<NativeFunction<Pointer<Utf8> Function(Pointer, Pointer)>>("filecoin_signer_extended_key_private_key")
       .asFunction();
-  static final Pointer<Utf8> Function(Pointer) extendedKeyPublicKey =
+  static final Pointer<Utf8> Function(Pointer, Pointer) extendedKeyPublicKey =
     filecoin
-      .lookup<NativeFunction<Pointer<Utf8> Function(Pointer)>>("filecoin_signer_extended_key_public_key")
+      .lookup<NativeFunction<Pointer<Utf8> Function(Pointer, Pointer)>>("filecoin_signer_extended_key_public_key")
       .asFunction();
   static final void Function(Pointer) extendedKeyFree =
     filecoin
diff --git a/signer-ffi/java/src/main/java/ch/zondax/FilecoinSigner.java b/signer-ffi/java/src/main/java/ch/zondax/FilecoinSigner.java
@@ -6,11 +6,11 @@ public class FilecoinSigner {
     public static native String errorMessage(long ptr);
     public static native void errorFree(long ptr);
 
-    public static native String extendedKeyPrivateKey(long ptr);
-    public static native String extendedKeyPublicKey(long ptr);
+    public static native String extendedKeyPrivateKey(long ptr, long err);
+    public static native String extendedKeyPublicKey(long ptr, long err);
     public static native void extendedKeyFree(long ptr);
 
-    public static native long keyDerive(String mnemonic, String path, String password, long ptr);
+    public static native long keyDerive(String mnemonic, String path, String password, long err);
 
     static {
         System.loadLibrary("filecoin_signer_ffi");
diff --git a/signer-ffi/src/extended_key.rs b/signer-ffi/src/extended_key.rs
@@ -1,11 +1,22 @@
+use ffi_support::{call_with_result, ExternError};
 use filecoin_signer::ExtendedKey;
 
-create_fn!(filecoin_signer_extended_key_private_key|Java_ch_zondax_FilecoinSigner_extendedKeyPrivateKey: (ek: &mut ExtendedKey) -> str_ret_ty!(), |etc| {
-    create_str!(etc, hex::encode(&ek.private_key.0))
+create_fn!(filecoin_signer_extended_key_private_key|Java_ch_zondax_FilecoinSigner_extendedKeyPrivateKey: (
+    ek: &mut ExtendedKey,
+    error: &mut ExternError
+) -> str_ret_ty!(), |etc| {
+    call_with_result(error, || -> Result<str_ret_ty!(), ExternError> {
+        create_string!(etc, hex::encode(&ek.private_key.0))
+    })
 });
 
-create_fn!(filecoin_signer_extended_key_public_key|Java_ch_zondax_FilecoinSigner_extendedKeyPublicKey: (ek: &mut ExtendedKey) -> str_ret_ty!(), |etc| {
-    create_str!(etc, hex::encode(&ek.public_key.0[..]))
+create_fn!(filecoin_signer_extended_key_public_key|Java_ch_zondax_FilecoinSigner_extendedKeyPublicKey: (
+    ek: &mut ExtendedKey,
+    error: &mut ExternError
+) -> str_ret_ty!(), |etc| {
+    call_with_result(error, || -> Result<str_ret_ty!(), ExternError> {
+        create_string!(etc, hex::encode(&ek.public_key.0[..]))
+    })
 });
 
 create_fn_destructor!(
diff --git a/signer-ffi/src/lib.rs b/signer-ffi/src/lib.rs
@@ -1,3 +1,13 @@
+#![cfg_attr(
+    not(test),
+    deny(
+        clippy::option_unwrap_used,
+        clippy::option_expect_used,
+        clippy::result_unwrap_used,
+        clippy::result_expect_used,
+    )
+)]
+
 #[macro_use]
 mod macros;
 
@@ -14,9 +24,25 @@ create_fn!(filecoin_signer_key_derive|Java_ch_zondax_FilecoinSigner_keyDerive: (
     error: &mut ExternError
 ) -> ptr!(ExtendedKey), |etc| {
     call_with_result(error, || -> Result<ExtendedKey, ExternError> {
-        Ok(key_derive(get_str!(etc, mnemonic), get_str!(etc, path), get_str!(etc, password))?)
+        let mnemonic = get_string!(etc, mnemonic)?;
+        let path = get_string!(etc, path)?;
+        let password = get_string!(etc, password)?;
+        Ok(key_derive(
+            get_string_ref(&mnemonic),
+            get_string_ref(&path),
+            get_string_ref(&password),
+        )?)
     })
 });
 
 #[cfg(not(feature = "with-jni"))]
 ffi_support::define_string_destructor!(filecoin_signer_string_free);
+
+#[cfg(feature = "with-jni")]
+fn get_string_ref<'a>(s: &'a std::ffi::CStr) -> &'a str {
+    ffi_support::FfiStr::from_cstr(s).as_str()
+}
+#[cfg(not(feature = "with-jni"))]
+fn get_string_ref<'a>(s: &'a ffi_support::FfiStr) -> &'a str {
+    s.as_str()
+}
diff --git a/signer-ffi/src/macros.rs b/signer-ffi/src/macros.rs
diff --git a/signer-wasm/src/ledger.rs b/signer-wasm/src/ledger.rs
diff --git a/signer-wasm/src/lib.rs b/signer-wasm/src/lib.rs
diff --git a/signer-wasm/src/utils.rs b/signer-wasm/src/utils.rs
diff --git a/signer/src/lib.rs b/signer/src/lib.rs

Original file line number	Diff line number	Diff line change
`@@ -17,7 +17,7 @@ int main(int argc, char *argv[]) {`
`17`	`17`	`std::cerr << filecoin_signer_error_message(error);`
`18`	`18`	`}`
`19`	`19`	`else {`
`20`		`- char *private_key = filecoin_signer_extended_key_private_key(extended_key);`
	`20`	`+ char *private_key = filecoin_signer_extended_key_private_key(extended_key, error);`
`21`	`21`	`assert(strcmp(private_key, "f15716d3b003b304b8055d9cc62e6b9c869d56cc930c3858d4d7c31f5f53f14a") == 0);`
`22`	`22`	`filecoin_signer_string_free(private_key);`
`23`	`23`	`}`
Original file line number	Diff line number	Diff line change
`@@ -26,7 +26,7 @@ class _MyAppState extends State<MyApp> {`
`26`	`26`	`stderr.write(Filecoin.errorMessage(error));`
`27`	`27`	`}`
`28`	`28`	`else {`
`29`		`- var privateKeyPtr = Filecoin.extendedKeyPrivateKey(extendedKey);`
	`29`	`+ var privateKeyPtr = Filecoin.extendedKeyPrivateKey(extendedKey, error);`
`30`	`30`	`privateKey = Utf8.fromUtf8(privateKeyPtr);`
`31`	`31`	`assert(privateKey == 'f15716d3b003b304b8055d9cc62e6b9c869d56cc930c3858d4d7c31f5f53f14a');`
`32`	`32`	`Filecoin.stringFree(privateKeyPtr);`
Original file line number	Diff line number	Diff line change
`@@ -21,7 +21,7 @@ func main() {`
`21`	`21`	`err := C.filecoin_signer_error_message(error);`
`22`	`22`	`fmt.Fprintln(os.Stderr, C.GoString(err))`
`23`	`23`	`} else {`
`24`		`- private_key := C.filecoin_signer_extended_key_private_key(extended_key);`
	`24`	`+ private_key := C.filecoin_signer_extended_key_private_key(extended_key, error);`
`25`	`25`	`if C.GoString(private_key) != "f15716d3b003b304b8055d9cc62e6b9c869d56cc930c3858d4d7c31f5f53f14a" {`
`26`	`26`	`panic("Bad key");`
`27`	`27`	`}`
Original file line number	Diff line number	Diff line change
`@@ -14,7 +14,7 @@ public static void main(String[] args) {`
`14`	`14`	`System.err.println(FilecoinSigner.errorMessage(error));`
`15`	`15`	`}`
`16`	`16`	`else {`
`17`		`- String privateKey = FilecoinSigner.extendedKeyPrivateKey(extendedKey);`
	`17`	`+ String privateKey = FilecoinSigner.extendedKeyPrivateKey(extendedKey, error);`
`18`	`18`	`assert privateKey.equals("f15716d3b003b304b8055d9cc62e6b9c869d56cc930c3858d4d7c31f5f53f14a");`
`19`	`19`	`}`
`20`	`20`
Original file line number	Diff line number	Diff line change
`@@ -13,7 +13,7 @@ fun main() {`
`13`	`13`	`System.err.println(FilecoinSigner.errorMessage(error))`
`14`	`14`	`}`
`15`	`15`	`else {`
`16`		`- val privateKey = FilecoinSigner.extendedKeyPrivateKey(extendedKey);`
	`16`	`+ val privateKey = FilecoinSigner.extendedKeyPrivateKey(extendedKey, error);`
`17`	`17`	`assert(privateKey == "f15716d3b003b304b8055d9cc62e6b9c869d56cc930c3858d4d7c31f5f53f14a")`
`18`	`18`	`}`
`19`	`19`
Original file line number	Diff line number	Diff line change
`@@ -11,7 +11,7 @@ if (filecoin_signer_error_code(error) != 0) {`
`11`	`11`	`fputs(err, stderr)`
`12`	`12`	`}`
`13`	`13`	`else {`
`14`		`- let private_key = filecoin_signer_extended_key_private_key(extended_key);`
	`14`	`+ let private_key = filecoin_signer_extended_key_private_key(extended_key, error);`
`15`	`15`	`assert(String(cString: private_key!) == "f15716d3b003b304b8055d9cc62e6b9c869d56cc930c3858d4d7c31f5f53f14a");`
`16`	`16`	`filecoin_signer_string_free(private_key);`
`17`	`17`	`}`