Rebase polymer develop with latest changes #1
Conversation
… is properly implemented
WalkthroughThe changes across the codebase primarily involve enhancements to error handling, type safety, and logging capabilities. New features include the integration of a Gosec Security Scanner into the CI/CD pipeline, modifications to several configuration structures to enforce non-negative values, and the introduction of new error types for improved clarity. Additionally, documentation has been updated to reflect new request directives, while various functions have been renamed or refactored for better organization and readability. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant HTTP_Server as Server
participant Upstream
participant Logger
User->>Server: Send Request
Server->>Upstream: Forward Request
Upstream->>Logger: Log Request Details
Upstream-->>Server: Response
Server-->>User: Send Response
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Files skipped from review due to trivial changes (1)
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: 0
Outside diff range and nitpick comments (5)
docs/pages/operation/directives.mdx (3)
21-23: LGTM, but consider adding a comma.The changes to the "Retry empty responses" directive are approved. The updated documentation clarifies that users can disable the retry behavior for empty responses using either a header or a query parameter, enhancing the flexibility of the API.
Consider adding a comma after "For example" in the following sentence to improve 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 reecent transactions and prefer to immeditely get an empty response and handle it on your client side:
58-60: LGTM, but consider adding a comma.The changes to the "Retry pending transactions" directive are approved. The updated documentation clarifies that users can disable the retry behavior for pending transactions using either a header or a query parameter, enhancing the flexibility of the API.
Consider adding a comma after "For example" in the following sentence to improve 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 immeditely get the pending tx data:
87-91: LGTM, but consider rephrasing and adding a comma.The changes to the "Skip cache read" directive are approved. The updated documentation clarifies that users can instruct eRPC to skip reading responses from cache using either a header or a query parameter, enhancing the usability of the API.
Consider rephrasing the following sentence to improve clarity:
-To instruct eRPC to skip 'reading' responses from cache, and make actual calls to upstreams. This directive is "false" by default, which means cache will be used. +To instruct eRPC to skip reading responses from cache and make actual calls to upstreams, set this directive to "true". By default, this directive is set to "false", which means the cache will be used.Also, consider adding a comma after "For example" in the following sentence to improve readability:
-For example if you want to make sure that request is sent to a specific upstream: +For example, if you want to make sure that request is sent to a specific upstream:upstream/pimlico_http_json_rpc_client.go (1)
136-136: LGTM, but consider using a cryptographically secure random number generator if needed.The changes are approved for the following reasons:
- Increasing the range of the random integer enhances the uniqueness of the generated request ID, which is important for distinguishing between multiple concurrent requests in a JSON-RPC context.
- The added comment indicates that suppressing the security warning related to the use of
rand.Intnis intentional.However, please note that
rand.Intnis not cryptographically secure. If the generated IDs need to be unpredictable and secure, consider using a cryptographically secure random number generator from thecrypto/randpackage instead.erpc/networks_test.go (1)
Line range hint
36-2289: Consider breaking down large subtests into smaller subtests.Some of the subtests in this test function are quite large and can be further broken down into smaller subtests for better readability and maintainability. For example, the
ForwardCorrectlyRateLimitedOnNetworkLevelandForwardNotRateLimitedOnNetworkLevelsubtests can be broken down based on the different rate limiting scenarios they test.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (24)
- .github/workflows/test.yml (1 hunks)
- common/config.go (3 hunks)
- common/errors.go (2 hunks)
- common/request.go (4 hunks)
- data/redis.go (1 hunks)
- docs/pages/operation/directives.mdx (7 hunks)
- erpc/evm_json_rpc_cache_test.go (15 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)
Files skipped from review due to trivial changes (4)
- data/redis.go
- erpc/evm_json_rpc_cache_test.go
- test/fake_server.go
- upstream/failsafe.go
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)
[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 not posted (69)
.github/workflows/test.yml (1)
19-22: LGTM!The addition of the Gosec Security Scanner is a great enhancement to the CI/CD pipeline. It will help identify potential security vulnerabilities in the codebase early in the development process.
The configuration to exclude the
testdirectory and skip tests during the scan is appropriate to focus the scan on the main codebase.The placement of the scanner step after the test execution step and before the coverage report generation step is suitable in the workflow.
The code changes are approved.
upstream/ratelimiter_budget.go (1)
39-39: LGTM! The change frominttouintfor thenewMaxCountparameter is a good improvement.Using
uintfor thenewMaxCountparameter has the following benefits:
- It ensures that only non-negative values can be passed to this parameter, which prevents unintended behavior and errors that could occur if negative values were allowed.
- It improves type safety by explicitly communicating that the
newMaxCountparameter should always be a non-negative value.- It aligns with the best practice of using unsigned integers for values that should never be negative, such as counts or sizes.
While this change may break existing code that passes negative integers to this method, it is a necessary breaking change to improve the robustness and correctness of the code.
upstream/ratelimiter_autotuner.go (5)
8-8: LGTM!The import statement is correctly added to support the new logger field.
12-12: LGTM!The logger field is correctly added to the struct to enable structured logging capabilities.
32-32: LGTM!The constructor is correctly modified to accept the logger parameter, ensuring that instances of
RateLimitAutoTunercan log relevant events.
95-99: LGTM!The type change from
inttouintis correct as it enforces non-negative values fornewMaxCount, enhancing type safety.
104-107: LGTM!The error handling is correctly introduced to provide better visibility into potential issues during runtime.
upstream/ratelimiter_registry.go (1)
73-73: Verify the type ofMaxCountand ensure passing it directly is safe and intended.The change in the argument passed to
ratelimiter.BurstyBuilderallows for a broader range of values to be accepted forMaxCount, depending on its type.If
MaxCountis of a signed integer type (e.g.,int), this change will allow negative values to be passed, which could lead to unexpected behavior or panics in theratelimiter.BurstyBuilderfunction.Please verify the type of
MaxCountand ensure that passing it directly toratelimiter.BurstyBuilderis safe and intended.If
MaxCountcan be negative, consider adding validation to ensure only non-negative values are passed to prevent unexpected behavior or panics:if rule.MaxCount < 0 { return nil, common.NewErrRateLimitInvalidConfig(fmt.Errorf("invalid MaxCount value for limit %v: must be non-negative", rule)) }To confirm the type of
MaxCount, run:Verification successful
Verification successful: The change is safe and does not introduce any issues.
The
MaxCountfield in theRateLimitRuleConfigstruct is of typeuint, which is an unsigned integer type. Therefore, the change fromuint(rule.MaxCount)torule.MaxCountis safe and does not affect the behavior of the code. There is no risk of negative values being passed toratelimiter.BurstyBuilder.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the type of `MaxCount` in the `RateLimitRuleConfig` struct. # Test: Search for the `RateLimitRuleConfig` struct definition. # Expect: `MaxCount` to be of type `uint` or another unsigned integer type. ast-grep --lang go --pattern $'type RateLimitRuleConfig struct { $$$ MaxCount $_ $$$ }'Length of output: 452
erpc/init.go (1)
89-91: LGTM!The addition of
ReadHeaderTimeoutis a good practice to prevent potential denial-of-service scenarios caused by slow clients. The timeout value of 10 seconds seems reasonable for most use cases.upstream/thirdweb_http_json_rpc_client.go (1)
62-62: LGTM! The change enhances the randomness of the identifier.The change increases the range of the random integer used for generating the identifier (
rid) in the JSON-RPC request. This enhancement potentially reduces the likelihood of collisions in concurrent requests, improving the robustness of the identifier generation.The addition of the
// #nosec G404comment suppresses a security warning related to the use of therand.Intnfunction, indicating that the developer has acknowledged the security concern but deemed it acceptable in this context.Overall, this change improves the identifier generation without altering the method's overall functionality or control flow.
upstream/envio_http_json_rpc_client.go (1)
120-120: LGTM! The changes enhance the uniqueness of the request identifier.The increased range of
ridfromrand.Intn(1000000)torand.Intn(100_000_000)reduces the likelihood of collisions in concurrent environments.The
#nosec G404comment is used to suppress a security warning from the Gosec Security Scanner, which is likely configured in the CI/CD pipeline. Whilerand.Intnis not recommended for security-sensitive purposes due to its reliance on a pseudo-random number generator (PRNG), its use here as a request identifier is acceptable since it is not security-critical.docs/pages/operation/directives.mdx (1)
113-142: LGTM!The new "Use specific upstream(s)" directive is a valuable addition to the eRPC behavior directives. It provides users with greater control over which upstreams are utilized for their requests, including the ability to use wildcard characters for matching multiple upstreams. The documentation is clear, and the example demonstrates the usage effectively.
Tools
LanguageTool
[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)
common/request.go (4)
31-35: LGTM!The addition of the
UseUpstreamfield to theRequestDirectivesstruct is a valuable enhancement. It provides more granular control over upstream routing by allowing the specification of a single upstream or using a wildcard to target multiple upstreams. This flexibility will be beneficial for directing requests to specific upstreams as needed.
140-140: LGTM!The change to convert
idftoint64before formatting it as a string in theIdmethod is a good improvement. It ensures type consistency by always setting theuidfield to a string representation of an integer value whenidfis available. This explicit conversion prevents potential issues related to type mismatches.
165-190: LGTM!The changes to the
ApplyDirectivesFromHttpmethod significantly enhance its capability to configure request directives based on a wider range of inputs. By accepting both HTTP headers and query arguments, it allows for more flexible directive processing.The extraction of the
UseUpstreamdirective from both headers and query arguments ensures that the most relevant directive is applied. This improves the robustness of the directive application process.Additionally, updating the existing directives (
RetryEmpty,RetryPending, andSkipCacheRead) to consider values from query arguments further enhances the flexibility and reliability of directive processing.Overall, these changes strengthen the method's ability to handle directives effectively based on various input sources.
238-238: LGTM!The modification to the
IDfield initialization in theJsonRpcRequestmethod simplifies the code by directly usingrand.Intn(math.MaxInt32). This generates a random integer value within the range ofmath.MaxInt32, which is sufficient for the purpose of generating a unique identifier.The addition of the
#nosec G404comment is appropriate to suppress the security warning related to the use of themath/randpackage. It indicates that the random value generation is intentional and acceptable in this context.Overall, this change improves the readability and maintainability of the code while ensuring the generation of unique identifiers for JSON-RPC requests.
upstream/evm_state_poller.go (2)
Line range hint
185-189: LGTM!The change to the condition for setting the
Syncingstatus enhances the accuracy of the syncing status by ensuring that it reflects the actual syncing state of the node. This improves the reliability of the logging and status reporting.The new logic is correct and does not introduce any issues.
298-298: LGTM!Increasing the range for generating random IDs from
10,000,000to100,000,000is a simple and effective way to reduce the likelihood of ID collisions in requests. This enhances the robustness of the request handling mechanism without introducing any negative side effects.The change is straightforward and does not introduce any issues.
upstream/registry.go (2)
171-182: LGTM!The changes improve the robustness of the code by ensuring that all necessary map levels in
upstreamScoresare properly initialized before assignment. The repeated pattern of checking if the outer map entry exists, and if not, initializing it with a new nested map structure, handles both the specificnetworkIdand the wildcard"*"entries.The code changes are approved.
294-297: LGTM!The changes improve error handling in
scheduleScoreCalculationTimersby capturing and logging any errors returned by theRefreshUpstreamNetworkMethodScoresfunction. This provides valuable feedback that was absent in the previous implementation.The overall control flow remains intact, but the robustness of the code has been enhanced through these error checks.
The code changes are approved.
erpc/networks.go (2)
150-156: LGTM!The changes to retrieve and evaluate upstreams before proceeding with the request handling look good. The error handling logic is correct and the changes integrate well with the existing code.
Also applies to: 158-164
381-391: LGTM!The new
shouldHandleMethodcorrectly implements the logic to restrict the handling of specific methods based on the number of upstreams. The error message clearly communicates the reason for not supporting the methods in certain cases.erpc/http_server.go (12)
148-148: Variable renaming improves clarity.Renaming
argsCopytoqueryArgsCopyenhances code readability by clearly indicating that the variable holds a copy of the query arguments.
154-154: Function signature update maintains consistency.Updating the function signature in the goroutine to use the renamed
queryArgsCopyvariable ensures consistency with the variable renaming.
172-172: Code change is consistent with variable renaming.The
ApplyDirectivesFromHttpmethod call on thenqvariable, passing theheadersCopyandqueryArgsCopyvariables as arguments, is consistent with the variable renaming and appears to be correct.
177-177: Code change is consistent with variable renaming.The
NewPayloadFromHttpfunction call, passing theheadersCopyandqueryArgsCopyvariables as arguments, is consistent with the variable renaming and appears to be correct.
260-260: Code change is consistent with variable renaming.Updating the goroutine function call to pass the
queryArgsCopyvariable as an argument is consistent with the variable renaming and ensures that the correct variable is passed to the goroutine.
269-275: Added error handling improves robustness.The added error handling for the
encoder.Encodefunction call in the batch response scenario improves the robustness of the code. If an encoding error occurs, the server appropriately responds with a 500 Internal Server Error status code and a JSON error message.
280-285: Added error handling improves robustness.The added error handling for the
encoder.Encodefunction call in the single response scenario improves the robustness of the code. If an encoding error occurs, the server appropriately responds with a 500 Internal Server Error status code and an error message.
403-403: Updated logging statement provides more detailed information.The updated error logging statement, which uses the
Debuglog level and includes the request object, provides more detailed information for debugging purposes when a client-side exception occurs.
406-406: Updated logging statement provides more specific error information.The updated error logging statement, which includes the deepest error message when the error is a
StandardError, provides more specific error information. This can aid in identifying the root cause of the issue.
408-408: Updated logging statement ensures error message is logged.The updated error logging statement, which includes the error message when the error is not a
StandardError, ensures that the error message is logged even in such cases. This provides valuable information for debugging.
464-473: Added error handling and response header setting improve robustness.The added error handling for the
encoder.Encodefunction call in thehandleErrorResponsefunction improves the robustness of the code. If an encoding error occurs, the server appropriately responds with a 500 Internal Server Error status code and a JSON error message. Additionally, the response headers are set correctly based on the encoding result.
497-500: Added error handling and logging for listener closing.The added error handling ensures that the IPv4 listener is properly closed when an error occurs while starting the IPv6 listener. Logging the error using the
Errorlog level provides visibility into any issues that may occur during the listener closing process.common/config.go (6)
216-216: LGTM!Changing
FailureThresholdCountfrominttouintis a good move to ensure the field only accepts non-negative values, which aligns with the goal of enhancing robustness by preventing invalid negative configurations.
217-217: LGTM!Changing
FailureThresholdCapacityfrominttouintis a good move to ensure the field only accepts non-negative values, which aligns with the goal of enhancing robustness by preventing invalid negative configurations.
219-219: LGTM!Changing
SuccessThresholdCountfrominttouintis a good move to ensure the field only accepts non-negative values, which aligns with the goal of enhancing robustness by preventing invalid negative configurations.
220-220: LGTM!Changing
SuccessThresholdCapacityfrominttouintis a good move to ensure the field only accepts non-negative values, which aligns with the goal of enhancing robustness by preventing invalid negative configurations.
243-243: LGTM!Changing
MaxCountfrominttouintis a good move to ensure the field only accepts non-negative values, which aligns with the goal of enhancing robustness by preventing invalid negative configurations.
370-370: LGTM!Updating
MarshalZerologObjectto useUintinstead ofIntfor logging theMaxCountfield is consistent with the type change ofMaxCountfrominttouint.erpc/http_server_test.go (4)
Line range hint
22-227: LGTM!The
TestHttpServer_RaceTimeoutsfunction is well-structured and covers important timeout scenarios. The use ofgocklibrary for mocking the upstream responses is appropriate. The test cases are comprehensive and cover concurrent requests, rapid successive requests, and mixed timeout and non-timeout requests.
Line range hint
228-436: LGTM!The
TestHttpServer_SingleUpstreamfunction is well-structured and covers important scenarios related to a single upstream configuration. The use ofgocklibrary for mocking the upstream responses is appropriate. The test cases are comprehensive and cover various error scenarios.
438-505: LGTM!The
createServerTestFixturesfunction is well-structured and encapsulates the setup logic for creating a test HTTP server. It provides a convenient way to send requests to the test server using the returnedsendRequestfunction. The function also handles the teardown of the test server properly.
507-615: LGTM!The
TestHttpServer_MultipleUpstreamsfunction is well-structured and covers scenarios related to multiple upstream configurations. The use ofgocklibrary for mocking the upstream responses is appropriate. The test cases verify that the correct upstream is selected based on the provided directives in headers or query parameters.upstream/upstream.go (3)
82-85: LGTM!The changes to error handling in the
NewUpstreamfunction are approved. Assigning the result ofpup.guessUpstreamType()toerrand checking for nil before proceeding ensures that errors during upstream type guessing are properly handled and returned, preventing execution with an invalid state.
462-462: LGTM!The changes to pass the
u.Loggerreference to theNewRateLimitAutoTunerconstructor in theinitRateLimitAutoTunermethod are approved. This allows the rate limiter auto-tuner to have access to the logger, enabling better logging and debugging capabilities related to rate limiting.
623-640: LGTM!The changes to the
shouldSkipmethod are approved. The additional checks for theEvmconfiguration, specifically theSyncingfield and theUseUpstreamdirective, improve the method's robustness by adding more conditions under which requests can be skipped. This refines the control flow based on the upstream's state and configuration.upstream/http_json_rpc_client.go (2)
262-267: LGTM!The code changes improve error handling by ensuring the response body is properly closed if it is not nil when an error occurs during the HTTP request. This prevents potential resource leaks.
300-310: LGTM!The code changes optimize performance by making the logging of the response body conditional based on the logger's level. This avoids unnecessary string conversion and logging when the debug level is not set to
DebugLevel, improving efficiency in non-debug scenarios.common/errors.go (8)
462-462: LGTM!The
ErrNotImplementedstruct type is correctly defined and follows the existing pattern of defining custom error types.
464-471: LGTM!The
NewErrNotImplementedconstructor function is correctly implemented and follows the existing pattern of defining constructor functions for custom error types.
473-475: LGTM!The
ErrorStatusCodemethod forErrNotImplementedis correctly implemented and follows the existing pattern of definingErrorStatusCodemethods for custom error types.
930-930: LGTM!The
ErrUpstreamSyncingstruct type is correctly defined and follows the existing pattern of defining custom error types.
932-932: LGTM!The
ErrCodeUpstreamSyncingconstant is correctly defined and follows the existing pattern of defining error code constants.
934-944: LGTM!The
NewErrUpstreamSyncingconstructor function is correctly implemented and follows the existing pattern of defining constructor functions for custom error types.
946-946: LGTM!The
ErrUpstreamNotAllowedstruct type is correctly defined and follows the existing pattern of defining custom error types.
948-958: LGTM!The
NewErrUpstreamNotAllowedconstructor function is correctly implemented and follows the existing pattern of defining constructor functions for custom error types.erpc/networks_test.go (12)
33-33: LGTM!Setting the log level to
ErrorLevelin tests is a good practice to avoid cluttering the test output with unnecessary logs.
Line range hint
39-132: LGTM!The subtest correctly tests that requests are rate limited at the network level based on the configured rate limit budget. It sets up the rate limiter config, makes requests that exceed the rate limit, and verifies that the last request returns a
ErrNetworkRateLimitRuleExceedederror. The test is well-implemented.
Line range hint
134-217: LGTM!The subtest correctly tests that requests are not rate limited at the network level when the rate limit budget is not exceeded. It sets up the rate limiter config with a high limit, makes multiple requests, and verifies that the last request does not return a
ErrNetworkRateLimitRuleExceedederror. The test is well-implemented.
Line range hint
219-324: LGTM!The subtest correctly tests that requests are retried on intermittent failures from the upstream without success. It sets up the upstream mock to return 503 errors, makes a request, and verifies that the request returns a
ErrUpstreamsExhaustederror after exhausting all retries. The test is well-implemented.
Line range hint
326-430: LGTM!The subtest correctly tests that requests are retried on failures with error code from the upstream without success. It sets up the upstream mock to return 503 errors with JSON-RPC error code, makes a request, and verifies that the request returns a
ErrUpstreamsExhaustederror after exhausting all retries. The test is well-implemented.
Line range hint
432-582: LGTM!The subtest correctly tests that non-retryable failures from upstreams are skipped and the request is sent to the next upstream. It sets up one upstream mock to return a non-retryable 401 error and another upstream mock to return a 200 success response, makes a request, and verifies that the request returns a successful response from the second upstream after skipping the first upstream due to the non-retryable 401 error. The test is well-implemented.
Line range hint
584-733: LGTM!The subtest correctly tests that retryable failures from upstreams are not skipped and the request is retried on the same upstream. It sets up two upstream mocks to return retryable 503 errors initially and then return a 200 success response, makes a request, and verifies that the request returns a successful response after retrying on the upstreams due to the retryable 503 errors. The test is well-implemented.
Line range hint
735-1001: LGTM!The subtest correctly tests that requests are not retried when the requested block is finalized and the node is synced. It sets up the upstream mocks to return different responses for the latest and finalized block numbers and the actual request, makes a request for logs from a block range, and verifies that the request returns the response from the first upstream without retrying because the requested block range is finalized and the node is synced. The test is well-implemented.
Line range hint
1003-1268: LGTM!The subtest correctly tests that requests are retried when the node is not synced. It sets up the upstream mocks with one node not synced, makes a request for logs from a block range, and verifies that the request returns the response from the second upstream after retrying because the first node is not synced. The test is well-implemented.
Line range hint
1270-1535: LGTM!The subtest correctly tests that requests are retried when the node sync state is unknown. It sets up the upstream mocks with unknown sync state for both nodes, makes a request for logs from a block range, and verifies that the request returns the response from the second upstream after retrying because the sync state of both nodes is unknown. The test is well-implemented.
Line range hint
1537-1736: LGTM!The subtest correctly tests that requests are retried when the requested block is not finalized. It sets up the upstream mocks such that the requested block is not finalized, makes a request for logs from a block range, and verifies that the request returns the response from the second upstream after retrying because the requested block is not finalized. The test is well-implemented.
Line range hint
1738-1937: LGTM!The subtest correctly tests that requests are retried when block finalization information is not available. It sets up the upstream mocks to return 0 for both latest and finalized block numbers, makes
What?
This PR rebases the latest changes from the upstream erpc repo onto polymer-develop. In addition, it mocks the
HasTTL()method so that tests will pass.Why?
The latest changes in upstream erpc lead to significant performance improvements and reduces the likelihood of crashes.
How?
First I updated the main branch of this repository to match the main branch of upstream erpc. Now I am rebasing the main branch changes on to polymer-develop so that we can continue to develop polymer specific features with the erpc improvements.
Testing?
N/A
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores