Test enabling rewards after shielding and large amounts in the masp

.changelog/unreleased/bug-fixes/3959-fix-masp-change.md

@@ -0,0 +1,5 @@
+- Fix a variety of MASP rewards related client bugs. Moreover, add
+  regression tests that cover the expected behavior. The client logic
+  that made MASP fee payments possible was also dramatically simplified,
+  making this code easier to maintain in the foreseeable future.
+  ([\#3959](https://github.com/anoma/namada/pull/3959))

crates/apps_lib/src/cli.rs

@@ -4661,16 +4661,13 @@ pub mod args {
                 });
             }
 
-            let gas_spending_keys = self
-                .gas_spending_keys
-                .iter()
-                .map(|key| chain_ctx.get_cached(key))
-                .collect();
+            let gas_spending_key =
+                self.gas_spending_key.map(|key| chain_ctx.get_cached(&key));
 
             Ok(TxShieldedTransfer::<SdkTypes> {
                 tx,
                 data,
-                gas_spending_keys,
+                gas_spending_key,
                 disposable_signing_key: self.disposable_signing_key,
                 tx_code_path: self.tx_code_path.to_path_buf(),
             })
@@ -4691,16 +4688,13 @@ pub mod args {
                 token,
                 amount,
             }];
-            let mut gas_spending_keys = vec![];
-            if let Some(key) = GAS_SPENDING_KEY.parse(matches) {
-                gas_spending_keys.push(key);
-            }
+            let gas_spending_key = GAS_SPENDING_KEY.parse(matches);
             let disposable_gas_payer = DISPOSABLE_SIGNING_KEY.parse(matches);
 
             Self {
                 tx,
                 data,
-                gas_spending_keys,
+                gas_spending_key,
                 disposable_signing_key: disposable_gas_payer,
                 tx_code_path,
             }
@@ -4830,16 +4824,13 @@ pub mod args {
                     amount: transfer_data.amount,
                 });
             }
-            let gas_spending_keys = self
-                .gas_spending_keys
-                .iter()
-                .map(|key| chain_ctx.get_cached(key))
-                .collect();
+            let gas_spending_key =
+                self.gas_spending_key.map(|key| chain_ctx.get_cached(&key));
 
             Ok(TxUnshieldingTransfer::<SdkTypes> {
                 tx,
                 data,
-                gas_spending_keys,
+                gas_spending_key,
                 disposable_signing_key: self.disposable_signing_key,
                 source: chain_ctx.get_cached(&self.source),
                 tx_code_path: self.tx_code_path.to_path_buf(),
@@ -4860,17 +4851,14 @@ pub mod args {
                 token,
                 amount,
             }];
-            let mut gas_spending_keys = vec![];
-            if let Some(key) = GAS_SPENDING_KEY.parse(matches) {
-                gas_spending_keys.push(key);
-            }
+            let gas_spending_key = GAS_SPENDING_KEY.parse(matches);
             let disposable_signing_key = DISPOSABLE_SIGNING_KEY.parse(matches);
 
             Self {
                 tx,
                 source,
                 data,
-                gas_spending_keys,
+                gas_spending_key,
                 disposable_signing_key,
                 tx_code_path,
             }
@@ -4919,11 +4907,8 @@ pub mod args {
         ) -> Result<TxIbcTransfer<SdkTypes>, Self::Error> {
             let tx = self.tx.to_sdk(ctx)?;
             let chain_ctx = ctx.borrow_mut_chain_or_exit();
-            let gas_spending_keys = self
-                .gas_spending_keys
-                .iter()
-                .map(|key| chain_ctx.get_cached(key))
-                .collect();
+            let gas_spending_key =
+                self.gas_spending_key.map(|key| chain_ctx.get_cached(&key));
 
             Ok(TxIbcTransfer::<SdkTypes> {
                 tx,
@@ -4938,7 +4923,7 @@ pub mod args {
                 refund_target: chain_ctx.get_opt(&self.refund_target),
                 ibc_shielding_data: self.ibc_shielding_data,
                 ibc_memo: self.ibc_memo,
-                gas_spending_keys,
+                gas_spending_key,
                 disposable_signing_key: self.disposable_signing_key,
                 tx_code_path: self.tx_code_path.to_path_buf(),
             })
@@ -4965,10 +4950,7 @@ pub mod args {
                         .expect("Failed to decode IBC shielding data")
                 });
             let ibc_memo = IBC_MEMO.parse(matches);
-            let mut gas_spending_keys = vec![];
-            if let Some(key) = GAS_SPENDING_KEY.parse(matches) {
-                gas_spending_keys.push(key);
-            }
+            let gas_spending_key = GAS_SPENDING_KEY.parse(matches);
             let disposable_signing_key = DISPOSABLE_SIGNING_KEY.parse(matches);
             let tx_code_path = PathBuf::from(TX_IBC_WASM);
             Self {
@@ -4984,7 +4966,7 @@ pub mod args {
                 refund_target,
                 ibc_shielding_data,
                 ibc_memo,
-                gas_spending_keys,
+                gas_spending_key,
                 disposable_signing_key,
                 tx_code_path,
             }

crates/core/src/masp.rs

@@ -195,11 +195,9 @@ impl AssetData {
 
     /// Give this pre-asset type the given epoch if already has an epoch. Return
     /// the replaced value.
-    pub fn redate(&mut self, to: MaspEpoch) -> Option<MaspEpoch> {
+    pub fn redate(&mut self, to: MaspEpoch) {
         if self.epoch.is_some() {
-            self.epoch.replace(to)
-        } else {
-            None
+            self.epoch = Some(to);
         }
     }
 
@@ -1121,14 +1119,12 @@ mod test {
         assert!(data.epoch.is_none());
 
         let epoch_0 = MaspEpoch::new(3);
-        let old = data.redate(epoch_0);
-        assert!(old.is_none());
+        data.redate(epoch_0);
         assert!(data.epoch.is_none());
         data.epoch = Some(epoch_0);
 
         let epoch_1 = MaspEpoch::new(5);
-        let old = data.redate(epoch_1);
-        assert_eq!(old, Some(epoch_0));
+        data.redate(epoch_1);
         assert_eq!(data.epoch, Some(epoch_1));
     }
 

crates/core/src/token.rs

@@ -56,6 +56,11 @@ pub const NATIVE_SCALE: u64 = 1_000_000;
 pub type Change = I256;
 
 impl Amount {
+    /// Iterate over all words in this [`Amount`].
+    pub fn iter_words(self) -> impl Iterator<Item = u64> {
+        self.raw.0.into_iter()
+    }
+
     /// Convert a [`u64`] to an [`Amount`].
     pub const fn from_u64(x: u64) -> Self {
         Self {
@@ -905,23 +910,30 @@ pub enum MaspDigitPos {
     Three,
 }
 
-impl From<u8> for MaspDigitPos {
-    fn from(denom: u8) -> Self {
+impl TryFrom<u8> for MaspDigitPos {
+    type Error = &'static str;
+
+    fn try_from(denom: u8) -> Result<Self, Self::Error> {
         match denom {
-            0 => Self::Zero,
-            1 => Self::One,
-            2 => Self::Two,
-            3 => Self::Three,
-            _ => panic!("Possible MASP denominations must be between 0 and 3"),
+            0 => Ok(Self::Zero),
+            1 => Ok(Self::One),
+            2 => Ok(Self::Two),
+            3 => Ok(Self::Three),
+            _ => Err("Possible MASP denominations must be between 0 and 3"),
         }
     }
 }
 
 impl MaspDigitPos {
     /// Iterator over the possible denominations
     pub fn iter() -> impl Iterator<Item = MaspDigitPos> {
-        // 0, 1, 2, 3
-        (0u8..4).map(Self::from)
+        [
+            MaspDigitPos::Zero,
+            MaspDigitPos::One,
+            MaspDigitPos::Two,
+            MaspDigitPos::Three,
+        ]
+        .into_iter()
     }
 
     /// Get the corresponding u64 word from the input uint256.

crates/core/src/uint.rs

@@ -546,6 +546,8 @@ impl FromStr for I256 {
 }
 
 impl I256 {
+    const N_WORDS: usize = 4;
+
     /// Compute the two's complement of a number.
     pub fn negate(&self) -> Option<Self> {
         let (uint, overflow) = self.0.negate();
@@ -609,38 +611,35 @@ impl I256 {
         Self(self.0.canonical())
     }
 
-    /// the maximum I256 value
+    /// The maximum value of an [`I256`].
     pub fn maximum() -> Self {
         Self(MAX_SIGNED_VALUE)
     }
 
-    /// Attempt to convert a MASP-denominated integer to an I256
-    /// using the given denomination.
+    /// Given an i128 and [`MaspDigitPos`], construct the corresponding
+    /// amount.
     pub fn from_masp_denominated(
-        value: impl Into<i128>,
+        val: i128,
         denom: MaspDigitPos,
-    ) -> Result<Self, AmountParseError> {
-        let value = value.into();
-        let is_negative = value < 0;
-        let value = value.unsigned_abs();
-        let mut result = [0u64; 4];
-        result[denom as usize] = u64::try_from(value)
-            .map_err(|_e| AmountParseError::PrecisionOverflow)?;
-        let result = Uint(result);
-        if result <= MAX_SIGNED_VALUE {
-            if is_negative {
-                let (inner, overflow) = result.negate();
-                if overflow {
-                    Err(AmountParseError::InvalidRange)
-                } else {
-                    Ok(Self(inner))
-                }
+    ) -> Result<Self, <i64 as TryFrom<u64>>::Error> {
+        let abs = val.unsigned_abs();
+        #[allow(clippy::cast_possible_truncation)]
+        let lo = abs as u64;
+        let hi = (abs >> 64) as u64;
+        let lo_pos = denom as usize;
+        #[allow(clippy::arithmetic_side_effects)]
+        let hi_pos = lo_pos + 1;
+        let mut raw = [0u64; Self::N_WORDS];
+        raw[lo_pos] = lo;
+        raw[hi_pos] = hi;
+        i64::try_from(raw[Self::N_WORDS - 1]).map(|_| {
+            let res = Self(Uint(raw));
+            if val.is_negative() {
+                res.checked_neg().unwrap()
             } else {
-                Ok(Self(result).canonical())
+                res
             }
-        } else {
-            Err(AmountParseError::InvalidRange)
-        }
+        })
     }
 
     /// Multiply by a decimal [`Dec`] with the result rounded up. Checks for
@@ -777,6 +776,23 @@ impl CheckedAdd for I256 {
     }
 }
 
+// NOTE: This is here only because MASP requires it for `ValueSum` subtraction
+impl CheckedSub for &I256 {
+    type Output = I256;
+
+    fn checked_sub(self, rhs: Self) -> Option<Self::Output> {
+        self.checked_sub(*rhs)
+    }
+}
+
+impl CheckedSub for I256 {
+    type Output = I256;
+
+    fn checked_sub(self, rhs: Self) -> Option<Self::Output> {
+        I256::checked_sub(&self, rhs)
+    }
+}
+
 // NOTE: This is here only because num_traits::CheckedAdd requires it
 impl std::ops::Add for I256 {
     type Output = Self;

crates/sdk/src/args.rs

@@ -356,7 +356,7 @@ pub struct TxShieldedTransfer<C: NamadaTypes = SdkTypes> {
     /// Transfer-specific data
     pub data: Vec<TxShieldedTransferData<C>>,
     /// Optional additional keys for gas payment
-    pub gas_spending_keys: Vec<C::SpendingKey>,
+    pub gas_spending_key: Option<C::SpendingKey>,
     /// Generate an ephemeral signing key to be used only once to sign the
     /// wrapper tx
     pub disposable_signing_key: bool,
@@ -453,7 +453,7 @@ pub struct TxUnshieldingTransfer<C: NamadaTypes = SdkTypes> {
     /// Transfer-specific data
     pub data: Vec<TxUnshieldingTransferData<C>>,
     /// Optional additional keys for gas payment
-    pub gas_spending_keys: Vec<C::SpendingKey>,
+    pub gas_spending_key: Option<C::SpendingKey>,
     /// Generate an ephemeral signing key to be used only once to sign the
     /// wrapper tx
     pub disposable_signing_key: bool,
@@ -499,7 +499,7 @@ pub struct TxIbcTransfer<C: NamadaTypes = SdkTypes> {
     /// Memo for IBC transfer packet
     pub ibc_memo: Option<String>,
     /// Optional additional keys for gas payment
-    pub gas_spending_keys: Vec<C::SpendingKey>,
+    pub gas_spending_key: Option<C::SpendingKey>,
     /// Generate an ephemeral signing key to be used only once to sign the
     /// wrapper tx
     pub disposable_signing_key: bool,
@@ -591,12 +591,9 @@ impl<C: NamadaTypes> TxIbcTransfer<C> {
     }
 
     /// Gas spending keys
-    pub fn gas_spending_keys(
-        self,
-        gas_spending_keys: Vec<C::SpendingKey>,
-    ) -> Self {
+    pub fn gas_spending_keys(self, gas_spending_key: C::SpendingKey) -> Self {
         Self {
-            gas_spending_keys,
+            gas_spending_key: Some(gas_spending_key),
             ..self
         }
     }

crates/sdk/src/lib.rs

@@ -173,12 +173,12 @@ pub trait Namada: NamadaIo {
     fn new_shielded_transfer(
         &self,
         data: Vec<args::TxShieldedTransferData>,
-        gas_spending_keys: Vec<ExtendedSpendingKey>,
+        gas_spending_key: Option<ExtendedSpendingKey>,
         disposable_signing_key: bool,
     ) -> args::TxShieldedTransfer {
         args::TxShieldedTransfer {
             data,
-            gas_spending_keys,
+            gas_spending_key,
             tx_code_path: PathBuf::from(TX_TRANSFER_WASM),
             disposable_signing_key,
             tx: self.tx_builder(),
@@ -206,13 +206,13 @@ pub trait Namada: NamadaIo {
         &self,
         source: ExtendedSpendingKey,
         data: Vec<args::TxUnshieldingTransferData>,
-        gas_spending_keys: Vec<ExtendedSpendingKey>,
+        gas_spending_key: Option<ExtendedSpendingKey>,
         disposable_signing_key: bool,
     ) -> args::TxUnshieldingTransfer {
         args::TxUnshieldingTransfer {
             source,
             data,
-            gas_spending_keys,
+            gas_spending_key,
             disposable_signing_key,
             tx_code_path: PathBuf::from(TX_TRANSFER_WASM),
             tx: self.tx_builder(),
@@ -318,7 +318,7 @@ pub trait Namada: NamadaIo {
             refund_target: None,
             ibc_shielding_data: None,
             ibc_memo: None,
-            gas_spending_keys: Default::default(),
+            gas_spending_key: Default::default(),
             tx: self.tx_builder(),
             tx_code_path: PathBuf::from(TX_IBC_WASM),
         }