-
Notifications
You must be signed in to change notification settings - Fork 622
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
feat: Refactor Config out of Runtime #3459
Conversation
@@ -113,6 +114,7 @@ impl EpochInfoProvider for SafeEpochManager { | |||
/// TODO: this possibly should be merged with the runtime cargo or at least reconciled on the interfaces. | |||
pub struct NightshadeRuntime { | |||
genesis_config: GenesisConfig, | |||
genesis_runtime_config: Arc<RuntimeConfig>, |
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.
why do we need to do this? runtime config is part of genesis_config
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.
You'd have to clone it every time to wrap into Arc
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.
It seems that you could implement it with lifetimes, but Arc is fine
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.
Lifetimes tend to poison code. It is better if we first solidify our codebase with smart pointers and then switch to references.
@@ -113,6 +114,7 @@ impl EpochInfoProvider for SafeEpochManager { | |||
/// TODO: this possibly should be merged with the runtime cargo or at least reconciled on the interfaces. | |||
pub struct NightshadeRuntime { | |||
genesis_config: GenesisConfig, | |||
genesis_runtime_config: Arc<RuntimeConfig>, |
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.
It seems that you could implement it with lifetimes, but Arc is fine
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.
Some nits.
It is unclear what RuntimeConfig::from_protocol_version
is for.
@@ -113,6 +114,7 @@ impl EpochInfoProvider for SafeEpochManager { | |||
/// TODO: this possibly should be merged with the runtime cargo or at least reconciled on the interfaces. | |||
pub struct NightshadeRuntime { | |||
genesis_config: GenesisConfig, | |||
genesis_runtime_config: Arc<RuntimeConfig>, |
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.
Lifetimes tend to poison code. It is better if we first solidify our codebase with smart pointers and then switch to references.
Move `RuntimeConfig` from `Runtime` initialization to `ApplyState` object. This makes `Runtime` completely stateless and allows to execute transitions based on different configs. It allows to upgrade `RuntimeConfig` with the new fees based on the protocol version for the current block. This change should be NOOP and doesn't change the protocol. NEP: near/NEPs#120 Issue: #3158 # Test plan: - [X] CI - [x] http://nayduck.eastus.cloudapp.azure.com:3000/#/run/553
Move `RuntimeConfig` from `Runtime` initialization to `ApplyState` object. This makes `Runtime` completely stateless and allows to execute transitions based on different configs. It allows to upgrade `RuntimeConfig` with the new fees based on the protocol version for the current block. This change should be NOOP and doesn't change the protocol. NEP: near/NEPs#120 Issue: #3158 # Test plan: - [X] CI - [x] http://nayduck.eastus.cloudapp.azure.com:3000/#/run/553
Move `RuntimeConfig` from `Runtime` initialization to `ApplyState` object. This makes `Runtime` completely stateless and allows to execute transitions based on different configs. It allows to upgrade `RuntimeConfig` with the new fees based on the protocol version for the current block. This change should be NOOP and doesn't change the protocol. NEP: near/NEPs#120 Issue: #3158 # Test plan: - [X] CI - [x] http://nayduck.eastus.cloudapp.azure.com:3000/#/run/553
Move
RuntimeConfig
fromRuntime
initialization toApplyState
object.This makes
Runtime
completely stateless and allows to execute transitions based on different configs.It allows to upgrade
RuntimeConfig
with the new fees based on the protocol version for the current block.This change should be NOOP and doesn't change the protocol.
NEP: near/NEPs#120
Issue: #3158
Test plan: