fix: remove txs after execution in DummyExecutor
#48
Conversation
Original implementation of `DummyExecutor` incorrectly removed transaction from "mempool" (in-memory slice in this case) during `GetTxs`.
WalkthroughThe pull request introduces modifications to the Changes
Suggested labels
Suggested reviewers
Possibly related PRs
Poem
Finishing Touches
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,
gupadhyaya 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: 0
🧹 Nitpick comments (1)
test/dummy.go (1)
75-79: Consider optimizing transaction removal for better performance.The current implementation has O(n*m) complexity where n is the size of injectedTxs and m is the size of txs. For better performance, especially with larger transaction volumes, consider using a map-based approach for O(n) complexity.
Here's a more efficient implementation:
func (e *DummyExecutor) removeExecutedTxs(txs []types.Tx) { + // Create a map for O(1) lookup + txMap := make(map[string]struct{}, len(txs)) + for _, tx := range txs { + txMap[string(tx)] = struct{}{} + } + + // Single pass through injectedTxs + e.injectedTxs = slices.DeleteFunc(e.injectedTxs, func(tx types.Tx) bool { + _, exists := txMap[string(tx)] + return exists + }) - e.injectedTxs = slices.DeleteFunc(e.injectedTxs, func(tx types.Tx) bool { - return slices.ContainsFunc(txs, func(t types.Tx) bool { return bytes.Equal(tx, t) }) - }) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/dummy.go(3 hunks)
🔇 Additional comments (2)
test/dummy.go (2)
4-5: LGTM! Import changes are appropriate.The added imports from the standard library (
bytesandslices) are necessary for the new transaction removal functionality.Also applies to: 9-9
61-61: LGTM! Transaction cleanup is now handled correctly.The addition of
removeExecutedTxscall ensures that transactions are removed from the mempool only after successful execution, which aligns with the PR's objective.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/dummy_test.go (1)
25-57: Consider adding more test cases.The test could be enhanced by:
- Using a table-driven approach to test different scenarios
- Adding edge cases like:
- Executing non-existent transactions
- Executing the same transaction multiple times
- Executing multiple transactions in one call
Would you like me to help implement these additional test cases?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/dummy_test.go(2 hunks)
🧰 Additional context used
🪛 GitHub Check: lint / golangci-lint
test/dummy_test.go
[failure] 5-5:
File is not properly formatted (goimports)
🪛 golangci-lint (1.62.2)
test/dummy_test.go
5-5: File is not goimports-ed with -local github.com/rollkit
(goimports)
🪛 GitHub Actions: CI and Release
test/dummy_test.go
[error] 5-5: File is not properly formatted (goimports)
🔇 Additional comments (1)
test/dummy_test.go (1)
25-57: Well-structured test implementation!The test thoroughly verifies the transaction removal behavior with clear steps and comprehensive assertions.
tzdybal
force-pushed
the
tzdybal/fix_mempool
branch
from
13f1c5a to
42c630f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
test/dummy_test.go (2)
26-26: Add descriptive comment explaining the test's purpose.While the test implementation is good, please add a comment explaining that this test validates the fix for transaction removal behavior in DummyExecutor, specifically that transactions are only removed after execution and not during GetTxs.
+// TestTxRemoval validates that transactions in DummyExecutor are only removed +// after being executed, and not during GetTxs calls func TestTxRemoval(t *testing.T) {
26-58: Consider adding more test cases.The current test effectively validates the basic transaction removal behavior. Consider adding these additional test cases to improve coverage:
- Executing an empty transaction list
- Executing multiple transactions at once
- Executing a non-existent transaction
- Executing the same transaction twice
Would you like me to provide an implementation for these additional test cases?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/dummy_test.go(2 hunks)
🔇 Additional comments (1)
test/dummy_test.go (1)
4-11: Fix import ordering using goimports.The imports need to be reordered according to the project's formatting rules.
|
🎉 This PR is included in version 0.2.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Overview
Original implementation of
DummyExecutorincorrectly removed transaction from "mempool" (in-memory slice in this case) duringGetTxs.Summary by CodeRabbit
New Features
Tests