Lean into Transfer events with data as a map (#1534)

### What

Remove the `token::publish_transfer_event` function.

Rename Transfer to TransferLegacy.

Rename TransferMuxed to Transfer.

### Why

To encourage developers publishing the newer and easier to extend
Transfer event.

While it's true that the Stellar Asset Contract, for largely legacy
reasons, continues to publish a `transfer` event with the amount as the
data, there's no reason for the SDK to encourage other token
implementations to do the same.

Rather than giving developers a function to call to publish the transfer
event across these two different data forms, we should just point them
to publishing the newer form that is extendable.

### Merging

This PR is intended to merge to `main` after #1533. It is marked as a
draft PR until #1533 is merged.

Author: leighmcculloch

Cargo.lock

@@ -46,6 +46,7 @@ soroban-env-host = { workspace = true, features = ["testutils"] }
 soroban-ledger-snapshot = { workspace = true, features = ["testutils"] }
 stellar-xdr = { workspace = true, features = ["curr", "std"] }
 soroban-spec = { workspace = true }
+soroban-token-sdk = { workspace = true }
 ed25519-dalek = "2.0.0"
 rand = "0.8.5"
 ctor = "0.2.9"

soroban-sdk/Cargo.toml

@@ -19,20 +19,16 @@
 //! `transfer` implementation that still uses `Address` destination:
 //!
 //! ```
-//! use soroban_sdk::{Env, Address, token};
+//! use soroban_sdk::{Env, Address};
+//! use soroban_token_sdk::TokenUtils;
 //! // ... inside some token contract ...
 //! fn transfer(env: Env, from: Address, to: Address, amount: i128) {
 //!    // Authorize the transfer source.
 //!    from.require_auth();
 //!    // Token-specific implementation of balance movement.
 //!    token_impl::move_balance(&env, &from, &to, amount);
-//!    // Publish an event (notice that this uses the new event format - see
-//!    // the previous migration step).
-//!    token::Transfer {
-//!        from,
-//!        to,
-//!        amount,
-//!    }.publish(&env);
+//!    // Publish the event.
+//!    TokenUtils::new(&env).events().transfer(from, to, amount);
 //! }
 //!
 //! mod token_impl {
@@ -55,9 +51,13 @@
 //!    let to = muxed_to.address();
 //!    // Token-specific implementation of balance movement (same as before).
 //!    token_impl::move_balance(&env, &from, &to, amount);
-//!    // Publish an appropriate transfer event that includes the muxed ID
-//!    // when it's non-None.
-//!    token::publish_transfer_event(&env, &from, &muxed_to, amount);
+//!    // Publish the transfer event.
+//!    token::Transfer {
+//!        from,
+//!        to,
+//!        to_muxed_id: muxed_to.id(),
+//!        amount,
+//!    }.publish(&env);
 //! }
 //!
 //! mod token_impl {

soroban-sdk/src/_migrating/v23_token_transfer.rs

@@ -7,7 +7,7 @@ use soroban_sdk::{
     contract, symbol_short,
     testutils::{Address as _, Events as _, MuxedAddress as _},
     token::StellarAssetClient,
-    token::{Approve, Burn, Clawback, Mint, Transfer, TransferMuxed},
+    token::{Approve, Burn, Clawback, Mint, Transfer, TransferLegacy},
     vec, xdr, Address, Env, IntoVal, Map, MuxedAddress, Symbol, Val,
 };
 
@@ -66,15 +66,15 @@ fn test_approve() {
 }
 
 #[test]
-fn test_transfer() {
+fn test_transfer_legacy() {
     let env = Env::default();
     env.mock_all_auths();
 
     let from = Address::generate(&env);
     let to = Address::generate(&env);
     let amount = 123;
 
-    let event = Transfer {
+    let event = TransferLegacy {
         from: from.clone(),
         to: to.clone(),
         amount,
@@ -116,18 +116,62 @@ fn test_transfer() {
 }
 
 #[test]
-fn test_transfer_muxed() {
+fn test_transfer_without_id() {
+    let env = Env::default();
+    env.mock_all_auths();
+
+    let from = Address::generate(&env);
+    let to: MuxedAddress = MuxedAddress::generate(&env).address().into();
+    let amount = 123;
+
+    let event = Transfer {
+        from: from.clone(),
+        to: to.address(),
+        to_muxed_id: to.id(),
+        amount,
+    };
+
+    // Verify the event publishes the expected topics and data.
+    let topics = (symbol_short!("transfer"), from.clone(), to.address());
+    let data = Map::<Symbol, Val>::from_array(
+        &env,
+        [
+            (Symbol::new(&env, "to_muxed_id"), Val::VOID.to_val()),
+            (Symbol::new(&env, "amount"), amount.into_val(&env)),
+        ],
+    );
+
+    let id = env.register(Contract, ());
+    env.as_contract(&id, || event.publish(&env));
+    let token_events = env.events().all();
+    assert_eq!(
+        token_events,
+        vec![
+            &env,
+            (id.clone(), topics.into_val(&env), data.into_val(&env))
+        ]
+    );
+
+    // No comparison is made with the Stellar Asset Contract for publishing Transfer with a
+    // MuxedAddress that does not contain an ID, because the Stellar Asset Contract for legacy
+    // reasons, to minimise the changes to its behavior over time, still publishes TransferLegacy
+    // in this case. See [`test_transfer_with_id`] for a test that exercises Transfer in the case
+    // that the Stellar Asset contract does publish that event.
+}
+
+#[test]
+fn test_transfer_with_id() {
     let env = Env::default();
     env.mock_all_auths();
 
     let from = Address::generate(&env);
     let to = MuxedAddress::generate(&env);
     let amount = 123;
 
-    let event = TransferMuxed {
+    let event = Transfer {
         from: from.clone(),
         to: to.address(),
-        to_muxed_id: to.id().unwrap(),
+        to_muxed_id: to.id(),
         amount,
     };
 

soroban-sdk/src/tests/token_events.rs

@@ -139,16 +139,9 @@ pub trait TokenInterface {
     ///
     /// # Events
     ///
-    /// Emits an event with topics
-    /// `["transfer", from: Address, to: Address]
-    /// If `to` is not a muxed address (i.e. has no muxed_id set), then data
-    /// will  have the following format (the `Transfer` event):
-    /// `data = amount: i128`
-    ///
-    /// If `to` is a muxed address (i.e. has a muxed_id set), then data will
-    /// have be emitted as map with Symbol keys in the following format (the
-    /// `TransferMuxed` event):
-    /// `data = { "amount": i128, "to_muxed_id": u64 }`
+    /// Publishes the [`Transfer`] event.
+    ///
+    /// Legacy implementations may publish [`TransferLegacy`].
     fn transfer(env: Env, from: Address, to: MuxedAddress, amount: i128);
 
     /// Transfer `amount` from `from` to `to`, consuming the allowance that
@@ -249,7 +242,7 @@ pub struct Approve {
 }
 
 #[contractevent(crate_path = "crate", topics = ["transfer"], data_format = "single-value", export = false)]
-pub struct Transfer {
+pub struct TransferLegacy {
     #[topic]
     pub from: Address,
     #[topic]
@@ -258,12 +251,12 @@ pub struct Transfer {
 }
 
 #[contractevent(crate_path = "crate", topics = ["transfer"], data_format = "map", export = false)]
-pub struct TransferMuxed {
+pub struct Transfer {
     #[topic]
     pub from: Address,
     #[topic]
     pub to: Address,
-    pub to_muxed_id: u64,
+    pub to_muxed_id: Option<u64>,
     pub amount: i128,
 }
 
@@ -288,30 +281,6 @@ pub struct Clawback {
     pub amount: i128,
 }
 
-/// Publish a `transfer` event.
-///
-/// Publishes a [`Transfer`] event if the `to` has no muxed ID.
-///
-/// Publishes a [`TransferMuxed`] event if the `to` has a muxed ID.
-pub fn publish_transfer_event(env: &Env, from: &Address, to: &MuxedAddress, amount: i128) {
-    if let Some(to_muxed_id) = to.id() {
-        TransferMuxed {
-            from: from.clone(),
-            to: to.address(),
-            to_muxed_id,
-            amount,
-        }
-        .publish(env);
-    } else {
-        Transfer {
-            from: from.clone(),
-            to: to.address(),
-            amount,
-        }
-        .publish(env);
-    }
-}
-
 /// Spec contains the contract spec of Token contracts.
 #[doc(hidden)]
 pub struct TokenSpec;
@@ -328,14 +297,14 @@ pub(crate) const TOKEN_SPEC_XDR_INPUT: &[&[u8]] = &[
     &TokenSpec::spec_xdr_transfer(),
     &TokenSpec::spec_xdr_transfer_from(),
     &Approve::spec_xdr(),
+    &TransferLegacy::spec_xdr(),
     &Transfer::spec_xdr(),
-    &TransferMuxed::spec_xdr(),
     &Burn::spec_xdr(),
     &Mint::spec_xdr(),
     &Clawback::spec_xdr(),
 ];
 
-pub(crate) const TOKEN_SPEC_XDR_LEN: usize = 5724;
+pub(crate) const TOKEN_SPEC_XDR_LEN: usize = 5392;
 
 impl TokenSpec {
     /// Returns the XDR spec for the Token contract.
@@ -427,16 +396,9 @@ pub trait StellarAssetInterface {
     ///
     /// # Events
     ///
-    /// Emits an event with topics
-    /// `["transfer", from: Address, to: Address]
-    /// If `to` is not a muxed address (i.e. has no muxed_id set), then data
-    /// will  have the following format (the `Transfer` event):
-    /// `data = amount: i128`
+    /// Publishes the [`Transfer`] event.
     ///
-    /// If `to` is a muxed address (i.e. has a muxed_id set), then data will
-    /// have be emitted as map with Symbol keys in the following format (the
-    /// `TransferMuxed` event):
-    /// `data = { "amount": i128, "to_muxed_id": u64 }`
+    /// Legacy implementations may publish [`TransferLegacy`].
     fn transfer(env: Env, from: Address, to: MuxedAddress, amount: i128);
 
     /// Transfer `amount` from `from` to `to`, consuming the allowance that
@@ -633,16 +595,16 @@ pub(crate) const STELLAR_ASSET_SPEC_XDR_INPUT: &[&[u8]] = &[
     &StellarAssetSpec::spec_xdr_transfer(),
     &StellarAssetSpec::spec_xdr_transfer_from(),
     &Approve::spec_xdr(),
+    &TransferLegacy::spec_xdr(),
     &Transfer::spec_xdr(),
-    &TransferMuxed::spec_xdr(),
     &Burn::spec_xdr(),
     &Mint::spec_xdr(),
     &Clawback::spec_xdr(),
     &SetAdmin::spec_xdr(),
     &SetAuthorized::spec_xdr(),
 ];
 
-pub(crate) const STELLAR_ASSET_SPEC_XDR_LEN: usize = 7664;
+pub(crate) const STELLAR_ASSET_SPEC_XDR_LEN: usize = 7332;
 
 impl StellarAssetSpec {
     /// Returns the XDR spec for the Token contract.