diff --git a/.changelog/unreleased/improvements/1973-refine-commission-tx.md b/.changelog/unreleased/improvements/1973-refine-commission-tx.md new file mode 100644 index 0000000000..04a00bac66 --- /dev/null +++ b/.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)) \ No newline at end of file diff --git a/proof_of_stake/src/lib.rs b/proof_of_stake/src/lib.rs index 0fbbf2231b..2c3bf37f6b 100644 --- a/proof_of_stake/src/lib.rs +++ b/proof_of_stake/src/lib.rs @@ -176,6 +176,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( @@ -2355,13 +2359,21 @@ pub fn change_validator_commission_rate( 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)?; @@ -2386,14 +2398,7 @@ where .get(storage, pipeline_epoch.prev(), ¶ms)? .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, diff --git a/shared/src/sdk/tx.rs b/shared/src/sdk/tx.rs index 9d7fe0cfe4..c2edfc69ba 100644 --- a/shared/src/sdk/tx.rs +++ b/shared/src/sdk/tx.rs @@ -560,6 +560,18 @@ pub async fn build_validator_commission_change< commission_rate, max_commission_change_per_epoch, }) => { + if rate.is_negative() || rate > Dec::one() { + edisplay_line!( + 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 {