sputnikvm icon indicating copy to clipboard operation
sputnikvm copied to clipboard

Option improvements

Open rocky opened this issue 7 years ago • 3 comments
trafficstars

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?

rocky avatar Jun 06 '18 00:06 rocky

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.

whilei avatar Jun 06 '18 07:06 whilei

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.

rocky avatar Jun 08 '18 08:06 rocky

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.

rocky avatar Jun 18 '18 22:06 rocky