Rebase polymer develop to match v0.0.24 #2
Conversation
|
Warning Rate limit exceeded@OBlackmon3 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 1 minutes and 36 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe changes encompass updates to various files, primarily focusing on enhancing the GitHub Actions workflows for software release and testing, introducing new configuration structures, and adding error handling mechanisms. Notably, the Dockerfile has been modified for binary management, and several connectors have been updated to include time-to-live (TTL) functionalities. Overall, the modifications aim to improve configuration capabilities, error management, and the CI/CD pipeline. Changes
Possibly related PRs
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 32
🧹 Outside diff range and nitpick comments (37)
.github/workflows/test.yml (2)
19-22: Excellent addition of security scanning, with some suggestions for improvement.The inclusion of the Gosec Security Scanner is a great step towards enhancing the project's security. Here are some suggestions to further improve this addition:
Consider pinning the Gosec action to a specific version (e.g.,
securego/[email protected]) instead of usingmaster. This ensures reproducibility and prevents unexpected changes due to action updates.It might be beneficial to add explicit handling of scan results. For example, you could fail the workflow if high-severity issues are found:
- name: Run Gosec Security Scanner uses: securego/[email protected] with: args: -exclude-dir=test -tests=false -fmt=sarif -out=results.sarif ./... - name: Upload SARIF file uses: github/codeql-action/upload-sarif@v2 with: sarif_file: results.sarif
- To optimize the workflow, consider running this step only on specific branches or pull requests to reduce CI time on frequent pushes.
Would you like me to provide a full example of these optimizations?
Line range hint
1-28: Consider further optimizations for the entire workflow.The workflow is well-structured and now includes important security scanning. To further enhance its efficiency, consider the following optimizations:
- Implement caching for Go modules to speed up the build process:
- uses: actions/cache@v3 with: path: | ~/.cache/go-build ~/go/pkg/mod key: ${{ runner.os }}-go-${{ hashFiles('**/go.sum') }} restore-keys: | ${{ runner.os }}-go-
- Use
go work syncif you're using Go workspaces to ensure consistent versioning:- name: Sync Go workspaces run: go work sync
- Consider parallelizing steps that can run independently, such as linting and testing:
jobs: lint: # ... linting job test: # ... testing job security-scan: # ... security scanning job build: needs: [lint, test, security-scan] # ... build job
- If applicable, implement matrix testing for multiple Go versions or operating systems.
Would you like me to provide a full example of an optimized workflow incorporating these suggestions?
Dockerfile (1)
Line range hint
32-45: Overall assessment: Improved organization with a minor security concernThe changes to this Dockerfile improve the organization of the container by moving the binaries to a standard location (
/usr/bin/). This is a positive change that aligns with best practices.However, the permission setting of 777 for the binaries is overly permissive and could potentially be a security risk. While the impact might be limited in a container environment, it's generally recommended to use more restrictive permissions.
The command to run the application has been correctly updated to reflect the new binary locations, maintaining the existing functionality.
Consider the following recommendations:
- Keep the improved binary locations in
/usr/bin/.- Adjust the permissions to be more restrictive (e.g., 755) as suggested earlier.
- If possible, consider running the container as a non-root user for improved security. This would involve adding a user, changing ownership of the necessary files, and using the
USERinstruction in the Dockerfile.upstream/ratelimiter_budget.go (2)
Line range hint
44-44: Update logging format specifier for unsigned integerConsider updating the logging statement to use
%uinstead of%dfor thenewMaxCountvalue, as it's now an unsigned integer. This provides more precise formatting for unsigned values.Apply this change:
-b.logger.Warn().Str("method", rule.Config.Method).Msgf("adjusting rate limiter budget from: %d to: %d ", rule.Config.MaxCount, newMaxCount) +b.logger.Warn().Str("method", rule.Config.Method).Msgf("adjusting rate limiter budget from: %d to: %u ", rule.Config.MaxCount, newMaxCount)
Line range hint
39-58: Add check for unnecessary updatesConsider adding a check to avoid unnecessary updates when the new count is the same as the old count. This can improve performance by skipping the creation of a new rate limiter when no change is needed.
Here's a suggested implementation:
func (b *RateLimiterBudget) AdjustBudget(rule *RateLimitRule, newMaxCount uint) error { b.rulesMu.Lock() defer b.rulesMu.Unlock() + if rule.Config.MaxCount == newMaxCount { + return nil // No change needed + } + b.logger.Warn().Str("method", rule.Config.Method).Msgf("adjusting rate limiter budget from: %d to: %u ", rule.Config.MaxCount, newMaxCount) newCfg := &common.RateLimitRuleConfig{ Method: rule.Config.Method, Period: rule.Config.Period, MaxCount: newMaxCount, WaitTime: rule.Config.WaitTime, } newLimiter, err := b.registry.createRateLimiter(newCfg) if err != nil { return err } rule.Config = newCfg rule.Limiter = newLimiter return nil }data/mock_connector.go (2)
32-36: LGTM! Consider enhancing the comment.The implementation of the
SetTTLmethod is correct and consistent with the existing mock methods. It properly usesmock.Calledto simulate behavior for testing purposes.Consider expanding the comment to provide more context about the purpose of the TTL and the expected format of the
ttlStrparameter. This can help developers using this mock in their tests. For example:// SetTTL mocks the SetTTL method of the Connector interface. // It simulates setting a Time-To-Live (TTL) for a specific method. // The ttlStr parameter is expected to be a string representation of the TTL duration.
38-42: LGTM! Consider enhancing the comment.The implementation of the
HasTTLmethod is correct and consistent with the existing mock methods. It properly usesmock.Calledto simulate behavior for testing purposes.Consider expanding the comment to provide more context about the purpose of the method. This can help developers using this mock in their tests. For example:
// HasTTL mocks the HasTTL method of the Connector interface. // It simulates checking whether a Time-To-Live (TTL) is set for a specific method. // Returns true if a TTL is set, false otherwise.data/memory.go (2)
50-52: Change receiver name for consistencyThe receiver name
dis inconsistent with other methods in this struct, which usem. For better code consistency, consider changing the receiver name.Here's the suggested change:
-func (d *MemoryConnector) HasTTL(_ string) bool { +func (m *MemoryConnector) HasTTL(_ string) bool { return false }
45-52: Add documentation about TTL support statusThe addition of
SetTTLandHasTTLmethods suggests that TTL support is planned for the future. However, the current implementation is just a placeholder. To prevent misunderstandings and potential bugs, consider adding documentation to clearly state the current status of TTL support.Here's a suggested addition at the beginning of the
MemoryConnectorstruct definition:// MemoryConnector implements the Connector interface using in-memory storage. // Note: TTL functionality is not currently supported. type MemoryConnector struct { logger *zerolog.Logger cache *lru.Cache[string, string] }upstream/envio_http_json_rpc_client.go (1)
120-120: Approve the change with a suggestion for improvementThe modification to increase the range of random IDs from 1,000,000 to 100,000,000 is a good improvement. It reduces the likelihood of ID collisions in high-traffic scenarios, which aligns with the PR's objective of enhancing performance during high traffic.
The added comment
// #nosec G404to suppress the security warning is appropriate in this context. The random ID doesn't require cryptographic randomness as it's not used for security-critical operations.Consider creating a package-level random number generator to improve code organization and potentially avoid issues with concurrent access:
var ( rng = rand.New(rand.NewSource(time.Now().UnixNano())) rngMu sync.Mutex ) func generateRandomID() int { rngMu.Lock() defer rngMu.Unlock() return rng.Intn(100_000_000) }Then, you can replace the current line with:
rid := generateRandomID()This approach ensures thread-safe access to the random number generator and centralizes the random ID generation logic.
docs/pages/operation/directives.mdx (3)
21-29: LGTM with minor suggestion: Clear explanation of disabling retry behaviorThe added information about disabling the retry behavior for empty responses is clear and helpful. The callout provides important context about the behavior's application to unfinalized data.
Consider adding a comma after "For example" in line 29 for better readability:
-For example when you're requesting eth_getTransactionReceipt of mostly reecent transactions and prefer to immeditely get an empty response and handle it on your client side: +For example, when you're requesting eth_getTransactionReceipt of mostly recent transactions and prefer to immediately get an empty response and handle it on your client side:Also, note the typos "reecent" and "immeditely" in the same line, which should be corrected to "recent" and "immediately" respectively.
🧰 Tools
🪛 LanguageTool
[typographical] ~29-~29: After the expression ‘for example’ a comma is usually used.
Context: ...l and won't be retried. For example when you're requesting eth_getTransacti...(COMMA_FOR_EXAMPLE)
58-62: LGTM with minor suggestion: Clear explanation of disabling retry for pending transactionsThe added information about disabling the retry behavior for pending transactions is clear and consistent with the previous section. The callout about caching behavior for pending transactions is informative and important.
Consider adding a comma after "For example" in line 62 for better readability:
-For example if you're intentionally looking to query data of pending transactions (e.g. MEV bot) and prefer to immeditely get the pending tx data: +For example, if you're intentionally looking to query data of pending transactions (e.g. MEV bot) and prefer to immediately get the pending tx data:Also, note the typo "immeditely" in the same line, which should be corrected to "immediately".
🧰 Tools
🪛 LanguageTool
[typographical] ~62-~62: After the expression ‘for example’ a comma is usually used.
Context: ...y parameter?retry-pending=falseFor example if you're intentionally looking to quer...(COMMA_FOR_EXAMPLE)
113-142: LGTM with minor suggestion: Well-documented new "Use specific upstream(s)" directiveThe new section for the "Use specific upstream(s)" directive is well-structured and provides clear explanations and examples. The inclusion of information about wildcard matching is particularly helpful.
Consider rephrasing line 115 for better clarity:
-When sending requests to eRPC you can instruct to use only one specific upstream (or multiple via wildcard match) using: +When sending requests to eRPC, you can specify the use of one or more specific upstreams (including via wildcard matching) using:This rephrasing improves readability and avoids the awkward "instruct to use" construction.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~115-~115: Possible missing comma found.
Context: ...c upstream(s) When sending requests to eRPC you can instruct to use only one specif...(AI_HYDRA_LEO_MISSING_COMMA)
[grammar] ~115-~115: Did you mean “using”? Or maybe you should add a pronoun? In active voice, ‘instruct’ + ‘to’ takes an object, usually a pronoun.
Context: ...nding requests to eRPC you can instruct to use only one specific upstream (or multiple...(ALLOW_TO)
[typographical] ~125-~125: After the expression ‘for example’ a comma is usually used.
Context: ...s starting with "priv-" For example if you want to make sure that request i...(COMMA_FOR_EXAMPLE)
upstream/pimlico_http_json_rpc_client.go (1)
Line range hint
18-77: Consider a more maintainable solution for supported chainsThe
pimlicoSupportedChainsmap is currently hardcoded with a large number of chain IDs. While this works, it might become difficult to maintain as new chains are added or removed. Consider implementing a more dynamic solution, such as:
- Fetching the supported chains from a Pimlico API endpoint.
- Loading the supported chains from a configuration file.
- Implementing a method to update the supported chains at runtime.
This would make it easier to keep the supported chains up-to-date without requiring code changes and redeployment.
Would you like assistance in implementing one of these solutions?
data/postgresql.go (2)
97-99: Add documentation to HasTTL method and consider loggingThe
HasTTLmethod correctly indicates that TTL is not supported, but it could benefit from some improvements:
- Add a comment explaining the method's purpose and why it always returns false.
- Consider logging when this method is called, as it might indicate that a user is expecting TTL functionality.
Here's a suggested implementation with comments:
+// HasTTL always returns false as PostgreSQLConnector does not support TTL. +// The input parameter is ignored. func (p *PostgreSQLConnector) HasTTL(_ string) bool { + p.logger.Debug().Msg("HasTTL called, but TTL is not supported for PostgreSQLConnector") return false }This change improves code documentation and adds logging for better observability.
92-99: Make lack of TTL support more explicit in PostgreSQLConnectorWhile the addition of
SetTTLandHasTTLmethods prepares the ground for potential future TTL support, it's important to make the current lack of support more explicit. Consider the following suggestions:
- Add a constant or field to the
PostgreSQLConnectorstruct indicating TTL support status.- Update the
NewPostgreSQLConnectorfunction to log a warning about lack of TTL support.- Consider adding a
SupportsTTL() boolmethod to theConnectorinterface and implement it forPostgreSQLConnector.Here's an example of how you might implement these suggestions:
type PostgreSQLConnector struct { // ... existing fields ... supportsTTL bool } func NewPostgreSQLConnector(ctx context.Context, logger *zerolog.Logger, cfg *common.PostgreSQLConnectorConfig) (*PostgreSQLConnector, error) { // ... existing code ... p := &PostgreSQLConnector{ // ... existing fields ... supportsTTL: false, } logger.Warn().Msg("PostgreSQLConnector does not support TTL") // ... rest of the function ... } func (p *PostgreSQLConnector) SupportsTTL() bool { return p.supportsTTL }These changes would make it clearer throughout the codebase that TTL is not supported, reducing the chance of misunderstandings or incorrect usage.
common/json_rpc.go (2)
54-60: Approve panic recovery, but suggest improvementsThe addition of panic recovery is a good practice to prevent crashes during JSON unmarshalling. However, there are a few points to consider:
Instead of using
log.Printf, consider using structured logging with zerolog for consistency with the rest of the codebase. This will make it easier to parse and analyze logs.Be cautious about logging raw JSON responses, as they might contain sensitive data. Consider implementing a sanitization mechanism or logging only non-sensitive parts of the response.
While logging the panic is helpful, it might be better to return an error from the method instead of silently recovering. This would allow callers to handle the error appropriately.
Here's a suggested improvement:
defer func() { if rec := recover(); rec != nil { // Create a sanitized version of the raw JSON response sanitizedJSON := sanitizeJSON(r.Result) // Use zerolog for structured logging zerolog.Error(). Str("panic", fmt.Sprintf("%v", rec)). Str("rawJSON", sanitizedJSON). Msg("Panic occurred during JSON unmarshalling") // Set parsedResult to nil and return an error r.parsedResult = nil err = fmt.Errorf("panic during unmarshalling: %v", rec) } }()Note: You'll need to implement the
sanitizeJSONfunction to remove or mask sensitive data.
Inconsistent Logging in Panic Recovery
The
ParsedResultmethod introduces a panic recovery block that useslog.Printffor logging. However, the rest of the codebase utilizeszerologfor logging purposes. To maintain consistency and improve maintainability, consider updating the panic recovery to usezerologinstead oflog.Printf.🔗 Analysis chain
Line range hint
1-280: Summary: Improved error handling with room for refinementThe changes in this file enhance error handling by adding panic recovery to the
ParsedResultmethod. While this is a positive improvement, there are opportunities to refine the implementation for better consistency with the codebase's logging practices and to address potential security concerns.To ensure that this change doesn't mask any underlying issues and that it's consistent with error handling in other parts of the codebase, please run the following verification:
This will help verify the consistency of error handling and logging practices across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for similar unmarshalling operations and their error handling # Search for Unmarshal calls in the codebase echo "Unmarshalling operations in the codebase:" rg --type go 'sonic\.Unmarshal|json\.Unmarshal' # Search for panic recovery patterns echo "\nExisting panic recovery patterns:" rg --type go 'defer func\(\) \{.*if rec := recover\(\);' # Search for zerolog usage echo "\nZerolog usage in the codebase:" rg --type go 'zerolog\.(Error|Warn|Info|Debug|Trace)'Length of output: 2568
erpc/evm_json_rpc_cache_test.go (9)
83-83: Addition of TTL check enhances test coverageThe inclusion of the
HasTTLmock expectation is a valuable addition to the test case. It ensures that the caching behavior is correctly tested when items don't have a Time To Live (TTL) set. This change improves the test's comprehensiveness.Consider adding an additional test case where
HasTTLreturnstrueto cover both scenarios and ensure complete test coverage for TTL-related behavior.
152-152: TTL check addition enhances test coverage for block reference scenariosThe inclusion of the
HasTTLmock expectation within the test case loop is a good addition. It ensures that TTL behavior is consistently tested across different block reference scenarios.Consider moving the
HasTTLmock setup outside the loop if the behavior is expected to be the same for all sub-test cases. This could slightly improve test performance and reduce redundancy.
186-186: TTL check addition for unfinalized block scenarioThe inclusion of the
HasTTLmock expectation maintains consistency with other test cases. However, since this test case is designed to skip caching for unfinalized blocks, theHasTTLcheck might be redundant.Consider removing this line if the
Setmethod is never called in this scenario, as it may not contribute to the test's effectiveness and could potentially confuse readers about the expected behavior.
200-200: TTL check addition for unsynced node scenarioThe inclusion of the
HasTTLmock expectation maintains consistency with other test cases. However, since this test case is designed to skip caching when the node is not synced, theHasTTLcheck might be redundant.Consider removing this line if the
Setmethod is never called in this scenario, as it may not contribute to the test's effectiveness and could potentially confuse readers about the expected behavior.
214-214: TTL check addition for unknown sync state scenarioThe inclusion of the
HasTTLmock expectation maintains consistency with other test cases. However, since this test case is designed to skip caching when the sync state is unknown, theHasTTLcheck might be redundant.Consider removing this line if the
Setmethod is never called in this scenario, as it may not contribute to the test's effectiveness and could potentially confuse readers about the expected behavior.
228-228: TTL check addition for non-finalized block scenarioThe inclusion of the
HasTTLmock expectation maintains consistency with other test cases. However, since this test case is designed to skip caching when the block is not finalized, theHasTTLcheck might be redundant.Consider removing this line if the
Setmethod is never called in this scenario, as it may not contribute to the test's effectiveness and could potentially confuse readers about the expected behavior.
242-242: TTL check addition for indeterminable block number scenarioThe inclusion of the
HasTTLmock expectation maintains consistency with other test cases. However, since this test case is designed to skip caching when the block number cannot be determined, theHasTTLcheck might be redundant.Consider removing this line if the
Setmethod is never called in this scenario, as it may not contribute to the test's effectiveness and could potentially confuse readers about the expected behavior.
292-292: TTL check addition for unfinalized block cache skip scenarioThe inclusion of the
HasTTLmock expectation maintains consistency with other test cases. However, since this test case is designed to skip cache retrieval for unfinalized blocks, theHasTTLcheck might be redundant.Consider removing this line if the
Getmethod is never called in this scenario, as it may not contribute to the test's effectiveness and could potentially confuse readers about the expected behavior.
Line range hint
1-300: Overall assessment of changes to evm_json_rpc_cache_test.goThe changes made to this file primarily involve the addition of
HasTTLchecks across various test cases, which generally improves the test coverage by considering Time To Live (TTL) behavior in the caching mechanism. The consistent pattern of adding these checks maintains uniformity across test cases and enhances the overall robustness of the test suite.However, there are several instances where the
HasTTLcheck might be redundant, particularly in test cases designed to skip caching. To optimize the test suite:
- Consider removing redundant
HasTTLchecks in test cases where caching is explicitly skipped.- Evaluate the possibility of consolidating common setup code, such as the
HasTTLmock, to reduce repetition.- Consider adding test cases where
HasTTLreturnstrueto ensure complete coverage of TTL-related behavior.These optimizations would further improve the clarity and efficiency of the test suite while maintaining its comprehensive coverage.
data/dynamodb.go (1)
269-271: Add comment explaining TTL support statusThe
HasTTLmethod correctly returnsfalse, indicating that TTL functionality is not supported. However, to improve code clarity and maintainability, consider adding a comment explaining the current status of TTL support.Here's a suggested improvement:
func (d *DynamoDBConnector) HasTTL(_ string) bool { + // TTL functionality is not currently supported for DynamoDBConnector return false }This addition provides context for why the method always returns
falseand makes it easier for future developers to understand the current state of TTL support.test/fake_server.go (4)
Line range hint
22-30: Validate that 'MaxDelay' is greater than or equal to 'MinDelay'In the
NewFakeServerfunction, there is no check to ensure thatMaxDelayis greater than or equal toMinDelay. IfMaxDelayis less thanMinDelay, therandomDelayfunction may cause a panic due to calculating a negative duration.Apply this diff to add validation for the delay parameters:
func NewFakeServer(port int, failureRate float64, minDelay, maxDelay time.Duration, sampleFilePath string) (*FakeServer, error) { + if maxDelay < minDelay { + return nil, fmt.Errorf("maxDelay must be greater than or equal to minDelay") + } fs := &FakeServer{ Port: port, FailureRate: failureRate, MinDelay: minDelay, MaxDelay: maxDelay, }
Line range hint
64-72: Use 'sonic' for JSON decoding to improve performance and consistencyIn the
handleRequestmethod, you are usingencoding/jsonto decode the request, while elsewhere you use thesoniclibrary for JSON operations. For consistency and performance improvements, consider usingsonicfor decoding the JSON-RPC request.Apply this diff to switch to
sonicfor decoding:func (fs *FakeServer) handleRequest(w http.ResponseWriter, r *http.Request) { fs.mu.Lock() fs.requestsHandled++ fs.mu.Unlock() // Simulate delay time.Sleep(fs.randomDelay()) // Decode JSON-RPC request - var req JSONRPCRequest - err := json.NewDecoder(r.Body).Decode(&req) + data, err := io.ReadAll(r.Body) + if err != nil { + fs.mu.Lock() + fs.requestsFailed++ + fs.mu.Unlock() + http.Error(w, "Failed to read request body", http.StatusBadRequest) + return + } + var req JSONRPCRequest + err = sonic.Unmarshal(data, &req) if err != nil { fs.mu.Lock() fs.requestsFailed++Add the necessary import at the top of the file:
import ( - "encoding/json" + "io"
Line range hint
89-101: Safely handle type assertions and improve parameter comparison in 'findMatchingSample'In the
findMatchingSamplemethod, type assertions are performed onsample.Request.Paramsandreq.Paramswithout checking if the assertions succeed. This can cause a panic ifParamsis not of type[]interface{}. Additionally, the parameter comparison may not correctly compare nested parameters or complex types.Apply this diff to safely handle type assertions and enhance parameter comparison:
func (fs *FakeServer) findMatchingSample(req JSONRPCRequest) *JSONRPCResponse { for _, sample := range fs.samples { if sample.Request.Method == req.Method { - // if params values are also equal one by one - if len(sample.Request.Params.([]interface{})) == len(req.Params.([]interface{})) { - for i, param := range sample.Request.Params.([]interface{}) { - if param != req.Params.([]interface{})[i] { - break - } - } - } + sampleParams, ok1 := sample.Request.Params.([]interface{}) + reqParams, ok2 := req.Params.([]interface{}) + if ok1 && ok2 && len(sampleParams) == len(reqParams) { + match := true + for i, param := range sampleParams { + if !reflect.DeepEqual(param, reqParams[i]) { + match = false + break + } + } + if match { + return &sample.Response + } + } + // Handle other types of Params if necessary } } return nil }Add the necessary import at the top of the file:
import ( "fmt" + "reflect" )
Line range hint
62-73: Consider moving the simulated delay after request validationCurrently, the simulated delay is applied before decoding and validating the request. This means that malformed or invalid requests experience the same delay as valid ones, which may not be desirable. Moving the delay after successful decoding could improve responsiveness to invalid requests.
data/redis.go (2)
117-124: Log errors when parsing TTL durations inSetTTLCurrently, if there's an error parsing the TTL string, it's returned but not logged, which might make debugging difficult.
Enhance the error handling by logging the parsing error:
func (r *RedisConnector) SetTTL(method string, ttlStr string) error { ttl, err := time.ParseDuration(ttlStr) if err != nil { + r.logger.Error().Err(err).Msgf("Failed to parse TTL '%s' for method '%s'", ttlStr, method) return err } r.ttls[strings.ToLower(method)] = ttl return nil }
126-129: Consider removing the unusedHasTTLmethodThe
HasTTLmethod is not used anywhere in the codebase.If this method is not planned for future use, removing it can keep the code clean.
erpc/networks.go (1)
158-165: Inconsistent step numbering in commentsThe step numbering in the comments is inconsistent:
- The comment on line 158~ is labeled as
// 3) Check if we should handle this method on this network.- The subsequent comment (not shown in the provided code) likely continues with
// 3)instead of// 4).This could lead to confusion when reading the code. Consider updating the step numbers to maintain sequential order and improve readability.
erpc/http_server_test.go (1)
451-453: Remove or uncomment code to maintain code cleanlinessThe lines initializing a cancellable context and deferring
cancel()are commented out:// ctx, cancel := context.WithCancel(context.Background()) // defer cancel()Additionally, the deferred shutdown of the HTTP server is commented out:
// defer httpServer.server.Shutdown()If these lines are unnecessary, consider removing them to keep the code clean. If they are needed for proper resource management, uncomment them to ensure that the context is appropriately canceled and the server is gracefully shut down.
Also applies to: 469-469
erpc/networks_test.go (1)
33-33: Consider using a lower log level for tests.Using
ErrorLevelmay suppress useful debugging information during test failures. Consider usingDebugLevelorInfoLevelinstead to capture more detailed logs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (33)
- .github/workflows/release.yml (2 hunks)
- .github/workflows/test.yml (1 hunks)
- Dockerfile (2 hunks)
- common/config.go (5 hunks)
- common/errors.go (2 hunks)
- common/json_rpc.go (2 hunks)
- common/request.go (4 hunks)
- data/connector.go (1 hunks)
- data/dynamodb.go (1 hunks)
- data/memory.go (1 hunks)
- data/mock_connector.go (1 hunks)
- data/postgresql.go (1 hunks)
- data/redis.go (4 hunks)
- docs/pages/operation/directives.mdx (7 hunks)
- erpc/evm_json_rpc_cache.go (3 hunks)
- erpc/evm_json_rpc_cache_test.go (11 hunks)
- erpc/http_server.go (7 hunks)
- erpc/http_server_test.go (10 hunks)
- erpc/init.go (1 hunks)
- erpc/networks.go (3 hunks)
- erpc/networks_test.go (10 hunks)
- test/fake_server.go (1 hunks)
- upstream/envio_http_json_rpc_client.go (1 hunks)
- upstream/evm_state_poller.go (2 hunks)
- upstream/failsafe.go (1 hunks)
- upstream/http_json_rpc_client.go (3 hunks)
- upstream/pimlico_http_json_rpc_client.go (1 hunks)
- upstream/ratelimiter_autotuner.go (3 hunks)
- upstream/ratelimiter_budget.go (1 hunks)
- upstream/ratelimiter_registry.go (1 hunks)
- upstream/registry.go (2 hunks)
- upstream/thirdweb_http_json_rpc_client.go (1 hunks)
- upstream/upstream.go (3 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/pages/operation/directives.mdx
[typographical] ~29-~29: After the expression ‘for example’ a comma is usually used.
Context: ...l and won't be retried. For example when you're requesting eth_getTransacti...(COMMA_FOR_EXAMPLE)
[typographical] ~62-~62: After the expression ‘for example’ a comma is usually used.
Context: ...y parameter?retry-pending=falseFor example if you're intentionally looking to quer...(COMMA_FOR_EXAMPLE)
[style] ~111-~111: This phrase is redundant. Consider writing “usual”.
Context: ...nse will still be subject to caching as per usual. ## Use specific upstream(s) When sen...(PER_USUAL)
[uncategorized] ~115-~115: Possible missing comma found.
Context: ...c upstream(s) When sending requests to eRPC you can instruct to use only one specif...(AI_HYDRA_LEO_MISSING_COMMA)
[grammar] ~115-~115: Did you mean “using”? Or maybe you should add a pronoun? In active voice, ‘instruct’ + ‘to’ takes an object, usually a pronoun.
Context: ...nding requests to eRPC you can instruct to use only one specific upstream (or multiple...(ALLOW_TO)
[typographical] ~125-~125: After the expression ‘for example’ a comma is usually used.
Context: ...s starting with "priv-" For example if you want to make sure that request i...(COMMA_FOR_EXAMPLE)
🔇 Additional comments (57)
.github/workflows/test.yml (1)
19-22: Monitor workflow performance after adding security scanning.The addition of the Gosec Security Scanner step enhances the CI/CD pipeline by introducing crucial security checks. This aligns well with best practices for secure development. However, it's important to monitor the impact of this new step on overall workflow performance, especially on larger codebases.
Consider the following:
- Monitor the execution time of this step across different runs to ensure it doesn't significantly slow down the CI process.
- If performance becomes an issue, consider running this step less frequently (e.g., only on PRs or specific branches) or in parallel with other steps if possible.
- Regularly review the scan results to ensure they provide valuable insights and adjust the configuration as needed.
To help monitor the impact, you can run the following script to check the workflow run times:
This script uses the GitHub CLI to fetch the last 10 runs of the test workflow, showing their creation time, conclusion, and duration in minutes. Regular monitoring of these metrics will help you assess the performance impact of the new security scanning step.
data/connector.go (1)
Line range hint
23-40: Consider updating NewConnector for TTL functionalityThe
NewConnectorfunction remains unchanged despite the addition of TTL-related methods to theConnectorinterface. This might indicate that the new TTL functionality is not yet integrated into the connector creation process.Consider whether the
NewConnectorfunction needs to be updated to accommodate the new TTL functionality. This could involve:
- Updating the
ConnectorConfigstruct to include TTL-related configuration.- Modifying the connector creation logic to initialize TTL settings.
To check if
ConnectorConfighas been updated elsewhere, run:#!/bin/bash # Description: Check if ConnectorConfig struct includes TTL-related fields # Test: Search for ConnectorConfig struct definition echo "ConnectorConfig struct definition:" rg --type go -n 'type ConnectorConfig struct' # Test: Check if ConnectorConfig includes any TTL-related fields echo "\nTTL-related fields in ConnectorConfig:" rg --type go -n 'TTL' -C 5 | rg 'type ConnectorConfig struct' -A 10Dockerfile (2)
32-33: LGTM: Improved binary placementMoving the binaries to
/usr/bin/is a good practice. It aligns with standard Linux filesystem hierarchy and improves the organization of the container's filesystem.
45-45: LGTM: Updated command reflects new binary locationsThe command has been correctly updated to use the new paths in
/usr/bin/. This change is consistent with the earlier modifications to the binary locations.upstream/ratelimiter_budget.go (1)
39-39: Approve type change touintwith a note on compatibilityThe change from
inttouintfornewMaxCountis a good improvement as it ensures non-negative values for the rate limit count. However, be aware that this might break backward compatibility if any existing code relies on passing negative values or using theinttype.To check for potential compatibility issues, run the following script:
✅ Verification successful
Type Change to
uintVerified with No Negative Value Issues FoundThe change from
inttouintfornewMaxCounthas been verified. No instances ofAdjustBudgetbeing called with negative literals or variables were found, ensuring that the rate limit counts remain non-negative.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for calls to AdjustBudget with potentially negative values # Test: Look for AdjustBudget calls with negative literals or variables that might be negative rg --type go 'AdjustBudget\([^)]+,\s*-?\d+\s*\)' || rg --type go 'AdjustBudget\([^)]+,\s*[a-zA-Z_][a-zA-Z0-9_]*\s*\)'Length of output: 205
data/mock_connector.go (1)
32-42: Ensure consistency with the actual Connector interface and implementation.The addition of
SetTTLandHasTTLmethods to theMockConnectorsuggests that these methods have been added to the actualConnectorinterface. To maintain consistency and ensure proper functionality:
- Verify that these methods are indeed part of the
Connectorinterface definition.- Ensure that all concrete implementations of the
Connectorinterface have been updated to include these new methods.- Update any relevant documentation describing the
Connectorinterface to include these new TTL-related functionalities.- Consider adding or updating tests for the actual implementation to cover these new TTL-related methods.
To verify the changes in the Connector interface and its implementations, you can run the following script:
This script will help identify any inconsistencies or missing implementations across the codebase.
.github/workflows/release.yml (3)
57-57: LGTM: Minor syntax change in version specificationThe change from double quotes to single quotes for the version specification is a valid syntax modification. It doesn't affect the functionality of the workflow.
77-80: Excellent addition: Enhanced Docker build capabilitiesThe addition of QEMU and Docker Buildx setup steps significantly improves the Docker image build process. These changes enable:
- Cross-platform builds (as seen in the
platformsparameter later in the workflow).- More efficient and flexible Docker image creation.
This enhancement aligns well with modern Docker best practices and allows for wider compatibility of the built images.
Line range hint
1-103: Summary: Improved release workflow with enhanced Docker supportThe changes to this workflow file are well-considered and beneficial:
- The minor syntax change in the release job maintains functionality while potentially improving consistency.
- The additions to the docker-image job significantly enhance the Docker build process, enabling cross-platform image creation.
These improvements align with best practices for modern CI/CD pipelines and Docker image distribution. The workflow now supports a wider range of deployment scenarios and improves compatibility across different architectures.
upstream/ratelimiter_registry.go (1)
73-73: Approve change with suggestions for improvementThe removal of the
uint()type conversion simplifies the code and may offer a minor performance improvement, aligning with the PR's performance enhancement goals. However, I have a few suggestions to ensure robustness and clarity:
Please verify that the
BurstyBuilderfunction in the current version offailsafe-goaccepts the type ofrule.MaxCountwithout conversion. This change suggests there might have been an update to the library's API.Consider adding a comment explaining the rationale for removing the type conversion, especially if it's related to a change in the
failsafe-golibrary or if it allows for a wider range of values.If
rule.MaxCountis expected to be non-negative, it would be prudent to add a check to ensure this, as the removal of theuintconversion removes an implicit non-negative constraint.To verify the
BurstyBuilderfunction signature and usage in the current version offailsafe-go, please run the following command:This will help confirm that the change is compatible with the current library version.
erpc/init.go (2)
Line range hint
1-114: Suggest thorough testing of the rebased changes.While the visible change in this file is minimal and beneficial, it's important to note that this PR represents a rebase of the main branch onto the polymer-develop branch to incorporate changes from v0.0.24. There might be other changes introduced by this rebase that are not visible in this specific diff.
To ensure the stability and correctness of the rebased changes:
- Conduct thorough testing of the entire system, particularly focusing on high traffic scenarios as mentioned in the PR objectives.
- Verify that all existing functionality remains intact after the rebase.
- Run the full test suite and ensure all tests pass.
#!/bin/bash # Description: Check for changes introduced by the rebase # List all changed files in this PR echo "Files changed in this PR:" git diff --name-only origin/main...HEAD # Show a summary of changes (stats only) echo "\nSummary of changes:" git diff --stat origin/main...HEADThis will help identify the full scope of changes introduced by the rebase, allowing for more targeted testing and review if necessary.
89-91: Approve the addition of ReadHeaderTimeout and suggest consistency check.The addition of a
ReadHeaderTimeoutof 10 seconds to the HTTP server configuration for metrics is a positive change. This timeout helps prevent slow-client attacks and aligns with the PR's objective of enhancing performance during high traffic scenarios.To ensure consistency across the codebase, let's verify if similar timeouts are applied to other HTTP servers:
This will help us identify if the timeout is consistently applied across all HTTP servers in the project.
✅ Verification successful
Approve the addition of
ReadHeaderTimeout.The
ReadHeaderTimeoutof 10 seconds has been successfully added to the HTTP server configuration inerpc/init.go. This enhancement helps prevent slow-client attacks and aligns with the PR's objective of improving performance during high traffic scenarios.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for ReadHeaderTimeout usage in HTTP server configurations # Search for HTTP server configurations echo "Searching for HTTP server configurations:" rg --type go -n 'http\.Server\s*{' # Search for ReadHeaderTimeout usage echo "\nSearching for ReadHeaderTimeout usage:" rg --type go -n 'ReadHeaderTimeout\s*:'Length of output: 685
Script:
#!/bin/bash # Description: Check for ReadHeaderTimeout usage in HTTP server configurations echo "Searching for ReadHeaderTimeout usage in HTTP server configurations:" rg --type go 'ReadHeaderTimeout\s*:'Length of output: 238
data/memory.go (1)
45-52: LGTM with minor suggestionsThe additions of
SetTTLandHasTTLmethods are good placeholders for future TTL support. The changes are minimal and don't affect existing functionality. Please consider the suggestions in the previous comments to improve error handling, consistency, and documentation.docs/pages/operation/directives.mdx (3)
10-10: LGTM: New directive added correctlyThe new directive "Use specific upstream(s)" has been added to the list correctly, maintaining consistency with the existing format and including a proper link to its section.
87-91: LGTM: Improved clarity for "Skip cache read" directiveThe updates to the "Skip cache read" section enhance the clarity of the documentation. The explanation of the default behavior and how to override it is well-written and consistent with other sections. The added context about when this directive is useful provides valuable information to the users.
Line range hint
1-142: Overall: Excellent improvements to the directives documentationThe changes made to this file significantly enhance the quality and completeness of the eRPC directives documentation. The addition of the new "Use specific upstream(s)" directive and the updates to existing directives provide users with more control and flexibility in their eRPC requests. The explanations are clear, and the examples are helpful.
While there are a few minor typographical and grammatical issues (as noted in previous comments), they do not detract from the overall quality and usefulness of the documentation. Addressing these minor issues will further polish the document.
Great job on improving this crucial part of the eRPC documentation!
🧰 Tools
🪛 LanguageTool
[style] ~111-~111: This phrase is redundant. Consider writing “usual”.
Context: ...nse will still be subject to caching as per usual. ## Use specific upstream(s) When sen...(PER_USUAL)
[uncategorized] ~115-~115: Possible missing comma found.
Context: ...c upstream(s) When sending requests to eRPC you can instruct to use only one specif...(AI_HYDRA_LEO_MISSING_COMMA)
[grammar] ~115-~115: Did you mean “using”? Or maybe you should add a pronoun? In active voice, ‘instruct’ + ‘to’ takes an object, usually a pronoun.
Context: ...nding requests to eRPC you can instruct to use only one specific upstream (or multiple...(ALLOW_TO)
[typographical] ~125-~125: After the expression ‘for example’ a comma is usually used.
Context: ...s starting with "priv-" For example if you want to make sure that request i...(COMMA_FOR_EXAMPLE)
upstream/pimlico_http_json_rpc_client.go (1)
136-136: Approved: Increased random ID range improves concurrent request handlingThe change from
rand.Intn(1_000_000)torand.Intn(100_000_000)is a good improvement. It reduces the likelihood of ID collisions in concurrent requests, which aligns with the PR objective of enhancing performance during high traffic scenarios.The
// #nosec G404comment is appropriate here, as the random ID doesn't need to be cryptographically secure for this use case.erpc/evm_json_rpc_cache_test.go (5)
18-18: Improved function naming enhances code clarityThe renaming of
createFixturestocreateCacheTestFixturesis a positive change. It more accurately describes the function's purpose, which is to create test fixtures specifically for cache tests. This improvement in naming convention enhances code readability and maintainability.
99-99: Consistent TTL check addition maintains test integrityThe addition of the
HasTTLmock expectation in this test case is consistent with the previous one. This change ensures that TTL behavior is uniformly tested across different caching scenarios, including when the block number is used as part of the primary key. This consistency in test structure is commendable and helps maintain the overall integrity of the test suite.
172-172: Consistent TTL check addition for finalized block cachingThe addition of the
HasTTLmock expectation in this test case maintains consistency with previous cases. This change ensures that TTL behavior is uniformly tested for scenarios involving finalized blocks. The consistency in applying this check across different test cases is commendable and contributes to a more robust test suite.
257-257: Relevant TTL check addition for synced node and finalized block scenarioThe inclusion of the
HasTTLmock expectation is appropriate and relevant in this test case. Unlike some previous cases where caching was skipped, this scenario expects caching to occur. The TTL check ensures that the caching behavior is correctly tested when the node is synced and the block is finalized, even when no TTL is set. This addition contributes to the comprehensiveness of the test suite.
275-275: Relevant TTL check addition for cached response retrievalThe inclusion of the
HasTTLmock expectation is appropriate and relevant in this test case. It ensures that the cache retrieval behavior is correctly tested for finalized blocks, even when no TTL is set. This addition contributes to the comprehensiveness of the test suite by verifying that the absence of a TTL does not affect the retrieval of cached responses for finalized blocks.data/dynamodb.go (1)
264-271: LGTM: TTL method additions are well-integratedThe addition of
SetTTLandHasTTLmethods to theDynamoDBConnectorstruct is well-integrated into the existing codebase. These methods:
- Implement the required interface methods for TTL functionality.
- Are placed appropriately within the file structure.
- Don't introduce any breaking changes or side effects to existing functionality.
- Allow for future expansion of TTL support without affecting current operations.
The placeholder implementations provide a clear path for future development of TTL functionality when needed.
common/request.go (2)
31-35: Addition ofUseUpstreamfield enhances request routingThe inclusion of the
UseUpstreamfield in theRequestDirectivesstruct provides more granular control over request forwarding, allowing the proxy to target specific upstreams with wildcard support.
165-191: MethodApplyDirectivesFromHttpcorrectly integrates query parametersThe updated
ApplyDirectivesFromHttpmethod now acceptsqueryArgsand appropriately sets directives based on both headers and query parameters. This enhances flexibility in directive configuration.erpc/evm_json_rpc_cache.go (5)
38-43: Setting TTL for Method Overrides CorrectlyThe addition of the loop to set TTL for each method in
cfg.Methodsensures that the cache respects the TTL configurations specified for different JSON-RPC methods. This is a proper implementation that enhances the flexibility and performance of the caching mechanism.
66-67: IncorporatinghasTTLinto Caching LogicInitializing
hasTTLby checkingc.conn.HasTTL(rpcReq.Method)prepares the caching logic to consider method-specific TTLs. This is a necessary step to determine whether to cache responses for methods without block references or numbers.
74-74: Updating Caching Conditions to Consider TTLBy modifying the condition to include
!hasTTL, the caching logic now correctly allows methods with a TTL to be cached even ifblockRefandblockNumberare not present. This ensures that methods intended to be cached based on TTLs are handled appropriately.
141-141: InitializinghasTTLin theSetMethodRetrieving
hasTTLwithin theSetmethod aligns the caching behavior with the logic in theGetmethod. It ensures consistency in determining whether to cache responses based on the presence of TTLs for specific methods.
143-163: Refining Caching Logic Based on TTL PresenceThe updated logic properly skips block reference and finality checks when
hasTTListrue. This allows responses for methods with specified TTLs to be cached without depending on block information, enhancing caching for methods likeeth_callthat may not involve blocks.upstream/evm_state_poller.go (1)
185-185: LGTM!The log statement correctly informs when the node is marked as fully synced.
upstream/registry.go (1)
294-297: Appropriate error handling for score refresh failuresThe added error handling correctly logs a warning if
RefreshUpstreamNetworkMethodScoresreturns an error. This improves observability and aids in diagnosing issues during the score refresh process.erpc/http_server.go (1)
271-275:⚠️ Potential issueSet Content-Type header in error response for batch requests
When encoding the batch
responsesfails, the error response does not include theContent-Typeheader, which may cause clients to misinterpret the response.Add the
Content-Typeheader before sending the error response.Apply this diff:
if err != nil { fastCtx.SetStatusCode(fasthttp.StatusInternalServerError) + fastCtx.Response.Header.Set("Content-Type", "application/json") fastCtx.SetBodyString(fmt.Sprintf(`{"jsonrpc":"2.0","error":{"code":-32603,"message":"%s"}}`, err.Error())) return }Likely invalid or redundant comment.
common/config.go (4)
50-50: Addition of 'Methods' field in 'ConnectorConfig'The addition of the
Methodsfield inConnectorConfigallows for method-specific caching configurations, enhancing flexibility.
216-220: Update counts from 'int' to 'uint' in 'CircuitBreakerPolicyConfig'Changing
FailureThresholdCount,FailureThresholdCapacity,SuccessThresholdCount, andSuccessThresholdCapacityfrominttouintis appropriate, as these values should not be negative.
243-243: Change 'MaxCount' to 'uint' in 'RateLimitRuleConfig'Updating
MaxCounttouintmakes sense since negative counts are invalid.
370-370: Update logging method to match 'uint' type for 'MaxCount'The change from
InttoUintin the logging statement forMaxCountcorrectly reflects its updated type.upstream/upstream.go (4)
82-85: Proper error handling forguessUpstreamType()The added error handling correctly captures and handles errors from the
guessUpstreamType()function.
462-462: Passingu.Loggeras a pointerThe code correctly passes a pointer to
u.LoggerintoNewRateLimitAutoTuner, ensuring that the logger is referenced properly within the auto-tuner.
623-628: Added upstream syncing checkThe code appropriately adds a check to skip requests if the upstream is syncing, preventing requests from being processed when the upstream is not ready.
634-640: Added directive check forUseUpstreamThe code correctly handles request directives by checking if the upstream is allowed to handle the request based on the
UseUpstreamdirective. This ensures that requests are routed to the appropriate upstream.upstream/http_json_rpc_client.go (3)
262-267: Properly closing response body on errorGood job ensuring that
resp.Bodyis closed when an error occurs andrespis not nil. This helps prevent resource leaks.
300-300: Adding defer to close response bodyUsing
defer resp.Body.Close()ensures that the response body is properly closed after processing, which is important for resource management.
431-431: Reading response body correctlyProperly reading the response body using
io.ReadAll(resp.Body)ensures that the entire response is read for further processing.upstream/ratelimiter_autotuner.go (3)
12-12: Initialize theloggerfield inRateLimitAutoTunerstruct.You've added the
loggerfield to theRateLimitAutoTunerstruct, which is appropriate for logging purposes.
106-106: Ensurearl.loggeris initialized before logging.Attempting to use
arl.loggerwithout initialization will result in anilpointer dereference. This issue has been previously noted.
95-99: Verify the parameter type ofAdjustBudgetmethod.Ensure that the
newMaxCountvariable matches the expected type in theAdjustBudgetmethod. Passing auintwhen anintis expected can lead to compilation errors.Run the following script to confirm the parameter type of the
AdjustBudgetmethod:erpc/networks_test.go (10)
67-74: LGTM!The upstream configuration looks good with the correct type, ID, endpoint, and EVM chain ID specified.
103-110: Ensure error handling is in place.The code is correctly checking for errors after calling
upsReg.BootstrapandupsReg.PrepareUpstreamsForNetwork. This is a good practice to catch and handle any potential issues.
158-165: The upstream configuration looks good.The EVM upstream is correctly configured with the expected type, ID, endpoint, and chain ID.
181-184: The network configuration looks good.The network is correctly configured with the EVM architecture and the expected chain ID.
194-201: Ensure error handling is in place.The code is correctly checking for errors after calling
upsReg.BootstrapandupsReg.PrepareUpstreamsForNetwork. This is a good practice to catch and handle any potential issues.
1930-1930: Ensure the directives are being applied correctly.The code is calling
ApplyDirectivesFromHttpon the fake request to apply directives from the HTTP headers and arguments. This is the correct way to set directives on a request.
2151-2151: Ensure the directives are being applied correctly.The code is calling
ApplyDirectivesFromHttpon the fake request to apply directives from the HTTP headers and arguments. This is the correct way to set directives on a request.
2289-2289: Ensure the directives are being applied correctly.The code is calling
ApplyDirectivesFromHttpon the fake request to apply directives from the HTTP headers and arguments. This is the correct way to set directives on a request.
169-171: Verify that the upstream configuration is being used correctly.Ensure that the
up1upstream configuration is being properly utilized by the upstreams registry and network.Run the following script to confirm:
#!/bin/bash # Description: Verify that the `up1` upstream config is used by the registry and network. # Test: Search for the upstream ID in the upstreams registry. Expect: At least one match. rg --type go 'NewUpstreamsRegistry' -A 10 | rg 'up1' # Test: Search for the upstream ID in the network creation. Expect: At least one match. rg --type go 'NewNetwork' -A 20 | rg 'up1'
78-80: Verify that the upstream configuration is being used correctly.Ensure that the
up1upstream configuration is being properly utilized by the upstreams registry and network.Run the following script to confirm:
data/connector.go
Outdated
| SetTTL(method string, ttlStr string) error | ||
| HasTTL(method string) bool |
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
Missing implementations of SetTTL and HasTTL methods in Connector implementations
The verification indicates that none of the Connector implementations include the newly added SetTTL and HasTTL methods. This could lead to issues as the interface now requires these methods.
🔗 Analysis chain
New TTL methods added to Connector interface
The addition of SetTTL and HasTTL methods enhances the Connector interface with TTL management capabilities. This aligns with the PR objective of incorporating changes from v0.0.24 to improve performance during high traffic scenarios.
However, there are a few points to consider:
- These new methods lack documentation, which may affect their usability.
- Existing implementations of the
Connectorinterface will need to be updated to include these new methods.
Consider adding documentation for these new methods to improve clarity:
// SetTTL sets the Time To Live (TTL) for a specific method.
// method: The name of the method to set TTL for.
// ttlStr: A string representation of the TTL duration.
// Returns an error if setting the TTL fails.
SetTTL(method string, ttlStr string) error
// HasTTL checks if a specific method has a TTL set.
// method: The name of the method to check.
// Returns true if the method has a TTL set, false otherwise.
HasTTL(method string) boolTo ensure all implementations of the Connector interface are updated, run the following script:
[/run_scripts]
[/run_scripts]
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if all Connector implementations include the new TTL methods
# Test: Search for struct types that implement Connector interface
echo "Connector implementations:"
rg --type go -n 'type \w+ struct' | rg 'Connector'
# Test: Check if these implementations include SetTTL and HasTTL methods
echo "\nImplementations with new TTL methods:"
rg --type go -n 'func \(\w+ \*?\w+\) (SetTTL|HasTTL)'
Length of output: 485
Script:
#!/bin/bash
# Description: Verify that all Connector implementations include SetTTL and HasTTL methods using ast-grep
# Find all implementations of SetTTL method
echo "Searching for SetTTL method implementations:"
ast-grep --lang go --pattern 'func $_Receiver SetTTL($_, $_) error { $$$ }' --output path
# Find all implementations of HasTTL method
echo -e "\nSearching for HasTTL method implementations:"
ast-grep --lang go --pattern 'func $_Receiver HasTTL($_) bool { $$$ }' --output path
Length of output: 782
Script:
#!/bin/bash
# Description: Verify that all Connector implementations include SetTTL and HasTTL methods using ast-grep
# Find all implementations of SetTTL method
echo "Searching for SetTTL method implementations:"
ast-grep --lang go --pattern 'func $_Receiver SetTTL($_, $_) error { $$$ }'
# Find all implementations of HasTTL method
echo -e "\nSearching for HasTTL method implementations:"
ast-grep --lang go --pattern 'func $_Receiver HasTTL($_) bool { $$$ }'
Length of output: 354
Dockerfile
Outdated
| COPY --from=builder /app/erpc-server /usr/bin/erpc-server | ||
| COPY --from=builder /app/erpc-server-pprof /usr/bin/erpc-server-pprof | ||
|
|
||
| RUN chmod 777 /usr/bin/erpc-server* |
There was a problem hiding this comment.
Consider using more restrictive permissions
Setting permissions to 777 (read, write, execute for all) is generally considered a security risk. While this might not be as critical in a container environment, it's still not a best practice.
Consider using more restrictive permissions. For example:
-RUN chmod 777 /usr/bin/erpc-server*
+RUN chmod 755 /usr/bin/erpc-server*This change would give read and execute permissions to all users, but only write permissions to the owner, which is typically sufficient for running the application.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| RUN chmod 777 /usr/bin/erpc-server* | |
| RUN chmod 755 /usr/bin/erpc-server* |
| @@ -59,7 +59,7 @@ func (c *ThirdwebHttpJsonRpcClient) SupportsNetwork(networkId string) (bool, err | |||
| ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) | |||
| defer cancel() | |||
|
|
|||
| rid := rand.Intn(1000000) | |||
| rid := rand.Intn(100_000_000) // #nosec G404 | |||
There was a problem hiding this comment.
💡 Codebase verification
Enhance Security by Using Cryptographically Secure Random Number Generators
Multiple instances of rand.Intn() have been identified across the codebase, all accompanied by // #nosec G404 comments. While increasing the range of random numbers reduces collision probability, using math/rand is not cryptographically secure and can lead to predictability.
Issues Identified:
- Files Affected:
upstream/pimlico_http_json_rpc_client.goupstream/evm_state_poller.goupstream/envio_http_json_rpc_client.goerpc/erpc_test.goerpc/http_server_test.gocommon/request.gocmd/erpc/main_test.go
Recommendations:
-
Replace
math/randwithcrypto/rand:import ( "crypto/rand" "encoding/binary" "fmt" ) // ... randomBytes := make([]byte, 4) _, err := rand.Read(randomBytes) if err != nil { return false, fmt.Errorf("failed to generate random ID: %w", err) } rid := int(binary.BigEndian.Uint32(randomBytes))
-
Remove
// #nosec G404Comments:
After implementing a secure random number generator, the suppression comments should be removed to allow security linters to validate the changes properly. -
Review All Instances:
Ensure that each instance ofrand.Intn()serves a purpose that requires cryptographic security. For non-critical randomness, ifmath/randis still appropriate, consider removing the// #noseccomments after a thorough assessment.
Benefits:
- Enhances the unpredictability of random IDs, reducing the risk of exploitation.
- Aligns with best security practices by utilizing cryptographically secure random number generators.
🔗 Analysis chain
Improve random ID generation for better security
The change from rand.Intn(1000000) to rand.Intn(100_000_000) reduces the likelihood of ID collisions in high-traffic scenarios, which is a positive improvement. However, there are some security concerns to address:
- The use of
math/randis not cryptographically secure and can be predictable. - The
// #nosec G404comment suppresses a security warning without addressing the underlying issue.
Consider implementing the following improvements:
- Replace
math/randwithcrypto/randfor better security:
import "crypto/rand"
// ...
randomBytes := make([]byte, 4)
_, err := rand.Read(randomBytes)
if err != nil {
return false, fmt.Errorf("failed to generate random ID: %w", err)
}
rid := int(binary.BigEndian.Uint32(randomBytes))- Remove the
// #nosec G404comment after implementing a secure random number generation method.
These changes will enhance the security of your random ID generation without compromising on uniqueness.
To ensure this change doesn't affect other parts of the codebase, run the following command:
This will help identify any other instances of rand.Intn() that might need similar security improvements.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
rg --type go 'rand\.Intn\(' -g '!upstream/thirdweb_http_json_rpc_client.go'
Length of output: 919
data/memory.go
Outdated
| func (m *MemoryConnector) SetTTL(_ string, _ string) error { | ||
| m.logger.Debug().Msgf("Method TTLs not implemented for MemoryConnector") | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Consider returning an error instead of nil
While the method correctly logs that TTLs are not implemented, returning nil might mislead callers into thinking the operation was successful. Consider returning an error to clearly indicate that the operation is not supported.
Here's a suggested implementation:
func (m *MemoryConnector) SetTTL(_ string, _ string) error {
m.logger.Debug().Msgf("Method TTLs not implemented for MemoryConnector")
- return nil
+ return fmt.Errorf("SetTTL not implemented for MemoryConnector")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (m *MemoryConnector) SetTTL(_ string, _ string) error { | |
| m.logger.Debug().Msgf("Method TTLs not implemented for MemoryConnector") | |
| return nil | |
| } | |
| func (m *MemoryConnector) SetTTL(_ string, _ string) error { | |
| m.logger.Debug().Msgf("Method TTLs not implemented for MemoryConnector") | |
| return fmt.Errorf("SetTTL not implemented for MemoryConnector") | |
| } |
data/postgresql.go
Outdated
| func (p *PostgreSQLConnector) SetTTL(_ string, _ string) error { | ||
| p.logger.Debug().Msgf("Method TTLs not implemented for PostgresSQLConnector") | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Improve SetTTL method implementation and error handling
The current implementation of SetTTL is a placeholder that doesn't actually set any TTL. Consider the following improvements:
- Change the log level from debug to warn to make it more visible that TTL is not supported.
- Return an error instead of nil to indicate that the operation is not supported.
- Use the provided parameters in the log message for better debugging.
Here's a suggested implementation:
func (p *PostgreSQLConnector) SetTTL(_ string, _ string) error {
- p.logger.Debug().Msgf("Method TTLs not implemented for PostgresSQLConnector")
- return nil
+ p.logger.Warn().Msgf("SetTTL called with (%s, %s) but TTLs are not implemented for PostgreSQLConnector", _, _)
+ return fmt.Errorf("SetTTL not implemented for PostgreSQLConnector")
}This change will make it clear to users that TTL functionality is not available and prevent silent failures.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (p *PostgreSQLConnector) SetTTL(_ string, _ string) error { | |
| p.logger.Debug().Msgf("Method TTLs not implemented for PostgresSQLConnector") | |
| return nil | |
| } | |
| func (p *PostgreSQLConnector) SetTTL(key string, value string) error { | |
| p.logger.Warn().Msgf("SetTTL called with (%s, %s) but TTLs are not implemented for PostgreSQLConnector", key, value) | |
| return fmt.Errorf("SetTTL not implemented for PostgreSQLConnector") | |
| } |
common/errors.go
Outdated
| type ErrUpstreamNotAllowed struct{ BaseError } | ||
|
|
||
| var NewErrUpstreamNotAllowed = func(upstreamId string) error { | ||
| return &ErrUpstreamNotAllowed{ | ||
| BaseError{ | ||
| Code: "ErrUpstreamNotAllowed", | ||
| Message: "upstream not allowed based on use-upstream directive", | ||
| Details: map[string]interface{}{ | ||
| "upstreamId": upstreamId, | ||
| }, | ||
| }, | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Define a constant for the error code and add an ErrorStatusCode method in ErrUpstreamNotAllowed.
To maintain consistency, define a constant ErrCodeUpstreamNotAllowed and use it in the Code field. Additionally, add an ErrorStatusCode method to specify the appropriate HTTP status code.
Apply this diff to implement the changes:
+const ErrCodeUpstreamNotAllowed ErrorCode = "ErrUpstreamNotAllowed"
type ErrUpstreamNotAllowed struct{ BaseError }
var NewErrUpstreamNotAllowed = func(upstreamId string) error {
return &ErrUpstreamNotAllowed{
BaseError{
- Code: "ErrUpstreamNotAllowed",
+ Code: ErrCodeUpstreamNotAllowed,
Message: "upstream not allowed based on use-upstream directive",
Details: map[string]interface{}{
"upstreamId": upstreamId,
},
},
}
}
+func (e *ErrUpstreamNotAllowed) ErrorStatusCode() int {
+ return http.StatusForbidden
+}Committable suggestion was skipped due to low confidence.
common/errors.go
Outdated
| type ErrNotImplemented struct{ BaseError } | ||
|
|
||
| var NewErrNotImplemented = func(msg string) error { | ||
| return &ErrNotImplemented{ | ||
| BaseError{ | ||
| Code: "ErrNotImplemented", | ||
| Message: msg, | ||
| }, | ||
| } | ||
| } | ||
|
|
||
| func (e *ErrNotImplemented) ErrorStatusCode() int { | ||
| return http.StatusNotImplemented | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Define a constant for the error code in ErrNotImplemented.
To maintain consistency with other error types, define a constant ErrCodeNotImplemented and use it in the Code field of ErrNotImplemented. This standardizes error codes throughout the codebase.
Apply this diff to implement the change:
+const ErrCodeNotImplemented ErrorCode = "ErrNotImplemented"
type ErrNotImplemented struct{ BaseError }
var NewErrNotImplemented = func(msg string) error {
return &ErrNotImplemented{
BaseError{
- Code: "ErrNotImplemented",
+ Code: ErrCodeNotImplemented,
Message: msg,
},
}
}
func (e *ErrNotImplemented) ErrorStatusCode() int {
return http.StatusNotImplemented
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| type ErrNotImplemented struct{ BaseError } | |
| var NewErrNotImplemented = func(msg string) error { | |
| return &ErrNotImplemented{ | |
| BaseError{ | |
| Code: "ErrNotImplemented", | |
| Message: msg, | |
| }, | |
| } | |
| } | |
| func (e *ErrNotImplemented) ErrorStatusCode() int { | |
| return http.StatusNotImplemented | |
| } | |
| const ErrCodeNotImplemented ErrorCode = "ErrNotImplemented" | |
| type ErrNotImplemented struct{ BaseError } | |
| var NewErrNotImplemented = func(msg string) error { | |
| return &ErrNotImplemented{ | |
| BaseError{ | |
| Code: ErrCodeNotImplemented, | |
| Message: msg, | |
| }, | |
| } | |
| } | |
| func (e *ErrNotImplemented) ErrorStatusCode() int { | |
| return http.StatusNotImplemented | |
| } |
common/errors.go
Outdated
| type ErrUpstreamSyncing struct{ BaseError } | ||
|
|
||
| const ErrCodeUpstreamSyncing ErrorCode = "ErrUpstreamSyncing" | ||
|
|
||
| var NewErrUpstreamSyncing = func(upstreamId string) error { | ||
| return &ErrUpstreamSyncing{ | ||
| BaseError{ | ||
| Code: ErrCodeUpstreamSyncing, | ||
| Message: "upstream is syncing", | ||
| Details: map[string]interface{}{ | ||
| "upstreamId": upstreamId, | ||
| }, | ||
| }, | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add an ErrorStatusCode method to ErrUpstreamSyncing.
To ensure appropriate HTTP status codes are returned, add an ErrorStatusCode method for ErrUpstreamSyncing. This aligns with error handling practices used in other error types.
Apply this diff to implement the change:
type ErrUpstreamSyncing struct{ BaseError }
const ErrCodeUpstreamSyncing ErrorCode = "ErrUpstreamSyncing"
var NewErrUpstreamSyncing = func(upstreamId string) error {
return &ErrUpstreamSyncing{
BaseError{
Code: ErrCodeUpstreamSyncing,
Message: "upstream is syncing",
Details: map[string]interface{}{
"upstreamId": upstreamId,
},
},
}
}
+func (e *ErrUpstreamSyncing) ErrorStatusCode() int {
+ return http.StatusServiceUnavailable
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| type ErrUpstreamSyncing struct{ BaseError } | |
| const ErrCodeUpstreamSyncing ErrorCode = "ErrUpstreamSyncing" | |
| var NewErrUpstreamSyncing = func(upstreamId string) error { | |
| return &ErrUpstreamSyncing{ | |
| BaseError{ | |
| Code: ErrCodeUpstreamSyncing, | |
| Message: "upstream is syncing", | |
| Details: map[string]interface{}{ | |
| "upstreamId": upstreamId, | |
| }, | |
| }, | |
| } | |
| } | |
| type ErrUpstreamSyncing struct{ BaseError } | |
| const ErrCodeUpstreamSyncing ErrorCode = "ErrUpstreamSyncing" | |
| var NewErrUpstreamSyncing = func(upstreamId string) error { | |
| return &ErrUpstreamSyncing{ | |
| BaseError{ | |
| Code: ErrCodeUpstreamSyncing, | |
| Message: "upstream is syncing", | |
| Details: map[string]interface{}{ | |
| "upstreamId": upstreamId, | |
| }, | |
| }, | |
| } | |
| } | |
| func (e *ErrUpstreamSyncing) ErrorStatusCode() int { | |
| return http.StatusServiceUnavailable | |
| } |
upstream/ratelimiter_autotuner.go
Outdated
| var newMaxCount uint | ||
| if errorRate > arl.errorRateThreshold { | ||
| newMaxCount = int(math.Ceil(float64(currentMax) * arl.decreaseFactor)) | ||
| newMaxCount = uint(math.Ceil(float64(currentMax) * arl.decreaseFactor)) | ||
| } else if errorRate == 0 { | ||
| newMaxCount = int(math.Ceil(float64(currentMax) * arl.increaseFactor)) | ||
| newMaxCount = uint(math.Ceil(float64(currentMax) * arl.increaseFactor)) |
There was a problem hiding this comment.
Consider using int instead of uint for newMaxCount.
Changing newMaxCount to uint may introduce type inconsistencies, especially if other count variables or method parameters use int. In Go, it's conventional to use int unless there is a specific reason to use unsigned integers.
Consider keeping newMaxCount as an int to maintain type consistency and avoid potential type conversion issues.
upstream/ratelimiter_autotuner.go
Outdated
| @@ -26,6 +29,7 @@ | |||
| } | |||
|
|
|||
| func NewRateLimitAutoTuner( | |||
| logger *zerolog.Logger, | |||
There was a problem hiding this comment.
Assign logger in the NewRateLimitAutoTuner constructor.
While you've added logger as a parameter to the constructor, it's not being assigned to the logger field of the RateLimitAutoTuner struct. This will result in arl.logger being nil, leading to a potential nil pointer dereference when logging.
Apply this diff to initialize the logger field:
func NewRateLimitAutoTuner(
logger *zerolog.Logger,
budget *RateLimiterBudget,
adjustmentPeriod time.Duration,
errorRateThreshold,
increaseFactor,
decreaseFactor float64,
minBudget,
maxBudget int,
) *RateLimitAutoTuner {
return &RateLimitAutoTuner{
+ logger: logger,
budget: budget,
errorCounts: make(map[string]*ErrorCounter),
lastAdjustments: make(map[string]time.Time),
adjustmentPeriod: adjustmentPeriod,
errorRateThreshold: errorRateThreshold,
increaseFactor: increaseFactor,
decreaseFactor: decreaseFactor,
minBudget: minBudget,
maxBudget: maxBudget,
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| logger *zerolog.Logger, | |
| func NewRateLimitAutoTuner( | |
| logger *zerolog.Logger, | |
| budget *RateLimiterBudget, | |
| adjustmentPeriod time.Duration, | |
| errorRateThreshold, | |
| increaseFactor, | |
| decreaseFactor float64, | |
| minBudget, | |
| maxBudget int, | |
| ) *RateLimitAutoTuner { | |
| return &RateLimitAutoTuner{ | |
| logger: logger, | |
| budget: budget, | |
| errorCounts: make(map[string]*ErrorCounter), | |
| lastAdjustments: make(map[string]time.Time), | |
| adjustmentPeriod: adjustmentPeriod, | |
| errorRateThreshold: errorRateThreshold, | |
| increaseFactor: increaseFactor, | |
| decreaseFactor: decreaseFactor, | |
| minBudget: minBudget, | |
| maxBudget: maxBudget, | |
| } | |
| } |
OBlackmon3
force-pushed
the
oblackmon3/rebase-polymer-develop-to-match-v0.0.24
branch
from
4538b59 to
4a84a1d
Compare
OBlackmon3
force-pushed
the
oblackmon3/rebase-polymer-develop-to-match-v0.0.24
branch
from
4a84a1d to
9d06958
Compare
What?
This PR rebases main onto polymer-develop so that the changes in v0.0.24 are included.
Why?
According to the developer of eRPC, these changes should help with high traffic scenarios.
How?
Just performed a rebase of main onto polymer-develop
Testing?
N/A
Summary by CodeRabbit
New Features
Bug Fixes
Chores