!fixup address review comments

dmkozh · dmkozh · commit c5077b8d19dc · 2025-03-31T21:14:28.000-04:00
diff --git a/soroban-sdk/src/address.rs b/soroban-sdk/src/address.rs
@@ -55,7 +55,11 @@ impl Debug for Address {
                         let strkey = Strkey::Contract(Contract(contract_id.0));
                         write!(f, "Contract({})", strkey.to_string())?;
                     }
-                    _ => return Err(core::fmt::Error),
+                    ScAddress::MuxedAccount(_)
+                    | ScAddress::ClaimableBalance(_)
+                    | ScAddress::LiquidityPool(_) => {
+                        return Err(core::fmt::Error);
+                    }
                 }
             } else {
                 return Err(core::fmt::Error);
diff --git a/soroban-sdk/src/env.rs b/soroban-sdk/src/env.rs
@@ -354,27 +354,6 @@ impl Env {
         internal::Env::require_auth(self, address.to_object()).unwrap_infallible();
     }
 
-    #[doc(hidden)]
-    pub(crate) fn get_address_from_muxed_address(
-        &self,
-        muxed_address: MuxedAddressObject,
-    ) -> Address {
-        Address::try_from_val(
-            self,
-            &internal::Env::get_address_from_muxed_address(self, muxed_address).unwrap_infallible(),
-        )
-        .unwrap_infallible()
-    }
-
-    #[doc(hidden)]
-    pub(crate) fn get_id_from_muxed_address(&self, muxed_address: MuxedAddressObject) -> u64 {
-        u64::try_from_val(
-            self,
-            &internal::Env::get_id_from_muxed_address(self, muxed_address).unwrap_infallible(),
-        )
-        .unwrap()
-    }
-
     /// Invokes a function of a contract that is registered in the [Env].
     ///
     /// # Panics
diff --git a/soroban-sdk/src/muxed_address.rs b/soroban-sdk/src/muxed_address.rs
@@ -4,7 +4,7 @@ use super::{
     env::internal::{AddressObject, Env as _, MuxedAddressObject, Tag},
     ConversionError, Env, TryFromVal, TryIntoVal, Val,
 };
-use crate::{unwrap::UnwrapInfallible, Address};
+use crate::{env::internal, unwrap::UnwrapInfallible, Address};
 
 #[cfg(not(target_family = "wasm"))]
 use crate::env::internal::xdr::{ScAddress, ScVal};
@@ -20,15 +20,15 @@ enum AddressObjectWrapper {
 /// and can be used for representing the 'virtual' accounts that allows for
 /// managing multiple balances off-chain with only a single on-chain balance
 /// entry. The address part can be used as a regular `Address`, and the id
-/// part can be used only in the events for the off-chain processing.
+/// part should be used only in the events for the off-chain processing.
 ///
 /// This type is only necessary in a few special cases, such as token transfers
 /// that support non-custodial accounts (e.g. for the exchange support). Prefer
 /// using the regular `Address` type unless multiplexing support is necessary.
 ///
 /// This type is compatible with `Address` at the contract interface level, i.e.
 /// if a contract accepts `MuxedAddress` as an input, then its callers may still
-/// pass `Address` into the call and nothing will break. This means that if a
+/// pass `Address` into the call successfully. This means that if a
 /// contract has upgraded its interface to switch from `Address` argument to
 /// `MuxedAddress` argument, it won't break any of its existing clients.
 ///
@@ -178,9 +178,12 @@ impl MuxedAddress {
             AddressObjectWrapper::Address(address_object) => {
                 Address::try_from_val(&self.env, address_object).unwrap_infallible()
             }
-            AddressObjectWrapper::MuxedAddress(muxed_address_object) => self
-                .env
-                .get_address_from_muxed_address(*muxed_address_object),
+            AddressObjectWrapper::MuxedAddress(muxed_address_object) => Address::try_from_val(
+                &self.env,
+                &internal::Env::get_address_from_muxed_address(&self.env, *muxed_address_object)
+                    .unwrap_infallible(),
+            )
+            .unwrap_infallible(),
         }
     }
 
@@ -194,9 +197,14 @@ impl MuxedAddress {
     pub fn id(&self) -> Option<u64> {
         match &self.obj {
             AddressObjectWrapper::Address(_) => None,
-            AddressObjectWrapper::MuxedAddress(muxed_address_object) => {
-                Some(self.env.get_id_from_muxed_address(*muxed_address_object))
-            }
+            AddressObjectWrapper::MuxedAddress(muxed_address_object) => Some(
+                u64::try_from_val(
+                    &self.env,
+                    &internal::Env::get_id_from_muxed_address(&self.env, *muxed_address_object)
+                        .unwrap_infallible(),
+                )
+                .unwrap(),
+            ),
         }
     }
 
@@ -251,7 +259,9 @@ impl TryFromVal<Env, ScVal> for MuxedAddress {
                 ScAddress::MuxedAccount(_) => Ok(MuxedAddressObject::try_from_val(env, &v)?
                     .try_into_val(env)
                     .unwrap_infallible()),
-                _ => panic!("unsupported ScAddress type"),
+                ScAddress::ClaimableBalance(_) | ScAddress::LiquidityPool(_) => {
+                    panic!("unsupported ScAddress type")
+                }
             },
             _ => panic!("incorrect scval type"),
         }
@@ -269,13 +279,32 @@ impl TryFromVal<Env, ScAddress> for MuxedAddress {
 #[cfg(any(test, feature = "testutils"))]
 #[cfg_attr(feature = "docs", doc(cfg(feature = "testutils")))]
 impl crate::testutils::MuxedAddress for MuxedAddress {
-    fn from_account_id(env: &Env, account_key: &[u8; 32], id: u64) -> crate::MuxedAddress {
+    fn generate(env: &Env, id: u64) -> crate::MuxedAddress {
         let sc_val = ScVal::Address(crate::env::internal::xdr::ScAddress::MuxedAccount(
             crate::env::internal::xdr::MuxedEd25519Account {
-                ed25519: crate::env::internal::xdr::Uint256(account_key.clone()),
+                ed25519: crate::env::internal::xdr::Uint256(
+                    env.with_generator(|mut g| g.address()),
+                ),
                 id,
             },
         ));
         sc_val.try_into_val(env).unwrap()
     }
+
+    fn clone_with_id(&self, new_id: u64) -> crate::MuxedAddress {
+        let mut sc_val = ScVal::try_from_val(&self.env, self.as_val()).unwrap();
+        match &mut sc_val {
+            ScVal::Address(address) => match address {
+                ScAddress::MuxedAccount(muxed_account) => {
+                    muxed_account.id = new_id;
+                }
+                ScAddress::Account(_)
+                | ScAddress::Contract(_)
+                | ScAddress::ClaimableBalance(_)
+                | ScAddress::LiquidityPool(_) => unreachable!(),
+            },
+            _ => unreachable!(),
+        }
+        sc_val.try_into_val(&self.env).unwrap()
+    }
 }
diff --git a/soroban-sdk/src/tests/muxed_address.rs b/soroban-sdk/src/tests/muxed_address.rs
@@ -1,3 +1,5 @@
+use core::u64;
+
 use crate::testutils::MuxedAddress as _;
 use crate::{
     env::xdr::{AccountId, ScAddress, Uint256},
@@ -29,14 +31,20 @@ fn test_contract_address_to_muxed_address_conversion() {
 #[test]
 fn test_muxed_address_component_getters() {
     let env = Env::default();
-    let muxed_address = MuxedAddress::from_account_id(&env, &[1; 32], 123_456);
+    let muxed_address = MuxedAddress::generate(&env, 123_456);
+    let mut expected_id = [0_u8; 32];
+    expected_id[31] = 1;
     let expected_address = Address::try_from_val(
         &env,
         &ScAddress::Account(AccountId(
-            soroban_env_host::xdr::PublicKey::PublicKeyTypeEd25519(Uint256([1; 32])),
+            soroban_env_host::xdr::PublicKey::PublicKeyTypeEd25519(Uint256(expected_id)),
         )),
     )
     .unwrap();
     assert_eq!(muxed_address.address(), expected_address);
     assert_eq!(muxed_address.id(), Some(123_456));
+
+    let muxed_address_with_another_id = muxed_address.clone_with_id(u64::MAX);
+    assert_eq!(muxed_address_with_another_id.address(), expected_address);
+    assert_eq!(muxed_address_with_another_id.id(), Some(u64::MAX));
 }
diff --git a/soroban-sdk/src/testutils.rs b/soroban-sdk/src/testutils.rs
@@ -439,9 +439,16 @@ pub trait Address {
 }
 
 pub trait MuxedAddress {
-    /// Create a new MuxedAddress from a provided account public key and
+    /// Create a new MuxedAddress with arbitrary `Address` part and provided
     /// multiplexing identifier.
-    fn from_account_id(env: &Env, account_key: &[u8; 32], id: u64) -> crate::MuxedAddress;
+    /// 
+    /// Note, that since currently only accounts can be multiplexed, the 
+    /// underlying `Address` will be an account (not contract) address.
+    fn generate(env: &Env, id: u64) -> crate::MuxedAddress;
+
+    /// Returns a new `MuxedAddress` that has the same `Address` part as this
+    /// `MuxedAdress`, but a new multiplexing identifier (`new_id`).
+    fn clone_with_id(&self, new_id: u64) -> crate::MuxedAddress;
 }
 
 pub trait Deployer {

Original file line number	Diff line number	Diff line change
`@@ -55,7 +55,11 @@ impl Debug for Address {`
`55`	`55`	`let strkey = Strkey::Contract(Contract(contract_id.0));`
`56`	`56`	`write!(f, "Contract({})", strkey.to_string())?;`
`57`	`57`	`}`
`58`		`- _ => return Err(core::fmt::Error),`
	`58`	`+ ScAddress::MuxedAccount(_)`
	`59`	`+ \| ScAddress::ClaimableBalance(_)`
	`60`	`+ \| ScAddress::LiquidityPool(_) => {`
	`61`	`+ return Err(core::fmt::Error);`
	`62`	`+ }`
`59`	`63`	`}`
`60`	`64`	`} else {`
`61`	`65`	`return Err(core::fmt::Error);`