Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Option improvements #329

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Option improvements #329

wants to merge 3 commits into from

Conversation

rocky
Copy link
Contributor

@rocky rocky commented Jun 6, 2018

Not totally sure about this one. I at least wanted better error reporting on giving a bad option.

I had considered putting all of the patches into a HashMap where the keys are the patch option and the values contain the particular VM parametrized by the specific patch. But this would mean creating all of the combinations up front, even though one of them is used.

A midway approach is to list all of the valid patches in a vector that can be displayed in both help and on error. What do you think?

@whilei
Copy link
Contributor

whilei commented Jun 6, 2018

I guess I would lean toward the "midway" option... (since a VM could also use a trait object with multiple patches...)

But for the "hashmap" idea -- although it seems kind of wasteful, I guess the actual overhead would be pretty low if it would only need to be created once, or?

Just another idea on the table - while on one hand I like the idea of keeping configuration documentation "close to code" (where available options on error would be, like, printed to the terminal), it also seems kind of redundant to create extra variables just for logging configuration errors when available configuration is already auto-documented thru Rust, ala https://docs.rs/sputnikvm/0.10.1/sputnikvm/struct.EmbeddedByzantiumPatch.html. Might there be a clean way for an error to simply print a relevant URL for reference? Just an idea.

@rocky
Copy link
Contributor Author

rocky commented Jun 8, 2018

But for the "hashmap" idea -- although it seems kind of wasteful, I guess the actual overhead would be pretty low if it would only need to be created once,

It occurs to me that how you'd do this in other programming languages is just store a function pointer and then instantiate or call the "new" only after seeing the option. My Rust knowledge just isn't up to speed yet to do that.

Might there be a clean way for an error to simply print a relevant URL for reference? Just an idea.

Sure, that makes sense. I need to put my work on this on hold for a little bit, but I'll do that if I come back to this.

code is a little more DRY. Also we can show valid patch names by listing
the keys of map.
@rocky
Copy link
Contributor Author

rocky commented Jun 18, 2018

To get my rust knowlege a little more up to speed, I've now done this. I think it slightly better in that the variations for the different kinds of patches and transaction/vm options are now more isolated at the top of the file rather than buried in the code in two specific places. And we now give a slightly more informative error when an invalid patch is igiven.

There is more that could probably be done in the way of DRY'ing the code between the transaction versus VM based branches of the code. But that would probably require more advanced knowlege of creating macros or more advanced generic templating than I currently have.

I think more important would be to redo testing so this works off the the ethereum standard VM test suite unmodified. Currently there's an old (and modified?) copy that is somewhat hard-coded in.

Copy link

@BuHeP BuHeP left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ETC

@ETCDEVTeam ETCDEVTeam deleted a comment from BuHeP Apr 27, 2019
@ETCDEVTeam ETCDEVTeam deleted a comment from BuHeP Apr 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants