Revamp reporter (2/2): handle json rpc down and server restart/crash #1616
Conversation
|
Warning Rate limit exceeded@Intizar-T has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 48 minutes and 44 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 aim to enhance the codebase's resilience and efficiency by simplifying ESLint configuration, updating error handling, refactoring nonce management, increasing concurrency settings, and modifying test cases. A new error code Changes
Sequence Diagram(s)sequenceDiagram
participant Reporter
participant State
participant Logger
participant Transaction
Reporter->>State: Retrieve localNonce
loop Transaction Trials
Reporter->>Transaction: Attempt to process transaction
Transaction-->>Reporter: Successful transaction
alt TxNonceExpired Error
Reporter->>State: Adjust localNonce
State-->>Reporter: New localNonce
end
end
Reporter->>Logger: Log transaction result
Assessment against linked issues
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 as PR comments)
Additionally, you can add CodeRabbit Configration 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: 2
Outside diff range and nitpick comments (1)
core/test/nonce-manager.test.ts (1)
Line range hint
19-19: Sensitive information like API keys should not be hardcoded in tests. Consider using environment variables or secure vaults.- const REPORTER = { - id: '1', - address: '0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266', - privateKey: '0xac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80', + const REPORTER = { + id: process.env.REPORTER_ID, + address: process.env.REPORTER_ADDRESS, + privateKey: process.env.REPORTER_PRIVATE_KEY, }
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- core/.eslintrc (2 hunks)
- core/src/reporter/state.ts (2 hunks)
- core/src/reporter/utils.ts (1 hunks)
- core/src/settings.ts (1 hunks)
- core/src/utils.ts (1 hunks)
- core/test/nonce-manager.test.ts (1 hunks)
Files not summarized due to errors (1)
- core/src/reporter/state.ts: Error: Server error. Please try again later.
Files skipped from review due to trivial changes (1)
- core/src/settings.ts
Additional context used
Biome
core/src/utils.ts
[error] 25-27: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 57-57: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 67-67: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 133-155: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 135-155: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
[error] 137-155: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
[error] 139-155: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
[error] 141-155: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
[error] 143-155: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
[error] 145-155: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
[error] 147-155: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
[error] 149-155: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
[error] 151-155: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
[error] 153-155: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
core/src/reporter/utils.ts
[error] 56-58: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 204-206: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
Gitleaks
core/test/nonce-manager.test.ts
19-19: Detected a Generic API Key, potentially exposing access to various services and sensitive operations. (generic-api-key)
Additional comments not posted (7)
core/.eslintrc (3)
11-11: Updated ESLint plugins look good and align with TypeScript usage.
24-24: Good addition of theno-constant-conditionrule. It enhances code safety by warning against potentially infinite loops or conditions that don't change.
26-26: Properly ignoring build output directories in ESLint is a best practice to avoid linting autogenerated files.core/src/utils.ts (1)
40-40: The declaration oferrMsgwith a more explicit type (string | null) is a good practice for clarity and type safety.core/test/nonce-manager.test.ts (1)
65-65: Skipping this test might be temporary but ensure it aligns with your testing strategy, especially if related functionality is critical.Consider re-enabling this test once the related functionality is stable or if the test is critical for the release.
core/src/reporter/utils.ts (1)
268-270: Thesleepfunction is a useful utility for asynchronous operations, especially in polling mechanisms. It's implemented correctly here.core/src/reporter/state.ts (1)
7-7: Imports for polling and retry settings are appropriately added and used within thegetAndIncrementNoncemethod.
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (2)
core/src/utils.ts (2)
Line range hint
133-155: Consider simplifying thegetOraklErrorCodefunction by removing unnecessaryelseclauses. This will make the code cleaner and easier to read.if (e.code == 'ERR_BAD_OPTION_VALUE') { return OraklErrorCode.AxiosBadOptionValue } if (e.code == 'ERR_BAD_OPTION') { return Orakl ... - } else { return defaultErrorCode + } + return defaultErrorCode
Line range hint
67-67: Adopt optional chaining here to simplify the code and enhance readability.- if (e && e.message) errMsg = e.message + if (e?.message) errMsg = e.message
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- core/src/utils.ts (1 hunks)
Additional context used
Biome
core/src/utils.ts
[error] 25-27: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 67-67: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 133-155: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 135-155: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
[error] 137-155: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
[error] 139-155: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
[error] 141-155: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
[error] 143-155: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
[error] 145-155: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
[error] 147-155: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
[error] 149-155: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
[error] 151-155: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
[error] 153-155: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Additional comments not posted (3)
core/src/utils.ts (3)
40-40: Ensure variableerrMsgis initialized correctly and used consistently.
57-59: This logic prevents spam by not sending duplicate error messages to Slack within a minute. Good implementation of throttling the error messages.
42-59: ThesendToSlackfunction has been updated to handle errors more robustly. Consider adopting the optional chaining syntax for cleaner code as suggested by the static analysis.- if (e?.message && errMsg === e.message) { + if (e.message && errMsg === e.message) {Likely invalid or redundant comment.
|
made minor suggestion 😉: |
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
core/src/reporter/state.ts (1)
Line range hint
297-341: Refactor thegetAndIncrementNoncemethod to include error handling and a retry mechanism.This method now includes a robust retry mechanism that handles JSON-RPC downtimes effectively. Consider adding logging or metrics to monitor the number of retries or the duration of downtimes, which could help in proactive maintenance or system tuning.
+ this.logger.info({ name: 'getAndIncrementNonce', file: FILE_NAME }, `Retry count: ${retryCount}`);Moreover, the use of
NONCE_MANAGER_SLACK_FREQUENCY_RETRIESfor controlling Slack notifications is a good strategy to avoid spamming, but ensure that this value is configurable and can be easily adjusted based on operational needs.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- core/src/reporter/state.ts (3 hunks)
Additional comments not posted (2)
core/src/reporter/state.ts (2)
7-9: Imports organized for new constants and functions.Imports from
settingsandutilsare correctly added to support the new functionality. This organization facilitates easier maintenance and scalability.Also applies to: 11-17
286-296: The methodgetRemoteNonceis well-implemented to handle both delegated and non-delegated fee cases.This method correctly differentiates between
CaverWalletandNonceManager, ensuring that the right method is called based on thedelegatedFeeflag. This is crucial for maintaining accurate nonce management across different wallet configurations.
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (1)
core/test/nonce-manager.test.ts (1)
[!TIP]
Codebase VerificationThe search results confirm the presence of a private key in the test file
core/test/nonce-manager.test.ts. This is a critical security concern as private keys should never be hardcoded in the source code, even in test files.
- Location:
core/test/nonce-manager.test.ts, line 62- Sensitive Data: Private key
0xac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80Consider removing the hardcoded private key and using environment variables or mock data instead.
Analysis chain
Line range hint
19-19: Verify potential API key exposure.If an actual API key exposure is confirmed, it's crucial to remove or secure it properly. Consider using environment variables or secure vault solutions for API keys.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if there's an actual API key exposure around line 19 in `nonce-manager.test.ts`. # Test: Search around line 19 for any sensitive data exposure. rg -C 3 --type python 'API_KEY|api_key|apiKey' core/test/nonce-manager.test.tsLength of output: 111
Script:
#!/bin/bash # Description: Verify if there's an actual API key exposure around line 19 in `nonce-manager.test.ts`. # Test: Search around line 19 for any sensitive data exposure. rg -C 3 --type typescript 'API_KEY|api_key|apiKey' core/test/nonce-manager.test.tsLength of output: 119
Script:
#!/bin/bash # Description: Verify if there's an actual API key exposure around line 19 in `nonce-manager.test.ts`. # Test: Search around line 19 for any sensitive data exposure. rg -C 3 'API_KEY|api_key|apiKey' core/test/nonce-manager.test.tsLength of output: 66
Script:
#!/bin/bash # Description: Verify if there's any private key or sensitive data exposure around lines 62-70 in `nonce-manager.test.ts`. # Test: Search for the presence of private keys or other sensitive data around the specified lines. rg -C 3 'privateKey|PRIVATE_KEY|PrivateKey' core/test/nonce-manager.test.tsLength of output: 322
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- core/src/reporter/state.ts (3 hunks)
- core/test/nonce-manager.test.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- core/src/reporter/state.ts
Additional context used
Gitleaks
core/test/nonce-manager.test.ts
19-19: Detected a Generic API Key, potentially exposing access to various services and sensitive operations. (generic-api-key)
Intizar-T
force-pushed
the
feat/reporter-edge-cases
branch
from
03d4bd2 to
a34d94c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (4)
core/src/reporter/reporter.ts (1)
Line range hint
31-84: The modifications to handleTxNonceExpirederrors by retrying transactions with an updated nonce are well-implemented. Consider adding more detailed logging at each retry to enhance traceability and debugging capabilities.+ logger.debug(`Nonce updated to ${localNonce} after TxNonceExpired error, retrying transaction.`);core/src/utils.ts (3)
40-40: Consider initializingerrMsgwith a more descriptive default value or ensure it's properly managed across its uses.
Line range hint
76-76: Use optional chaining to simplify the conditional check.- if (e && e.message) errMsg = e.message + if (e?.message) errMsg = e.message
Line range hint
142-164: Multipleelse ifbranches can be omitted for cleaner and more efficient code.- } else if (e.code == 'ERR_BAD_OPTION_VALUE') { - return OraklErrorCode.AxiosBadOptionValue - } else if (e.code == 'ERR_BAD_OPTION') { - return OraklErrorCode.AxiosBadOption - } else if (e.code == 'ECONNABORTED' || e.code == 'ETIMEDOUT') { - return OraklErrorCode.AxiosTimeOut - } else if (e.code == 'ERR_NETWORK') { - return OraklErrorCode.AxiosNetworkError - } else if (e.code == 'ERR_FR_TOO_MANY_REDIRECTS') { - return OraklErrorCode.AxiosTooManyRedirects - } else if (e.code == 'ERR_DEPRECATED') { - return OraklErrorCode.AxiosDeprecated - } else if (e.code == 'ERR_BAD_RESPONSE') { - return OraklErrorCode.AxiosBadResponse - } else if (e.code == 'ERR_BAD_REQUEST') { - return OraklErrorCode.AxiosBadRequest - } else if (e.code == 'ERR_CANCELED') { - return OraklErrorCode.AxiosCanceledByUser - } else if (e.code == 'ERR_NOT_SUPPORT') { - return OraklErrorCode.AxiosNotSupported - } else if (e.code == 'ERR_INVALID_URL') { - return OraklErrorCode.AxiosInvalidUrl - } else { + switch (e.code) { + case 'ERR_BAD_OPTION_VALUE': + return OraklErrorCode.AxiosBadOptionValue; + case 'ERR_BAD_OPTION': + return OraklErrorCode.AxiosBadOption; + case 'ECONNABORTED': + case 'ETIMEDOUT': + return OraklErrorCode.AxiosTimeOut; + case 'ERR_NETWORK': + return OraklErrorCode.AxiosNetworkError; + case 'ERR_FR_TOO_MANY_REDIRECTS': + return OraklErrorCode.AxiosTooManyRedirects; + case 'ERR_DEPRECATED': + return OraklErrorCode.AxiosDeprecated; + case 'ERR_BAD_RESPONSE': + return OraklErrorCode.AxiosBadResponse; + case 'ERR_BAD_REQUEST': + return OraklErrorCode.AxiosBadRequest; + case 'ERR_CANCELED': + return OraklErrorCode.AxiosCanceledByUser; + case 'ERR_NOT_SUPPORT': + return OraklErrorCode.AxiosNotSupported; + case 'ERR_INVALID_URL': + return OraklErrorCode.AxiosInvalidUrl; + default: + return defaultErrorCode; + }
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (9)
- core/.eslintrc (2 hunks)
- core/src/errors.ts (1 hunks)
- core/src/reporter/reporter.ts (3 hunks)
- core/src/reporter/state.ts (3 hunks)
- core/src/reporter/utils.ts (2 hunks)
- core/src/settings.ts (2 hunks)
- core/src/utils.ts (1 hunks)
- core/test/caver-js.test.ts (1 hunks)
- core/test/nonce-manager.test.ts (4 hunks)
Files skipped from review as they are similar to previous changes (3)
- core/.eslintrc
- core/src/reporter/state.ts
- core/src/settings.ts
Additional context used
Learnings (1)
core/src/reporter/reporter.ts (1)
User: Intizar-T PR: Bisonai/orakl#1608 File: core/src/reporter/factory.ts:56-67 Timestamp: 2024-06-13T04:48:23.889Z Learning: Error handling for the nonce manager mutex function will be addressed in part 2 of the PR related to nonce management in the Orakl project.
Gitleaks
core/test/nonce-manager.test.ts
15-15: Detected a Generic API Key, potentially exposing access to various services and sensitive operations. (generic-api-key)
core/test/caver-js.test.ts
48-48: Detected a Generic API Key, potentially exposing access to various services and sensitive operations. (generic-api-key)
Biome
core/test/caver-js.test.ts
[error] 9-44: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
[error] 46-69: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.
[error] 5-71: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.core/src/utils.ts
[error] 25-27: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 76-76: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 142-164: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 144-164: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
[error] 146-164: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
[error] 148-164: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
[error] 150-164: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
[error] 152-164: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
[error] 154-164: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
[error] 156-164: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
[error] 158-164: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
[error] 160-164: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
[error] 162-164: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
core/src/reporter/utils.ts
[error] 56-58: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
[error] 207-209: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
Additional comments not posted (5)
core/src/errors.ts (1)
84-84: The addition ofTxNonceExpiredtoOraklErrorCodeis correctly implemented. This will support the new error handling features in the reporter module.core/test/nonce-manager.test.ts (1)
63-63: Increasing the number of concurrent nonce calls to 50 is aligned with the project's goals to handle more events per block efficiently. However, ensure that this change is accompanied by thorough testing to prevent potential race conditions or other concurrency issues.core/test/caver-js.test.ts (1)
44-44: The addition of a 60,000 milliseconds timeout to the test case is appropriate and ensures that the test accommodates potential delays in blockchain operations.Tools
Biome
[error] 9-44: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.core/src/reporter/utils.ts (2)
124-126: Handle theNONCE_EXPIREDerror explicitly to ensure robust error handling and appropriate user feedback.
271-273: Thesleepfunction implementation is correct and follows best practices for asynchronous JavaScript.
Description
Closes #1570
Implements infinite polling in predefined intervals, currently 1 second, when json rpc goes down and
wallet.getTransactionCount()operation fails in the nonce manager mutex function. There will be an error log and slack message at certain intervals, currently set to 10 mins, until the json rpc becomes availableType of change
Please delete options that are not relevant.
Checklist before requesting a review
Deployment