refactor: introduce ViewConfig for storing max_gas_burnt_view
#4716
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Longarithm
requested review from
ailisp,
bowenwang1996,
EgorKulikov,
frol,
matklad,
MaksymZavershynskyi and
olonho
as code owners
Longarithm
added
the
A-transaction-runtime
matklad
reviewed
Aug 20, 2021
runtime/near-vm-logic/src/logic.rs
Outdated
| let max_gas_burnt = if context.view_config.is_view { | ||
| match context.view_config.max_gas_burnt_view { | ||
| Some(max_gas_burnt_view) => max_gas_burnt_view, | ||
| None => config.limit_config.max_gas_burnt, |
There was a problem hiding this comment.
Hm, am I correct that this is essentially dead code? Every time we view_config.is_view is true, view_cconfig.max_gas_burnt is Some?
If that's the case, I suggest making this code staticall unreachable, as follows:
impl VmContext {
view_config: Option<ViewConfig>
}
struct ViewConfig {
max_gas_burnt: gas
}
than, we'll be passing Option<VIewConfig> everywhere, with the semantics that we are in a view if the config is Some.
core/primitives-core/src/config.rs
Outdated
Comment on lines
30
to
31
| /// TODO: remove this field because it does not affect the protocol | ||
| pub max_gas_burnt_view: Gas, |
There was a problem hiding this comment.
How hard is it to fix this? I'd like to prioritize this follow up, if possible, as this is a significant chunk of debt.
There was a problem hiding this comment.
ah, I didn't read the PR description properly, you are tackling this in the next change, thanks!
| deposit: 0, | ||
| }; | ||
| let view_config = | ||
| ViewConfig { is_view: true, max_gas_burnt_view: Some(self.max_gas_burnt_view) }; |
There was a problem hiding this comment.
This looks so much nicer now, thanks!
added 2 commits
August 20, 2021 18:04
matklad
approved these changes
Aug 20, 2021
bowenwang1996
approved these changes
Aug 20, 2021
| pub is_view: bool, | ||
| /// If non-None, means that execution is made in a view mode and defines its configuration. | ||
| /// View mode means that only read-only operations are allowed. | ||
| /// See https://nomicon.io/Proposals/0018-view-change-method.html for more details. |
Co-authored-by: Bowen Wang <bowenwang1996@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
One step towards #4649.
max_gas_burnt_viewshould not be a part ofRuntimeConfig, see #3080 for more details. While it is the case, we have to modifyRuntimeConfiginside runtime in a hacky way.Here we introduce
ViewConfigto store view parameters and remove usages ofmax_gas_burnt_viewfromRuntimeConfig. In the following protocol upgrade, this field will be removed.Test plan
python3 tests/sanity/rpc_max_gas_burnt.pyNayduck run: http://nayduck.eastus.cloudapp.azure.com:3000/#/run/2013