Skip to content

Commit

Permalink
[cherrypick][jsonrpc] fix estimated rewards during safe mode (#20182) (
Browse files Browse the repository at this point in the history
…#20187)

## Description 

Currently if the exchange rate for an epoch is not found, we would
assign it the default exchange rate while we should've used the exchange
rate from the previous existing epoch instead. This PR fixes that.

## Test plan 

Added some unit tests.

---

## Release notes

Check each box that your changes affect. If none of the boxes relate to
your changes, release notes aren't required.

For each box you select, include information after the relevant heading
that describes the impact of your changes that a user might notice and
any actions they must take to implement updates.

- [ ] Protocol: 
- [ ] Nodes (Validators and Full nodes): 
- [ ] Indexer: 
- [x] JSON-RPC: fixed reward estimation in `getStakes` API so that we
don't overestimate stake rewards of stakes activated during safe mode
epochs.
- [ ] GraphQL: 
- [ ] CLI: 
- [ ] Rust SDK:
- [ ] REST API:

## Description 

Describe the changes or additions included in this PR.

## Test plan 

How did you test the new or updated feature?

---

## Release notes

Check each box that your changes affect. If none of the boxes relate to
your changes, release notes aren't required.

For each box you select, include information after the relevant heading
that describes the impact of your changes that a user might notice and
any actions they must take to implement updates.

- [ ] Protocol: 
- [ ] Nodes (Validators and Full nodes): 
- [ ] Indexer: 
- [ ] JSON-RPC: 
- [ ] GraphQL: 
- [ ] CLI: 
- [ ] Rust SDK:
- [ ] REST API:
  • Loading branch information
emmazzz authored Nov 12, 2024
1 parent 102280a commit f77489d
Show file tree
Hide file tree
Showing 11 changed files with 130 additions and 58 deletions.
49 changes: 26 additions & 23 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 1 addition & 2 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -328,10 +328,9 @@ cynic-codegen = "= 3.7.3"
dashmap = "5.5.3"
# datatest-stable = "0.1.2"
datatest-stable = { git = "https://github.com/nextest-rs/datatest-stable.git", rev = "72db7f6d1bbe36a5407e96b9488a581f763e106f" }
derivative = "2.2.0"
derive-syn-parse = "0.1.5"
derive_builder = "0.12.0"
derive_more = "0.99.17"
derive_more = "1.0.0"
diesel = "2.2"
diesel_migrations = "2.2"
diesel-async = "0.5"
Expand Down
2 changes: 1 addition & 1 deletion crates/sui-cluster-test/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ uuid.workspace = true
prometheus.workspace = true
fastcrypto.workspace = true
shared-crypto.workspace = true
derivative.workspace = true
derive_more = { workspace = true, features = ["debug"] }
regex.workspace = true

sui-indexer.workspace = true
Expand Down
27 changes: 8 additions & 19 deletions crates/sui-cluster-test/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

use clap::*;
use regex::Regex;
use std::{fmt, path::PathBuf};
use std::path::PathBuf;

#[derive(Parser, Clone, ValueEnum, Debug)]
pub enum Env {
Expand All @@ -16,8 +16,7 @@ pub enum Env {
NewLocal,
}

#[derive(derivative::Derivative, Parser)]
#[derivative(Debug)]
#[derive(derive_more::Debug, Parser)]
#[clap(name = "", rename_all = "kebab-case")]
pub struct ClusterTestOpt {
#[clap(value_enum)]
Expand All @@ -33,7 +32,12 @@ pub struct ClusterTestOpt {
pub indexer_address: Option<String>,
/// URL for the Indexer Postgres DB
#[clap(long)]
#[derivative(Debug(format_with = "obfuscated_pg_address"))]
#[debug("{}", match pg_address {
None => "None".into(),
Some(val) => Regex::new(r":.*@")
.unwrap()
.replace_all(val.as_str(), ":*****@"),
})]
pub pg_address: Option<String>,
#[clap(long)]
pub config_dir: Option<PathBuf>,
Expand All @@ -47,21 +51,6 @@ pub struct ClusterTestOpt {
pub with_indexer_and_graphql: bool,
}

fn obfuscated_pg_address(val: &Option<String>, f: &mut fmt::Formatter<'_>) -> fmt::Result {
match val {
None => write!(f, "None"),
Some(val) => {
write!(
f,
"{}",
Regex::new(r":.*@")
.unwrap()
.replace_all(val.as_str(), ":*****@")
)
}
}
}

impl ClusterTestOpt {
pub fn new_local() -> Self {
Self {
Expand Down
77 changes: 76 additions & 1 deletion crates/sui-json-rpc/src/governance_api.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand All @@ -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.
/// 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();
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()
Expand All @@ -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);
}
}
3 changes: 1 addition & 2 deletions crates/sui-types/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,6 @@ roaring.workspace = true
enum_dispatch.workspace = true
eyre.workspace = true
indexmap.workspace = true
derivative.workspace = true
jsonrpsee.workspace = true
move-binary-format.workspace = true
move-bytecode-utils.workspace = true
Expand Down Expand Up @@ -70,7 +69,7 @@ fastcrypto-zkp.workspace = true
passkey-types.workspace = true

typed-store-error.workspace = true
derive_more.workspace = true
derive_more = { workspace = true, features = ["as_ref", "debug", "display", "from"] }
proptest.workspace = true
proptest-derive.workspace = true
better_any.workspace = true
Expand Down
9 changes: 4 additions & 5 deletions crates/sui-types/src/move_package.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ use crate::{
object::OBJECT_START_VERSION,
SUI_FRAMEWORK_ADDRESS,
};
use derive_more::Display;
use fastcrypto::hash::HashFunction;
use move_binary_format::binary_config::BinaryConfig;
use move_binary_format::file_format::CompiledModule;
Expand Down Expand Up @@ -114,13 +113,13 @@ pub struct MovePackage {
// associated constants before storing in any serialization setting.
/// Rust representation of upgrade policy constants in `sui::package`.
#[repr(u8)]
#[derive(Display, Debug, Clone, Copy)]
#[derive(derive_more::Display, Debug, Clone, Copy)]
pub enum UpgradePolicy {
#[display(fmt = "COMPATIBLE")]
#[display("COMPATIBLE")]
Compatible = 0,
#[display(fmt = "ADDITIVE")]
#[display("ADDITIVE")]
Additive = 128,
#[display(fmt = "DEP_ONLY")]
#[display("DEP_ONLY")]
DepOnly = 192,
}

Expand Down
7 changes: 7 additions & 0 deletions crates/sui-types/src/sui_system_state/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -416,6 +416,13 @@ impl PoolTokenExchangeRate {
self.pool_token_amount as f64 / self.sui_amount as f64
}
}

pub fn new(sui_amount: u64, pool_token_amount: u64) -> Self {
Self {
sui_amount,
pool_token_amount,
}
}
}

#[derive(Debug, Serialize, Deserialize, Clone, Eq, PartialEq)]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -88,14 +88,13 @@ pub struct ValidatorMetadataV1 {
pub extra_fields: Bag,
}

#[derive(derivative::Derivative, Clone, Eq, PartialEq)]
#[derivative(Debug)]
#[derive(derive_more::Debug, Clone, Eq, PartialEq)]
pub struct VerifiedValidatorMetadataV1 {
pub sui_address: SuiAddress,
pub protocol_pubkey: AuthorityPublicKey,
pub network_pubkey: NetworkPublicKey,
pub worker_pubkey: NetworkPublicKey,
#[derivative(Debug = "ignore")]
#[debug(ignore)]
pub proof_of_possession_bytes: Vec<u8>,
pub name: String,
pub description: String,
Expand Down
2 changes: 0 additions & 2 deletions crates/telemetry-subscribers/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ tracing.workspace = true
tracing-appender.workspace = true
tracing-subscriber.workspace = true
opentelemetry = { version = "0.25.0", optional = true }
opentelemetry_api = { version = "0.20.0", optional = true }
opentelemetry_sdk = { version = "0.25.0", features = ["rt-tokio"], optional = true }
opentelemetry-otlp = { version = "0.25.0", features = ["grpc-tonic"], optional = true }
tracing-opentelemetry = { version = "0.26.0", optional = true }
Expand All @@ -42,7 +41,6 @@ otlp = [
"opentelemetry",
"opentelemetry-otlp",
"opentelemetry-proto",
"opentelemetry_api",
"opentelemetry_sdk"
]

Expand Down
4 changes: 4 additions & 0 deletions deny.toml
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,10 @@ ignore = [

# Temporarily allow until arrow family of crates updates `lexical-core` to 1.0
"RUSTSEC-2023-0086",
# allow unmaintained instant crate used in transitive dependencies (backoff, cached, fastrand, parking_lot_*)
"RUSTSEC-2024-0384",
# allow unmaintained derivative crate used in transitive dependencies (ark-*)
"RUSTSEC-2024-0388",
]
# Threshold for security vulnerabilities, any vulnerability with a CVSS score
# lower than the range specified will be ignored. Note that ignored advisories
Expand Down

0 comments on commit f77489d

Please sign in to comment.