Wrap the Monero wallet client in a Mutex

In order to ensure that we can atomically generate_from_keys and then reload a wallet,
we have to wrap the client of the monero wallet RPC inside a mutex.
When introducing the Mutex I noticed that several inner RPC calls were leaking to the
swap crate monero wallet. As this is a violation of boundaries I introduced the traits
`GetAddress`, `WalletBlockHeight` and `Refresh`.

Note that the monero wallet could potentially know its own public view key and
public spend key. If we refactor the wallet to include this information upon wallet
creation we can also generate addresses using `monero::Address::standard`.

swap/src/bin/asb.rs

@@ -31,7 +31,7 @@ use swap::{
     execution_params::GetExecutionParams,
     fs::default_config_path,
     monero,
-    monero::{Amount, CreateWallet, OpenWallet},
+    monero::{Amount, CreateWallet, GetAddress, OpenWallet},
     protocol::alice::EventLoop,
     seed::Seed,
     trace::init_tracing,
@@ -177,7 +177,7 @@ async fn init_wallets(
 
     let balance = monero_wallet.get_balance().await?;
     if balance == Amount::ZERO {
-        let deposit_address = monero_wallet.inner.get_address(0).await?.address;
+        let deposit_address = monero_wallet.get_main_address().await?;
         warn!(
             "The Monero balance is 0, make sure to deposit funds at: {}",
             deposit_address

swap/src/bin/swap_cli.rs

@@ -30,7 +30,7 @@ use swap::{
     execution_params::GetExecutionParams,
     fs::default_config_path,
     monero,
-    monero::{CreateWallet, OpenWallet},
+    monero::{CreateWallet, OpenWallet, WalletBlockHeight},
     protocol::{
         bob,
         bob::{cancel::CancelError, Builder},
@@ -316,7 +316,7 @@ async fn init_wallets(
         );
     }
 
-    let _test_wallet_connection = monero_wallet.inner.block_height().await?;
+    let _test_wallet_connection = monero_wallet.block_height().await?;
     info!("The Monero wallet RPC is set up correctly!");
 
     Ok((bitcoin_wallet, monero_wallet))

swap/src/monero.rs

@@ -8,6 +8,8 @@ use crate::bitcoin;
 use ::bitcoin::hashes::core::fmt::Formatter;
 use anyhow::Result;
 use async_trait::async_trait;
+use monero::Address;
+use monero_rpc::wallet::{BlockHeight, Refreshed};
 use rand::{CryptoRng, RngCore};
 use rust_decimal::{
     prelude::{FromPrimitive, ToPrimitive},
@@ -228,6 +230,21 @@ pub trait CreateWallet {
     async fn create_wallet(&self, file_name: &str) -> Result<()>;
 }
 
+#[async_trait]
+pub trait WalletBlockHeight {
+    async fn block_height(&self) -> Result<BlockHeight>;
+}
+
+#[async_trait]
+pub trait GetAddress {
+    async fn get_main_address(&self) -> Result<Address>;
+}
+
+#[async_trait]
+pub trait Refresh {
+    async fn refresh(&self) -> Result<Refreshed>;
+}
+
 #[derive(thiserror::Error, Debug, Clone, PartialEq)]
 #[error("Overflow, cannot convert {0} to u64")]
 pub struct OverflowError(pub String);

swap/src/monero/wallet.rs

@@ -1,38 +1,43 @@
 use crate::monero::{
-    Amount, CreateWallet, CreateWalletForOutput, InsufficientFunds, OpenWallet, PrivateViewKey,
+    Amount, CreateWallet, CreateWalletForOutput, GetAddress, InsufficientFunds, OpenWallet,
-    PublicViewKey, Transfer, TransferProof, TxHash, WatchForTransfer,
+    PrivateViewKey, PublicViewKey, Refresh, Transfer, TransferProof, TxHash, WalletBlockHeight,
+    WatchForTransfer,
 };
 use ::monero::{Address, Network, PrivateKey, PublicKey};
 use anyhow::Result;
 use async_trait::async_trait;
 use backoff::{backoff::Constant as ConstantBackoff, future::retry};
 use bitcoin::hashes::core::sync::atomic::AtomicU32;
-use monero_rpc::wallet;
+use monero_rpc::{
+    wallet,
+    wallet::{BlockHeight, Refreshed},
+};
 use std::{
     str::FromStr,
     sync::{atomic::Ordering, Arc},
     time::Duration,
 };
+use tokio::sync::Mutex;
 use tracing::info;
 use url::Url;
 
 #[derive(Debug)]
 pub struct Wallet {
-    pub inner: wallet::Client,
+    pub inner: Mutex<wallet::Client>,
     pub network: Network,
 }
 
 impl Wallet {
     pub fn new(url: Url, network: Network) -> Self {
         Self {
-            inner: wallet::Client::new(url),
+            inner: Mutex::new(wallet::Client::new(url)),
             network,
         }
     }
 
     /// Get the balance of the primary account.
     pub async fn get_balance(&self) -> Result<Amount> {
-        let amount = self.inner.get_balance(0).await?;
+        let amount = self.inner.lock().await.get_balance(0).await?;
 
         Ok(Amount::from_piconero(amount))
     }
@@ -51,6 +56,8 @@ impl Transfer for Wallet {
 
         let res = self
             .inner
+            .lock()
+            .await
             .transfer(0, amount.as_piconero(), &destination_address.to_string())
             .await?;
 
@@ -82,6 +89,8 @@ impl CreateWalletForOutput for Wallet {
 
         let _ = self
             .inner
+            .lock()
+            .await
             .generate_from_keys(
                 &address.to_string(),
                 &private_spend_key.to_string(),
@@ -97,7 +106,7 @@ impl CreateWalletForOutput for Wallet {
 #[async_trait]
 impl OpenWallet for Wallet {
     async fn open_wallet(&self, file_name: &str) -> Result<()> {
-        self.inner.open_wallet(file_name).await?;
+        self.inner.lock().await.open_wallet(file_name).await?;
         Ok(())
     }
 }
@@ -105,7 +114,7 @@ impl OpenWallet for Wallet {
 #[async_trait]
 impl CreateWallet for Wallet {
     async fn create_wallet(&self, file_name: &str) -> Result<()> {
-        self.inner.create_wallet(file_name).await?;
+        self.inner.lock().await.create_wallet(file_name).await?;
         Ok(())
     }
 }
@@ -129,7 +138,6 @@ impl WatchForTransfer for Wallet {
         }
 
         let address = Address::standard(self.network, public_spend_key, public_view_key.into());
-        let wallet = self.inner.clone();
 
         let confirmations = Arc::new(AtomicU32::new(0u32));
 
@@ -137,7 +145,10 @@ impl WatchForTransfer for Wallet {
             // NOTE: Currently, this is conflicting IO errors with the transaction not being
             // in the blockchain yet, or not having enough confirmations on it. All these
             // errors warrant a retry, but the strategy should probably differ per case
-            let proof = wallet
+            let proof = self
+                .inner
+                .lock()
+                .await
                 .check_tx_key(
                     &String::from(transfer_proof.tx_hash()),
                     &transfer_proof.tx_key().to_string(),
@@ -176,3 +187,25 @@ impl WatchForTransfer for Wallet {
         Ok(())
     }
 }
+
+#[async_trait]
+impl WalletBlockHeight for Wallet {
+    async fn block_height(&self) -> Result<BlockHeight> {
+        self.inner.lock().await.block_height().await
+    }
+}
+
+#[async_trait]
+impl GetAddress for Wallet {
+    async fn get_main_address(&self) -> Result<Address> {
+        let address = self.inner.lock().await.get_address(0).await?;
+        Ok(Address::from_str(address.address.as_str())?)
+    }
+}
+
+#[async_trait]
+impl Refresh for Wallet {
+    async fn refresh(&self) -> Result<Refreshed> {
+        self.inner.lock().await.refresh().await
+    }
+}

swap/src/protocol/bob/swap.rs

@@ -4,7 +4,7 @@ use crate::{
     database::{Database, Swap},
     execution_params::ExecutionParams,
     monero,
-    monero::InsufficientFunds,
+    monero::{InsufficientFunds, WalletBlockHeight},
     protocol::bob::{self, event_loop::EventLoopHandle, state::*, QuoteRequest},
 };
 use anyhow::{bail, Result};
@@ -132,8 +132,7 @@ async fn run_until_internal(
                     // TODO: This can be optimized further by extracting the block height when
                     //  tx-lock was included. However, scanning a few more blocks won't do any harm
                     //  and is simpler.
-                    let monero_wallet_restore_blockheight =
+                    let monero_wallet_restore_blockheight = monero_wallet.block_height().await?;
-                        monero_wallet.inner.block_height().await?;
 
                     select! {
                         transfer_proof = transfer_proof_watcher => {

swap/tests/testutils/mod.rs

@@ -21,12 +21,17 @@ use swap::{
     execution_params,
     execution_params::{ExecutionParams, GetExecutionParams},
     monero,
+    monero::Refresh,
     protocol::{alice, alice::AliceState, bob, bob::BobState},
     seed::Seed,
 };
 use tempfile::tempdir;
 use testcontainers::{clients::Cli, Container, Docker, RunArgs};
-use tokio::{sync::mpsc, task::JoinHandle, time::interval};
+use tokio::{
+    sync::{mpsc, Mutex},
+    task::JoinHandle,
+    time::interval,
+};
 use tracing::dispatcher::DefaultGuard;
 use tracing_log::LogTracer;
 use url::Url;
@@ -168,12 +173,7 @@ impl TestContext {
         assert_eq!(btc_balance_after_swap, self.alice_starting_balances.btc);
 
         // Ensure that Alice's balance is refreshed as we use a newly created wallet
-        self.alice_monero_wallet
+        self.alice_monero_wallet.as_ref().refresh().await.unwrap();
-            .as_ref()
-            .inner
-            .refresh()
-            .await
-            .unwrap();
         let xmr_balance_after_swap = self
             .alice_monero_wallet
             .as_ref()
@@ -232,12 +232,7 @@ impl TestContext {
         );
 
         // Ensure that Bob's balance is refreshed as we use a newly created wallet
-        self.bob_monero_wallet
+        self.bob_monero_wallet.as_ref().refresh().await.unwrap();
-            .as_ref()
-            .inner
-            .refresh()
-            .await
-            .unwrap();
         let xmr_balance_after_swap = self.bob_monero_wallet.as_ref().get_balance().await.unwrap();
         assert_eq!(
             xmr_balance_after_swap,
@@ -595,7 +590,7 @@ async fn init_test_wallets(
         .unwrap();
 
     let xmr_wallet = swap::monero::Wallet {
-        inner: monero.wallet(name).unwrap().client(),
+        inner: Mutex::new(monero.wallet(name).unwrap().client()),
         network: monero::Network::default(),
     };