-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
feat(chisel): Introduces a Solidity REPL #3465
Conversation
feat: Solang Parser
Start builtin command module
this is pretty cool. fyi: I started hacking on this here #240 perhaps some parts are still useful |
Awesome!! Please shoot over any comments and let us know if there's a better way of architecting! |
fix: Environment Versioning
@@ -464,18 +465,17 @@ fn write_json( | |||
path: impl AsRef<Path>, | |||
json_path_or_none: Option<&str>, | |||
) -> Result<Bytes, Bytes> { | |||
let json: Value = serde_json::from_str(object).unwrap_or(Value::String(object.to_owned())); | |||
let json: Value = | |||
serde_json::from_str(object).unwrap_or_else(|_| Value::String(object.to_owned())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why have a map here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm, curious about why clippy would prefer a map. @mattsse, any idea?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the big book o' clippy lints, it probably has something to do with allocating heap memory for the object
str slice via to_owned
, but not 100% sure. It likely is an unnecessary change given that it's not very expensive.
chisel/src/cli.rs
Outdated
}); | ||
|
||
// Print intro header | ||
println!("Welcome to Chisel! Type `{}` to show available commands.", Paint::green("!help")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resolved
Should we ship a lib called That way we could instantly make |
This would be a pretty nice addition! Could also make a cheatcode for accessing the address book, i.e. |
yes please. this would save me time lost searching plus the context switching many times per month |
Resurfacing the relevant address book discussion started in gakonst/ethers-rs#769 (comment), since trueblocks has a large mainnet DB that could be a good starting point |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay, finally went through this.
Overall, I think this looks pretty good already.
Have a bunch of nits/suggestions.
Once addressed, I'll take another look at it.
.gitmodules
Outdated
[submodule "testdata/lib/forge-std"] | ||
path = testdata/lib/forge-std | ||
url = https://github.com/foundry-rs/forge-std |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this has since been removed
does chisel require forge-std in tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Chisel includes Vm.sol
in the binary for use within the compilation source, so the inclusion of cheatcodes would have to be reworked if we decide to remove it. The idea here was to not have to fetch the source from GitHub so that chisel can be used in an offline setting.
See: https://github.com/whitenois3/foundry/blob/feat/repl/chisel/src/session_source.rs#L20
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
having the submodule there caused the tests to fail iirc.
can we replace this with the Cheats.sol
file in testdata instead? perhaps rename to Vm?
The reason why we don't have forge-std as a module here is that we'd otherwise have a cyclical dependency forge <-> forge-std
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good- adjusted the session source to use Cheats.sol
. We could rename it for continuity, but shouldn't be necessary. Kept the variable name as vm
, so no ux changes apart from changing the import.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@clabby okay play around with this a bit and had a closer look. I have a pretty good understanding now, so should be able to debug any discovered bugs I think.
last blocker is the q regarding the submodule vs. existing Cheats.sol
good to merge otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
needs |
Whoops, sorry about that- done 👍 |
great, should we merge now or wait until we have this in the book? |
I think let's ship ASAP and @Vex can fast-follow-up on the book before tweeting out the full thing? |
Current PR Status:
RFC
This pr introduces a Solidity REPL called
chisel
.chisel
is a foundry-native solidity REPL that is largely based off of soli with the goal of being "feature complete" (insofaras soli presents feature completion).Motivation
Closes issue #230
Solution
Creates a new
chisel
crate