From 8ba3663b43ca260ee4760772623bc4c38a837794 Mon Sep 17 00:00:00 2001 From: Tiago Carvalho Date: Fri, 6 Dec 2024 11:01:08 +0100 Subject: [PATCH] Improve docs and runtime checks of osmosis swaps --- Cargo.lock | 1 + crates/apps_lib/src/cli.rs | 9 ++++-- crates/apps_lib/src/cli/client.rs | 2 +- crates/sdk/Cargo.toml | 1 + crates/sdk/src/args.rs | 52 +++++++++++++++++++++---------- wasm/Cargo.lock | 1 + 6 files changed, 46 insertions(+), 20 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 6ce1d2a9fc..e3c76de2e5 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5321,6 +5321,7 @@ dependencies = [ "arbitrary", "assert_matches", "async-trait", + "bech32 0.8.1", "bimap", "borsh", "borsh-ext", diff --git a/crates/apps_lib/src/cli.rs b/crates/apps_lib/src/cli.rs index 3d96082b0c..5fb8704e30 100644 --- a/crates/apps_lib/src/cli.rs +++ b/crates/apps_lib/src/cli.rs @@ -5206,13 +5206,16 @@ pub mod args { "The IBC denomination (on Osmosis) of the desired asset." ))) .arg(TARGET.def().help(wrap!( - "The address that shall receive the swapped tokens." + "The Namada address that shall receive the swapped tokens." ))) .arg(SLIPPAGE.def().help(wrap!( - "The slippage percentage as an integer between 0 and 100." + "The slippage percentage as an integer between 0 and 100. \ + Represents the maximum acceptable deviation from the \ + expected price during a trade." ))) .arg(WINDOW_SECONDS.def().help(wrap!( - "A mysterious thing that should be set to 10s." + "The time period (in seconds) over which the average \ + price is calculated." ))) .arg(LOCAL_RECOVERY_ADDR.def().help(wrap!( "An address on Osmosis from which to recover funds in \ diff --git a/crates/apps_lib/src/cli/client.rs b/crates/apps_lib/src/cli/client.rs index 258192ca48..13c6563d4d 100644 --- a/crates/apps_lib/src/cli/client.rs +++ b/crates/apps_lib/src/cli/client.rs @@ -123,7 +123,7 @@ impl CliApi { let args = args.to_sdk(&mut ctx)?; let namada = ctx.to_sdk(client, io); - let args = args.assemble(&namada).await; + let args = args.into_ibc_transfer(&namada).await?; tx::submit_ibc_transfer(&namada, args).await?; } diff --git a/crates/sdk/Cargo.toml b/crates/sdk/Cargo.toml index ccb795e9e9..2fadc694ce 100644 --- a/crates/sdk/Cargo.toml +++ b/crates/sdk/Cargo.toml @@ -91,6 +91,7 @@ namada_wallet = {path = "../wallet" } arbitrary = { workspace = true, optional = true } async-trait.workspace = true +bech32.workspace = true bimap.workspace = true borsh.workspace = true borsh-ext.workspace = true diff --git a/crates/sdk/src/args.rs b/crates/sdk/src/args.rs index 7cd4d110e8..18339fc0b2 100644 --- a/crates/sdk/src/args.rs +++ b/crates/sdk/src/args.rs @@ -26,6 +26,7 @@ use namada_tx::Memo; use serde::{Deserialize, Serialize, Serializer}; use zeroize::Zeroizing; +use crate::error::Error; use crate::eth_bridge::bridge_pool; use crate::ibc::core::host::types::identifiers::{ChannelId, PortId}; use crate::ibc::{NamadaMemo, NamadaMemoData}; @@ -498,15 +499,16 @@ impl FromStr for OsmosisPoolHop { pub struct TxOsmosisSwap { /// The IBC transfer data pub transfer: TxIbcTransfer, - /// The token we wish to receive + /// The token we wish to receive (on Osmosis) pub output_denom: String, - /// Recipient address + /// Address of the recipient on Namada pub recipient: C::Address, - /// Slippage percent + /// The maximum percentage difference allowed between the estimated and + /// actual trade price pub slippage_percent: u64, - /// TODO! Figure out what this is + /// The time period (in seconds) over which the average price is calculated pub window_seconds: u64, - /// A recovery address (on Osmosis) in case of failure + /// Recovery address (on Osmosis) in case of failure pub local_recovery_addr: String, /// The route to take through Osmosis pools pub route: Option>, @@ -514,7 +516,10 @@ pub struct TxOsmosisSwap { impl TxOsmosisSwap { /// Create an IBC transfer from the input arguments - pub async fn assemble(self, ctx: &impl Namada) -> TxIbcTransfer { + pub async fn into_ibc_transfer( + self, + ctx: &impl Namada, + ) -> crate::error::Result> { #[derive(Serialize)] struct Memo { wasm: Wasm, @@ -533,14 +538,13 @@ impl TxOsmosisSwap { #[derive(Serialize)] struct OsmosisSwap { + receiver: String, output_denom: String, slippage: Slippage, - receiver: String, - #[serde(skip_serializing_if = "Option::is_none")] - next_memo: Option, on_failed_delivery: LocalRecoveryAddr, + route: Vec, #[serde(skip_serializing_if = "Option::is_none")] - route: Option>, + next_memo: Option, } #[derive(Serialize)] @@ -582,13 +586,25 @@ impl TxOsmosisSwap { route, } = self; + // validate `local_recovery_addr` + if !bech32::decode(&local_recovery_addr) + .is_ok_and(|(hrp, _, _)| hrp == "osmo") + { + return Err(Error::Other(format!( + "Invalid Osmosis address {local_recovery_addr:?}" + ))); + } + let next_memo = transfer.ibc_memo.take().map(|memo| { serde_json::to_string(&NamadaMemo { namada: NamadaMemoData::Memo(memo), }) .unwrap() }); - let route = if route.is_none() { + + let route = if let Some(route) = route { + route + } else { query_osmosis_pool_routes( ctx, &transfer.token, @@ -597,11 +613,15 @@ impl TxOsmosisSwap { &output_denom, OSMOSIS_SQS_SERVER, ) - .await - .unwrap() + .await? .pop() - } else { - route + .ok_or_else(|| { + Error::Other(format!( + "No route found to swap {:?} of Namada's {} with Osmosis' \ + {}", + transfer.amount, transfer.token, output_denom + )) + })? }; let memo = Memo { @@ -628,7 +648,7 @@ impl TxOsmosisSwap { }; transfer.ibc_memo = Some(serde_json::to_string(&memo).unwrap()); - transfer + Ok(transfer) } } diff --git a/wasm/Cargo.lock b/wasm/Cargo.lock index f842fef93b..14edb18ae0 100644 --- a/wasm/Cargo.lock +++ b/wasm/Cargo.lock @@ -3927,6 +3927,7 @@ name = "namada_sdk" version = "0.46.0" dependencies = [ "async-trait", + "bech32 0.8.1", "bimap", "borsh", "borsh-ext",