-
Notifications
You must be signed in to change notification settings - Fork 610
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
refactor: handle state patching in a more principled way #6967
Conversation
follow up to #6962 |
chain/chain/src/chain.rs
Outdated
self.runtime_adapter.get_epoch_protocol_version(block.header().epoch_id())?; | ||
/// Sandbox node specific operations | ||
impl Chain { | ||
pub fn patch_state(&mut self, patch: StatePatch) { |
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.
Should we still add #[cfg(feature = "sandbox")] here?
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.
I think we shouldn't. The current design guarantees that you only can have an StatePatch
if cfg(feature = "sandbox")
is true. That is, the only thing which is gated by the feature are StatePatch
constructors.
I am bit torn on whether this is a good approach. I like it because it is more convenient (conditional compilation is localized) and because it is more robust (you don't need to make sure that every use-site is feature gated, there's type-system level guarantee that, to do anything with StatePatch, you have to be sandbox).
I don't like it because it is somewhat non-obvious, and any future reader would think "wait, shouldn't this be gated by cfg", and would have to spend some cycles looking at the definition of StatePatch and figuring the pattern of gating constructors out.
On balance, I think this falls under acceptable and encourages uses of the type system, and is not an abuse just to add some fanciness. But happy to reconsider if folks feel otherwise!
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.
Can we add a comment in the code documenting that? I think that should be good!
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.
+1 for more documentation here..
and I wonder if we maybe should call it 'SandboxStatePatch'?
(as I'm worried that future reader would think that state_patch is a 'normal' part of the system used in production)
996418b
to
b9b7eb7
Compare
9c81fe1
to
61bf628
Compare
) -> Result<ApplyResult, RuntimeError> { | ||
let _span = tracing::debug_span!( | ||
target: "runtime", | ||
"apply", | ||
num_transactions = transactions.len()) | ||
.entered(); | ||
|
||
if states_to_patch.is_some() && !cfg!(feature = "sandbox") { |
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.
I'd leave this -- as the 'final' line of defense.
core/primitives/src/state_patch.rs
Outdated
} | ||
|
||
impl StatePatch { | ||
// NB: it's crucial that all creation APIs are guarded with `#[cfg(feature = "sandbox")]`. |
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.
'records' is a public field, so people could create StatePatch as:
StatePatch {records: XX}
right?
chain/chain/src/chain.rs
Outdated
self.runtime_adapter.get_epoch_protocol_version(block.header().epoch_id())?; | ||
/// Sandbox node specific operations | ||
impl Chain { | ||
pub fn patch_state(&mut self, patch: StatePatch) { |
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.
+1 for more documentation here..
and I wonder if we maybe should call it 'SandboxStatePatch'?
(as I'm worried that future reader would think that state_patch is a 'normal' part of the system used in production)
The main idea here is to a avoid cfg-guarding all places related to state patching, and instead guard only patch-creation API. Stated positively, presense of `StatePatch` object is a proof that you are in a sandbox.
The main idea here is to a avoid cfg-guarding all places related to state patching, and instead guard only patch-creation API. Stated positively, presense of
StatePatch
object is a proof that you are in a sandbox.