Add initial support for pluggable syscalls #1922
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #1922 +/- ##
==========================================
- Coverage 75.64% 75.46% -0.19%
==========================================
Files 153 156 +3
Lines 15120 15234 +114
==========================================
+ Hits 11438 11496 +58
- Misses 3682 3738 +56
|
fridrik01
force-pushed
the
pluggable-syscalls
branch
3 times, most recently
from
ae8f628 to
f689c46
Compare
fridrik01
force-pushed
the
pluggable-syscalls
branch
3 times, most recently
from
bc72b56 to
a4f8261
Compare
| pub caller: ActorID, | ||
| pub actor_id: ActorID, | ||
| pub method: MethodNum, | ||
| pub value_received: TokenAmount, | ||
| pub read_only: bool, |
There was a problem hiding this comment.
I'm starting to wonder how this will interact with testing. E.g., I want to support FilecoinKernel<TestKernel<DefaultKernel>>. Options include:
- Using accessor methods here. IIRC, we never actually change any of these fields (or the ones below), we just read them.
- Require that all kernels "deref" to some
BaseKernel. - Alternatively, require that all kernels expose some form of "invocation state"?
I think we can leave it alone for now, but I figured I'd comment on this in case you had any ideas.
fvm/src/kernel/mod.rs
Outdated
| pub trait Kernel: | ||
| ActorOps | ||
| SyscallHandler<<Self as Kernel>::Kernel> |
There was a problem hiding this comment.
Do we need this? IMO:
- This really should be the other way around (kernels are syscall handlers).
- I'm not sure if we even need to care about that either. We can likely treat them as different things.
So, I think the SyscallHandler implementation changes to:
impl<K, T> SyscallHandler<K> for DefaultFilecoinKernel<T>
where
K: Kernel,
T: SyscallHandler<Self>There was a problem hiding this comment.
Are you suggesting removing SyscallHandler from the Kernel? How would we call the bind_syscalls on it when initializing the engine then, or am I misunderstanding?
There was a problem hiding this comment.
I was... but yeah, that might be even more annoying (we'd have to change all : Kernel constraints to : SyscallHandler or : Kernel + SyscallHandler. So, maybe later but not now.
I could not get this working as a method on the ActorOps trait due to ambassador constantly acting up.
fridrik01
force-pushed
the
pluggable-syscalls
branch
from
408ea3f to
a57a4ca
Compare
fridrik01
force-pushed
the
pluggable-syscalls
branch
from
c52f141 to
1c2ee28
Compare
| @@ -185,6 +172,100 @@ where | |||
| }, | |||
| }) | |||
| } | |||
|
|
|||
| fn upgrade_actor<K: Kernel<CallManager = C>>( | |||
There was a problem hiding this comment.
It may make sense to move this, but can we wait for a new patch?
There was a problem hiding this comment.
(or we can just leave it, we're going to have to refactor all this anyways)
| self.0.machine() | ||
| } | ||
|
|
||
| fn send<K: Kernel<CallManager = C>>( |
There was a problem hiding this comment.
Hm. I hadn't realized we had moved this to the root trait (should have been more thorough when I reviewed that patch...). Having to manually delegate this is... annoying, but I guess we can fix it in a followup patch.
Really, I'm now convinced we need to refactor the entire Kernel trait stack.
fridrik01
changed the title
See: #1787
This PR adds initial support for pluggable syscall by allowing us to extend the default kernel and add new custom syscalls through a new kernel.
The implementation does this by moving the global
bind_syscallfunction into kernel via aSyscallHandlertrait where each kernel needs to add their new syscalls.The implementation uses Ambassador crate which automatically create stubs for delegating kernel implementations we are not overriding in new kernels, minimizing the code we need to write.