From 68a33934cd304e90a8b5a7ae64633a98246474fa Mon Sep 17 00:00:00 2001 From: brentstone Date: Fri, 6 Oct 2023 12:35:17 -0400 Subject: [PATCH 1/3] add checks and simplify code --- proof_of_stake/src/lib.rs | 23 ++++++++--------------- shared/src/sdk/tx.rs | 12 ++++++++++++ 2 files changed, 20 insertions(+), 15 deletions(-) diff --git a/proof_of_stake/src/lib.rs b/proof_of_stake/src/lib.rs index 0fbbf2231b..9051aae725 100644 --- a/proof_of_stake/src/lib.rs +++ b/proof_of_stake/src/lib.rs @@ -2355,13 +2355,13 @@ 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()); + } let max_change = read_validator_max_commission_rate_change(storage, validator)?; @@ -2386,14 +2386,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 { From 2e84ddc803b3c2f632d711b6176e359bd4a92801 Mon Sep 17 00:00:00 2001 From: brentstone Date: Fri, 6 Oct 2023 12:41:54 -0400 Subject: [PATCH 2/3] changelog: add #1973 --- .changelog/unreleased/improvements/1973-refine-commission-tx.md | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 .changelog/unreleased/improvements/1973-refine-commission-tx.md 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 From 66a756761a8144c3b88e0d75d6378bf1c2c74c30 Mon Sep 17 00:00:00 2001 From: brentstone Date: Wed, 11 Oct 2023 11:18:34 -0400 Subject: [PATCH 3/3] check if rate > 1 in lib code --- proof_of_stake/src/lib.rs | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/proof_of_stake/src/lib.rs b/proof_of_stake/src/lib.rs index 9051aae725..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( @@ -2363,6 +2367,14 @@ where .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)?; if max_change.is_none() {