-
Notifications
You must be signed in to change notification settings - Fork 79
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: runtime: Add read_only getter #1127
Conversation
61e43e0
to
532a384
Compare
0ea75b1
to
b711fb6
Compare
532a384
to
532b4b5
Compare
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## asr/send-generalized #1127 +/- ##
========================================================
- Coverage 89.00% 88.97% -0.03%
========================================================
Files 95 95
Lines 19782 19803 +21
========================================================
+ Hits 17607 17620 +13
- Misses 2175 2183 +8
|
ExitCode::USR_READ_ONLY, | ||
format!("cannot create actor {target} in read-only mode"), | ||
)); | ||
} |
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 you point to the spec for read-only? Depending on what it says, I would expect either:
- no checks like this, but something at the end of an invocation that restores the state root to its pre-invocation value, or
- a lot more checks in
create
,transaction
,new_actor_address
,create_actor
, delete_actor` (or something lower down that propagates through these) that actively prevent modifications.
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're right. I've added checks to create
, create_actor
, transaction
, set_state_root
and invoke
.
I'm tweaking the behaviour of the test_vm's new_actor_address
to be read-only, which is how it's actually implemented. The increment in moving to create_actor
which is where it belongs.
b711fb6
to
6e294f8
Compare
532b4b5
to
ae56c79
Compare
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.
Thanks, the main change looks good. Ok as is, but a potential improvement:
It struck me while reviewing the test VM that a single check inside self.v.set_actor()
would suffice to catch all the read-only cases (after a bit of re-ordering to ensure that was the first modification that the VM processes). But I see why we want the read_only
flag to be in the invocation context rather than the self.v
VM. One step better might be to implement a wrapper self.set_actor
which does the check then calls through to self.v
. This wouldn't prevent someone making a mistake, but make future modifications somewhat more likely to be correct. A step further would be to hide self.v
even further inside the context so someone can't call directly by accident; but probably not worth it.
ae56c79
to
d7a03cf
Compare
Extracted from
next
. Needs:This PR simply introduces a getter to the
Runtime
to indicate whether an invocation is inREAD_ONLY
mode, and updates the test framework to consider this status as part of execution.This PR is a pre-factor necessary for landing the changes involved in the FEVM FIPs -- it introduces new functionality, but doesn't start to use it anywhere.