-
Notifications
You must be signed in to change notification settings - Fork 11.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[jsonrpc] fix estimated rewards during safe mode #20182
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -166,6 +166,7 @@ impl GovernanceReadApi { | |
let status = if !exists { | ||
StakeStatus::Unstaked | ||
} else if system_state_summary.epoch >= stake.activation_epoch() { | ||
// TODO: use dev_inspect to call a move function to get the estimated reward | ||
let estimated_reward = if let Some(current_rate) = current_rate { | ||
let stake_rate = rate_table | ||
.rates | ||
|
@@ -431,7 +432,8 @@ async fn exchange_rates( | |
}) | ||
.collect::<Result<Vec<_>, _>>()?; | ||
|
||
rates.sort_by(|(a, _), (b, _)| a.cmp(b).reverse()); | ||
// Rates for some epochs might be missing due to safe mode, we need to backfill them. | ||
rates = backfill_rates(rates); | ||
|
||
exchange_rates.push(ValidatorExchangeRates { | ||
address, | ||
|
@@ -451,6 +453,38 @@ pub struct ValidatorExchangeRates { | |
pub rates: Vec<(EpochId, PoolTokenExchangeRate)>, | ||
} | ||
|
||
/// Backfill missing rates for some epochs due to safe mode. If a rate is missing for epoch e, | ||
/// we will use the rate for epoch e-1 to fill it. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "we will use the value from the nearest prior epoch" |
||
/// Rates returned are in descending order by epoch. | ||
fn backfill_rates( | ||
rates: Vec<(EpochId, PoolTokenExchangeRate)>, | ||
) -> Vec<(EpochId, PoolTokenExchangeRate)> { | ||
if rates.is_empty() { | ||
return rates; | ||
} | ||
|
||
let min_epoch = *rates.iter().map(|(e, _)| e).min().unwrap(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's probably easier to sort it first before processing. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure if sorting it first will make things easier because then we'd have to keep track of the index we are at separately and insert the missing entry, etc. The logic may get messier. Unless I'm missing something. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You just iterate the sorted vector. No need for min/max/find calls. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the alternative implementation I came up with: fn backfill_rates(
mut rates: Vec<(EpochId, PoolTokenExchangeRate)>,
) -> Vec<(EpochId, PoolTokenExchangeRate)> {
if rates.is_empty() {
return rates;
}
rates.sort_by(|(a, _), (b, _)| a.cmp(b));
let mut filled_rates = vec![];
let mut next_epoch_to_fill = rates[0].0;
let mut prev_rate = rates[0].1.clone();
for (epoch, rate) in rates {
// Fill in all the gaps between the previous entry and the current entry.
while epoch > next_epoch_to_fill {
filled_rates.push((next_epoch_to_fill, prev_rate.clone()));
next_epoch_to_fill += 1;
}
filled_rates.push((next_epoch_to_fill, rate.clone()));
next_epoch_to_fill += 1;
prev_rate = rate;
}
filled_rates.reverse();
filled_rates
} I think this is harder to understand and get right. It took me quite a few tries to get the code right, with off-by-one errors etc |
||
let max_epoch = *rates.iter().map(|(e, _)| e).max().unwrap(); | ||
let mut filled_rates = Vec::new(); | ||
let mut prev_rate = None; | ||
|
||
for epoch in min_epoch..=max_epoch { | ||
match rates.iter().find(|(e, _)| *e == epoch) { | ||
Some((e, rate)) => { | ||
prev_rate = Some(rate.clone()); | ||
filled_rates.push((*e, rate.clone())); | ||
} | ||
None => { | ||
if let Some(rate) = prev_rate.clone() { | ||
filled_rates.push((epoch, rate)); | ||
} | ||
} | ||
} | ||
} | ||
filled_rates.reverse(); | ||
filled_rates | ||
} | ||
|
||
impl SuiRpcModule for GovernanceReadApi { | ||
fn rpc(self) -> RpcModule<Self> { | ||
self.into_rpc() | ||
|
@@ -460,3 +494,44 @@ impl SuiRpcModule for GovernanceReadApi { | |
GovernanceReadApiOpenRpc::module_doc() | ||
} | ||
} | ||
|
||
#[cfg(test)] | ||
mod tests { | ||
use super::*; | ||
use sui_types::sui_system_state::PoolTokenExchangeRate; | ||
|
||
#[test] | ||
fn test_backfill_rates_empty() { | ||
let rates = vec![]; | ||
assert_eq!(backfill_rates(rates), vec![]); | ||
} | ||
|
||
#[test] | ||
fn test_backfill_rates_no_gaps() { | ||
let rate1 = PoolTokenExchangeRate::new(100, 100); | ||
let rate2 = PoolTokenExchangeRate::new(200, 220); | ||
let rate3 = PoolTokenExchangeRate::new(300, 330); | ||
let rates = vec![(2, rate2.clone()), (3, rate3.clone()), (1, rate1.clone())]; | ||
|
||
let expected: Vec<(u64, PoolTokenExchangeRate)> = | ||
vec![(3, rate3.clone()), (2, rate2), (1, rate1)]; | ||
assert_eq!(backfill_rates(rates), expected); | ||
} | ||
|
||
#[test] | ||
fn test_backfill_rates_with_gaps() { | ||
let rate1 = PoolTokenExchangeRate::new(100, 100); | ||
let rate3 = PoolTokenExchangeRate::new(300, 330); | ||
let rate5 = PoolTokenExchangeRate::new(500, 550); | ||
let rates = vec![(3, rate3.clone()), (1, rate1.clone()), (5, rate5.clone())]; | ||
|
||
let expected = vec![ | ||
(5, rate5.clone()), | ||
(4, rate3.clone()), | ||
(3, rate3.clone()), | ||
(2, rate1.clone()), | ||
(1, rate1), | ||
]; | ||
assert_eq!(backfill_rates(rates), expected); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is line 466/467 attempting to do the equivalent of this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The loop in
backfill_rates
does backfilling + sorting in ascending order by epoch number. And line 484 reverses the vector so it becomes sorted in descending order.