[FVM] refactor4 - separate script and transaction environments #939
Conversation
ramtinms
changed the title
Codecov Report
@@ Coverage Diff @@
## master #939 +/- ##
==========================================
- Coverage 54.76% 54.16% -0.61%
==========================================
Files 286 287 +1
Lines 18913 19213 +300
==========================================
+ Hits 10357 10406 +49
- Misses 7157 7388 +231
- Partials 1399 1419 +20
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
| if e.transactionEnv == nil { | ||
| return errors.NewOperationNotSupportedError("AddEncodedAccountKey") | ||
| } |
There was a problem hiding this comment.
we got rid of this type of conditioning in several places.
| func (e *transactionEnv) RemoveAccountKey(address runtime.Address, keyIndex int) (encodedPublicKey []byte, err error) { | ||
| return e.accountKeys.RemoveAccountKey(address, keyIndex) | ||
| } |
There was a problem hiding this comment.
Methods like this are merged into the main function with the same name so we don't do branching based on the condition transactionEnv != nil
…flow-go into ramtin/fvm-separate-environments
|
I see your point but, I believe in the next PR we can optimize the scriptEnv and get rid of a lot of functions (I didn't want to change any behaviour in this PR), so in the end, might not be that much operations shared between transaction and scripts. The long-term plan is to get rid of something like env and just inject handlers directly to the cadence runtime (we would have readOnlyAccountHander and ... so we are going to reuse the common functionality there) |
ramtinms
changed the title
|
|
||
| // TODO set the flags on context | ||
| eventHandler := handler.NewEventHandler(ctx.Chain, | ||
| ctx.EventCollectionEnabled, |
There was a problem hiding this comment.
Ideally, instead of controlling behaviour with switches it would be better to have different implementations here - if collection is not enabled, something like NoopEventsCollector etc.
There was a problem hiding this comment.
Exactly, that is what is in the list of future PRs, I didn't want to change the logic that much in this PR.
| traceSpan opentracing.Span | ||
| } | ||
|
|
||
| func NewScriptEnvironment(ctx Context, vm *VirtualMachine, sth *state.StateHolder, programs *programs.Programs) *ScriptEnv { |
There was a problem hiding this comment.
This looks like a copy of TransactionEnvironment. Maybe we should have a one common base for both?
There was a problem hiding this comment.
it looks the same now, but won't be the same soon, when we switch the handlers with readonly version of them.
This PR
In the follow-up PRs we are going to make environments even lighter and move the remaining common operations into handlers, basically environments would be a collection of pointers until we just directly inject handlers to the cadence runtime according to the long term plan.