test: add more tests and sentinel errors #66
Conversation
WalkthroughThis pull request enhances the blockchain executor by improving validation and error handling. In Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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,
tuxcanfly,
MSevey and
Manav-Aggarwal
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: 1
🔭 Outside diff range comments (1)
test/dummy.go (1)
60-95: 🛠️ Refactor suggestionAdd documentation for the ExecuteTxs method.
The method implementation is solid with comprehensive validation. However, it needs documentation to comply with Go's best practices and fix the pipeline failures.
Apply this diff to add documentation:
+// ExecuteTxs executes a list of transactions at the specified block height and timestamp. +// It validates the previous state root, timestamp, block height, and transaction size. +// Returns the new state root and maximum allowed transaction size, or an error if validation fails. func (e *DummyExecutor) ExecuteTxs(ctx context.Context, txs []types.Tx, blockHeight uint64, timestamp time.Time, prevStateRoot types.Hash) (types.Hash, uint64, error) {🧰 Tools
🪛 golangci-lint (1.62.2)
[warning] 60-60: exported: exported method DummyExecutor.ExecuteTxs should have comment or be unexported
(revive)
🪛 GitHub Check: lint / golangci-lint
[failure] 60-60:
exported: exported method DummyExecutor.ExecuteTxs should have comment or be unexported (revive)
🧹 Nitpick comments (4)
test/dummy.go (2)
15-15: Consider making the regex pattern more restrictive.The current regex pattern allows any alphanumeric character followed by alphanumeric characters or hyphens. Consider adding an upper limit to the pattern length and restricting the trailing character to prevent ending with a hyphen.
Apply this diff to improve the regex pattern:
-var validChainIDRegex = regexp.MustCompile(`^[a-zA-Z0-9][a-zA-Z0-9-]*`) +var validChainIDRegex = regexp.MustCompile(`^[a-zA-Z0-9][a-zA-Z0-9-]{0,30}[a-zA-Z0-9]$`)
109-123: Consider adding capacity hint to slice allocation.The implementation of GetTxs and InjectTx is correct. However, for better performance when dealing with large transaction volumes, consider pre-allocating the slice with a capacity hint.
Apply this diff to optimize slice allocation:
-func (e *DummyExecutor) GetTxs(context.Context) ([]types.Tx, error) { +// GetTxs returns a copy of all injected transactions. +func (e *DummyExecutor) GetTxs(context.Context) ([]types.Tx, error) { e.mu.RLock() defer e.mu.RUnlock() - txs := make([]types.Tx, len(e.injectedTxs)) + txs := make([]types.Tx, len(e.injectedTxs), cap(e.injectedTxs)) copy(txs, e.injectedTxs) return txs, nil } -func (e *DummyExecutor) InjectTx(tx types.Tx) { +// InjectTx adds a new transaction to the pool of injected transactions. +func (e *DummyExecutor) InjectTx(tx types.Tx) {test/dummy_test.go (2)
67-123: Add edge cases for transaction size and block height.The test cases are well-structured, but consider adding:
- Transaction exceeding maxBytes
- Zero block height
- Multiple empty transactions in a batch
Apply this diff to add more test cases:
{ + name: "transaction too large", + txs: []types.Tx{bytes.Repeat([]byte("x"), 1000001)}, + blockHeight: 1, + timestamp: time.Now().UTC(), + prevStateRoot: types.Hash{1, 2, 3}, + expectedErr: types.ErrTxTooLarge, + }, + { + name: "zero block height", + txs: []types.Tx{[]byte("tx1")}, + blockHeight: 0, + timestamp: time.Now().UTC(), + prevStateRoot: types.Hash{1, 2, 3}, + expectedErr: types.ErrInvalidBlockHeight, + }, + { name: "empty transaction",
199-230: Consider adding variable transaction sizes in concurrent test.The concurrent test effectively verifies transaction uniqueness and thread safety. Consider adding variable transaction sizes to stress test the system more thoroughly.
Apply this diff to enhance the test:
for j := 0; j < txsPerGoroutine; j++ { - tx := types.Tx([]byte(fmt.Sprintf("tx-%d-%d", id, j))) + // Create transactions with varying sizes + size := (j % 5) * 100 // Sizes: 0, 100, 200, 300, 400 bytes + data := fmt.Sprintf("tx-%d-%d-%s", id, j, strings.Repeat("x", size)) + tx := types.Tx([]byte(data)) s.TxInjector.InjectTx(tx) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
test/dummy.go(3 hunks)test/dummy_test.go(4 hunks)types/errors.go(1 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
types/errors.go
[warning] 6-6: exported: comment on exported var ErrZeroInitialHeight should be of the form "ErrZeroInitialHeight ..."
(revive)
[warning] 8-8: exported: exported var ErrEmptyChainID should have comment or be unexported
(revive)
[warning] 13-13: exported: comment on exported var ErrEmptyStateRoot should be of the form "ErrEmptyStateRoot ..."
(revive)
[warning] 20-20: exported: comment on exported var ErrBlockNotFound should be of the form "ErrBlockNotFound ..."
(revive)
[warning] 25-25: exported: comment on exported var ErrTxAlreadyExists should be of the form "ErrTxAlreadyExists ..."
(revive)
[warning] 30-30: exported: comment on exported var ErrContextCanceled should be of the form "ErrContextCanceled ..."
(revive)
test/dummy.go
[warning] 60-60: exported: exported method DummyExecutor.ExecuteTxs should have comment or be unexported
(revive)
🪛 GitHub Check: lint / golangci-lint
types/errors.go
[failure] 6-6:
exported: comment on exported var ErrZeroInitialHeight should be of the form "ErrZeroInitialHeight ..." (revive)
[failure] 8-8:
exported: exported var ErrEmptyChainID should have comment or be unexported (revive)
[failure] 13-13:
exported: comment on exported var ErrEmptyStateRoot should be of the form "ErrEmptyStateRoot ..." (revive)
[failure] 20-20:
exported: comment on exported var ErrBlockNotFound should be of the form "ErrBlockNotFound ..." (revive)
[failure] 25-25:
exported: comment on exported var ErrTxAlreadyExists should be of the form "ErrTxAlreadyExists ..." (revive)
[failure] 30-30:
exported: comment on exported var ErrContextCanceled should be of the form "ErrContextCanceled ..." (revive)
test/dummy.go
[failure] 60-60:
exported: exported method DummyExecutor.ExecuteTxs should have comment or be unexported (revive)
🪛 GitHub Actions: CI and Release
types/errors.go
[error] 6-6: exported: comment on exported var ErrZeroInitialHeight should be of the form "ErrZeroInitialHeight ..." (revive)
🔇 Additional comments (4)
test/dummy.go (2)
34-58: LGTM! Comprehensive validation in InitChain.The validation checks in InitChain are thorough and well-structured, covering all edge cases:
- Zero initial height
- Empty chain ID
- Invalid chain ID format
- Future genesis time
- Chain ID length
🧰 Tools
🪛 golangci-lint (1.62.2)
[warning] 34-34: exported: exported method DummyExecutor.InitChain should have comment or be unexported
(revive)
🪛 GitHub Check: lint / golangci-lint
[failure] 34-34:
exported: exported method DummyExecutor.InitChain should have comment or be unexported (revive)
97-107: LGTM! Simplified error handling.Good use of the sentinel error
types.ErrBlockNotFoundinstead of a formatted error message.🧰 Tools
🪛 golangci-lint (1.62.2)
[warning] 97-97: exported: exported method DummyExecutor.SetFinal should have comment or be unexported
(revive)
🪛 GitHub Check: lint / golangci-lint
[failure] 97-97:
exported: exported method DummyExecutor.SetFinal should have comment or be unexported (revive)test/dummy_test.go (2)
31-65: LGTM! Well-structured test with clear assertions.The test effectively verifies transaction removal behavior with appropriate assertions at each step.
125-197: LGTM! Comprehensive test cases for chain initialization.The test cases thoroughly cover all validation scenarios in InitChain, including edge cases for chain ID format and length.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Overview
The sentinel errors are kept very generic, as there are some cases in which an execution layer would return errors and in others it would not (like initial height being 0).
Closes #16
Partially closes #17
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Tests