Closed brianmcmichael closed 4 years ago
So in general this looks very good, there are just a few general comments I want to make:
Not really sure if it is a super convention but we were trying that the shell files pass the shellcheck
revisions, at least the simplest ones. If you use vscode there is a nice plugging that underlines these warnings. Most of them are due missing double quotes. The cli had a lot of them and when I needed to do some change in a file I tried to get it without any shellcheck
warning. We might try to do the same for the files were touched in this PR.
There are several places which I think we could simplify the nested calls. All the signatures that do not specify the return value will return the bytes32 hex version. So just removing the return value we would avoid having to call seth --to-hex
which is the major source of new shellcheck
warnings as it is missing to wrap the new nested thing in quotes.
There are other cases where it gets a decimal due the return value, converts to hex with --to-hex
and then back to decimal with --to-dec
. This is super minor stuff but we might want to make them shorter.
Have just tested a couple of commands, meaning that I'm far to say that all is working fine, so we should evaluate if we really want to do a massive round of testing or maybe when releasing we specify it is an alpha version and let the users to test it out.
Thanks, Gonzalo, fwiw, this was merely to get the app back to a working state, I didn't spend much time optimizing the code, I can go back and clean up some of these extra commands though. I'd worry about relying too much on the shellcheck linter without having the unit tests in here to catch the places where it gives a false positive and breaks something. I'll put it in there and see what I can come up with though.
Also, note that I did comment out the functions in mcd test
, because it does some things like wrap 10 keth and send them into the system, which is pretty scary if you're not sure it's going to do that. For now, the dev should run the tests by uncommenting the functions in the test suite and then running them, I don't feel comfortable having code that can take your eth (even test eth) when you call mcd test
. We may be better off somehow implementing hevm
in to these tests, but that's a separate project.
I agree we need to be careful touching old stuff due linter issues. That is why I basically was touching only files that I was modifying for other reasons, so basically testing them as well.
Most of the new linter errors have to do with the addendum of seth --tohex
which should be necessary removing the return value of the sig, making these files even more similar to what they were.
Looking forward for this one.
All looks good now. As I said before I haven't actually tested the different commands, just looking the code. We might want to release this with a proper warning.
There is something I'd like to evaluate for the future and is the usage of mcd --to-wad
and --to-ray
as I think it would be much better they expect a decimal than an hex.