Merge branch 'brent/refine-commission-tx' (#1973)

* origin/brent/refine-commission-tx:
  check if rate > 1 in lib code
  changelog: add #1973
  add checks and simplify code

Author: tzemanovic

.changelog/unreleased/improvements/1973-refine-commission-tx.md

@@ -0,0 +1,2 @@
+- Add missing checks for the commission rate change tx and code clean-up
+  ([\#1973](https://github.com/anoma/namada/pull/1973))

proof_of_stake/src/error.rs

@@ -97,6 +97,10 @@ pub enum SlashError {
 pub enum CommissionRateChangeError {
     #[error("Unexpected negative commission rate {0} for validator {1}")]
     NegativeRate(Dec, Address),
+    #[error(
+        "Unexpected commission rate {0} larger than 1.0 for validator {1}"
+    )]
+    LargerThanOne(Dec, Address),
     #[error("Rate change of {0} is too large for validator {1}")]
     RateChangeTooLarge(Dec, Address),
     #[error(

proof_of_stake/src/lib.rs

@@ -2996,13 +2996,21 @@ pub fn change_validator_commission_rate<S>(
 where
     S: StorageRead + StorageWrite,
 {
-    // if new_rate < Uint::zero() {
-    //     return Err(CommissionRateChangeError::NegativeRate(
-    //         new_rate,
-    //         validator.clone(),
-    //     )
-    //     .into());
-    // }
+    if new_rate.is_negative() {
+        return Err(CommissionRateChangeError::NegativeRate(
+            new_rate,
+            validator.clone(),
+        )
+        .into());
+    }
+
+    if new_rate > Dec::one() {
+        return Err(CommissionRateChangeError::LargerThanOne(
+            new_rate,
+            validator.clone(),
+        )
+        .into());
+    }
 
     let max_change =
         read_validator_max_commission_rate_change(storage, validator)?;
@@ -3027,14 +3035,7 @@ where
         .get(storage, pipeline_epoch.prev(), &params)?
         .expect("Could not find a rate in given epoch");
 
-    // TODO: change this back if we use `Dec` type with a signed integer
-    // let change_from_prev = new_rate - rate_before_pipeline;
-    // if change_from_prev.abs() > max_change.unwrap() {
-    let change_from_prev = if new_rate > rate_before_pipeline {
-        new_rate - rate_before_pipeline
-    } else {
-        rate_before_pipeline - new_rate
-    };
+    let change_from_prev = new_rate.abs_diff(&rate_before_pipeline);
     if change_from_prev > max_change.unwrap() {
         return Err(CommissionRateChangeError::RateChangeTooLarge(
             change_from_prev,

sdk/src/tx.rs

@@ -544,6 +544,18 @@ pub async fn build_validator_commission_change<'a>(
                 commission_rate,
                 max_commission_change_per_epoch,
             }) => {
+                if rate.is_negative() || *rate > Dec::one() {
+                    edisplay_line!(
+                        context.io(),
+                        "New rate is outside of the allowed range of values \
+                         between 0.0 and 1.0."
+                    );
+                    if !tx_args.force {
+                        return Err(Error::from(
+                            TxError::InvalidCommissionRate(*rate),
+                        ));
+                    }
+                }
                 if rate.abs_diff(&commission_rate)
                     > max_commission_change_per_epoch
                 {