Extend ExecutionContext with states #193
Conversation
Codecov Report
@@ Coverage Diff @@
## master #193 +/- ##
==========================================
+ Coverage 79.07% 79.31% +0.23%
==========================================
Files 16 16
Lines 1405 1421 +16
==========================================
+ Hits 1111 1127 +16
Misses 294 294
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
I like these changes. Don't know how GetState compiles into library (being a template function), but still, looking good (an implicit cast is made).
I hope this function doesn't break my C++ portable vm of C# 😂
Advice is the same here @shargon, I think we should test and use it on ApplicationEngine before we merge this... so we are sure this is useful.
|
@erikzhang If I'm following your reasoning here, perhaps we should declare Context as sealed, and end future inheritance tries. This expresses more clearly the intend of not extending this. |
| @@ -1074,12 +1071,11 @@ private bool ExecuteInstruction() | |||
| return true; | |||
| } | |||
|
|
|||
| private void LoadContext(ExecutionContext context) | |||
| protected virtual void LoadContext(ExecutionContext context) | |||
There was a problem hiding this comment.
Will we still inherit from this?
There was a problem hiding this comment.
Is the unique way to set the states
There was a problem hiding this comment.
So event based may be better than this, if we can seal the class and use event to manage internals...
Co-Authored-By: Igor Machado Coelho <igor.machado@gmail.com>
|
How can I share the script Hash on ContextLoader? Also I want to cache this in this states, but i only can check if it is the same as the calling Context |
|
What is ContextLoader? |
|
sorry, |
|
You can override |
|
We should decide if is better with events or with |
|
There are currently no other places where these events are used. And the event can be listened to by multiple listeners, and you can't control the order of execution, which is very unfavorable for the method that can modify the parameters. |
|
Is ready to me, i am not sure if this could help with the script hash cache, because is related to the script hash, but this is not the priority now, just focus on neo-project/neo#927 |
|
@erikzhang we can still overide a method on sealed class? learning things every day... |
Closes #191