feat: introduce cache layer for fee denom and decimals #110
Conversation
… periodically fetch it
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces significant modifications to the Changes
Possibly related PRs
Suggested reviewers
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: 5
🧹 Outside diff range and nitpick comments (5)
jsonrpc/namespaces/cosmos/api.go (1)
20-20: Good improvement in context propagation!The addition of the context parameter aligns with Go's best practices for context propagation and will enable proper lifecycle management for the cache layer being introduced.
Ensure that this context is properly used for managing the lifecycle of any background operations, especially the periodic fee information fetching mentioned in the PR objectives.
jsonrpc/backend/gas.go (1)
42-44: Consider adding a retry mechanism for readiness check.While the readiness check is good, consider adding a retry mechanism with timeout for cases where the fee cache is temporarily unavailable. This would improve resilience and user experience.
// jsonrpc is not ready for querying if b.feeDenom == "" { + // Retry a few times with backoff before giving up + if err := b.waitForFeeDenom(b.ctx); err != nil { return hexutil.Uint64(0), NewInternalError("jsonrpc is not ready") + } }jsonrpc/backend/eth.go (1)
33-33: Consider enhancing error handlingThe use of cached values improves performance, but consider adding more context to error scenarios:
- When ERC20Keeper().GetBalance fails
- When balance conversion fails
balance, err := b.app.EVMKeeper.ERC20Keeper().GetBalance(queryCtx, sdk.AccAddress(address[:]), b.feeDenom) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to get balance for address %s: %w", address.Hex(), err) } -return (*hexutil.Big)(types.ToEthersUint(b.feeDecimals, balance.BigInt())), nil +ethBalance := types.ToEthersUint(b.feeDecimals, balance.BigInt()) +if ethBalance == nil { + return nil, fmt.Errorf("failed to convert balance to ethers unit with decimals %d", b.feeDecimals) +} +return (*hexutil.Big)(ethBalance), nilAlso applies to: 38-38
jsonrpc/jsonrpc.go (1)
Line range hint
67-106: Well-structured architecture for cache implementation.The changes introduce context propagation in a clean and consistent manner, which is essential for the new caching mechanism. The implementation:
- Maintains clear separation of concerns
- Preserves existing error handling and shutdown logic
- Uses error groups for proper cleanup
Consider documenting the caching behavior and cleanup mechanisms in the package documentation to help future maintainers understand the system's lifecycle.
jsonrpc/namespaces/eth/filters/api.go (1)
166-167: Consider adding proper resource cleanup on shutdown.While the context cancellation is handled correctly, consider closing channels and cleaning up resources when the event loop terminates to prevent potential resource leaks.
Consider adding cleanup code:
case <-api.ctx.Done(): + // Close channels + for _, s := range api.subscriptions { + close(s.headerChan) + close(s.logsChan) + close(s.txChan) + close(s.hashChan) + } return
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
jsonrpc/backend/backend.go(5 hunks)jsonrpc/backend/eth.go(1 hunks)jsonrpc/backend/gas.go(2 hunks)jsonrpc/jsonrpc.go(2 hunks)jsonrpc/namespaces/cosmos/api.go(1 hunks)jsonrpc/namespaces/eth/api.go(1 hunks)jsonrpc/namespaces/eth/filters/api.go(4 hunks)jsonrpc/namespaces/net/api.go(1 hunks)jsonrpc/namespaces/txpool/api.go(1 hunks)jsonrpc/namespaces/web3/api.go(1 hunks)
🔇 Additional comments (19)
jsonrpc/namespaces/cosmos/api.go (2)
22-24: LGTM! Clean struct initialization with proper context assignment.
The initialization is clean and maintains good practices with both context handling and structured logging.
20-24: Verify context usage in the fee cache implementation.
Since this context will be used for the fee cache layer, let's verify its proper usage in the implementation.
jsonrpc/namespaces/net/api.go (2)
27-27: LGTM! Good improvement in context management.
The addition of the context parameter to NewNetAPI is a positive change that allows for better request lifecycle management and aligns with best practices for context propagation.
29-29: Verify context usage in API methods.
While the context is now properly stored in the struct, none of the API methods (Listening, PeerCount, Version) currently utilize it. Consider whether these methods should respect context cancellation.
Consider propagating the context to backend calls if they involve network operations or other cancellable operations. This would improve request cancellation handling.
jsonrpc/namespaces/web3/api.go (2)
30-30: Verify context usage in API methods.
While storing the context is correct, neither ClientVersion() nor Sha3() currently utilize it. If these methods will interact with the new cache layer, they should use the stored context.
Let's check if other API methods in the codebase use context for cache interactions:
28-28: LGTM! Verify consistent context propagation.
The addition of context parameter aligns with Go best practices and supports the PR's caching objectives.
Let's verify that context is consistently propagated across related API constructors:
✅ Verification successful
Context propagation is consistently implemented across API constructors
The context parameter has been consistently added to all API constructors in the jsonrpc namespace:
- web3.NewWeb3API
- txpool.NewTxPoolAPI
- cosmos.NewCosmosAPI
- net.NewNetAPI
- eth.filters.NewFilterAPI
- eth.NewEthAPI
Each constructor properly stores the context in its API struct, following the same pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent context parameter usage in API constructors
# Expected: All API constructors should accept context.Context
# Search for API constructors
ast-grep --pattern 'func New$_API($$$) $_'
# Search for any remaining context.TODO() usage that might need updating
rg 'context\.TODO\(\)'
Length of output: 11103
Script:
#!/bin/bash
# Let's try a different approach to verify context propagation
# Search for API constructors and their context usage
rg -A 3 "func New.*API.*\("
# Search for context parameter in function signatures
rg -A 3 "context\.Context.*\)"
Length of output: 129164
Script:
#!/bin/bash
# Let's search for API constructors and their context usage more specifically
rg -A 3 "func New.*API.*\(" jsonrpc/namespaces/
Length of output: 1865
jsonrpc/namespaces/txpool/api.go (1)
Line range hint 35-41: LGTM! Good improvement in context management.
The change to accept a context parameter instead of using context.TODO() is a good practice as it:
- Allows proper context propagation for concurrent operations
- Enables better control over cancellation and deadlines
- Aligns with the broader initiative to standardize context handling across the codebase
Let's verify that all callers of this constructor have been updated:
✅ Verification successful
All callers have been updated to pass context parameter
The verification shows that NewTxPoolAPI is called in only two locations:
jsonrpc/jsonrpc.go: The caller correctly passes thectxparameterjsonrpc/namespaces/txpool/api.go: This is the constructor definition itself
The changes are consistent across the codebase with no missing updates.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all instances where NewTxPoolAPI is called to ensure they've been updated
# to pass a context parameter
# Search for NewTxPoolAPI calls
rg -A 2 "NewTxPoolAPI\(" --type go
Length of output: 415
jsonrpc/backend/gas.go (1)
52-52: Add validation for feeDecimals.
The cached feeDecimals is used for value conversion without validation. Consider adding bounds checking to prevent potential overflow or invalid conversions.
Let's verify the current validation of feeDecimals:
Also applies to: 59-59
jsonrpc/backend/eth.go (2)
Line range hint 41-186: LGTM! Good isolation of cache-related changes
The changes are well-isolated to fee-related functionality, with no impact on other methods. This demonstrates good separation of concerns.
28-30: Verify cache initialization and thread safety
The guard clause for uninitialized state is good. However, we should ensure proper cache initialization and thread safety.
jsonrpc/jsonrpc.go (2)
76-106: Consistent context propagation across API services.
The changes consistently propagate context to all API services, which is good practice for lifecycle management. However, we should verify proper context handling in each service implementation.
#!/bin/bash
# Description: Verify context usage in API service implementations
# Expected: Each service should properly handle context cancellation
# Check context usage in each namespace
for ns in eth filters net web3 txpool cosmos; do
echo "Checking $ns namespace..."
# Look for constructor implementation
ast-grep --pattern 'func New${_}API(ctx context.Context, $$$) {
$$$
}'
# Look for goroutines that should use this context
rg -A 5 "go func" "namespaces/$ns"
done67-70: Verify context propagation in backend implementation.
The addition of context to backend initialization aligns with the PR's objective of implementing a cache layer. However, we should ensure proper context handling in the backend.
✅ Verification successful
Context is properly propagated and utilized in the backend implementation
The verification shows that the context is correctly used in the backend implementation:
- The context is stored in the
JSONRPCBackendstruct and properly initialized inNewJSONRPCBackend - The context is actively used for controlling the lifecycle of background operations, specifically in the
feeFetchergoroutine which monitors the context's cancellation via<-b.ctx.Done() - The context is used for fee denom and decimals caching, which aligns with the PR's objective
- The implementation includes proper cache initialization and management using the provided context
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify context usage in backend implementation
# Expected: The context should be used for cache management and cleanup
# Check backend implementation for context usage
ast-grep --pattern 'func NewJSONRPCBackend(ctx context.Context, $$$) {
$$$
}'
# Look for cache-related goroutines that should use this context
rg -A 5 "go func" backend/
Length of output: 1705
Script:
#!/bin/bash
# Let's try a different approach to find the backend implementation and context usage
# Find the backend package location first
fd -t f "backend.*go$"
# Look for context usage in backend files
rg -l "context.Context" .
# Search for NewJSONRPCBackend implementation
rg -A 10 "func NewJSONRPCBackend" .
Length of output: 2589
Script:
#!/bin/bash
# Let's examine the backend implementation to verify context usage
# Check the full implementation of backend.go
cat jsonrpc/backend/backend.go
# Look for any cache-related code in the backend
rg -A 5 "cache" jsonrpc/backend/
Length of output: 12083
jsonrpc/namespaces/eth/filters/api.go (3)
45-46: LGTM! Context field addition follows best practices.
The addition of the ctx field to FilterAPI struct is well-placed and follows Go conventions for field ordering.
68-72: LGTM! Constructor properly initializes context.
The context parameter addition follows Go's context package conventions by making it the first parameter, and it's correctly stored in the struct.
105-124: LGTM! Excellent concurrency handling with proper documentation.
The implementation demonstrates good practices:
- Proper mutex usage for thread safety
- Clear documentation explaining the unsubscribe processing outside the lock
- Correct context cancellation handling
jsonrpc/namespaces/eth/api.go (3)
110-112: Ensure consistent context usage across API methods.
While the constructor now accepts and stores a context, it's currently only used in GetBlockReceipts. Consider propagating the stored context to other API methods, especially those making backend calls, to ensure consistent timeout and cancellation handling.
Let's verify the context usage in API methods:
Line range hint 447-449: Review fee-related methods for cache interaction.
With the introduction of a fee cache layer, the following methods need careful consideration:
GasPriceEstimateGasMaxPriorityFeePerGas
Ensure these methods:
- Handle cache miss scenarios gracefully
- Consider potential race conditions during cache updates
- Implement proper error handling for cache invalidation
Let's examine the backend implementation of these methods:
Also applies to: 460-463, 466-469
110-112: Verify completion of caching implementation.
While the context changes prepare for the fee cache layer, the actual caching implementation seems to be missing. This might be part of a larger change set, but please ensure:
- The caching logic is implemented in the backend
- Cache invalidation strategy is documented
- Cache-related configuration (TTL, size limits) is properly defined
Let's check for cache-related implementations:
jsonrpc/backend/backend.go (1)
73-73: Ensure all callers of NewJSONRPCBackend pass the new context parameter
The constructor NewJSONRPCBackend now requires a context.Context parameter. Please verify that all invocations of this constructor throughout the codebase have been updated to pass the appropriate context.
Run the following script to identify any calls to NewJSONRPCBackend that may not have been updated:
Expected result: No matches indicating missing context parameters.
✅ Verification successful
Let me verify the implementation of NewJSONRPCBackend and its usage to ensure we have a complete picture.
All callers of NewJSONRPCBackend are properly updated
The verification shows that there is only one caller of NewJSONRPCBackend in jsonrpc/jsonrpc.go and it correctly passes the context parameter as the first argument: backend.NewJSONRPCBackend(ctx, app, logger, svrCtx, clientCtx, jsonRPCConfig). All parameters match the function signature and are in the correct order.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all calls to NewJSONRPCBackend pass the context parameter
# Search for calls to NewJSONRPCBackend with missing context parameter
rg 'NewJSONRPCBackend\s*\(' --glob '!jsonrpc/backend/backend.go' -A 1 | grep -B 1 -E 'NewJSONRPCBackend\s*\(\s*\w+\s*\,'
Length of output: 232
Script:
#!/bin/bash
# Get the implementation of NewJSONRPCBackend
ast-grep --pattern 'func NewJSONRPCBackend($$$) ($$$) {
$$$
}'
# Get all usages of NewJSONRPCBackend
rg 'NewJSONRPCBackend\s*\(' -A 2
Length of output: 3740
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #110 +/- ##
==========================================
- Coverage 28.25% 28.17% -0.08%
==========================================
Files 126 126
Lines 14237 14275 +38
==========================================
Hits 4022 4022
- Misses 9642 9680 +38
Partials 573 573
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
jsonrpc/backend/backend.go (2)
130-139: Ensure appropriate error handling infeeInfomethodThe
feeInfomethod returnsNewInternalError("jsonrpc is not ready")whenfeeDenomis empty. Verify thatNewInternalErroris the correct error constructor for this context and that it aligns with the overall error handling strategy of the project.
130-139: Consider adding unit tests for thefeeInfomethodImplementing unit tests for
feeInfocan help ensure it behaves correctly, especially in scenarios where fee data may not be initialized yet.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
jsonrpc/backend/backend.go(5 hunks)jsonrpc/backend/eth.go(2 hunks)jsonrpc/backend/gas.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- jsonrpc/backend/eth.go
🔇 Additional comments (5)
jsonrpc/backend/gas.go (1)
111-125: LGTM! Well-documented and secure implementation.
The changes have effectively addressed previous concerns by:
- Documenting the precision multiplier's purpose
- Adding overflow protection
- Using consistent fee information retrieval
jsonrpc/backend/backend.go (4)
43-47: Addition of fee cache fields is appropriate
The introduction of feeDenom, feeDecimals, and feeMutex to the JSONRPCBackend struct effectively sets up the necessary fields for caching fee information with proper synchronization.
74-74: Inclusion of context parameter enhances lifecycle management
Adding ctx context.Context to the NewJSONRPCBackend constructor allows for better control over the backend's lifecycle, enabling cancellation and timeout functionalities.
124-126: Potential goroutine leak due to feeFetcher persists
The issue regarding the potential goroutine leak remains unresolved. The feeFetcher goroutine may continue running if the context b.ctx is not properly canceled when the JSONRPCBackend is no longer needed.
141-185: Initial fetch missing in feeFetcher, may cause startup delays
The feeFetcher function waits for the first ticker interval (5 seconds) before fetching fee data, which leaves feeDenom and feeDecimals unset initially. This can cause errors if other methods access these fields before they are populated.
* loading fee requires executing evm call, so introduce cache layer and periodically fetch it * decrease update interval * introduce lock * decrease fetch interval
Description
currently fee decimals should be fetched by calling EVM static call, so it is degrading performance on json-rpc. to avoid this performance issue, we introduce fee cache layer by fetch fee denominated and decimals in a go-rountine periodically.
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
Release Notes
New Features
GetBlockReceiptsfor retrieving block receipts in the Ethereum API.Improvements
EstimateGasmethod for more efficient fee denomination retrieval.Bug Fixes
Documentation