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

feat: foundry-cheatcodes #5998

Merged
merged 1 commit into from
Oct 17, 2023
Merged

Conversation

DaniPopes
Copy link
Member

@DaniPopes DaniPopes commented Oct 5, 2023

Statically define all cheatcodes by leveraging Alloy and traits (for internal use) with a stable JSON schema (for external and third party use).

TODO

  • Finish spec
  • Migrate HEVM -> Vm interface from Ethers abigen! to Alloy sol!
  • Add docs to all cheatcodes
  • Migrate + refactor cheatcode implementations
    • Evm
    • Testing
    • Scripting
    • Filesystem
    • Environment
    • String
    • Json
    • Utilities
  • Migrate + refactor Cheatcodes Inspector implementation

In follow-up

  • Integrate with foundry-evm
  • Implement warnings and errors for deprecated and removed cheatcodes respectively
  • Write up some docs on how it works and how to add new cheatcodes

crates/cheatcodes/Cargo.toml Outdated Show resolved Hide resolved
Copy link
Collaborator

@mds1 mds1 left a comment

Choose a reason for hiding this comment

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

I know it's a draft but left a few small comments, lgtm overall though

Comment on lines 16 to 24
/// The cheatcode's identifier. Corresponds to the function's name,
/// thus is not unique if the function is overloaded.
pub id: &'a str,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not just use one of the signature fields, or equivalently the selector, as the ID so the ID can be unique? Or if there's no benefit to uniqueness, maybe we can remove this field?

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed to use the ID generated by the macro, which is name, or <name>_<index> when overloaded
Can remove, but I don't think it hurts

/// The full Solidity function signature, including parameter names, visibility, etc.
/// This is just for reference, and is identical to the function declaration.
pub full_signature: &'a str,
// TODO: add visibility, state mutability...?
Copy link
Collaborator

Choose a reason for hiding this comment

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

All will have visibility of external, but for mutability here's the way we define when to use pure vs. view vs. neither in forge-std: https://github.com/foundry-rs/forge-std/blob/f73c73d2018eb6a111f35e4dae7b4f27401e9421/src/Vm.sol#L6-L10. Might be useful to include a visibility field in the schema for consistency—there's been some recent PRs correcting visibility of cheats in forge-std that probably didn't make it upstream to foundry

Copy link
Member Author

Choose a reason for hiding this comment

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

Added here d68edfb

crates/cheatcodes/src/defs/mod.rs Outdated Show resolved Hide resolved
pub selector: &'a str,
/// The description of the cheatcode.
/// This is a markdown string derived from the documentation of the function declaration.
/// Empty when the function has no documentation.
Copy link
Member

Choose a reason for hiding this comment

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

We should probably enforce all fns to be documented, though.

Copy link
Member Author

Choose a reason for hiding this comment

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

We can leave this to the compiler with missing_docs

Copy link
Member Author

@DaniPopes DaniPopes Oct 9, 2023

Choose a reason for hiding this comment

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

Actually no, we can't do that because it triggers on struct fields (function params); I'll add it as a nightly-only warning (gets caught in clippy CI)

Copy link
Member

Choose a reason for hiding this comment

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

gotcha this sounds good—having us reviewing + nightly warning should be enough for now! if not, we can always come back and document later

crates/cheatcodes/src/defs/mod.rs Outdated Show resolved Hide resolved
Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

love this direction.

this is so much better

Copy link
Member

@gakonst gakonst left a comment

Choose a reason for hiding this comment

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

overall love this direction and incredible work. what should we do with the json defs? probably need a nice way to incorporate in the book? i also assume this PR will have a bunch of removed old cheatcode code once fully migrated

use super::{Cheatcode, CheatsCtxt, DatabaseExt, Result};
use crate::{impls::CreateFork, Cheatcodes, Vm::*};

impl Cheatcode for activeForkCall {
Copy link
Member

Choose a reason for hiding this comment

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

Should these be capitalized in alloy codegen?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think it should modify names by default, and instead have a rename_all like Serde (not implemented yet)

Copy link
Member

Choose a reason for hiding this comment

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

I remember that implementing this in abigen was also a bit painful to get right

Comment on lines +179 to +206
/// Returns `(signature, hex_selector, declaration, description)` from a given `sol!`-generated
/// docstring for a function.
///
/// # Examples
///
/// The following docstring (string literals are joined with newlines):
/// ```text
/// "Function with signature `foo(uint256)` and selector `0x1234abcd`."
/// "```solidity"
/// "function foo(uint256 x) external view returns (bool y);"
/// "```"
/// "Description of the function."
/// ```
///
/// Will return:
/// ```text
/// (
/// "foo(uint256)",
/// "0x1234abcd",
/// "function foo(uint256 x) external view returns (bool y);",
/// "Description of the function."
/// )
/// ```
fn func_docstring(doc: &str) -> (&str, &str, &str, &str) {
Copy link
Member

Choose a reason for hiding this comment

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

should this be upstreamed

Copy link
Member Author

@DaniPopes DaniPopes Oct 11, 2023

Choose a reason for hiding this comment

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

This is just a hack to get information between sol! internals and cheatcode macro internals, I figured it's good enough to rely on internally. I don't think it should be exposed anywhere

Comment on lines +131 to +136
/// All the cheatcodes in [this contract](self).
pub const CHEATCODES: &'static [&'static Cheatcode<'static>] = &[#(<#variant_tys as CheatcodeDef>::CHEATCODE,)*];
Copy link
Member

Choose a reason for hiding this comment

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

is there some type magic behind why this is const &'static?

Copy link
Member Author

@DaniPopes DaniPopes Oct 11, 2023

Choose a reason for hiding this comment

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

Each cheatcode defines its own &Cheatcode and this aggregates them without having a separate copy of the static data by being &[&T] instead of &[T].
const X: &'static T I think is kind of like static X: T

Comment on lines +10 to +13
fn apply(&self, _state: &mut Cheatcodes) -> Result {
let Self { value } = self;
Ok(value.to_string().abi_encode())
}
Copy link
Member

Choose a reason for hiding this comment

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

generally love this pattern

/// Gets the label for the specified address.
#[cheatcode(group = Utilities)]
function getLabel(address account) external returns (string memory currentLabel);
}
Copy link
Member

Choose a reason for hiding this comment

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

i love this usage of the sol! macro showcasing derive macro composition with the cheatcode macro, beautiful. had not appreciated how mayn cheats we have now

Copy link
Member

@Evalir Evalir left a comment

Choose a reason for hiding this comment

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

Man, we have a lot of cheatcodes.

I love this direction—just a few comments but so far so good

pub selector: &'a str,
/// The description of the cheatcode.
/// This is a markdown string derived from the documentation of the function declaration.
/// Empty when the function has no documentation.
Copy link
Member

Choose a reason for hiding this comment

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

gotcha this sounds good—having us reviewing + nightly warning should be enough for now! if not, we can always come back and document later

@@ -8,11 +8,19 @@ use foundry_config::{
};
use std::path::{Path, PathBuf};

// TODO
#[derive(Clone, Debug, Default)]
pub struct EvmOpts {
Copy link
Member

Choose a reason for hiding this comment

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

hm, this is because we need Config and related structs to be on alloy types right? Might tie in with #5986, but this is a cumbersome/significant refactor

Copy link
Member Author

Choose a reason for hiding this comment

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

It's that EvmOpts is in evm and we need it before in cheatcodes

Copy link
Member

Choose a reason for hiding this comment

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

yeah, one reason why we need a few more crates so we can get rid of circular deps

@DaniPopes DaniPopes marked this pull request as ready for review October 17, 2023 17:08
@DaniPopes DaniPopes changed the title refactor: stable cheatcodes refactor: foundry-cheatcodes Oct 17, 2023
@DaniPopes DaniPopes changed the title refactor: foundry-cheatcodes feat: foundry-cheatcodes Oct 17, 2023
Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

this looks really good, I like this direction a lot.

this is a great first step.
after #5986 we can restructure the evm crate and finally get rid of some mess here.

but since this only adds a new crate we can merge now to reduce conflicts.

@@ -8,11 +8,19 @@ use foundry_config::{
};
use std::path::{Path, PathBuf};

// TODO
#[derive(Clone, Debug, Default)]
pub struct EvmOpts {
Copy link
Member

Choose a reason for hiding this comment

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

yeah, one reason why we need a few more crates so we can get rid of circular deps

use super::{Cheatcode, CheatsCtxt, DatabaseExt, Result};
use crate::{impls::CreateFork, Cheatcodes, Vm::*};

impl Cheatcode for activeForkCall {
Copy link
Member

Choose a reason for hiding this comment

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

I remember that implementing this in abigen was also a bit painful to get right

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.

5 participants