feat(rpc): filter unconfirmed txs by tx hash #1053
Conversation
WalkthroughThis pull request introduces functionality for retrieving transactions based on a transaction hash. New methods named Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant S as RPC Server
participant E as Environment
participant M as Mempool
C->>S: Send /unconfirmed_tx request with txHash
S->>E: Invoke UnconfirmedTx(ctx, req)
E->>M: Call GetTxByHash(txHash)
M-->>E: Return transaction (or nil)
E->>S: Respond with ResultUnconfirmedTx or error
S-->>C: Return response to client
Suggested labels
Suggested reviewers
Poem
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 review all |
![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
🧹 Nitpick comments (2)
rpc/coretypes/responses.go (1)
315-318: Consider enriching the response type with additional metadata.While the current implementation is functional, consider adding useful metadata fields such as:
- Transaction timestamp
- Transaction priority
- Time spent in mempool
- Transaction size
type ResultUnconfirmedTx struct { Tx types.Tx `json:"tx"` + Timestamp time.Time `json:"timestamp"` + Priority int64 `json:"priority"` + TimeInMempool time.Duration `json:"time_in_mempool"` + Size int64 `json:"size"` }internal/rpc/core/mempool.go (1)
200-207: Consider improving error handling.The error handling could be enhanced for better user experience:
- Split validation errors into separate checks for clearer error messages
- Use structured errors for better error handling by clients
Consider this refactoring:
- if req == nil || req.TxHash.IsZero() || len(req.TxHash) != crypto.HashSize { - return nil, fmt.Errorf("you must provide a valid %d-byte transaction hash in hash= argument", crypto.HashSize) + if req == nil { + return nil, errors.New("request cannot be nil") + } + if req.TxHash.IsZero() { + return nil, errors.New("transaction hash cannot be zero") + } + if len(req.TxHash) != crypto.HashSize { + return nil, fmt.Errorf("invalid hash size: got %d bytes, expected %d bytes", len(req.TxHash), crypto.HashSize) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
internal/consensus/replay_stubs.go(1 hunks)internal/mempool/mempool.go(1 hunks)internal/mempool/mocks/mempool.go(1 hunks)internal/mempool/types.go(1 hunks)internal/rpc/core/doc.go(1 hunks)internal/rpc/core/mempool.go(3 hunks)internal/rpc/core/routes.go(2 hunks)light/proxy/routes.go(1 hunks)light/rpc/client.go(1 hunks)rpc/client/http/http.go(6 hunks)rpc/client/interface.go(1 hunks)rpc/client/local/local.go(1 hunks)rpc/client/mocks/client.go(1 hunks)rpc/client/mocks/remoteclient.go(1 hunks)rpc/client/rpc_test.go(1 hunks)rpc/coretypes/requests.go(1 hunks)rpc/coretypes/responses.go(1 hunks)rpc/openapi/openapi.yaml(4 hunks)types/tx.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- internal/rpc/core/doc.go
🔇 Additional comments (21)
internal/consensus/replay_stubs.go (1)
42-42: LGTM!The implementation of
GetTxByHashis consistent with the stub nature of theemptyMempoolstruct, returningnilas expected.rpc/coretypes/requests.go (1)
83-86: LGTM!The
RequestUnconfirmedTxtype is well-defined with appropriate documentation and field type. The JSON tagomitemptyis correctly used for optional fields.rpc/client/interface.go (1)
167-167: LGTM!The
UnconfirmedTxmethod is well-defined with appropriate parameters and return types, maintaining consistency with other methods in the interface.rpc/client/local/local.go (1)
108-110: LGTM!The implementation follows the established pattern of delegating to the environment's corresponding method, with proper parameter passing and error handling.
internal/mempool/mempool.go (1)
263-272: LGTM! Clean and efficient implementation.The implementation is thread-safe and provides O(1) lookup performance using the existing
txByKeymap.light/proxy/routes.go (1)
143-145: LGTM! Consistent with proxy pattern.The implementation follows the established pattern of delegating calls to the underlying client.
internal/mempool/types.go (1)
43-44: LGTM! Well-documented interface addition.The interface method is clearly documented and follows the established pattern of the codebase.
light/rpc/client.go (1)
243-245: LGTM!The implementation follows the established pattern of delegating to the next client, maintaining consistency with other similar methods in the file.
types/tx.go (1)
27-31: LGTM! Good optimization.The change to use the transaction's hash as the key is a good optimization as it:
- Reuses the existing hash computation
- Ensures consistent transaction identification across the system
- Makes it easier to lookup transactions in the mempool
internal/rpc/core/routes.go (1)
59-59: LGTM!The new RPC route and interface method are added consistently with the existing patterns in the codebase.
Also applies to: 117-117
internal/rpc/core/mempool.go (1)
198-210: LGTM! Implementation looks solid.The implementation correctly:
- Validates the input request and transaction hash
- Uses the mempool's GetTxByHash to retrieve the transaction
- Returns appropriate responses for both success and failure cases
rpc/client/rpc_test.go (1)
667-700: LGTM! Well-structured test case.The test case thoroughly validates the new
UnconfirmedTxfunctionality:
- Proper test setup with multiple transactions
- Robust synchronization using channels
- Timeout handling for transaction processing
- Verification of transaction retrieval by hash
- Proper cleanup
rpc/openapi/openapi.yaml (1)
2739-2761: LGTM! Well-defined response schema.The schema follows JSONRPC standards and properly defines the response structure with base64-encoded transaction data.
internal/mempool/mocks/mempool.go (2)
189-207: LGTM! The mock implementation forGetTxByHashis well-structured.The implementation follows the standard mockery pattern, with proper error handling and type assertions. The method signature matches the interface requirements.
209-235: LGTM! The mock helper types and methods are properly implemented.The implementation includes all necessary helper types and methods for setting up test expectations, following the standard mockery pattern consistently.
rpc/client/mocks/client.go (2)
1914-1942: LGTM! The mock implementation forUnconfirmedTxis well-structured.The implementation follows the standard mockery pattern, with proper error handling and type assertions. The method signature matches the interface requirements.
1944-1971: LGTM! The mock helper types and methods are properly implemented.The implementation includes all necessary helper types and methods for setting up test expectations, following the standard mockery pattern consistently.
rpc/client/mocks/remoteclient.go (1)
1959-2016: LGTM! The mock implementation follows best practices.The new
UnconfirmedTxmock method and its helper types are well-implemented:
- Consistent with the established pattern in the file
- Proper error handling and return value management
- Helper methods for test expectations
rpc/client/http/http.go (3)
10-10: LGTM! Import change aligns with the codebase.The change from
bytestotmbytespackage is consistent with the PR objectives and maintains uniformity across the codebase.
211-213: LGTM! Type changes maintain consistency.The change from
bytes.HexBytestotmbytes.HexBytesin method signatures:
- Maintains consistency across the codebase
- Properly handles transaction-related byte data
Also applies to: 215-226, 393-399, 421-429, 447-453
270-279: LGTM! UnconfirmedTx implementation follows best practices.The new
UnconfirmedTxmethod:
- Follows the established pattern of other RPC methods
- Properly handles errors
- Correctly converts txHash to HexBytes
🧩 Analysis chain🏁 Script executed: Length of output: 34752 @lklimek The changes look well-implemented! I particularly like:
A few minor suggestions to consider:
Let me know if you'd like me to elaborate on any of these points. ✅ Actions performedComments resolved. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
rpc/openapi/openapi.yaml (2)
1222-1251: New Endpoint for Retrieving an Unconfirmed TransactionThe new
/unconfirmed_txendpoint is integrated correctly. The query parameterhashis correctly marked as required with a clear example, and the response references the proper schema.Suggestions:
- Consider expanding the description to explain that this endpoint returns an unconfirmed transaction from the mempool if it exists, and clarify the expected behavior when no matching transaction is found (e.g. using a 404 status or a more specific error message).
2739-2761: Addition of the UnconfirmedTransactionResponse SchemaThe schema for
UnconfirmedTransactionResponseis well defined—it extends the standard JSON-RPC structure and specifies a requiredtxfield described as a Base-64 encoded transaction. This clear structure is consistent with the overall design of the API responses.Suggestions:
- If in the future the endpoint is expected to provide additional metadata (such as a status flag or timestamp), consider evolving this schema accordingly. For now, its simplicity aligns with the feature’s needs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
rpc/openapi/openapi.yaml(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: test_apps
- GitHub Check: test_abci_cli
- GitHub Check: tests (02)
- GitHub Check: tests (01)
- GitHub Check: e2e-test (rotate)
- GitHub Check: golangci-lint
- GitHub Check: e2e-test (dashcore)
- GitHub Check: Super linter
Issue being fixed or feature implemented
We need a way to check if a transaction exists in the mempool.
What was done?
Added
unconfirmed_tx?hash=0x.......endpointHow Has This Been Tested?
rotatee2e devnet.Breaking Changes
None
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Documentation
Refactor