Problem: tx is not replaced by priority #1563
Conversation
Closes: crypto-org-chain#1557 Solution: - add TxReplacement callback, only replace if priority is higher
yihuang
requested review from
leejw51crypto and
thomas-nguy
and removed request for
a team
WalkthroughThe changes introduce a new transaction replacement mechanism in the application, allowing a transaction to be replaced only if the new transaction's priority is higher than the existing one. This is implemented through a new function in the codebase and is accompanied by a test to ensure correct behavior in various scenarios involving transaction priorities. Changes
Assessment against linked issues
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 (
|
Signed-off-by: yihuang <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1563 +/- ##
==========================================
- Coverage 36.12% 36.11% -0.02%
==========================================
Files 97 97
Lines 7725 7725
==========================================
- Hits 2791 2790 -1
- Misses 4585 4586 +1
Partials 349 349 |
| @@ -375,6 +375,10 @@ func New( | |||
| TxPriority: mempool.NewDefaultTxPriority(), | |||
| SignerExtractor: evmapp.NewEthSignerExtractionAdapter(mempool.NewDefaultSignerExtractionAdapter()), | |||
| MaxTx: maxTxs, | |||
| TxReplacement: func(op, np int64, oTx, nTx sdk.Tx) bool { | |||
| // tx is only replaced if new priority is higher than old priority | |||
| return np > op | |||
There was a problem hiding this comment.
do we need align how ethereum replace tx
There was a problem hiding this comment.
- priceBump parameter
- Require both FeeCap and TipCap larger than the old one.
There was a problem hiding this comment.
the same nonce tx will fail in checkTx in the first place, might not be trivial to support this feature.
![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
Outside diff range, codebase verification and nitpick comments (1)
integration_tests/test_mempool.py (1)
85-116: LGTM! The test case is well-structured and covers the essential scenarios of transaction replacement based on gas prices.The use of
@pytest.mark.flakyis appropriate to handle potential flakiness in the test.Consider adding the following enhancements to further improve the test case:
- Add an assertion to explicitly verify the replacement of the first transaction by the second one. For example, you can check the transaction hash of the mined transaction and ensure it matches the hash of the second transaction.
receipt = w3.eth.wait_for_transaction_receipt(txhashes[1]) assert receipt.status == 1 assert receipt.transactionHash == txhashes[1] # Add this assertion
- Add a brief docstring to describe the purpose of the test function. For example:
def test_tx_replacement(cronos_mempool): """ Test the transaction replacement behavior based on gas prices. This test verifies that a transaction with a higher gas price replaces a pending transaction with a lower gas price, while a transaction with a lower gas price does not replace a pending transaction with a higher gas price. """ # Test code goes hereThese enhancements will make the test case more comprehensive and self-explanatory.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- CHANGELOG.md (1 hunks)
- app/app.go (1 hunks)
- integration_tests/test_mempool.py (1 hunks)
Additional comments not posted (2)
CHANGELOG.md (1)
34-34: Looks good!The new CHANGELOG entry accurately documents the feature added in PR #1563.
app/app.go (1)
378-381: LGTM!The new
TxReplacementcallback looks good. It correctly compares the new transaction's priority with the old transaction's priority, allowing replacement only if the new priority is higher. This aligns with the PR objectives of implementing transaction replacement based on priority.
Closes: #1557
Solution:
👮🏻👮🏻👮🏻 !!!! REFERENCE THE PROBLEM YOUR ARE SOLVING IN THE PR TITLE AND DESCRIBE YOUR SOLUTION HERE !!!! DO NOT FORGET !!!! 👮🏻👮🏻👮🏻
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
Bug Fixes
Tests