rust: make ExecutionContext optional in EvmcVm.execute()#350
Conversation
jakelang
left a comment
jakelang
left a comment
There was a problem hiding this comment.
I feel like this would be cleaner if ExecutionContext would make its context member optional instead
axic
force-pushed
the
rust-vm-fields
branch
2 times, most recently
from
504f0aa to
6fe50b1
Compare
axic
changed the title
The problem with that is every single feature of |
|
Can this be merged? |
|
I have to checked what this PR is about. I postponed the C++ API changes for Precompiles. BTW, there will be no 6.4. The 7.0 is next. |
axic
force-pushed
the
rust-vm-fields
branch
3 times, most recently
from
b2068c3 to
a273ac9
Compare
chfast
left a comment
chfast
left a comment
There was a problem hiding this comment.
I think this is not correct.
Here the logic is: null host context implies null host interface.
But in reality we have 3 cases:
- interface not null, context not null (usual case),
- interface not null, context null (Host does not want to store anything in the context),
- interface null, context null (precompiles).
| ) | ||
| }; | ||
| container.execute(revision, code_ref, &execution_message, &mut execution_context) | ||
| if context.is_null() { |
There was a problem hiding this comment.
Forgot to change this one line during rebasing, that's the reason it is broken @chfast.
There was a problem hiding this comment.
It looks good now, but is there an unit test what would detect the issue?
There was a problem hiding this comment.
I did run the vmtester on it with precompiles enabled but even the old one passed it.
| _context: Option<&'a mut ExecutionContext<'a>>, | ||
| ) -> ExecutionResult { | ||
| if _context.is_none() { | ||
| return ExecutionResult::failure(); |
There was a problem hiding this comment.
Because the code needs tx_context which is part of that?
3 participants
Fixes #318.