-
Notifications
You must be signed in to change notification settings - Fork 221
fix: reduce db calls for getTransactionReceipt RPC #3320
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
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.
Pull request overview
This PR optimizes the getTransactionReceipt RPC endpoint by reducing the number of database calls. Instead of making separate queries by transaction hash to get the transaction and receipt, it first retrieves the block number and index from the hash, then uses those coordinates to fetch the transaction and receipt directly. This reduces database lookups from potentially 3+ calls down to 3 calls (one for the index lookup, one for transaction, one for receipt).
Key changes:
- Introduced new methods
TxBlockNumberAndIndexByHashandReceiptByBlockNumberAndIndexto enable block-coordinate-based lookups - Refactored
TransactionReceiptByHashto use the new lookup pattern
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| rpc/v9/transaction.go | Updated TransactionReceiptByHash to use new block-coordinate-based lookup methods |
| blockchain/blockchain.go | Added TxBlockNumberAndIndexByHash and ReceiptByBlockNumberAndIndex methods to Reader interface |
| core/accessors.go | Added GetReceiptByBlockNumIndex helper function for direct receipt lookup by coordinates |
| mocks/mock_blockchain.go | Generated mock implementations for new Reader interface methods |
| rpc/v9/transaction_test.go | Updated all test expectations to use new method call patterns |
| rpc/v9/subscriptions_test.go | Updated subscription test expectations for new method calls |
| rpc/v9/l1_test.go | Updated L1 message status test expectations and fixed type inconsistencies |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
Co-authored-by: Copilot <[email protected]> Signed-off-by: Yaroslav Kukharuk <[email protected]>
Co-authored-by: Dat Duong <[email protected]> Signed-off-by: Yaroslav Kukharuk <[email protected]>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3320 +/- ##
==========================================
- Coverage 76.20% 76.07% -0.13%
==========================================
Files 346 346
Lines 32690 32722 +32
==========================================
- Hits 24912 24894 -18
- Misses 5987 6029 +42
- Partials 1791 1799 +8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Yaroslav Kukharuk <[email protected]>
rodrigo-pino
left a 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.
Overview review, I am not sure I notice how we are doing less calls here
| BlockNumberAndIndexByTxHash( | ||
| hash *felt.TransactionHash) (blockNumber uint64, index uint64, err error) |
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.
Let's improve the formatting style here. The standard we are trying to have in Juno looks like:
BlockNumberAndIndexByTxHash(
hash *felt.TransactionHash,
) (blockNumber uint64, index uint64, err error)| ReceiptByBlockNumberAndIndex( | ||
| blockNumber, index uint64) (receipt *core.TransactionReceipt, blockHash *felt.Felt, err error) | ||
|
|
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.
Similar here
ReceiptByBlockNumberAndIndex(
blockNumber, index uint64,
) (receipt *core.TransactionReceipt, blockHash *felt.Felt, err error)| ) (*TransactionReceipt, error) { | ||
| var numIdxKey db.BlockNumIndexKey | ||
| numIdxKey.Number = num | ||
| numIdxKey.Index = index | ||
| receipt, err := ReceiptsByBlockNumberAndIndexBucket.Get(r, numIdxKey) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return &receipt, nil |
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.
Please return TransactionReceipt by value
Based on my quick look,
with this PR, non-pending request will read from:
|
Fixes: #3258