refactor(ilp-cli): error handling

Author: Ben Striegel

Cargo.lock

@@ -9,7 +9,7 @@ repository = "https://github.com/interledger-rs/interledger-rs"
 
 [dependencies]
 clap = { version = "2.33.0", default-features = false }
-failure = { version = "0.1.6", default-features = false }
+thiserror = { version = "1.0.4", default-features = false }
 http = { version = "0.1.18", default-features = false }
 reqwest = { version = "0.9.21", default-features = false, features = ["default-tls"] }
 serde = { version = "1.0.101", default-features = false }

crates/ilp-cli/Cargo.toml

@@ -1,46 +1,32 @@
 use clap::ArgMatches;
-use failure::Fail;
 use http;
 use reqwest::{self, Client, Response};
 use std::{borrow::Cow, collections::HashMap};
 use tungstenite::{connect, handshake::client::Request};
 use url::Url;
 
-#[derive(Debug, Fail)]
+#[derive(Debug, thiserror::Error)]
 pub enum Error {
-    #[fail(display = "Usage error")]
+    // Custom errors
+    #[error("Usage error")]
     UsageErr(&'static str),
-    #[fail(display = "Error sending request: {}", _0)]
-    ClientErr(reqwest::Error),
-    #[fail(display = "Error receiving response: {}", _0)]
-    ResponseErr(String),
-    #[fail(display = "Error parsing URL protocol: {}", _0)]
-    ProtocolErr(&'static str),
-    #[fail(display = "Error altering URL scheme")]
-    SchemeErr(),
-    #[fail(display = "Error parsing URL: {}", _0)]
-    UrlErr(url::ParseError),
-    #[fail(display = "Error connecting to WebSocket host: {}", _0)]
-    WebsocketErr(tungstenite::error::Error),
-}
-
-impl From<url::ParseError> for Error {
-    fn from(error: url::ParseError) -> Self {
-        Error::UrlErr(error)
-    }
-}
-
-impl From<tungstenite::error::Error> for Error {
-    fn from(error: tungstenite::error::Error) -> Self {
-        Error::WebsocketErr(error)
-    }
+    #[error("Invalid protocol in URL: {0}")]
+    ProtocolErr(String),
+    // Foreign errors
+    #[error("Error sending HTTP request: {0}")]
+    ClientErr(#[from] reqwest::Error),
+    #[error("Error altering URL scheme")]
+    SchemeErr(()), // TODO: should be part of UrlError, see https://github.com/servo/rust-url/issues/299
+    #[error("Error parsing URL: {0}")]
+    UrlErr(#[from] url::ParseError),
+    #[error("WebSocket error: {0}")]
+    WebsocketErr(#[from] tungstenite::error::Error),
 }
 
 pub fn run(matches: &ArgMatches) -> Result<Response, Error> {
     let client = NodeClient {
         client: Client::new(),
-        // `--node` has a a default value, so will never be None
-        url: matches.value_of("node_url").unwrap(),
+        url: matches.value_of("node_url").unwrap(), // infallible unwrap
     };
 
     // Dispatch based on parsed input
@@ -92,7 +78,7 @@ impl NodeClient<'_> {
     // GET /accounts/:username/balance
     fn get_account_balance(&self, matches: &ArgMatches) -> Result<Response, Error> {
         let (auth, mut args) = extract_args(matches);
-        let user = args.remove("username").unwrap();
+        let user = args.remove("username").unwrap(); // infallible unwrap
         self.client
             .get(&format!("{}/accounts/{}/balance", self.url, user))
             .bearer_auth(auth)
@@ -143,12 +129,13 @@ impl NodeClient<'_> {
         let scheme = match url.scheme() {
             "http" => Ok("ws"),
             "https" => Ok("wss"),
-            _ => Err(Error::ProtocolErr("Unexpected protocol")),
+            s => Err(Error::ProtocolErr(format!(
+                "{} (only HTTP and HTTPS are supported)",
+                s
+            ))),
         }?;
 
-        if url.set_scheme(scheme).is_err() {
-            return Err(Error::SchemeErr());
-        };
+        url.set_scheme(scheme).map_err(Error::SchemeErr)?;
 
         let mut request: Request = url.into();
         request.add_header(
@@ -158,9 +145,7 @@ impl NodeClient<'_> {
 
         let (mut socket, _) = connect(request)?;
         loop {
-            let msg = socket
-                .read_message()
-                .expect("Could not receive WebSocket message");
+            let msg = socket.read_message()?;
             println!("{}", msg);
         }
     }
@@ -188,7 +173,7 @@ impl NodeClient<'_> {
     // PUT /accounts/:username/settings
     fn put_account_settings(&self, matches: &ArgMatches) -> Result<Response, Error> {
         let (auth, mut args) = extract_args(matches);
-        let user = args.remove("username").unwrap();
+        let user = args.remove("username").unwrap(); // infallible unwrap
         self.client
             .put(&format!("{}/accounts/{}/settings", self.url, user))
             .bearer_auth(auth)
@@ -200,7 +185,7 @@ impl NodeClient<'_> {
     // POST /accounts/:username/payments
     fn post_account_payments(&self, matches: &ArgMatches) -> Result<Response, Error> {
         let (auth, mut args) = extract_args(matches);
-        let user = args.remove("sender_username").unwrap();
+        let user = args.remove("sender_username").unwrap(); // infallible unwrap
         self.client
             .post(&format!("{}/accounts/{}/payments", self.url, user))
             .bearer_auth(&format!("{}:{}", user, auth))
@@ -276,6 +261,7 @@ impl NodeClient<'_> {
             .send()
             .map_err(Error::ClientErr)
     }
+
     /*
     {"http_endpoint": "https://rs3.xpring.dev/ilp", // ilp_over_http_url
     "passkey": "b0i3q9tbvfgek",  // ilp_over_http_outgoing_token = username:passkey
@@ -287,7 +273,6 @@ impl NodeClient<'_> {
     "payment_pointer": "$rs3.xpring.dev/accounts/user_g31tuju4/spsp"}
     routing_relation Parent
     */
-
     fn xpring_account(&self, matches: &ArgMatches) -> Result<Response, Error> {
         let (auth, cli_args) = extract_args(matches);
         // Note the Xpring API expects the asset code in lowercase
@@ -296,31 +281,17 @@ impl NodeClient<'_> {
             .client
             .get(&format!("https://xpring.io/api/accounts/{}", asset))
             .send()
-            .map_err(|err| {
-                Error::ResponseErr(format!(
-                    "Error requesting credentials from Xpring Testnet Signup API: {:?}",
-                    err
-                ))
-            })?
+            .map_err(Error::ClientErr)?
             .json()
-            .map_err(|err| {
-                Error::ResponseErr(format!(
-                    "Got unexpected response from Xpring Testnet Signup API: {:?}",
-                    err
-                ))
-            })?;
+            .map_err(Error::ClientErr)?;
         let mut args = HashMap::new();
-        let token = format!(
-            "{}:{}",
-            foreign_args.username.clone(),
-            foreign_args.passkey.clone()
-        );
-        args.insert("ilp_over_http_url", foreign_args.http_endpoint.clone());
+        let token = format!("{}:{}", foreign_args.username, foreign_args.passkey);
+        args.insert("ilp_over_http_url", foreign_args.http_endpoint);
         args.insert("ilp_over_http_outgoing_token", token.clone());
-        args.insert("ilp_over_btp_url", foreign_args.btp_endpoint.clone());
+        args.insert("ilp_over_btp_url", foreign_args.btp_endpoint);
         args.insert("ilp_over_btp_outgoing_token", token.clone());
         args.insert("asset_scale", foreign_args.asset_scale.to_string());
-        args.insert("asset_code", foreign_args.asset_code.clone());
+        args.insert("asset_code", foreign_args.asset_code);
         args.insert("username", format!("xpring_{}", asset));
         args.insert("routing_relation", String::from("Parent")); // TODO: weird behavior when deleting and re-inserting accounts with this
                                                                  // TODO should we set different parameters?
@@ -335,9 +306,9 @@ impl NodeClient<'_> {
             .send();
 
         if matches.is_present("return_testnet_credential") {
-            result.expect("Error creating account for testnet node on our local node");
+            result?;
             Ok(Response::from(
-                http::Response::builder().body(token).unwrap(),
+                http::Response::builder().body(token).unwrap(), // infallible unwrap
             ))
         } else {
             result.map_err(Error::ClientErr)

crates/ilp-cli/src/interpreter.rs

@@ -20,12 +20,12 @@ pub fn main() {
             app.get_matches_from(s.split(' '));
         }
         Err(e) => {
-            eprintln!("ILP CLI error: {}", e);
+            eprintln!("ilp-cli error: {}", e);
             exit(1);
         }
         Ok(mut response) => match response.text() {
             Err(e) => {
-                eprintln!("ILP CLI error: Failed to parse HTTP response: {}", e);
+                eprintln!("ilp-cli error: Failed to parse HTTP response: {}", e);
                 exit(1);
             }
             Ok(body) => {
@@ -35,7 +35,7 @@ pub fn main() {
                     }
                 } else {
                     eprintln!(
-                        "ILP CLI error: Unexpected response from server: {}: {}",
+                        "ilp-cli error: Unexpected response from server: {}: {}",
                         response.status(),
                         body,
                     );
@@ -55,9 +55,6 @@ pub fn main() {
 // run the interpreter in order to detect panics.
 // Conveniently this section also serves as a reference for example invocations.
 mod interface_tests {
-    use crate::interpreter;
-    use crate::parser;
-
     #[test]
     fn ilp_cli() {
         should_parse(&[
@@ -103,18 +100,6 @@ mod interface_tests {
         ]);
     }
 
-    #[test]
-    fn accounts_incoming_payments_invalid_protocol() {
-        should_parse(&[
-            "ilp-cli --node tftp://localhost:7770 accounts incoming-payments alice --auth foo",
-        ]);
-    }
-
-    #[test]
-    fn accounts_incoming_payments_invalid_host() {
-        should_parse(&["ilp-cli --node http://:7770 accounts incoming-payments alice --auth foo"]);
-    }
-
     #[test]
     fn accounts_info() {
         should_parse(&[
@@ -211,16 +196,19 @@ mod interface_tests {
     }
 
     fn should_parse(examples: &[&str]) {
+        use crate::interpreter::{run, Error};
+        use crate::parser;
+
         let mut app = parser::build();
         for example in examples {
             let parser_result = app.get_matches_from_safe_borrow(example.split(' '));
             match parser_result {
-                Err(e) => panic!("Failure while parsing command `{}`: {}", example, e),
-                Ok(matches) => {
-                    // Any unanticipated errors at this stage will result in a panic from
-                    // within the interpreter, so no need to manually check the result.
-                    let _interpreter_result = interpreter::run(&matches);
-                }
+                Err(e) => panic!("Failed to parse command `{}`: {}", example, e),
+                Ok(matches) => match run(&matches) {
+                    // Because these are interface tests, not integration tests, network errors are expected
+                    Ok(_) | Err(Error::ClientErr(_)) | Err(Error::WebsocketErr(_)) => (),
+                    Err(e) => panic!("Unexpected interpreter failure: {}", e),
+                },
             }
         }
     }