test: enhance DummyExecutor #39
Conversation
WalkthroughThe changes involve modifications to the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
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 (
|
RollkitBot
requested review from
a team,
yarikbratashchuk,
gupadhyaya and
MSevey
and removed request for
a team
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
test/suite.go (1)
57-59: Consider enhancing the finalization test coverageThe test creates and finalizes a state but doesn't verify the post-finalization behavior. Consider adding assertions to verify that:
- The state was actually finalized
- Subsequent operations on the finalized height behave correctly
Example enhancement:
_, _, err = s.Exec.ExecuteTxs(context.TODO(), nil, 2, time.Now(), types.Hash("test state")) s.Require().NoError(err) err = s.Exec.SetFinal(context.TODO(), 2) s.Require().NoError(err) +// Verify finalization +// Try to modify finalized height - should error +_, _, err = s.Exec.ExecuteTxs(context.TODO(), []types.Tx{[]byte("new tx")}, 2, time.Now(), types.Hash{}) +s.Require().Error(err, "should not allow modifications to finalized height")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
test/dummy.go(1 hunks)test/suite.go(2 hunks)
🧰 Additional context used
🪛 GitHub Check: lint / golangci-lint
test/dummy.go
[failure] 44-44:
exported: exported method DummyExecutor.InjectTx should have comment or be unexported (revive)
🪛 golangci-lint
test/dummy.go
[warning] 44-44: exported: exported method DummyExecutor.InjectTx should have comment or be unexported
(revive)
🔇 Additional comments (8)
test/suite.go (2)
35-35: LGTM: Improved assertion precision
The change from a non-nil check to an empty check provides better test precision and aligns with the updated GetTxs implementation that returns injectedTxs.
53-55: LGTM: Good addition of error case testing
Testing the error case for invalid height finalization is a good practice for ensuring robust error handling.
test/dummy.go (6)
5-6: Approved: Necessary imports added
The added imports crypto/sha512 and fmt are appropriate for the new functionalities.
14-17: Approved: Updated DummyExecutor struct with new fields
The addition of pendingRoots and renaming txs to injectedTxs enhance the executor's capability to track pending state roots and manage injected transactions.
23-25: Approved: Proper initialization in NewDummyExecutor
Initializing stateRoot, pendingRoots, and maxBytes sets up the executor correctly.
39-42: Approved: Handling of injected transactions in GetTxs
Retrieving and resetting injectedTxs ensures that transactions are processed once and not duplicated.
50-57: Approved: Correct calculation of pending state root in ExecuteTxs
The method properly updates the pending state root by hashing the previous state root with the transactions.
60-60: Approved: Proper error handling in SetFinal
The SetFinal method correctly updates the stateRoot and returns an error when the specified block height does not exist in pendingRoots.
Also applies to: 62-66
tzdybal
force-pushed
the
tzdybal/improved_dummy
branch
from
7cf6d40 to
03369cd
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (4)
cmd/dummy/main.go (2)
20-26: Consider making configuration values configurable.The timeout duration and request size are hardcoded. Consider making these configurable through environment variables or command-line flags for better flexibility in different deployment scenarios.
Example improvement:
+import ( + "flag" + "os" +) + +var ( + defaultTimeout = flag.Duration("timeout", 5*time.Second, "Default request timeout") + maxRequestSize = flag.Int("max-request-size", bufSize, "Maximum request size in bytes") +) + func main() { + flag.Parse() dummy := test.NewDummyExecutor() config := &grpcproxy.Config{ - DefaultTimeout: 5 * time.Second, - MaxRequestSize: bufSize, + DefaultTimeout: *defaultTimeout, + MaxRequestSize: *maxRequestSize, }
1-59: Consider separating server configuration and implementation.While the current implementation works, consider these architectural improvements:
- Move server configuration to a separate package/file
- Create a dedicated server struct to encapsulate the server logic
- Add metrics and health checks for better observability
- Consider implementing retry mechanisms for failed operations
test/dummy.go (2)
11-11: Clarify the interface implemented byDummyExecutorThe comment states that
DummyExecutoris a dummy implementation of theDummyExecutorinterface. If the interface is named differently, such asExecutor, consider updating the comment to reflect the correct interface name to avoid confusion.Apply this diff to correct the comment:
-// DummyExecutor is a dummy implementation of the DummyExecutor interface for testing +// DummyExecutor is a dummy implementation of the Executor interface for testing
52-52: Fix grammatical errors in function commentsThere are minor grammatical errors in the function comments that can be corrected for clarity.
Apply this diff to fix the comments:
-// ExecuteTxs simulate execution of transactions. +// ExecuteTxs simulates execution of transactions. ... -// SetFinal marks block at given height as finalized. +// SetFinal marks the block at the given height as finalized.Also applies to: 62-62
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
cmd/dummy/main.go(1 hunks)test/dummy.go(1 hunks)
🔇 Additional comments (2)
cmd/dummy/main.go (2)
1-16: LGTM! Well-organized imports.
The imports are properly organized and all necessary dependencies are included for the gRPC server implementation.
18-19: Document and verify the buffer size constant.
The 1MB buffer size seems arbitrary. Consider:
- Adding a comment explaining why this specific size was chosen
- Verifying if this size is sufficient for your use case
- Making it configurable if needed for different deployment scenarios
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (6)
cmd/dummy/main.go (2)
20-25: Consider making timeout configurable and adding context handling.The current implementation has the following potential improvements:
- Make the timeout configurable via environment variable or flag
- Add context with cancellation for graceful shutdown
func main() { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + timeout := 5 * time.Second + if envTimeout := os.Getenv("DUMMY_EXECUTOR_TIMEOUT"); envTimeout != "" { + if t, err := time.ParseDuration(envTimeout); err == nil { + timeout = t + } + } + dummy := test.NewDummyExecutor() config := &grpcproxy.Config{ - DefaultTimeout: 5 * time.Second, + DefaultTimeout: timeout, MaxRequestSize: bufSize, }
32-38: Add proper error handling in defer.The error from
listener.Close()should be logged rather than ignored.defer func() { - _ = listener.Close() + if err := listener.Close(); err != nil { + log.Printf("error closing listener: %v", err) + } }()test/dummy.go (4)
23-25: Consider defining constants for magic numbers.The initial state root
{1, 2, 3}uses magic numbers. Consider defining these as named constants to improve code clarity and maintainability.+const ( + // Initial state root values for testing + initialStateRoot = types.Hash{1, 2, 3} + defaultMaxBytes = 1000000 +) func NewDummyExecutor() *DummyExecutor { return &DummyExecutor{ - stateRoot: types.Hash{1, 2, 3}, + stateRoot: initialStateRoot, pendingRoots: make(map[uint64]types.Hash), - maxBytes: 1000000, + maxBytes: defaultMaxBytes, } }
40-43: Document the clearing behavior of GetTxs.The method clears
injectedTxsafter retrieval, which is an important side effect that should be documented.-// GetTxs returns the list of transactions (types.Tx) within the DummyExecutor instance and an error if any. +// GetTxs returns the list of transactions (types.Tx) within the DummyExecutor instance and clears the internal list. +// This simulates the behavior of retrieving pending transactions from a mempool. func (e *DummyExecutor) GetTxs(context.Context) ([]types.Tx, error) {
51-59: Document the state transition logic in ExecuteTxs.The method implements important state transition logic that should be documented for clarity.
-// ExecuteTxs simulate execution of transactions. +// ExecuteTxs simulates execution of transactions by computing a new state root +// from the previous state root and the provided transactions. The new state +// root is stored as pending for the given block height until finalized. func (e *DummyExecutor) ExecuteTxs(ctx context.Context, txs []types.Tx, blockHeight uint64, timestamp time.Time, prevStateRoot types.Hash) (types.Hash, uint64, error) {
64-68: Consider using a custom error type.Instead of using fmt.Errorf, consider defining a custom error type for better error handling by consumers.
+// ErrBlockNotFound is returned when attempting to finalize a block height that doesn't exist +type ErrBlockNotFound struct { + Height uint64 +} + +func (e ErrBlockNotFound) Error() string { + return fmt.Sprintf("cannot set finalized block at height %d", e.Height) +} func (e *DummyExecutor) SetFinal(ctx context.Context, blockHeight uint64) error { if pending, ok := e.pendingRoots[blockHeight]; ok { e.stateRoot = pending return nil } - return fmt.Errorf("cannot set finalized block at height %d", blockHeight) + return ErrBlockNotFound{Height: blockHeight} }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
cmd/dummy/main.go(1 hunks)test/dummy.go(1 hunks)
🔇 Additional comments (3)
cmd/dummy/main.go (2)
1-16: LGTM! Well-organized imports and package declaration.
The imports are properly grouped and all are being used in the code.
18-18: Document and verify the buffer size constant.
The 1MB buffer size might be insufficient for large transactions. Consider:
- Adding a comment explaining why this specific size was chosen
- Making it configurable via environment variable or flag
- Verifying if this size aligns with your expected transaction sizes
test/dummy.go (1)
5-6: LGTM! Structural changes align with PR objectives.
The new fields pendingRoots and injectedTxs provide proper state tracking capabilities, enabling the implementation of "acceptance" style tests as required.
Also applies to: 14-17
This prevents from memory leaking and makes it impossible to `SetFinal` more than once.
tzdybal
force-pushed
the
tzdybal/improved_dummy
branch
from
4f56d58 to
71c9376
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
cmd/dummy/main.go (1)
7-7: Organize imports according to Go conventionsThe imports are not grouped according to Go conventions. It's recommended to separate standard library imports, third-party imports, and local project imports with blank lines for better readability.
Apply this diff to reorganize the imports:
import ( "errors" "flag" "fmt" + "log" "net" "os" "os/signal" "syscall" + "google.golang.org/grpc" + "github.com/rollkit/go-execution/test" + grpcproxy "github.com/rollkit/go-execution/proxy/grpc" + pb "github.com/rollkit/go-execution/types/pb/execution" )Alternatively, run
goimports -local github.com/rollkitto automatically format the imports.🧰 Tools
🪛 golangci-lint (1.62.2)
7-7: File is not
goimports-ed with -local github.com/rollkit(goimports)
test/dummy.go (3)
40-43: Add comment for exported methodGetTxsThe method
GetTxsis exported but lacks a comment. In Go, exported methods should have a comment that begins with the method name to improve code documentation.Apply this diff to add the comment:
+// GetTxs returns the list of injected transactions and resets the internal list. func (e *DummyExecutor) GetTxs(context.Context) ([]types.Tx, error) { txs := e.injectedTxs e.injectedTxs = nil return txs, nil }
52-59: Add comment for exported methodExecuteTxsThe method
ExecuteTxsis exported but lacks a comment. In Go, exported methods should have a comment that begins with the method name to improve code documentation.Apply this diff to add the comment:
+// ExecuteTxs simulates the execution of transactions and computes a pending state root. func (e *DummyExecutor) ExecuteTxs(ctx context.Context, txs []types.Tx, blockHeight uint64, timestamp time.Time, prevStateRoot types.Hash) (types.Hash, uint64, error) { hash := sha512.New() hash.Write(prevStateRoot) for _, tx := range txs { hash.Write(tx) } pending := hash.Sum(nil) e.pendingRoots[blockHeight] = pending return pending, e.maxBytes, nil }
64-69: Add comment for exported methodSetFinalThe method
SetFinalis exported but lacks a comment. In Go, exported methods should have a comment that begins with the method name to improve code documentation.Apply this diff to add the comment:
+// SetFinal finalizes the state root for the given block height. func (e *DummyExecutor) SetFinal(ctx context.Context, blockHeight uint64) error { if pending, ok := e.pendingRoots[blockHeight]; ok { e.stateRoot = pending delete(e.pendingRoots, blockHeight) return nil } return fmt.Errorf("cannot set finalized block at height %d", blockHeight) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
cmd/dummy/main.go(1 hunks)test/dummy.go(1 hunks)
🧰 Additional context used
📓 Learnings (1)
cmd/dummy/main.go (1)
Learnt from: tzdybal
PR: rollkit/go-execution#39
File: cmd/dummy/main.go:40-44
Timestamp: 2024-12-03T16:03:42.249Z
Learning: The `cmd/dummy/main.go` file implements a dummy gRPC server for testing purposes and is not intended for production use; therefore, adding TLS support is not required.
🪛 golangci-lint (1.62.2)
cmd/dummy/main.go
7-7: File is not goimports-ed with -local github.com/rollkit
(goimports)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
test/dummy.go (4)
40-43: Consider adding mutex for concurrent access protectionThe current implementation might have race conditions if multiple goroutines access
injectedTxssimultaneously. Consider adding a mutex to protect concurrent access to this slice.type DummyExecutor struct { stateRoot types.Hash pendingRoots map[uint64]types.Hash maxBytes uint64 injectedTxs []types.Tx + mu sync.Mutex } func (e *DummyExecutor) GetTxs(context.Context) ([]types.Tx, error) { + e.mu.Lock() + defer e.mu.Unlock() txs := e.injectedTxs e.injectedTxs = nil return txs, nil }
46-47: Protect InjectTx from concurrent accessSimilar to GetTxs, this method should be protected with a mutex to prevent race conditions during concurrent access.
func (e *DummyExecutor) InjectTx(tx types.Tx) { + e.mu.Lock() + defer e.mu.Unlock() e.injectedTxs = append(e.injectedTxs, tx) }
52-59: Protect pendingRoots map from concurrent accessThe
pendingRootsmap should be protected with a mutex to prevent race conditions during concurrent access.func (e *DummyExecutor) ExecuteTxs(ctx context.Context, txs []types.Tx, blockHeight uint64, timestamp time.Time, prevStateRoot types.Hash) (types.Hash, uint64, error) { + e.mu.Lock() + defer e.mu.Unlock() hash := sha512.New() hash.Write(prevStateRoot) for _, tx := range txs { hash.Write(tx) } pending := hash.Sum(nil) e.pendingRoots[blockHeight] = pending return pending, e.maxBytes, nil }
64-69: LGTM! Good error handling and cleanupThe implementation properly handles finalization, includes appropriate error handling, and cleans up the
pendingRootsmap to prevent memory leaks. However, consider adding mutex protection for concurrent access.func (e *DummyExecutor) SetFinal(ctx context.Context, blockHeight uint64) error { + e.mu.Lock() + defer e.mu.Unlock() if pending, ok := e.pendingRoots[blockHeight]; ok { e.stateRoot = pending delete(e.pendingRoots, blockHeight) return nil } return fmt.Errorf("cannot set finalized block at height %d", blockHeight) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
cmd/dummy/main.go(1 hunks)test/dummy.go(1 hunks)test/suite.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/dummy/main.go
- test/suite.go
🔇 Additional comments (3)
test/dummy.go (3)
14-17: LGTM! Well-structured field additions
The new struct fields are well-named and properly organized. The pendingRoots map for tracking state roots by height and injectedTxs for managing transactions align well with the PR objectives for enhanced testing capabilities.
23-25: LGTM! Proper initialization of new fields
The constructor properly initializes all fields, including the new pendingRoots map, with appropriate default values.
32-34: LGTM! Correct hash computation implementation
The hash computation is now properly implemented using hash.Write() followed by hash.Sum(nil), which is the correct pattern for generating cryptographic hashes in Go.
Overview
Resolves #24
Summary by CodeRabbit
New Features
Bug Fixes
Tests