Merge pull request #4146 from anoma/grarco/fix-masp-rewards-estimate

Fix masp rewards estimate

.changelog/unreleased/bug-fixes/4146-fix-masp-rewards-estimate.md

@@ -0,0 +1,4 @@
+- Fixed a bug in the masp rewards estimation logic that was not correctly
+  accounting for native token rewards and rewards accrued by assets shielded
+  at the current epoch. Updated the API of the function to better suite some UI
+  needs. Improved testing ([\#4146](https://github.com/anoma/namada/pull/4146))

crates/apps_lib/src/cli.rs

@@ -273,7 +273,9 @@ pub mod cmds {
                 .subcommand(QueryMaspRewardTokens::def().display_order(5))
                 .subcommand(QueryBlock::def().display_order(5))
                 .subcommand(QueryBalance::def().display_order(5))
-                .subcommand(QueryRewardsEstimate::def().display_order(5))
+                .subcommand(
+                    QueryShieldingRewardsEstimate::def().display_order(5),
+                )
                 .subcommand(QueryBonds::def().display_order(5))
                 .subcommand(QueryBondedStake::def().display_order(5))
                 .subcommand(QuerySlashes::def().display_order(5))
@@ -358,7 +360,7 @@ pub mod cmds {
             let query_block = Self::parse_with_ctx(matches, QueryBlock);
             let query_balance = Self::parse_with_ctx(matches, QueryBalance);
             let query_rewards_estimate =
-                Self::parse_with_ctx(matches, QueryRewardsEstimate);
+                Self::parse_with_ctx(matches, QueryShieldingRewardsEstimate);
             let query_bonds = Self::parse_with_ctx(matches, QueryBonds);
             let query_bonded_stake =
                 Self::parse_with_ctx(matches, QueryBondedStake);
@@ -525,7 +527,7 @@ pub mod cmds {
         QueryMaspRewardTokens(QueryMaspRewardTokens),
         QueryBlock(QueryBlock),
         QueryBalance(QueryBalance),
-        QueryRewardsEstimate(QueryRewardsEstimate),
+        QueryShieldingRewardsEstimate(QueryShieldingRewardsEstimate),
         QueryBonds(QueryBonds),
         QueryBondedStake(QueryBondedStake),
         QueryCommissionRate(QueryCommissionRate),
@@ -1880,27 +1882,29 @@ pub mod cmds {
     }
 
     #[derive(Clone, Debug)]
-    pub struct QueryRewardsEstimate(
-        pub args::QueryRewardsEstimate<args::CliTypes>,
+    pub struct QueryShieldingRewardsEstimate(
+        pub args::QueryShieldingRewardsEstimate<args::CliTypes>,
     );
 
-    impl SubCmd for QueryRewardsEstimate {
-        const CMD: &'static str = "estimate-rewards";
+    impl SubCmd for QueryShieldingRewardsEstimate {
+        const CMD: &'static str = "estimate-shielding-rewards";
 
         fn parse(matches: &ArgMatches) -> Option<Self> {
             matches.subcommand_matches(Self::CMD).map(|matches| {
-                QueryRewardsEstimate(args::QueryRewardsEstimate::parse(matches))
+                QueryShieldingRewardsEstimate(
+                    args::QueryShieldingRewardsEstimate::parse(matches),
+                )
             })
         }
 
         fn def() -> App {
             App::new(Self::CMD)
                 .about(wrap!(
-                    "Estimate the amount of MASP rewards accumulated by the \
+                    "Estimate the amount of MASP rewards for the \
                      next MASP epoch. Please run shielded-sync first for best \
                      results."
                 ))
-                .add_args::<args::QueryRewardsEstimate<args::CliTypes>>()
+                .add_args::<args::QueryShieldingRewardsEstimate<args::CliTypes>>()
         }
     }
 
@@ -6355,26 +6359,27 @@ pub mod args {
         }
     }
 
-    impl CliToSdk<QueryRewardsEstimate<SdkTypes>>
-        for QueryRewardsEstimate<CliTypes>
+    impl CliToSdk<QueryShieldingRewardsEstimate<SdkTypes>>
+        for QueryShieldingRewardsEstimate<CliTypes>
     {
         type Error = std::convert::Infallible;
 
         fn to_sdk(
             self,
             ctx: &mut Context,
-        ) -> Result<QueryRewardsEstimate<SdkTypes>, Self::Error> {
+        ) -> Result<QueryShieldingRewardsEstimate<SdkTypes>, Self::Error>
+        {
             let query = self.query.to_sdk(ctx)?;
             let chain_ctx = ctx.borrow_mut_chain_or_exit();
 
-            Ok(QueryRewardsEstimate::<SdkTypes> {
+            Ok(QueryShieldingRewardsEstimate::<SdkTypes> {
                 query,
                 owner: chain_ctx.get_cached(&self.owner),
             })
         }
     }
 
-    impl Args for QueryRewardsEstimate<CliTypes> {
+    impl Args for QueryShieldingRewardsEstimate<CliTypes> {
         fn parse(matches: &ArgMatches) -> Self {
             let query = Query::parse(matches);
             let owner = VIEWING_KEY.parse(matches);

crates/apps_lib/src/cli/client.rs

@@ -560,7 +560,9 @@ impl CliApi {
                         let namada = ctx.to_sdk(client, io);
                         rpc::query_balance(&namada, args).await;
                     }
-                    Sub::QueryRewardsEstimate(QueryRewardsEstimate(args)) => {
+                    Sub::QueryShieldingRewardsEstimate(
+                        QueryShieldingRewardsEstimate(args),
+                    ) => {
                         let chain_ctx = ctx.borrow_mut_chain_or_exit();
                         let ledger_address =
                             chain_ctx.get(&args.query.ledger_address);

crates/apps_lib/src/client/rpc.rs

@@ -50,9 +50,7 @@ use namada_sdk::rpc::{
 use namada_sdk::storage::BlockResults;
 use namada_sdk::tendermint_rpc::endpoint::status;
 use namada_sdk::time::DateTimeUtc;
-use namada_sdk::token::{
-    DenominatedAmount, MaspDigitPos, NATIVE_MAX_DECIMAL_PLACES,
-};
+use namada_sdk::token::{DenominatedAmount, MaspDigitPos};
 use namada_sdk::tx::display_batch_resp;
 use namada_sdk::wallet::AddressVpType;
 use namada_sdk::{error, state as storage, token, Namada};
@@ -403,19 +401,46 @@ pub async fn query_proposal_by_id<C: Client + Sync>(
 /// Estimate MASP rewards for next MASP epoch
 pub async fn query_rewards_estimate(
     context: &impl Namada,
-    args: args::QueryRewardsEstimate,
+    args: args::QueryShieldingRewardsEstimate,
 ) {
     let mut shielded = context.shielded_mut().await;
     let _ = shielded.load().await;
-    let rewards_estimate = shielded
-        .estimate_next_epoch_rewards(context, &args.owner.as_viewing_key())
+    let raw_balance = match shielded
+        .compute_shielded_balance(&args.owner.as_viewing_key())
         .await
-        .unwrap()
-        .unsigned_abs();
-    let rewards_estimate = DenominatedAmount::new(
-        Amount::from_u128(rewards_estimate),
-        NATIVE_MAX_DECIMAL_PLACES.into(),
-    );
+    {
+        Ok(balance) => balance,
+        Err(e) => {
+            edisplay_line!(
+                context.io(),
+                "Failed to query shielded balance: {}",
+                e
+            );
+            cli::safe_exit(1);
+        }
+    };
+
+    let rewards_estimate = match raw_balance {
+        Some(balance) => {
+            match shielded
+                .estimate_next_epoch_rewards(context, &balance)
+                .await
+            {
+                Ok(estimate) => estimate,
+                Err(e) => {
+                    edisplay_line!(
+                        context.io(),
+                        "Failed to estimate rewards for the next MASP epoch: \
+                         {}",
+                        e
+                    );
+                    cli::safe_exit(1);
+                }
+            }
+        }
+        None => DenominatedAmount::native(Amount::zero()),
+    };
+
     display_line!(
         context.io(),
         "Estimated native token rewards for the next MASP epoch: {}",

crates/sdk/src/args.rs

@@ -1607,10 +1607,10 @@ pub struct QueryBalance<C: NamadaTypes = SdkTypes> {
     pub height: Option<C::BlockHeight>,
 }
 
-/// Get an estimate for the MASP rewards accumulated by the next
+/// Get an estimate for the MASP rewards for the next
 /// MASP epoch.
 #[derive(Clone, Debug)]
-pub struct QueryRewardsEstimate<C: NamadaTypes = SdkTypes> {
+pub struct QueryShieldingRewardsEstimate<C: NamadaTypes = SdkTypes> {
     /// Common query args
     pub query: Query<C>,
     /// Viewing key

crates/shielded_token/src/conversion.rs

@@ -411,9 +411,8 @@ where
                         normed_inflation
                     });
                 // The conversion is computed such that if consecutive
-                // conversions are added together, the
-                // intermediate native tokens cancel/
-                // telescope out
+                // conversions are added together, the intermediate native
+                // tokens cancel/telescope out
                 let cur_conv = MaspAmount::from_pair(
                     old_asset,
                     i128::try_from(normed_inflation)
@@ -465,8 +464,7 @@ where
                 }
             } else {
                 // Express the inflation reward in real terms, that is, with
-                // respect to the native asset in the zeroth
-                // epoch
+                // respect to the native asset in the zeroth epoch
                 let reward_uint = Uint::from(reward);
                 let ref_inflation_uint = Uint::from(ref_inflation);
                 let inflation_uint = Uint::from(normed_inflation);
@@ -547,7 +545,6 @@ where
         .values_mut()
         .enumerate()
         .collect();
-    // ceil(assets.len() / num_threads)
 
     #[allow(clippy::arithmetic_side_effects)]
     let notes_per_thread_max = (assets.len() + num_threads - 1) / num_threads;