feat: runtime: Add send_generalized #1126
Conversation
arajasek
force-pushed
the
asr/send-generalized
branch
from
70bac1a to
0ea75b1
Compare
arajasek
force-pushed
the
asr/send-generalized
branch
from
0ea75b1 to
b711fb6
Compare
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #1126 +/- ##
==========================================
- Coverage 89.07% 89.04% -0.03%
==========================================
Files 94 95 +1
Lines 19718 19805 +87
==========================================
+ Hits 17563 17635 +72
- Misses 2155 2170 +15
|
There was a problem hiding this comment.
Thanks. I'll address some of the q's you asked in Slack here too. Basically I agree with your suggestions.
I think the Runtime interface should have just a single send method, with the more general signature, and we should refactor the existing call sites.
There are two essential helper things going on:
- default gas limit and flags
- error number interpretation
A question is whether to combine them into one wrapper, or split in the input/output. The default params would be fine in a default impl Runtime::send_simple() method. But there's too much code in the error-number interpretation to sit nicely in a default implementation there.
So if we can separate them and then have a helper in runtime/builtin/shared or runtime/util that does the error number interpretation, the refactor will be to change most call sites to send_simple, and then adapt the result/response.
test_vm/src/lib.rs
Outdated
| @@ -571,6 +578,13 @@ pub struct InvocationCtx<'invocation, 'bs> { | |||
| caller_validated: bool, | |||
| policy: &'invocation Policy, | |||
| subinvocations: RefCell<Vec<InvocationTrace>>, | |||
| actor_exit: RefCell<Option<ActorExit>>, | |||
There was a problem hiding this comment.
Please document this - it's not obvious what it is, when it's set, why it's needed, etc. It should probably move above the subinvocations too.
test_vm/src/lib.rs
Outdated
| |panic| { | ||
| if self.actor_exit.borrow().is_some() { | ||
| let exit = self.actor_exit.take().unwrap(); | ||
| if exit.code == 0 { |
There was a problem hiding this comment.
This shouldn't happen in a panic, should it? It seems wrong, and I'd rather the test crashed than let this throuhg.
test_vm/src/lib.rs
Outdated
| @@ -660,6 +675,27 @@ impl<'invocation, 'bs> InvocationCtx<'invocation, 'bs> { | |||
| self.resolve_target(&self.msg.to).unwrap().1 | |||
| } | |||
|
|
|||
| fn invoke_actor(&mut self) -> Result<Option<IpldBlock>, ActorError> { | |||
| std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| self.invoke())).unwrap_or_else( | |||
There was a problem hiding this comment.
What's this for? I think we view a panic as always a bug in actors, so we want tests to fail.
arajasek
force-pushed
the
asr/send-generalized
branch
from
b711fb6 to
6e294f8
Compare
arajasek
force-pushed
the
asr/send-generalized
branch
from
076e7b6 to
a27da93
Compare
|
@anorth I've performed the refactor as discussed. I think it's relatively clean, though I'd welcome a better name than I've also dropped the changes around |
There was a problem hiding this comment.
Thanks. This is good and I'd be happy for it to land as-is. I think there are opportunities to improve the aesthetics, depending on your enthusiasm.
It would be nice to chain here, so we could make calls like rt.send_simple().as_result(...)? or similar. We can do this if you make a new struct SendResult(Result<fvm_shared::Response, fvm_shared::error::ErrorNumber>) to wrap the FVM type, to which you can then attach a method equivalent to extract_send_result.
It might be worth adding another helper (or method on local result type) that wraps up the deserialize_block(extract_send_result(..)) sequence that appears a few times.
|
Thanks. I'm going to merge this in the interest of time as-is. |
* feat: runtime: Add send_generalized * Refactor: Runtime: Add send_simple and extract_send_result methods * Address review
Extracted from
next. Needs:This PR adds a new
send_generalizedmethod to theRuntime. The key differences betweensendandsend_generalizedare that:send_generalizedcan take an optionalgas_limitthat restricts the total gas usage of the sendsend_generalizedcan carry optionalSendFlags-- currently the onlySendFlagthe FVM supports isREAD_ONLYsend_generalizedsegregates failures arising from the syscall prior to target invocation (returned as anErrorNumber) and those arising from the invocation itself (returned as anExitCode).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.