LFVM: Refactor interpreter Configuration, test registration of experimental and default features. #868
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
LuisPH3
force-pushed
the
luis/lfvm_test_register_experimental
branch
2 times, most recently
from
08844a2
to
7743183
Compare
facuMH
reviewed
Oct 15, 2024
go/interpreter/lfvm/converter.go
Outdated
| // Converter converts EVM code to LFVM code. | ||
| type Converter struct { | ||
| // converter converts EVM code to LFVM code. | ||
| type converter struct { | ||
| config ConversionConfig |
There was a problem hiding this comment.
does ConversionConfig needs to be public ?
go/interpreter/lfvm/lfvm.go
Outdated
| // configuration for production purposes. | ||
| func NewInterpreter(Config) (*lfvm, error) { | ||
| return newVm(config{ | ||
| // DefaultConfiguration ist the set of configuration values sanctioned for |
There was a problem hiding this comment.
Suggested change
| // DefaultConfiguration ist the set of configuration values sanctioned for | |
| // DefaultConfiguration is the set of configuration values sanctioned for |
go/interpreter/lfvm/lfvm.go
Outdated
Comment on lines
35
to
39
| return Configuration{ | ||
| ConversionConfig: ConversionConfig{ | ||
| WithSuperInstructions: false, | ||
| SuperInstructionsEnabled: false, | ||
| }, | ||
| WithShaCache: true, | ||
| }) | ||
| ShaCache: true, |
There was a problem hiding this comment.
if these values are supposed to be changed by the functions below, should they not be private ?
| // This test is slightly different from other tests because of dealing with the | ||
| // global registry: | ||
| // - It is declared in it's own package to avoid leaking the registration to other tests. | ||
| // - It tests different properties, in one single function. The reason is that the |
There was a problem hiding this comment.
Suggested change
| // - It tests different properties, in one single function. The reason is that the | |
| // - It tests different properties in one single function. The reason is that the |
go/interpreter/lfvm/converter.go
Outdated
| // WithSuperInstructions enables the use of super instructions. | ||
| WithSuperInstructions bool | ||
| // SuperInstructionsEnabled mark the use of super instructions. | ||
| SuperInstructionsEnabled bool |
There was a problem hiding this comment.
what I do not like about these fields being public is that anyone can change them. Maybe they should only be changed during construction and to read them a getter should be provided ? wdyt ?
As recommended in best practices
LuisPH3
force-pushed
the
luis/lfvm_test_register_experimental
branch
from
7743183
to
1065d2f
Compare
|
refactor will not be done, instead the file will be tested as it is now. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
part of #684
This PR refactors lfvm.go to clean up the public interface of the lfvm package
There were three different configuration structs found, in the package, in the interpreter and in the step function. The three objects have been merged into 2 objects:
configurationis an opaque object that allows configuring the interpreter. Because the default value may not suffice, a functionDefaultConfigurationis created with the features planned for the release.configurationOverridesis a key-value map that can set a subset of configuration flags, final set of configs is TBD.The public interface of LFVM is:
ConfigurationDefaultConfigurationis the sanctioned features for the release.ConfigurationOverridesis created because the unused any type passed to the factory, this type can be rewritten in any way.Interpreteris the old lfvm instance. It is public to allow tests to query configuration parameters from outside the package.NewInterpreteris the only constructorRegisterExperimentalInterpreterConfigurationscan be called to register more configurationsTo prevent polluting the registry during testing, registry tests use the lfvm_test package.