refactor(cli): Refactor RPC server and fix tests

- Use the Request trait introduced in https://github.com/UnstoppableSwap/xmr-btc-swap/pull/10 for the RPC server
- Delegate deserialization of RPC server method parameters to serde by using structs like BuyXmrArgs
- Remove `get_raw_states` RPC server method because it's not used
- Fix RPC server tests including removing test for the "log reference id" feature which was removed as part of https://github.com/UnstoppableSwap/xmr-btc-swap/pull/10
- Rename GetCurrentSwap struct to GetCurrentSwapArgs

swap/sqlx-data.json

@@ -80,30 +80,6 @@
     },
     "query": "\n        insert into peers (\n            swap_id,\n            peer_id\n            ) values (?, ?);\n        "
   },
-  "3f2bfdd2d134586ccad22171cd85a465800fc5c4fdaf191d206974e530240c87": {
-    "describe": {
-      "columns": [
-        {
-          "name": "swap_id",
-          "ordinal": 0,
-          "type_info": "Text"
-        },
-        {
-          "name": "state",
-          "ordinal": 1,
-          "type_info": "Text"
-        }
-      ],
-      "nullable": [
-        false,
-        false
-      ],
-      "parameters": {
-        "Right": 0
-      }
-    },
-    "query": "\n                SELECT swap_id, state\n                FROM swap_states\n                "
-  },
   "50a5764546f69c118fa0b64120da50f51073d36257d49768de99ff863e3511e0": {
     "describe": {
       "columns": [],

swap/src/cli/api.rs

@@ -507,7 +507,6 @@ pub mod api_test {
             Self {
                 tor_socks5_port: 9050,
                 namespace: XmrBtcNamespace::from_is_testnet(is_testnet),
-                server_address: None,
                 env_config,
                 seed: Some(seed),
                 debug,

swap/src/cli/api/request.rs

@@ -304,9 +304,9 @@ impl Request for SuspendCurrentSwapArgs {
     }
 }
 
-pub struct GetCurrentSwap;
+pub struct GetCurrentSwapArgs;
 
-impl Request for GetCurrentSwap {
+impl Request for GetCurrentSwapArgs {
     type Response = serde_json::Value;
 
     async fn request(self, ctx: Arc<Context>) -> Result<Self::Response> {
@@ -860,13 +860,6 @@ pub async fn get_history(context: Arc<Context>) -> Result<GetHistoryResponse> {
     Ok(GetHistoryResponse { swaps: vec })
 }
 
-#[tracing::instrument(fields(method = "get_raw_states"), skip(context))]
-pub async fn get_raw_states(context: Arc<Context>) -> Result<serde_json::Value> {
-    let raw_history = context.db.raw_all().await?;
-
-    Ok(json!({ "raw_states": raw_history }))
-}
-
 #[tracing::instrument(fields(method = "get_config"), skip(context))]
 pub async fn get_config(context: Arc<Context>) -> Result<serde_json::Value> {
     let data_dir_display = context.config.data_dir.display();
@@ -1164,7 +1157,7 @@ where
                         min_deposit_until_swap_will_start,
                         max_deposit_until_maximum_amount_is_reached,
                         min_bitcoin_lock_tx_fee,
-                        quote: bid_quote.clone(),
+                        quote: bid_quote,
                     },
                 );
             }

swap/src/database/sqlite.rs

@@ -6,7 +6,6 @@ use async_trait::async_trait;
 use libp2p::{Multiaddr, PeerId};
 use sqlx::sqlite::Sqlite;
 use sqlx::{Pool, SqlitePool};
-use std::collections::HashMap;
 use std::path::Path;
 use std::str::FromStr;
 use time::OffsetDateTime;
@@ -352,36 +351,6 @@ impl Database for SqliteDatabase {
 
         Ok(Some(proof))
     }
-
-    async fn raw_all(&self) -> Result<HashMap<Uuid, Vec<serde_json::Value>>> {
-        let mut conn = self.pool.acquire().await?;
-        let rows = sqlx::query!(
-            r#"
-                SELECT swap_id, state
-                FROM swap_states
-                "#
-        )
-        .fetch_all(&mut conn)
-        .await?;
-
-        let mut swaps: HashMap<Uuid, Vec<serde_json::Value>> = HashMap::new();
-
-        for row in &rows {
-            let swap_id = Uuid::from_str(&row.swap_id)?;
-            let state = serde_json::from_str(&row.state)?;
-
-            if let std::collections::hash_map::Entry::Vacant(e) = swaps.entry(swap_id) {
-                e.insert(vec![state]);
-            } else {
-                swaps
-                    .get_mut(&swap_id)
-                    .ok_or_else(|| anyhow!("Error while retrieving the swap"))?
-                    .push(state);
-            }
-        }
-
-        Ok(swaps)
-    }
 }
 
 #[cfg(test)]

swap/src/protocol.rs

@@ -11,7 +11,6 @@ use serde::{Deserialize, Serialize};
 use sha2::Sha256;
 use sigma_fun::ext::dl_secp256k1_ed25519_eq::{CrossCurveDLEQ, CrossCurveDLEQProof};
 use sigma_fun::HashTranscript;
-use std::collections::HashMap;
 use std::convert::TryInto;
 use uuid::Uuid;
 
@@ -145,7 +144,6 @@ pub trait Database {
     async fn get_state(&self, swap_id: Uuid) -> Result<State>;
     async fn get_states(&self, swap_id: Uuid) -> Result<Vec<State>>;
     async fn all(&self) -> Result<Vec<(Uuid, State)>>;
-    async fn raw_all(&self) -> Result<HashMap<Uuid, Vec<serde_json::Value>>>;
     async fn insert_buffered_transfer_proof(
         &self,
         swap_id: Uuid,

swap/src/rpc/methods.rs

@@ -1,18 +1,13 @@
 use crate::bitcoin::bitcoin_address;
 use crate::cli::api::request::{
-    get_current_swap, get_history, get_raw_states, suspend_current_swap, BalanceArgs, BuyXmrArgs,
+    BalanceArgs, BuyXmrArgs, CancelAndRefundArgs, GetCurrentSwapArgs, GetHistoryArgs,
-    CancelAndRefundArgs, GetSwapInfoArgs, ListSellersArgs, MoneroRecoveryArgs, Request,
+    GetSwapInfoArgs, ListSellersArgs, MoneroRecoveryArgs, Request, ResumeSwapArgs,
-    ResumeSwapArgs, WithdrawBtcArgs,
+    SuspendCurrentSwapArgs, WithdrawBtcArgs,
 };
 use crate::cli::api::Context;
 use crate::monero::monero_address;
-use crate::{bitcoin, monero};
 use anyhow::Result;
 use jsonrpsee::server::RpcModule;
-use libp2p::core::Multiaddr;
-use std::collections::HashMap;
-use std::str::FromStr;
-use uuid::Uuid;
 
 trait ConvertToJsonRpseeError<T> {
     fn to_jsonrpsee_result(self) -> Result<T, jsonrpsee_core::Error>;
@@ -28,209 +23,92 @@ pub fn register_modules(outer_context: Context) -> Result<RpcModule<Context>> {
     let mut module = RpcModule::new(outer_context);
 
     module.register_async_method("suspend_current_swap", |_, context| async move {
-        suspend_current_swap(context).await.to_jsonrpsee_result()
+        SuspendCurrentSwapArgs {}
+            .request(context)
+            .await
+            .to_jsonrpsee_result()
     })?;
 
     module.register_async_method("get_swap_info", |params_raw, context| async move {
-        let params: HashMap<String, serde_json::Value> = params_raw.parse()?;
+        let params: GetSwapInfoArgs = params_raw.parse()?;
 
-        let swap_id = params
+        params.request(context).await.to_jsonrpsee_result()
-            .get("swap_id")
-            .ok_or_else(|| jsonrpsee_core::Error::Custom("Does not contain swap_id".to_string()))?;
-
-        let swap_id = as_uuid(swap_id)
-            .ok_or_else(|| jsonrpsee_core::Error::Custom("Could not parse swap_id".to_string()))?;
-
-        GetSwapInfoArgs { swap_id }
-            .request(context)
-            .await
-            .to_jsonrpsee_result()
     })?;
 
     module.register_async_method("get_bitcoin_balance", |params_raw, context| async move {
-        let params: HashMap<String, serde_json::Value> = params_raw.parse()?;
+        let params: BalanceArgs = params_raw.parse()?;
 
-        let force_refresh = params
+        params.request(context).await.to_jsonrpsee_result()
-            .get("force_refresh")
-            .ok_or_else(|| {
-                jsonrpsee_core::Error::Custom("Does not contain force_refresh".to_string())
-            })?
-            .as_bool()
-            .ok_or_else(|| {
-                jsonrpsee_core::Error::Custom("force_refesh is not a boolean".to_string())
-            })?;
-
-        BalanceArgs { force_refresh }
-            .request(context)
-            .await
-            .to_jsonrpsee_result()
     })?;
 
     module.register_async_method("get_history", |_, context| async move {
-        get_history(context).await.to_jsonrpsee_result()
+        GetHistoryArgs {}
-    })?;
+            .request(context)
-
+            .await
-    module.register_async_method("get_raw_states", |_, context| async move {
+            .to_jsonrpsee_result()
-        get_raw_states(context).await.to_jsonrpsee_result()
     })?;
 
     module.register_async_method("resume_swap", |params_raw, context| async move {
-        let params: HashMap<String, serde_json::Value> = params_raw.parse()?;
+        let params: ResumeSwapArgs = params_raw.parse()?;
 
-        let swap_id = params
+        params.request(context).await.to_jsonrpsee_result()
-            .get("swap_id")
-            .ok_or_else(|| jsonrpsee_core::Error::Custom("Does not contain swap_id".to_string()))?;
-
-        let swap_id = as_uuid(swap_id)
-            .ok_or_else(|| jsonrpsee_core::Error::Custom("Could not parse swap_id".to_string()))?;
-
-        ResumeSwapArgs { swap_id }
-            .request(context)
-            .await
-            .to_jsonrpsee_result()
     })?;
 
     module.register_async_method("cancel_refund_swap", |params_raw, context| async move {
-        let params: HashMap<String, serde_json::Value> = params_raw.parse()?;
+        let params: CancelAndRefundArgs = params_raw.parse()?;
 
-        let swap_id = params
+        params.request(context).await.to_jsonrpsee_result()
-            .get("swap_id")
-            .ok_or_else(|| jsonrpsee_core::Error::Custom("Does not contain swap_id".to_string()))?;
-
-        let swap_id = as_uuid(swap_id)
-            .ok_or_else(|| jsonrpsee_core::Error::Custom("Could not parse swap_id".to_string()))?;
-
-        CancelAndRefundArgs { swap_id }
-            .request(context)
-            .await
-            .to_jsonrpsee_result()
     })?;
 
     module.register_async_method(
         "get_monero_recovery_info",
         |params_raw, context| async move {
-            let params: HashMap<String, serde_json::Value> = params_raw.parse()?;
+            let params: MoneroRecoveryArgs = params_raw.parse()?;
 
-            let swap_id = params.get("swap_id").ok_or_else(|| {
+            params.request(context).await.to_jsonrpsee_result()
-                jsonrpsee_core::Error::Custom("Does not contain swap_id".to_string())
-            })?;
-
-            let swap_id = as_uuid(swap_id).ok_or_else(|| {
-                jsonrpsee_core::Error::Custom("Could not parse swap_id".to_string())
-            })?;
-
-            MoneroRecoveryArgs { swap_id }
-                .request(context)
-                .await
-                .to_jsonrpsee_result()
         },
     )?;
 
     module.register_async_method("withdraw_btc", |params_raw, context| async move {
-        let params: HashMap<String, String> = params_raw.parse()?;
+        let mut params: WithdrawBtcArgs = params_raw.parse()?;
 
-        let amount = if let Some(amount_str) = params.get("amount") {
+        params.address =
-            Some(
+            bitcoin_address::validate(params.address, context.config.env_config.bitcoin_network)
-                ::bitcoin::Amount::from_str_in(amount_str, ::bitcoin::Denomination::Bitcoin)
+                .to_jsonrpsee_result()?;
-                    .map_err(|_| {
-                        jsonrpsee_core::Error::Custom("Unable to parse amount".to_string())
-                    })?,
-            )
-        } else {
-            None
-        };
 
-        let withdraw_address =
+        params.request(context).await.to_jsonrpsee_result()
-            bitcoin::Address::from_str(params.get("address").ok_or_else(|| {
-                jsonrpsee_core::Error::Custom("Does not contain address".to_string())
-            })?)
-            .map_err(|err| jsonrpsee_core::Error::Custom(err.to_string()))?;
-        let withdraw_address =
-            bitcoin_address::validate(withdraw_address, context.config.env_config.bitcoin_network)?;
-
-        WithdrawBtcArgs {
-            amount,
-            address: withdraw_address,
-        }
-        .request(context)
-        .await
-        .to_jsonrpsee_result()
     })?;
 
     module.register_async_method("buy_xmr", |params_raw, context| async move {
-        let params: HashMap<String, String> = params_raw.parse()?;
+        let mut params: BuyXmrArgs = params_raw.parse()?;
 
-        let bitcoin_change_address =
+        params.bitcoin_change_address = bitcoin_address::validate(
-            bitcoin::Address::from_str(params.get("bitcoin_change_address").ok_or_else(|| {
+            params.bitcoin_change_address,
-                jsonrpsee_core::Error::Custom("Does not contain bitcoin_change_address".to_string())
-            })?)
-            .map_err(|err| jsonrpsee_core::Error::Custom(err.to_string()))?;
-
-        let bitcoin_change_address = bitcoin_address::validate(
-            bitcoin_change_address,
             context.config.env_config.bitcoin_network,
-        )?;
+        )
+        .to_jsonrpsee_result()?;
 
-        let monero_receive_address =
+        params.monero_receive_address = monero_address::validate(
-            monero::Address::from_str(params.get("monero_receive_address").ok_or_else(|| {
+            params.monero_receive_address,
-                jsonrpsee_core::Error::Custom("Does not contain monero_receiveaddress".to_string())
-            })?)
-            .map_err(|err| jsonrpsee_core::Error::Custom(err.to_string()))?;
-
-        let monero_receive_address = monero_address::validate(
-            monero_receive_address,
             context.config.env_config.monero_network,
-        )?;
+        )
+        .to_jsonrpsee_result()?;
 
-        let seller =
+        params.request(context).await.to_jsonrpsee_result()
-            Multiaddr::from_str(params.get("seller").ok_or_else(|| {
-                jsonrpsee_core::Error::Custom("Does not contain seller".to_string())
-            })?)
-            .map_err(|err| jsonrpsee_core::Error::Custom(err.to_string()))?;
-
-        BuyXmrArgs {
-            seller,
-            bitcoin_change_address,
-            monero_receive_address,
-        }
-        .request(context)
-        .await
-        .to_jsonrpsee_result()
     })?;
 
     module.register_async_method("list_sellers", |params_raw, context| async move {
-        let params: HashMap<String, serde_json::Value> = params_raw.parse()?;
+        let params: ListSellersArgs = params_raw.parse()?;
 
-        let rendezvous_point = params.get("rendezvous_point").ok_or_else(|| {
+        params.request(context).await.to_jsonrpsee_result()
-            jsonrpsee_core::Error::Custom("Does not contain rendezvous_point".to_string())
-        })?;
-
-        let rendezvous_point = rendezvous_point
-            .as_str()
-            .and_then(|addr_str| Multiaddr::from_str(addr_str).ok())
-            .ok_or_else(|| {
-                jsonrpsee_core::Error::Custom("Could not parse valid multiaddr".to_string())
-            })?;
-
-        ListSellersArgs {
-            rendezvous_point: rendezvous_point.clone(),
-        }
-        .request(context)
-        .await
-        .to_jsonrpsee_result()
     })?;
 
     module.register_async_method("get_current_swap", |_, context| async move {
-        get_current_swap(context).await.to_jsonrpsee_result()
+        GetCurrentSwapArgs {}
+            .request(context)
+            .await
+            .to_jsonrpsee_result()
     })?;
 
     Ok(module)
 }
-
-fn as_uuid(json_value: &serde_json::Value) -> Option<Uuid> {
-    if let Some(uuid_str) = json_value.as_str() {
-        Uuid::parse_str(uuid_str).ok()
-    } else {
-        None
-    }
-}

swap/tests/rpc.rs

@@ -101,23 +101,17 @@ mod test {
 
             let (client, _, _) = setup_daemon(harness_ctx).await;
 
-            let response: HashMap<String, Vec<(Uuid, String)>> = client
+            let response: HashMap<String, Vec<HashMap<String, String>>> = client
                 .request("get_history", ObjectParams::new())
                 .await
                 .unwrap();
-            let swaps: Vec<(Uuid, String)> = vec![(bob_swap_id, "btc is locked".to_string())];
 
-            assert_eq!(response, HashMap::from([("swaps".to_string(), swaps)]));
+            let swaps: Vec<HashMap<String, String>> = vec![HashMap::from([
+                ("swap_id".to_string(), bob_swap_id.to_string()),
+                ("state".to_string(), "btc is locked".to_string()),
+            ])];
 
-            let response: HashMap<String, HashMap<Uuid, Vec<Value>>> = client
+            assert_eq!(response.get("swaps").unwrap(), &swaps);
-                .request("get_raw_states", ObjectParams::new())
-                .await
-                .unwrap();
-
-            let response_raw_states = response.get("raw_states").unwrap();
-
-            assert!(response_raw_states.contains_key(&bob_swap_id));
-            assert_eq!(response_raw_states.get(&bob_swap_id).unwrap().len(), 2);
 
             let mut params = ObjectParams::new();
             params.insert("swap_id", bob_swap_id).unwrap();
@@ -128,27 +122,27 @@ mod test {
             assert_has_keys_hashmap(
                 &response,
                 &[
-                    "txRefundFee",
+                    "tx_refund_fee",
-                    "swapId",
+                    "swap_id",
-                    "cancelTimelock",
+                    "cancel_timelock",
                     "timelock",
-                    "punishTimelock",
+                    "punish_timelock",
-                    "stateName",
+                    "state_name",
-                    "btcAmount",
+                    "btc_amount",
-                    "startDate",
+                    "start_date",
-                    "btcRefundAddress",
+                    "btc_refund_address",
-                    "txCancelFee",
+                    "tx_cancel_fee",
-                    "xmrAmount",
+                    "xmr_amount",
                     "completed",
-                    "txLockId",
+                    "tx_lock_id",
                     "seller",
                 ],
             );
 
             // Assert specific fields
-            assert_eq!(response.get("swapId").unwrap(), &bob_swap_id.to_string());
+            assert_eq!(response.get("swap_id").unwrap(), &bob_swap_id.to_string());
             assert_eq!(
-                response.get("stateName").unwrap(),
+                response.get("state_name").unwrap(),
                 &"btc is locked".to_string()
             );
             assert_eq!(response.get("completed").unwrap(), &Value::Bool(false));
@@ -159,7 +153,7 @@ mod test {
                 .expect("Field 'seller' is missing from response")
                 .as_object()
                 .expect("'seller' is not an object");
-            assert_has_keys_serde(seller, &["peerId"]);
+            assert_has_keys_serde(seller, &["peer_id"]);
 
             // Check timelock object, nested 'None' object, and blocks_left
             let timelock = response
@@ -167,12 +161,20 @@ mod test {
                 .expect("Field 'timelock' is missing from response")
                 .as_object()
                 .expect("'timelock' is not an object");
-            let none_obj = timelock
+            let timelock_type = timelock
-                .get("None")
+                .get("type")
-                .expect("Field 'None' is missing from 'timelock'")
+                .expect("Field 'type' is missing from 'timelock'")
+                .as_str()
+                .expect("'type' is not a string");
+
+            assert_eq!(timelock_type, "None");
+
+            let timelock_content = timelock
+                .get("content")
+                .expect("Field 'content' is missing from 'None'")
                 .as_object()
-                .expect("'None' is not an object in 'timelock'");
+                .expect("'content' is not an object");
-            let blocks_left = none_obj
+            let blocks_left = timelock_content
                 .get("blocks_left")
                 .expect("Field 'blocks_left' is missing from 'None'")
                 .as_i64()
@@ -198,29 +200,29 @@ mod test {
             let (change_address, receive_address) =
                 harness_ctx.bob_params.get_change_receive_addresses().await;
 
-            let (client, writer, _) = setup_daemon(harness_ctx).await;
+            let (client, _, _) = setup_daemon(harness_ctx).await;
             assert!(client.is_connected());
 
             let mut params = ObjectParams::new();
 
             params.insert("force_refresh", false).unwrap();
-            let response: HashMap<String, i32> = client
+            let response: HashMap<String, i32> =
-                .request("get_bitcoin_balance", params)
+                client.request("get_bitcoin_balance", params).await.unwrap();
-                .await
-                .unwrap();
 
             assert_eq!(response, HashMap::from([("balance".to_string(), 10000000)]));
 
+            // TODO: Renable this test once the "log reference id" feature has been added again. The feature was removed as part of this PR:
+            // https://github.com/UnstoppableSwap/xmr-btc-swap/pull/10
+            //
+            // let mut params = ObjectParams::new();
+            // params.insert("log_reference_id", "test_ref_id").unwrap();
+            // params.insert("force_refresh", false).unwrap();
 
-            let mut params = ObjectParams::new();
+            // let _: HashMap<String, i32> = client.request("get_bitcoin_balance", params).await.unwrap();
-            params.insert("log_reference_id", "test_ref_id").unwrap();
-            params.insert("force_refresh", false).unwrap();
 
-            let _: HashMap<String, i32> = client.request("get_bitcoin_balance", params).await.unwrap();
+            // assert!(writer.captured().contains(
-
+            //     r#"method{method_name="Balance" log_reference_id="\"test_ref_id\""}: swap::api::request: Current Bitcoin balance as of last sync balance=0.1 BTC"#
-            assert!(writer.captured().contains(
+            // ));
-                r#"method{method_name="Balance" log_reference_id="\"test_ref_id\""}: swap::api::request: Current Bitcoin balance as of last sync balance=0.1 BTC"#
-            ));
 
             for method in ["get_swap_info", "resume_swap", "cancel_refund_swap"].iter() {
                 let mut params = ObjectParams::new();
@@ -274,20 +276,15 @@ mod test {
             response.expect_err("Expected an error when amount is 0");
 
             let mut params = ObjectParams::new();
-            params
+            params.insert("address", BITCOIN_ADDR).unwrap();
-                .insert("address", BITCOIN_ADDR)
+            params.insert("amount", 1000000).unwrap();
-                .unwrap();
-            params.insert("amount", "0.01").unwrap();
             let response: HashMap<String, Value> = client
                 .request("withdraw_btc", params)
                 .await
                 .expect("Expected a valid response");
 
-            assert_has_keys_hashmap(&response, &["signed_tx", "amount", "txid"]);
+            assert_has_keys_hashmap(&response, &["amount", "txid"]);
-            assert_eq!(
+            assert_eq!(response.get("amount").unwrap().as_u64().unwrap(), 1_000_000);
-                response.get("amount").unwrap().as_u64().unwrap(),
-                1_000_000
-            );
 
             let params = ObjectParams::new();
             let response: Result<HashMap<String, String>, _> =
@@ -347,7 +344,6 @@ mod test {
                 client.request("buy_xmr", params).await;
             response.expect_err("Expected an error when monero_receive_address is malformed");
 
-
             let mut params = ObjectParams::new();
             params
                 .insert("bitcoin_change_address", BITCOIN_ADDR)
@@ -379,7 +375,7 @@ mod test {
                 .await
                 .expect("Expected a HashMap, got an error");
 
-            assert_has_keys_hashmap(&response, &["swapId"]);
+            assert_has_keys_hashmap(&response, &["swap_id"]);
 
             Ok(())
         })
@@ -413,7 +409,7 @@ mod test {
                 .unwrap();
             assert_eq!(
                 response,
-                HashMap::from([("swapId".to_string(), SWAP_ID.to_string())])
+                HashMap::from([("swap_id".to_string(), SWAP_ID.to_string())])
             );
 
             cloned_ctx