Skip to content
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

chore(jsonrpc): extract config primitives #7063

Closed
wants to merge 7 commits into from

Conversation

miraclx
Copy link
Contributor

@miraclx miraclx commented Jun 18, 2022

Tracking issue: #6850

Drops the dependency requirement of near-chain-configs by duplicating all its structures within near-jsonrpc-primitives.

@miraclx miraclx requested review from frol and matklad June 18, 2022 05:20
@miraclx miraclx requested a review from a team as a code owner June 18, 2022 05:20
Copy link
Contributor

@matklad matklad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM as is, though we should kill old ProtocolConfigView in this PR or in the immediate follow up.

Also, I believe this is only used in an experimental RPC, so it would makes sense to maybe take advantage of experementality and to re-design the struct such that it's more forward-compatible. I would thing that something like HashMap<String, serde_json::Value> could probably make more sense as a JSON format.

@@ -583,7 +583,7 @@ impl Genesis {
// Note: this type cannot be placed in primitives/src/view.rs because of `RuntimeConfig` dependency issues.
// Ideally we should create `RuntimeConfigView`, but given the deeply nested nature and the number of fields inside
// `RuntimeConfig`, it should be its own endeavor.
#[derive(Serialize, Deserialize, Debug)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like we can kill this now completely?

diff --git a/chain/client-primitives/src/types.rs b/chain/client-primitives/src/types.rs
index ec0975b9d..954b34dbb 100644
--- a/chain/client-primitives/src/types.rs
+++ b/chain/client-primitives/src/types.rs
@@ -6,7 +6,7 @@ use actix::Message;
 use chrono::DateTime;
 use near_primitives::time::Utc;
 
-use near_chain_configs::ProtocolConfigView;
+use near_chain_configs::ProtocolConfig;
 use near_network_primitives::types::{AccountOrPeerIdOrHash, KnownProducer, PeerInfo};
 use near_primitives::errors::InvalidTxError;
 use near_primitives::hash::CryptoHash;
@@ -801,7 +801,7 @@ impl Message for GetReceipt {
 pub struct GetProtocolConfig(pub BlockReference);
 
 impl Message for GetProtocolConfig {
-    type Result = Result<ProtocolConfigView, GetProtocolConfigError>;
+    type Result = Result<ProtocolConfig, GetProtocolConfigError>;
 }
 
 #[derive(thiserror::Error, Debug)]
diff --git a/chain/client/src/view_client.rs b/chain/client/src/view_client.rs
index ad5a520be..ea065583d 100644
--- a/chain/client/src/view_client.rs
+++ b/chain/client/src/view_client.rs
@@ -17,7 +17,7 @@ use near_chain::{
     get_epoch_block_producers_view, Chain, ChainGenesis, ChainStoreAccess, DoomslugThresholdMode,
     RuntimeAdapter,
 };
-use near_chain_configs::{ClientConfig, ProtocolConfigView};
+use near_chain_configs::{ClientConfig, ProtocolConfig};
 use near_client_primitives::types::{
     Error, GetBlock, GetBlockError, GetBlockHash, GetBlockProof, GetBlockProofError,
     GetBlockProofResponse, GetBlockWithMerkleTree, GetChunkError, GetExecutionOutcome,
@@ -977,7 +977,7 @@ impl Handler<GetBlockProof> for ViewClientActor {
 }
 
 impl Handler<GetProtocolConfig> for ViewClientActor {
-    type Result = Result<ProtocolConfigView, GetProtocolConfigError>;
+    type Result = Result<ProtocolConfig, GetProtocolConfigError>;
 
     #[perf]
     fn handle(&mut self, msg: GetProtocolConfig, _: &mut Self::Context) -> Self::Result {
diff --git a/chain/jsonrpc/src/api/config.rs b/chain/jsonrpc/src/api/config.rs
index f6fd44057..c4240df0e 100644
--- a/chain/jsonrpc/src/api/config.rs
+++ b/chain/jsonrpc/src/api/config.rs
@@ -37,37 +37,47 @@ impl RpcFrom<GetProtocolConfigError> for RpcProtocolConfigError {
     }
 }
 
-impl RpcFrom<near_chain_configs::ProtocolConfigView>
+impl RpcFrom<near_chain_configs::ProtocolConfig>
     for near_jsonrpc_primitives::types::config::ProtocolConfigView
 {
-    fn rpc_from(config: near_chain_configs::ProtocolConfigView) -> Self {
+    fn rpc_from(config: near_chain_configs::ProtocolConfig) -> Self {
         near_jsonrpc_primitives::types::config::ProtocolConfigView {
-            protocol_version: config.protocol_version,
-            genesis_time: config.genesis_time,
-            chain_id: config.chain_id,
-            genesis_height: config.genesis_height,
-            num_block_producer_seats: config.num_block_producer_seats,
-            num_block_producer_seats_per_shard: config.num_block_producer_seats_per_shard,
-            avg_hidden_validator_seats_per_shard: config.avg_hidden_validator_seats_per_shard,
-            dynamic_resharding: config.dynamic_resharding,
-            protocol_upgrade_stake_threshold: config.protocol_upgrade_stake_threshold,
-            epoch_length: config.epoch_length,
-            gas_limit: config.gas_limit,
-            min_gas_price: config.min_gas_price,
-            max_gas_price: config.max_gas_price,
-            block_producer_kickout_threshold: config.block_producer_kickout_threshold,
-            chunk_producer_kickout_threshold: config.chunk_producer_kickout_threshold,
-            online_min_threshold: config.online_min_threshold,
-            online_max_threshold: config.online_max_threshold,
-            gas_price_adjustment_rate: config.gas_price_adjustment_rate,
+            protocol_version: config.genesis_config.protocol_version,
+            genesis_time: config.genesis_config.genesis_time,
+            chain_id: config.genesis_config.chain_id,
+            genesis_height: config.genesis_config.genesis_height,
+            num_block_producer_seats: config.genesis_config.num_block_producer_seats,
+            num_block_producer_seats_per_shard: config
+                .genesis_config
+                .num_block_producer_seats_per_shard,
+            avg_hidden_validator_seats_per_shard: config
+                .genesis_config
+                .avg_hidden_validator_seats_per_shard,
+            dynamic_resharding: config.genesis_config.dynamic_resharding,
+            protocol_upgrade_stake_threshold: config
+                .genesis_config
+                .protocol_upgrade_stake_threshold,
+            epoch_length: config.genesis_config.epoch_length,
+            gas_limit: config.genesis_config.gas_limit,
+            min_gas_price: config.genesis_config.min_gas_price,
+            max_gas_price: config.genesis_config.max_gas_price,
+            block_producer_kickout_threshold: config
+                .genesis_config
+                .block_producer_kickout_threshold,
+            chunk_producer_kickout_threshold: config
+                .genesis_config
+                .chunk_producer_kickout_threshold,
+            online_min_threshold: config.genesis_config.online_min_threshold,
+            online_max_threshold: config.genesis_config.online_max_threshold,
+            gas_price_adjustment_rate: config.genesis_config.gas_price_adjustment_rate,
             runtime_config: config.runtime_config.rpc_into(),
-            transaction_validity_period: config.transaction_validity_period,
-            protocol_reward_rate: config.protocol_reward_rate,
-            max_inflation_rate: config.max_inflation_rate,
-            num_blocks_per_year: config.num_blocks_per_year,
-            protocol_treasury_account: config.protocol_treasury_account,
-            fishermen_threshold: config.fishermen_threshold,
-            minimum_stake_divisor: config.minimum_stake_divisor,
+            transaction_validity_period: config.genesis_config.transaction_validity_period,
+            protocol_reward_rate: config.genesis_config.protocol_reward_rate,
+            max_inflation_rate: config.genesis_config.max_inflation_rate,
+            num_blocks_per_year: config.genesis_config.num_blocks_per_year,
+            protocol_treasury_account: config.genesis_config.protocol_treasury_account,
+            fishermen_threshold: config.genesis_config.fishermen_threshold,
+            minimum_stake_divisor: config.genesis_config.minimum_stake_divisor,
         }
     }
 }
diff --git a/chain/rosetta-rpc/src/utils.rs b/chain/rosetta-rpc/src/utils.rs
index 324a5a28a..a26ef9223 100644
--- a/chain/rosetta-rpc/src/utils.rs
+++ b/chain/rosetta-rpc/src/utils.rs
@@ -1,7 +1,7 @@
 use actix::Addr;
 use futures::StreamExt;
 
-use near_chain_configs::ProtocolConfigView;
+use near_chain_configs::ProtocolConfig;
 use near_client::ViewClientActor;
 use near_primitives::borsh::{BorshDeserialize, BorshSerialize};
 
@@ -412,7 +412,7 @@ pub(crate) async fn query_access_key(
 pub(crate) async fn query_protocol_config(
     block_hash: near_primitives::hash::CryptoHash,
     view_client_addr: &Addr<ViewClientActor>,
-) -> crate::errors::Result<ProtocolConfigView> {
+) -> crate::errors::Result<ProtocolConfig> {
     view_client_addr
         .send(near_client::GetProtocolConfig(near_primitives::types::BlockReference::from(
             near_primitives::types::BlockId::Hash(block_hash),
diff --git a/core/chain-configs/src/genesis_config.rs b/core/chain-configs/src/genesis_config.rs
index 13299cf4a..c946b9629 100644
--- a/core/chain-configs/src/genesis_config.rs
+++ b/core/chain-configs/src/genesis_config.rs
@@ -580,107 +580,11 @@ impl Genesis {
     }
 }
 
-// Note: this type cannot be placed in primitives/src/view.rs because of `RuntimeConfig` dependency issues.
-// Ideally we should create `RuntimeConfigView`, but given the deeply nested nature and the number of fields inside
-// `RuntimeConfig`, it should be its own endeavor.
-#[derive(Debug)]
-pub struct ProtocolConfigView {
-    /// Current Protocol Version
-    pub protocol_version: ProtocolVersion,
-    /// Official time of blockchain start.
-    pub genesis_time: DateTime<Utc>,
-    /// ID of the blockchain. This must be unique for every blockchain.
-    /// If your testnet blockchains do not have unique chain IDs, you will have a bad time.
-    pub chain_id: String,
-    /// Height of genesis block.
-    pub genesis_height: BlockHeight,
-    /// Number of block producer seats at genesis.
-    pub num_block_producer_seats: NumSeats,
-    /// Defines number of shards and number of block producer seats per each shard at genesis.
-    pub num_block_producer_seats_per_shard: Vec<NumSeats>,
-    /// Expected number of hidden validators per shard.
-    pub avg_hidden_validator_seats_per_shard: Vec<NumSeats>,
-    /// Enable dynamic re-sharding.
-    pub dynamic_resharding: bool,
-    /// Threshold of stake that needs to indicate that they ready for upgrade.
-    pub protocol_upgrade_stake_threshold: Rational,
-    /// Epoch length counted in block heights.
-    pub epoch_length: BlockHeightDelta,
-    /// Initial gas limit.
-    pub gas_limit: Gas,
-    /// Minimum gas price. It is also the initial gas price.
-    pub min_gas_price: Balance,
-    /// Maximum gas price.
-    pub max_gas_price: Balance,
-    /// Criterion for kicking out block producers (this is a number between 0 and 100)
-    pub block_producer_kickout_threshold: u8,
-    /// Criterion for kicking out chunk producers (this is a number between 0 and 100)
-    pub chunk_producer_kickout_threshold: u8,
-    /// Online minimum threshold below which validator doesn't receive reward.
-    pub online_min_threshold: Rational,
-    /// Online maximum threshold above which validator gets full reward.
-    pub online_max_threshold: Rational,
-    /// Gas price adjustment rate
-    pub gas_price_adjustment_rate: Rational,
-    /// Runtime configuration (mostly economics constants).
-    pub runtime_config: RuntimeConfig,
-    /// Number of blocks for which a given transaction is valid
-    pub transaction_validity_period: NumBlocks,
-    /// Protocol treasury rate
-    pub protocol_reward_rate: Rational,
-    /// Maximum inflation on the total supply every epoch.
-    pub max_inflation_rate: Rational,
-    /// Expected number of blocks per year
-    pub num_blocks_per_year: NumBlocks,
-    /// Protocol treasury account
-    pub protocol_treasury_account: AccountId,
-    /// Fishermen stake threshold.
-    pub fishermen_threshold: Balance,
-    /// The minimum stake required for staking is last seat price divided by this number.
-    pub minimum_stake_divisor: u64,
-}
-
 pub struct ProtocolConfig {
     pub genesis_config: GenesisConfig,
     pub runtime_config: RuntimeConfig,
 }
 
-impl From<ProtocolConfig> for ProtocolConfigView {
-    fn from(protocol_config: ProtocolConfig) -> Self {
-        let ProtocolConfig { genesis_config, runtime_config } = protocol_config;
-
-        ProtocolConfigView {
-            protocol_version: genesis_config.protocol_version,
-            genesis_time: genesis_config.genesis_time,
-            chain_id: genesis_config.chain_id,
-            genesis_height: genesis_config.genesis_height,
-            num_block_producer_seats: genesis_config.num_block_producer_seats,
-            num_block_producer_seats_per_shard: genesis_config.num_block_producer_seats_per_shard,
-            avg_hidden_validator_seats_per_shard: genesis_config
-                .avg_hidden_validator_seats_per_shard,
-            dynamic_resharding: genesis_config.dynamic_resharding,
-            protocol_upgrade_stake_threshold: genesis_config.protocol_upgrade_stake_threshold,
-            epoch_length: genesis_config.epoch_length,
-            gas_limit: genesis_config.gas_limit,
-            min_gas_price: genesis_config.min_gas_price,
-            max_gas_price: genesis_config.max_gas_price,
-            block_producer_kickout_threshold: genesis_config.block_producer_kickout_threshold,
-            chunk_producer_kickout_threshold: genesis_config.chunk_producer_kickout_threshold,
-            online_min_threshold: genesis_config.online_min_threshold,
-            online_max_threshold: genesis_config.online_max_threshold,
-            gas_price_adjustment_rate: genesis_config.gas_price_adjustment_rate,
-            runtime_config,
-            transaction_validity_period: genesis_config.transaction_validity_period,
-            protocol_reward_rate: genesis_config.protocol_reward_rate,
-            max_inflation_rate: genesis_config.max_inflation_rate,
-            num_blocks_per_year: genesis_config.num_blocks_per_year,
-            protocol_treasury_account: genesis_config.protocol_treasury_account,
-            fishermen_threshold: genesis_config.fishermen_threshold,
-            minimum_stake_divisor: genesis_config.minimum_stake_divisor,
-        }
-    }
-}
-
 pub fn get_initial_supply(records: &[StateRecord]) -> Balance {
     let mut total_supply = 0;
     for record in records {
diff --git a/core/chain-configs/src/lib.rs b/core/chain-configs/src/lib.rs
index e62d53f38..3ab57b8a2 100644
--- a/core/chain-configs/src/lib.rs
+++ b/core/chain-configs/src/lib.rs
@@ -8,5 +8,5 @@ pub use client_config::{
 };
 pub use genesis_config::{
     get_initial_supply, Genesis, GenesisConfig, GenesisRecords, GenesisValidationMode,
-    ProtocolConfig, ProtocolConfigView,
+    ProtocolConfig,
 };

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

*View structs served as an API layer for the view-client. I am not sure whether it is still worth to keep the view-client API stable, but I would decide about it in a separate PR that is outside of JSON RPC scope.

near-primitives = { path = "../../core/primitives" }
near-chain-configs = { path = "../../core/chain-configs" }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

@@ -10,7 +16,7 @@ pub struct RpcProtocolConfigRequest {
#[derive(Serialize, Deserialize, Debug)]
pub struct RpcProtocolConfigResponse {
#[serde(flatten)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are #[serde(flatten)] this anyway,do we need a separate ProtocolConfigView struct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I thought about that, but thought I'd retain an interface downstream users were already familiar with. But you're right. Since we're redesigning the crate anyway, might as well go all the way.

@matklad
Copy link
Contributor

matklad commented Jun 20, 2022

so it would makes sense to maybe take advantage of experementality and to re-design the struct such that it's more forward-compatible

To clarify, it would make sense to do that once we have time to do nice things, ha-ha

Comment on lines +492 to +493
#[serde(default = "StackLimiterVersion::v0")]
pub stack_limiter_version: StackLimiterVersion,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Presumably this can be just an Option? For all new fields we add in the future, I think they will be options in this struct, but not in the actual config. And if we are doing that for new fields, might as well do for fields which we added before?

@matklad
Copy link
Contributor

matklad commented Jun 20, 2022

Note to self: after this is merged:

  • remove serde defauls from runtime config and related structs
  • think if there's some smart way we can spin parametres infra to remove serializes from it as well?

@stale
Copy link

stale bot commented Jul 4, 2022

This PR has been automatically marked as stale because it has not had recent activity in the 2 weeks.
It will be closed in 3 days if no further activity occurs.
Thank you for your contributions.

@stale stale bot added the S-stale label Jul 4, 2022
@akhi3030 akhi3030 removed the S-stale label Jul 8, 2022
Copy link
Collaborator

@frol frol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lovely! ❤️

@@ -583,7 +583,7 @@ impl Genesis {
// Note: this type cannot be placed in primitives/src/view.rs because of `RuntimeConfig` dependency issues.
// Ideally we should create `RuntimeConfigView`, but given the deeply nested nature and the number of fields inside
// `RuntimeConfig`, it should be its own endeavor.
#[derive(Serialize, Deserialize, Debug)]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

*View structs served as an API layer for the view-client. I am not sure whether it is still worth to keep the view-client API stable, but I would decide about it in a separate PR that is outside of JSON RPC scope.

@akhi3030
Copy link
Collaborator

I believe this PR is no longer actively being worked on.

@akhi3030 akhi3030 closed this Apr 20, 2023
@Ekleog-NEAR Ekleog-NEAR deleted the decouple/jsonrpc/extract/config branch March 29, 2024 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants