feat: add fork block number and selector on fork exit#61
Open
Mouradif wants to merge 1 commit into
Open
Conversation
📝 WalkthroughWalkthroughThis pull request introduces fork block number specification for Anvil forks and adds interactive selection when exiting multiple active forks. The ForkBlockNumber parameter is propagated from CLI flag through use case parameters, domain models, and the Anvil adapter. Fork exit logic now detects and handles multiple active forks with optional user prompting. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI: fork enter
participant UC as EnterFork<br/>UseCase
participant Domain as AnvilInstance<br/>(domain)
participant Adapter as Anvil<br/>Adapter
participant Anvil as Anvil<br/>Process
CLI->>UC: EnterForkParams{ForkBlockNumber: N}
activate UC
UC->>Domain: Create AnvilInstance<br/>with ForkBlockNumber
activate Domain
Domain->>Domain: Store ForkBlockNumber field
deactivate Domain
UC->>Adapter: Build startup args<br/>from AnvilInstance
activate Adapter
Adapter->>Adapter: If ForkBlockNumber > 0:<br/>append --fork-block-number N
deactivate Adapter
UC->>Anvil: Execute anvil<br/>--fork-url ... --fork-block-number N
activate Anvil
Anvil->>Anvil: Fork at specified<br/>block number
deactivate Anvil
deactivate UC
sequenceDiagram
participant CLI as CLI: fork exit
participant UC as ExitFork<br/>UseCase
participant Repo as Fork<br/>Repository
participant State as ForkState
participant Picker as Interactive<br/>Picker
CLI->>Repo: Load fork state
activate Repo
Repo->>State: Load active forks
activate State
State->>State: Deserialize fork-state.json
deactivate State
deactivate Repo
alt Multiple forks active
Repo->>State: ActiveNetworks()
State-->>Repo: []string{networks...}
Repo->>Picker: Prompt user to select
activate Picker
Picker-->>Picker: Display fork options
Picker-->>CLI: Return selected network
deactivate Picker
else Single fork active
Repo->>Repo: Auto-select fork
else No forks active
Repo-->>CLI: Error
end
CLI->>UC: Exit selected fork
activate UC
UC->>UC: Cleanup fork state
deactivate UC
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip You can customize the high-level summary generated by CodeRabbit.Configure the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
test/integration/fork_test.go (1)
1688-1734: Extend this block-pin coverage throughfork restart.This case only exercises the initial
fork enterpath. The risky part of this feature is rehydrating the fork from saved state, so without a restart step the suite won't catch a pinned fork restarting atlatest.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/integration/fork_test.go` around lines 1688 - 1734, The test TestForkEnterBlockNumber only checks the initial fork enter; add a step to run the restart path and rehydrate from saved state: after asserting the fork was entered and before cleanupForkAnvil, invoke the CLI equivalent of "fork restart anvil-31337" via ctx.TrebContext.Treb, wait for the restarted process, re-read state with readForkState and verify the fork entry still exists, the restarted anvil is alive (isProcessAlive) and ethBlockNumber(fork.ForkURL) still equals forkBlock; also assert the PID changed if you want to ensure a true restart. Ensure you handle errors with require.NoError and reuse existing helpers like readForkState, ethBlockNumber, cleanupForkAnvil.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/cli/fork.go`:
- Around line 180-205: The help text and code diverge: the no-arg fork exit used
to fall back to the currently configured network but this branch always selects
from state.Forks; fix by restoring the old fallback or updating help.
Concretely, before loading/inspecting state.Forks (ForkStateStore.Load) check
the configured network value (the CLI/app config field that holds the current
network) and if it is non-empty and corresponds to an active fork in
state.Forks, set network = configuredNetwork and skip the interactive selection;
otherwise keep the existing switch that uses selectForkNetwork and
app.Config.NonInteractive. Alternatively, if you prefer the new behavior, update
the command help to say the no-arg form chooses from active forks instead of
using the configured network.
In `@internal/usecase/enter_fork.go`:
- Around line 148-154: The code only sets ForkBlockNumber on the transient
AnvilInstance but doesn't persist it to domain.ForkEntry, so a restarted fork
loses the pinned block; update the logic in the enter/fork creation path (the
function that builds the domain.ForkEntry and calls NewAnvilInstance) to copy
params.ForkBlockNumber into the persisted domain.ForkEntry (e.g., set
ForkEntry.ForkBlockNumber = params.ForkBlockNumber) and ensure when rebuilding
the instance from a saved entry (the code that constructs AnvilInstance from
domain.ForkEntry on restart) you restore that value onto the reconstructed
AnvilInstance.ForkBlockNumber so pinned-block restarts correctly.
- Around line 49-55: The EnterFork use case must reject the combination of an
ExternalURL with a non-zero ForkBlockNumber because executeExternal ignores
ForkBlockNumber; add a validation early in the use case (e.g., in the method
that orchestrates entering a fork such as Execute/Enter or the public Run method
that consumes the struct with fields Network, ExternalURL, ForkBlockNumber) that
returns an explicit error when ExternalURL is non-empty and ForkBlockNumber !=
0. Reference the ForkBlockNumber field and the executeExternal path in the error
message (e.g., "cannot pin fork block when attaching to external Anvil") so
callers get a clear failure instead of silently dropping the block pin. Ensure
unit tests or callers that expect this validation are updated accordingly.
---
Nitpick comments:
In `@test/integration/fork_test.go`:
- Around line 1688-1734: The test TestForkEnterBlockNumber only checks the
initial fork enter; add a step to run the restart path and rehydrate from saved
state: after asserting the fork was entered and before cleanupForkAnvil, invoke
the CLI equivalent of "fork restart anvil-31337" via ctx.TrebContext.Treb, wait
for the restarted process, re-read state with readForkState and verify the fork
entry still exists, the restarted anvil is alive (isProcessAlive) and
ethBlockNumber(fork.ForkURL) still equals forkBlock; also assert the PID changed
if you want to ensure a true restart. Ensure you handle errors with
require.NoError and reuse existing helpers like readForkState, ethBlockNumber,
cleanupForkAnvil.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cf068acf-7b9a-44c6-8f93-2ef22e470f64
📒 Files selected for processing (6)
internal/adapters/anvil/manager.gointernal/cli/fork.gointernal/domain/anvil.gointernal/domain/fork.gointernal/usecase/enter_fork.gotest/integration/fork_test.go
Comment on lines
+180
to
205
| // No network specified and not --all: check if multiple forks are active | ||
| state, err := app.ForkStateStore.Load(ctx) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to load fork state: %w", err) | ||
| } | ||
|
|
||
| switch len(state.Forks) { | ||
| case 0: | ||
| return fmt.Errorf("no active forks") | ||
| case 1: | ||
| // Only one fork — use it directly | ||
| for name := range state.Forks { | ||
| network = name | ||
| } | ||
| default: | ||
| // Multiple forks — prompt user to select | ||
| if app.Config.NonInteractive { | ||
| return fmt.Errorf("multiple active forks. Specify a network or use --all") | ||
| } | ||
|
|
||
| selected, err := selectForkNetwork(state.ActiveNetworks()) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| network = selected | ||
| } |
There was a problem hiding this comment.
Keep the no-arg fork exit help text in sync with this behavior.
Lines 155-156 still say the no-arg form uses the currently configured network, but this branch now ignores that and chooses from the active-fork set instead. Either preserve the old fallback before this switch or update the command help in the same change.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/cli/fork.go` around lines 180 - 205, The help text and code diverge:
the no-arg fork exit used to fall back to the currently configured network but
this branch always selects from state.Forks; fix by restoring the old fallback
or updating help. Concretely, before loading/inspecting state.Forks
(ForkStateStore.Load) check the configured network value (the CLI/app config
field that holds the current network) and if it is non-empty and corresponds to
an active fork in state.Forks, set network = configuredNetwork and skip the
interactive selection; otherwise keep the existing switch that uses
selectForkNetwork and app.Config.NonInteractive. Alternatively, if you prefer
the new behavior, update the command help to say the no-arg form chooses from
active forks instead of using the configured network.
Comment on lines
+49
to
55
| Network string // network name from foundry.toml | ||
| RPCURL string // resolved RPC URL (after env var expansion) | ||
| ChainID uint64 // chain ID | ||
| EnvVarName string // env var name that foundry.toml uses for the RPC endpoint | ||
| ExternalURL string // optional external Anvil endpoint URL (skips local Anvil startup) | ||
| ForkBlockNumber uint64 // optional block number to fork at (0 = latest) | ||
| } |
There was a problem hiding this comment.
Reject ForkBlockNumber when attaching to an external Anvil.
executeExternal ignores this field, so callers can pass ExternalURL and ForkBlockNumber together and silently attach to whatever head the external endpoint is already on. Validate that combination in the use case and return an explicit error instead of dropping the block pin.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/usecase/enter_fork.go` around lines 49 - 55, The EnterFork use case
must reject the combination of an ExternalURL with a non-zero ForkBlockNumber
because executeExternal ignores ForkBlockNumber; add a validation early in the
use case (e.g., in the method that orchestrates entering a fork such as
Execute/Enter or the public Run method that consumes the struct with fields
Network, ExternalURL, ForkBlockNumber) that returns an explicit error when
ExternalURL is non-empty and ForkBlockNumber != 0. Reference the ForkBlockNumber
field and the executeExternal path in the error message (e.g., "cannot pin fork
block when attaching to external Anvil") so callers get a clear failure instead
of silently dropping the block pin. Ensure unit tests or callers that expect
this validation are updated accordingly.
Comment on lines
+148
to
+154
| Name: fmt.Sprintf("fork-%s", params.Network), | ||
| Port: fmt.Sprintf("%d", port), | ||
| ChainID: fmt.Sprintf("%d", params.ChainID), | ||
| ForkURL: params.RPCURL, | ||
| ForkBlockNumber: params.ForkBlockNumber, | ||
| PidFile: filepath.Join(privDir, fmt.Sprintf("fork-%s.pid", params.Network)), | ||
| LogFile: filepath.Join(privDir, fmt.Sprintf("fork-%s.log", params.Network)), |
There was a problem hiding this comment.
Persist the pinned block number in fork state.
This only sets ForkBlockNumber on the transient AnvilInstance. fork restart later reconstructs a fresh instance from persisted domain.ForkEntry data without this field, so a fork entered at a specific block comes back at latest. Save the value on ForkEntry and restore it when rebuilding the instance.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/usecase/enter_fork.go` around lines 148 - 154, The code only sets
ForkBlockNumber on the transient AnvilInstance but doesn't persist it to
domain.ForkEntry, so a restarted fork loses the pinned block; update the logic
in the enter/fork creation path (the function that builds the domain.ForkEntry
and calls NewAnvilInstance) to copy params.ForkBlockNumber into the persisted
domain.ForkEntry (e.g., set ForkEntry.ForkBlockNumber = params.ForkBlockNumber)
and ensure when rebuilding the instance from a saved entry (the code that
constructs AnvilInstance from domain.ForkEntry on restart) you restore that
value onto the reconstructed AnvilInstance.ForkBlockNumber so pinned-block
restarts correctly.
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
1 participant
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.
Summary by CodeRabbit
Release Notes
New Features
--fork-block-numberflag in the fork enter command.Tests