-
Notifications
You must be signed in to change notification settings - Fork 277
fix: disable TxReplacement in mempool when FlagDisableTxReplacement set #1960
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
This comment has been minimized.
This comment has been minimized.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughAdds an AppOption to disable mempool transaction replacement and conditions mempool initialization to omit the TxReplacement rule when the flag is set; a unit test verifies the TxReplacement field is nil when disabled. Changes
Sequence Diagram(s)(Skipped — changes are localized configuration/control-flow without multi-component sequential interactions needing visualization.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
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. 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@app/mempool_test.go`:
- Around line 22-24: Remove the misplaced t.Helper() call from the test function
TestDisableTxReplacementRemovesMempoolRule; t.Helper() should only be used
inside helper functions (e.g., loadUnexportedField), so delete the t.Helper()
invocation in that test and ensure any shared helper logic that needs t.Helper()
is placed in the helper function instead.
🧹 Nitpick comments (2)
app/app.go (1)
993-996: Consider reusing thedisableTxReplacementvariable.The flag
FlagDisableTxReplacementis read again fromappOptshere, but it was already read intodisableTxReplacementat line 387. Reusing the variable would be cleaner and ensure consistency.♻️ Suggested refactor
- mempoolCacheMaxTxs := mempoolMaxTxs - if cast.ToBool(appOpts.Get(FlagDisableTxReplacement)) { + mempoolCacheMaxTxs := mempoolMaxTxs + if disableTxReplacement { mempoolCacheMaxTxs = -1 }app/mempool_test.go (1)
19-19: Consider renaming the import alias to avoid shadowing the package name.The import
app "github.com/evmos/ethermint/evmd"uses the aliasapp, which shadows the current package name. This could cause confusion when reading the code.♻️ Suggested refactor
- app "github.com/evmos/ethermint/evmd" + evmd "github.com/evmos/ethermint/evmd"Then update line 27:
- flags.FlagHome: app.DefaultNodeHome, + flags.FlagHome: evmd.DefaultNodeHome,
|
Sorry for the confusion TxReplacement is enabled by
This PR disable the If |
👮🏻👮🏻👮🏻 !!!! REFERENCE THE PROBLEM YOUR ARE SOLVING IN THE PR TITLE AND DESCRIBE YOUR SOLUTION HERE !!!! DO NOT FORGET !!!! 👮🏻👮🏻👮🏻
--cronos.disable-tx-replacement broken - Flag doesn't disable replacement in mempool
PR Checklist:
make)make test)go fmt)golangci-lint run)go list -json -m all | nancy sleuth)Thank you for your code, it's appreciated! :)
Summary by CodeRabbit
New Features
Behavior
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.