Skip to content

Conversation

@onbjerg
Copy link
Contributor

@onbjerg onbjerg commented Oct 27, 2025

Looking at the config struct, I noticed some fields seem to no longer be used, so I marked them as potential removal candidates. I also cleaned up some of the comments.

PR Checklist

  • Added Tests
  • Added Documentation
  • Breaking changes

@onbjerg onbjerg enabled auto-merge October 27, 2025 09:24
Comment on lines 436 to 441
/// Whether to print the names of the compiled contracts.
// todo(onbjerg): this seems unused.
pub names: bool,
/// Whether to print the sizes of the compiled contracts.
// todo(onbjerg): this seems unused.
pub sizes: bool,
Copy link
Member

@zerosnacks zerosnacks Oct 27, 2025

Choose a reason for hiding this comment

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

These should not be unused; should correspond with the forge build --sizes / forge build --names / forge build --names --sizes flags

I am not sure if this ever worked

https://github.com/morpho-org/morpho-optimizers/blob/04143d929372e53d3f6822f62ee90df1609fc784/foundry.toml#L9-L10

Comment on lines +404 to 405
// todo(onbjerg): this seems unused.
pub memory_limit: u64,
Copy link
Member

Choose a reason for hiding this comment

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

revm does allow configuring this and we are passing the value into it; must not be wired correctly

Copy link
Member

Choose a reason for hiding this comment

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

it is, it goes through the serialize config->deserialize into evmopts dance

Copy link
Member

Choose a reason for hiding this comment

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

actually it's broken but for a different reason, we set it correctly in cfg env #12299

/// List of file paths to ignore.
// todo(onbjerg): how is this different from `skip`?
#[serde(rename = "ignored_warnings_from")]
pub ignored_file_paths: Vec<PathBuf>,
Copy link
Member

Choose a reason for hiding this comment

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

@DaniPopes DaniPopes disabled auto-merge October 27, 2025 10:51
Copy link
Member

@DaniPopes DaniPopes left a comment

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

5 participants