fix: tx pool endpoint to return queued txs and apply mempool wrapper #77
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces several changes across multiple files. Notably, the Changes
Possibly related PRs
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 (
|
![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)
.gitignore (1)
57-58: LGTM! Consider specifying if it's a directory.The addition of
test-scriptsto.gitignoreis appropriate for excluding test-related files from version control. This aligns well with the PR objectives, as new tests might be introduced for the transaction pool endpoint changes.For clarity, consider specifying whether this is a directory or specific files. If it's a directory, you might want to add a trailing slash:
# testing -test-scripts +test-scripts/This makes it explicit that you're ignoring a directory and all its contents.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- .gitignore (1 hunks)
- app/app.go (1 hunks)
- jsonrpc/backend/txpool.go (3 hunks)
- proto/minievm/evm/v1/tx.proto (3 hunks)
- proto/minievm/evm/v1/types.proto (1 hunks)
✅ Files skipped from review due to trivial changes (2)
- proto/minievm/evm/v1/tx.proto
- proto/minievm/evm/v1/types.proto
🧰 Additional context used
🔇 Additional comments (5)
jsonrpc/backend/txpool.go (5)
60-87: Proper use of mutex for thread safetyThe mutex lock is correctly applied when accessing
b.queuedTxs, ensuring thread-safe operations when retrieving queued transactions.
98-99: Correct inclusion of queued transactions inTxPoolContentFromIncluding the queued transactions in the
accountContentmap provides a complete view of an account's transactions, both pending and queued.
111-114: Thread-safe retrieval of queued transaction countThe mutex lock is appropriately used when accessing
b.queuedTxs.Len(), ensuring accurate and thread-safe retrieval of the queued transaction count.
116-117: Accurate reporting of queued transactions inTxPoolStatusAdding the
"queued"key with the count of queued transactions enhances the transparency of the transaction pool's status.
149-155: Including queued transactions inTxPoolInspectoutputExtending the inspection output to include queued transactions allows for better monitoring and debugging of the transaction pool state.
| // load queued txs | ||
| b.mut.Lock() | ||
| queuedTxs := b.queuedTxs.Values() | ||
| b.mut.Unlock() | ||
|
|
||
| for _, tx := range queuedTxs { | ||
| cosmosTx, err := b.app.TxDecode(tx) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| ethTx, account, err := txUtils.ConvertCosmosTxToEthereumTx(ctx, cosmosTx) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if ethTx == nil { | ||
| continue | ||
| } | ||
|
|
||
| dump, ok := content["queued"][account.Hex()] | ||
| if !ok { | ||
| dump = make(map[string]*rpctypes.RPCTransaction) | ||
| content["queued"][account.Hex()] = dump | ||
| } | ||
|
|
||
| dump[fmt.Sprintf("%d", ethTx.Nonce())] = rpctypes.NewRPCTransaction(ethTx, common.Hash{}, 0, 0, chainID) | ||
| } | ||
|
|
There was a problem hiding this comment.
Consider limiting the number of queued transactions processed
While pending transactions are limited to 100 transactions, the queued transactions are processed without any limit. If the number of queued transactions becomes large, this could lead to performance issues. Consider implementing a similar limit for queued transactions to ensure consistent performance.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #77 +/- ##
==========================================
- Coverage 28.21% 28.17% -0.05%
==========================================
Files 124 124
Lines 13544 13621 +77
==========================================
+ Hits 3822 3838 +16
- Misses 9168 9224 +56
- Partials 554 559 +5
|
Description
Closes: #XXXX
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores
.gitignoreto excludetest-scriptsdirectory from version control.