feat(forge): add bypass prevrandao (foundry-rs#12125) & fix(fmt): filter libs when recursing (foundry-rs#12119) #147
Merged
Conversation
* feat(forge): add bypass prevrandao * Update crates/evm/networks/src/lib.rs Co-authored-by: 0xrusowsky <90208954+0xrusowsky@users.noreply.github.com> * changes after review: remove duped code --------- Co-authored-by: 0xrusowsky <90208954+0xrusowsky@users.noreply.github.com>
* fix(fmt): account for ternary operators when estimating size * fix(fmt): filter libs when recursing * style: clippy * test: wipe contracts before formatting * test: explicitly test ignore
Reviewer's GuideThis PR enhances the formatting command to properly exclude libraries during recursive runs, introduces a new bypass_prevrandao configuration option for networks, and refactors environment setup to propagate network settings (including the new flag) across EVM and CLI components. Workspace and test updates ensure defaults and dependencies align with these changes. Sequence diagram for environment setup with bypass_prevrandao propagationsequenceDiagram
participant "CLI/Forge/Cast/Anvil"
participant "EVM Core"
participant "NetworkConfigs"
participant "EnvMut"
participant "BlockResponse"
"CLI/Forge/Cast/Anvil"->>"EVM Core": Call environment(..., configs)
"EVM Core"->>"NetworkConfigs": Pass configs to apply_chain_and_block_specific_env_changes
"EVM Core"->>"EnvMut": Apply chain and block specific changes
"EVM Core"->>"NetworkConfigs": Call bypass_prevrandao(chain_id)
"NetworkConfigs"-->>"EVM Core": Return true/false
"EVM Core"->>"EnvMut": Set prevrandao if bypass required
"EVM Core"->>"EnvMut": Finalize environment setup
Entity relationship diagram for NetworkConfigs and chain typeserDiagram
NETWORKCONFIGS {
bool bypass_prevrandao
}
NAMEDCHAIN {
chain_id u64
}
NETWORKCONFIGS ||--o| NAMEDCHAIN : "uses chain_id for bypass_prevrandao logic"
Class diagram for updated NetworkConfigs struct and related methodsclassDiagram
class NetworkConfigs {
+bool optimism
+bool celo
+bool bypass_prevrandao
+bypass_prevrandao(chain_id: u64) bool
+is_optimism() bool
+is_celo() bool
}
NetworkConfigs <|-- AnyNetwork
class AnyNetwork {
// Inherits network config logic
}
Class diagram for FmtArgs filtering logic updateclassDiagram
class FmtArgs {
+run() Result
-is_under_ignored_dir(file_path: &Path, include_libs: bool) bool
}
FmtArgs --> Input
class Input {
+Paths(Vec<PathBuf>)
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
Summary of ChangesHello @Dargon789, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces significant improvements to Foundry's Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `crates/forge/src/cmd/fmt.rs:108` </location>
<code_context>
- SOLC_EXTENSIONS,
- ));
+ // If the input directory is not a lib directory, make sure to ignore libs.
+ let exclude_libs = !is_under_ignored_dir(path, true);
+ inputs.extend(
+ foundry_compilers::utils::source_files_iter(path, SOLC_EXTENSIONS)
</code_context>
<issue_to_address>
**suggestion:** The exclude_libs logic may not be robust for nested lib directories.
The current approach may not exclude all nested lib directories. Please review whether the logic covers cases where libs are nested but not directly marked, and consider a more comprehensive check for complex directory structures.
Suggested implementation:
```rust
// If the input directory is not a lib directory, make sure to ignore libs.
let exclude_libs = !is_under_ignored_dir(path, true);
inputs.extend(
foundry_compilers::utils::source_files_iter(path, SOLC_EXTENSIONS)
.filter(|p| {
!(ignored.contains(p)
|| ignored.contains(&cwd.join(p))
|| (exclude_libs && is_within_lib_dir(p)))
}),
);
```
```rust
} else if path.is_sol() {
```
```rust
fn is_within_lib_dir(path: &std::path::Path) -> bool {
path.ancestors().any(|ancestor| {
ancestor.file_name()
.map(|name| name == "lib")
.unwrap_or(false)
})
}
```
- If your project uses a different convention for library directories (e.g., "libs" or "libraries"), adjust the string in `is_within_lib_dir` accordingly.
- Ensure that the helper function is placed in a scope accessible to the code block using it.
- If you have a utility module for path helpers, consider moving `is_within_lib_dir` there and importing it.
</issue_to_address>
### Comment 2
<location> `crates/evm/networks/src/lib.rs:50-57` </location>
<code_context>
self.optimism
}
+ pub fn bypass_prevrandao(&self, chain_id: u64) -> bool {
+ if let Ok(
+ Moonbeam | Moonbase | Moonriver | MoonbeamDev | Rsk | RskTestnet | Gnosis | Chiado,
+ ) = NamedChain::try_from(chain_id)
+ {
+ return true;
+ }
+ self.bypass_prevrandao
+ }
+
</code_context>
<issue_to_address>
**suggestion:** The bypass_prevrandao method hardcodes chain IDs, which may be brittle.
Centralizing or making this logic data-driven will help ensure future chain additions or changes are handled consistently and reduce maintenance overhead.
Suggested implementation:
```rust
pub fn bypass_prevrandao(&self, chain_id: u64) -> bool {
// Centralized list of chains that require bypassing prevrandao
use once_cell::sync::Lazy;
use std::collections::HashSet;
static BYPASS_PREVRANDAO_CHAINS: Lazy<HashSet<NamedChain>> = Lazy::new(|| {
[
NamedChain::Moonbeam,
NamedChain::Moonbase,
NamedChain::Moonriver,
NamedChain::MoonbeamDev,
NamedChain::Rsk,
NamedChain::RskTestnet,
NamedChain::Gnosis,
NamedChain::Chiado,
]
.iter()
.cloned()
.collect()
});
if let Ok(chain) = NamedChain::try_from(chain_id) {
if BYPASS_PREVRANDAO_CHAINS.contains(&chain) {
return true;
}
}
self.bypass_prevrandao
}
```
- You may need to add `once_cell` to your dependencies in `Cargo.toml` if it's not already present.
- Ensure that `NamedChain` implements `Clone`, `Eq`, and `Hash` (it likely does if it's used in a HashSet).
- If you prefer not to use `once_cell`, you can use a simple array and `matches!` macro, but the HashSet is more scalable for many chains.
</issue_to_address>
### Comment 3
<location> `crates/evm/core/src/utils.rs:77-61` </location>
<code_context>
}
}
+ if configs.bypass_prevrandao(env.cfg.chain_id) && env.block.prevrandao.is_none() {
+ // <https://github.com/foundry-rs/foundry/issues/4232>
+ env.block.prevrandao = Some(B256::random());
+ }
+
</code_context>
<issue_to_address>
**suggestion:** Randomizing prevrandao may affect determinism in some scenarios.
If reproducibility is required, consider allowing this value to be seeded or controlled.
Suggested implementation:
```rust
if configs.bypass_prevrandao(env.cfg.chain_id) && env.block.prevrandao.is_none() {
// <https://github.com/foundry-rs/foundry/issues/4232>
// If a seed is provided in configs, use it for reproducibility, otherwise use true randomness.
if let Some(seed) = configs.prevrandao_seed() {
use rand::{RngCore, SeedableRng};
use rand_chacha::ChaCha20Rng;
let mut rng = ChaCha20Rng::seed_from_u64(seed);
let mut bytes = [0u8; 32];
rng.fill_bytes(&mut bytes);
env.block.prevrandao = Some(B256::from(bytes));
} else {
env.block.prevrandao = Some(B256::random());
}
}
```
1. You will need to add a `prevrandao_seed()` method to your `configs` object, which returns an `Option<u64>`.
2. Ensure that the `rand` and `rand_chacha` crates are included in your dependencies.
3. You may want to document the new configuration option for users who require reproducibility.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces two main changes: a feature to bypass prevrandao for certain networks and a fix for forge fmt to correctly ignore library paths during recursive formatting. The prevrandao bypass is implemented cleanly by adding a configuration option and centralizing the logic. The fmt fix correctly filters out library paths, but the implementation for path checking has a flaw in its use of the current working directory (cwd) instead of the project root, which could lead to incorrect behavior when forge fmt is run from a subdirectory. Additionally, there's a small opportunity for code simplification in the prevrandao logic.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Solution
PR Checklist
Summary by Sourcery
Enable bypassing prevrandao on configurable networks and improve the Forge fmt command to skip library directories during recursion
New Features:
Enhancements:
Tests: