Merge branch 'tiago/min-output-amount-from-slippage' (#4723)

* origin/tiago/min-output-amount-from-slippage:
  Changelog for #4723
  Encode Osmosis swap receivers in compat mode
  Test compat taddr serialization
  Encode taddrs in compat mode with bech32
  Improve thread join err msg
  Compute min output amount from TWAP

Author: tzemanovic

.changelog/unreleased/improvements/4723-min-output-amount-from-slippage.md

@@ -0,0 +1,3 @@
+- Improve the UX of Osmosis swaps in various ways. Moreover, start encoding the
+  MASP address with `bech32`, rather than `bech32m`, when shielding the output
+  of the trade. ([\#4723](https://github.com/anoma/namada/pull/4723))

crates/apps_lib/src/cli.rs

@@ -3611,7 +3611,8 @@ pub mod args {
     pub const NO_CONVERSIONS: ArgFlag = flag("no-conversions");
     pub const NO_EXPIRATION: ArgFlag = flag("no-expiration");
     pub const NUT: ArgFlag = flag("nut");
-    pub const OSMOSIS_REST_RPC: Arg<String> = arg("osmosis-rest-rpc");
+    pub const OSMOSIS_LCD_RPC: ArgOpt<String> = arg_opt("osmosis-lcd");
+    pub const OSMOSIS_SQS_RPC: ArgOpt<String> = arg_opt("osmosis-sqs");
     pub const OUT_FILE_PATH_OPT: ArgOpt<PathBuf> = arg_opt("out-file-path");
     pub const OUTPUT: ArgOpt<PathBuf> = arg_opt("output");
     pub const OUTPUT_DENOM: Arg<String> = arg("output-denom");
@@ -3673,7 +3674,7 @@ pub mod args {
     pub const SENDER: Arg<String> = arg("sender");
     pub const SHIELDED: ArgFlag = flag("shielded");
     pub const SHOW_IBC_TOKENS: ArgFlag = flag("show-ibc-tokens");
-    pub const SLIPPAGE: ArgOpt<f64> = arg_opt("slippage-percentage");
+    pub const SLIPPAGE: ArgOpt<Dec> = arg_opt("slippage-percentage");
     pub const SIGNING_KEYS: ArgMulti<WalletPublicKey, GlobStar> =
         arg_multi("signing-keys");
     pub const SIGNATURES: ArgMulti<PathBuf, GlobStar> = arg_multi("signatures");
@@ -3734,7 +3735,6 @@ pub mod args {
     pub const WASM_DIR: ArgOpt<PathBuf> = arg_opt("wasm-dir");
     pub const WASM_PATH: ArgOpt<PathBuf> = arg_opt("wasm-path");
     pub const WEBSITE_OPT: ArgOpt<String> = arg_opt("website");
-    pub const WINDOW_SECONDS: ArgOpt<u64> = arg_opt("window-seconds");
     pub const WITH_INDEXER: ArgOpt<String> = arg_opt("with-indexer");
     pub const WRAPPER_SIGNATURE_OPT: ArgOpt<PathBuf> = arg_opt("gas-signature");
     pub const TX_PATH: Arg<PathBuf> = arg("tx-path");
@@ -5241,49 +5241,41 @@ pub mod args {
                 slippage: self.slippage,
                 local_recovery_addr: self.local_recovery_addr,
                 route: self.route,
-                osmosis_rest_rpc: self.osmosis_rest_rpc,
+                osmosis_lcd_rpc: self.osmosis_lcd_rpc,
+                osmosis_sqs_rpc: self.osmosis_sqs_rpc,
             })
         }
     }
 
     impl Args for TxOsmosisSwap<CliTypes> {
         fn parse(matches: &ArgMatches) -> Self {
             let transfer = TxIbcTransfer::parse(matches);
-            let osmosis_rest_rpc = OSMOSIS_REST_RPC.parse(matches);
+            let osmosis_lcd_rpc = OSMOSIS_LCD_RPC.parse(matches);
+            let osmosis_sqs_rpc = OSMOSIS_SQS_RPC.parse(matches);
             let output_denom = OUTPUT_DENOM.parse(matches);
             let maybe_trans_recipient = TARGET_OPT.parse(matches);
             let maybe_shielded_recipient =
                 PAYMENT_ADDRESS_TARGET_OPT.parse(matches);
             let maybe_overflow = OVERFLOW_OPT.parse(matches);
             let slippage_percent = SLIPPAGE.parse(matches);
-            if slippage_percent
-                .is_some_and(|percent| !(0.0..=100.0).contains(&percent))
-            {
+            if slippage_percent.is_some_and(|percent| {
+                let zero = Dec::zero();
+                let hundred = Dec::new(100, 0).unwrap();
+
+                percent < zero || percent > hundred
+            }) {
                 panic!(
                     "The slippage percent must be a number between 0 and 100."
                 )
             }
-            let window_seconds = WINDOW_SECONDS.parse(matches);
             let minimum_amount = MINIMUM_AMOUNT.parse(matches);
             let slippage = minimum_amount
                 .map(|d| Slippage::MinOutputAmount(d.redenominate(0).amount()))
                 .or_else(|| {
                     Some(Slippage::Twap {
-                        slippage_percentage: slippage_percent
-                            .expect(
-                                "If a minimum amount was not provided, \
-                                 slippage-percentage and window-seconds must \
-                                 be specified.",
-                            )
-                            .to_string(),
-                        window_seconds: window_seconds.expect(
-                            "If a minimum amount was not provided, \
-                             slippage-percentage and window-seconds must be \
-                             specified.",
-                        ),
+                        slippage_percentage: slippage_percent?,
                     })
-                })
-                .unwrap();
+                });
             let local_recovery_addr = LOCAL_RECOVERY_ADDR.parse(matches);
             let route = match OSMOSIS_POOL_HOP.parse(matches) {
                 r if r.is_empty() => None,
@@ -5301,23 +5293,31 @@ pub mod args {
                 slippage,
                 local_recovery_addr,
                 route,
-                osmosis_rest_rpc,
+                osmosis_lcd_rpc,
+                osmosis_sqs_rpc,
             }
         }
 
         fn def(app: App) -> App {
             app.add_args::<TxIbcTransfer<CliTypes>>()
                 .arg(
-                    OSMOSIS_REST_RPC
+                    OSMOSIS_LCD_RPC
                         .def()
-                        .help(wrap!("A url pointing to an Osmosis REST rpc.")),
+                        .help(wrap!("URL pointing to an Osmosis lcd rpc.")),
                 )
-                .arg(OSMOSIS_POOL_HOP.def().help(wrap!(
-                    "Individual hop of the route to take through Osmosis \
-                     pools. This value takes the form \
-                     <osmosis-pool-id>:<pool-output-denom>. When unspecified, \
-                     the optimal route is queried on the fly."
-                )))
+                .arg(
+                    OSMOSIS_SQS_RPC
+                        .def()
+                        .help(wrap!("URL pointing to an Osmosis sqs rpc.")),
+                )
+                .arg(OSMOSIS_POOL_HOP.def().conflicts_with(SLIPPAGE.name).help(
+                    wrap!(
+                        "Individual hop of the route to take through Osmosis \
+                         pools. This value takes the form \
+                         <osmosis-pool-id>:<pool-output-denom>. When \
+                         unspecified, the optimal route is queried on the fly."
+                    ),
+                ))
                 .arg(OUTPUT_DENOM.def().help(wrap!(
                     "IBC trace path (on Namada) of the desired asset. This is \
                      a string of the form \
@@ -5353,34 +5353,21 @@ pub mod args {
                      of the trade. If unspecified, a disposable address is \
                      generated."
                 )))
-                .arg(SLIPPAGE.def().requires(WINDOW_SECONDS.name).help(wrap!(
+                .arg(SLIPPAGE.def().help(wrap!(
                     "Slippage percentage, as a number between 0 and 100. \
                      Represents the maximum acceptable deviation from the \
                      expected price during a trade."
                 )))
-                .arg(WINDOW_SECONDS.def().requires(SLIPPAGE.name).help(wrap!(
-                    "Time period (in seconds) over which the average price is \
-                     calculated."
-                )))
-                .arg(
-                    MINIMUM_AMOUNT
-                        .def()
-                        .conflicts_with(SLIPPAGE.name)
-                        .conflicts_with(WINDOW_SECONDS.name)
-                        .help(wrap!(
-                            "Minimum amount of target asset that the trade \
-                             should produce."
-                        )),
-                )
+                .arg(MINIMUM_AMOUNT.def().conflicts_with(SLIPPAGE.name).help(
+                    wrap!(
+                        "Minimum amount of target asset that the trade should \
+                         produce."
+                    ),
+                ))
                 .arg(LOCAL_RECOVERY_ADDR.def().help(wrap!(
                     "Address on Osmosis from which to recover funds in case \
                      of failure."
                 )))
-                .group(
-                    ArgGroup::new("slippage")
-                        .args([SLIPPAGE.name, MINIMUM_AMOUNT.name])
-                        .required(true),
-                )
                 .group(
                     ArgGroup::new("transfer-target")
                         .args([

crates/apps_lib/src/cli/client.rs

@@ -125,7 +125,48 @@ impl CliApi {
 
                         let args = args.to_sdk(&mut ctx)?;
                         let namada = ctx.to_sdk(client, io);
-                        let args = args.into_ibc_transfer(&namada).await?;
+                        let args = args
+                            .into_ibc_transfer(
+                                &namada,
+                                |_route, min_amount, quote_amount| {
+                                    use std::io::Write;
+
+                                    if let Some(quote_amount) = quote_amount {
+                                        print!(
+                                            "Minimum output is {min_amount}, \
+                                             while the quote price is \
+                                             {quote_amount}, proceed with \
+                                             trade (y/n)? "
+                                        );
+                                    } else {
+                                        print!(
+                                            "Minimum output is {min_amount}, \
+                                             proceed with trade (y/n)? "
+                                        );
+                                    }
+
+                                    std::io::stdout()
+                                        .flush()
+                                        .expect("Failed to flush stdout");
+
+                                    let line = {
+                                        let mut line = String::new();
+                                        std::io::stdin()
+                                            .read_line(&mut line)
+                                            .expect(
+                                                "Failed to read line from \
+                                                 stdin",
+                                            );
+                                        line
+                                    };
+
+                                    matches!(
+                                        line.trim(),
+                                        "y" | "Y" | "yes" | "YES" | "Yes"
+                                    )
+                                },
+                            )
+                            .await?;
 
                         tx::submit_ibc_transfer(&namada, args).await?;
                     }

crates/core/src/address.rs

@@ -278,6 +278,20 @@ impl Address {
         string_encoding::Format::encode(self)
     }
 
+    /// Encode an address in compatibility mode (i.e. with the legacy Bech32
+    /// encoding)
+    pub fn encode_compat(&self) -> String {
+        use crate::string_encoding::Format;
+
+        bech32::encode::<bech32::Bech32>(Self::HRP, self.to_bytes().as_ref())
+            .unwrap_or_else(|_| {
+                panic!(
+                    "The human-readable part {} should never cause a failure",
+                    Self::HRP
+                )
+            })
+    }
+
     /// Decode an address from Bech32m encoding
     pub fn decode(string: impl AsRef<str>) -> Result<Self> {
         string_encoding::Format::decode(string)
@@ -720,6 +734,19 @@ mod tests {
             let bytes = address.serialize_to_vec();
             assert_eq!(bytes.len(), ESTABLISHED_ADDRESS_BYTES_LEN);
         }
+
+        #[test]
+        fn test_compat_addr_decode_bech32(address in testing::arb_address()) {
+            let encoded: String = address.encode();
+            let encoded_compat: String = address.encode_compat();
+
+            assert_ne!(encoded, encoded_compat);
+
+            let decoded: Address = encoded.parse().unwrap();
+            let decoded_compat: Address = encoded_compat.parse().unwrap();
+
+            assert_eq!(decoded, decoded_compat);
+        }
     }
 }