new retry implementation web3 #77
Conversation
![codereviewbot-ai[bot]](https://avatars.githubusercontent.com/in/309638?s=60&v=4){kind=small}
src/plugins/eth/web3.js
Outdated
| } | ||
|
|
||
| function destroyWeb3(web3) { | ||
| web3.currentProvider.disconnect() |
There was a problem hiding this comment.
The destroyWeb3 function calls web3.currentProvider.disconnect() without checking if web3 or web3.currentProvider is defined. This could lead to a runtime error if web3 or web3.currentProvider is undefined. Always check that an object and its properties are defined before accessing them.
| this.currentProvider.send = (payload, callback) => { | ||
| originalSend(payload, (error, response) => { | ||
| if (error) { | ||
| // If the request fails, switch to the next provider and try again | ||
| this.currentIndex = (this.currentIndex + 1) % this.providers.length | ||
| this.setCustomProvider(this.providers[this.currentIndex]) | ||
| logger.error( | ||
| `Switched to provider: ${this.providers[this.currentIndex].host}` | ||
| ) | ||
| this.currentProvider.send(payload, callback) // Retry the request | ||
| } else { | ||
| callback(null, response) | ||
| } | ||
| }) |
There was a problem hiding this comment.
The current implementation of the provider switching mechanism can lead to an infinite loop if all providers fail. If a request fails, the code switches to the next provider and retries the request. However, there is no mechanism to stop this process if all providers are down or unresponsive. This could lead to an infinite loop of requests, which could consume significant resources and potentially lead to a denial of service.
To fix this, you could introduce a counter to track the number of failed attempts. If the number of failed attempts equals the number of providers, you could stop retrying and return an error. This would prevent an infinite loop and allow the system to fail gracefully.
src/plugins/eth/web3.js
Outdated
| } | ||
|
|
||
| function destroyWeb3(web3) { | ||
| web3.currentProvider.disconnect() |
There was a problem hiding this comment.
The destroyWeb3 function is calling web3.currentProvider.disconnect() without any error handling. If the currentProvider is not set or if the disconnect function throws an error for any reason, this could cause the application to crash. It's recommended to wrap this call in a try-catch block and handle any potential errors appropriately.
| function createWeb3(config) { | ||
| // debug.enabled = config.debug | ||
|
|
||
| providers = config.httpApiUrls.map((url) => { | ||
| return new Web3.providers.HttpProvider(url, { | ||
| agent: new https.Agent({ | ||
| rejectUnauthorized: false, // Set to false if your HTTPS node endpoint uses a self-signed certificate | ||
| }), | ||
| }) | ||
| }) | ||
|
|
||
| const web3 = new Web3(providers[0], { | ||
| agent: new https.Agent({ | ||
| rejectUnauthorized: false, // Set to false if your HTTPS node endpoint uses a self-signed certificate | ||
| }), | ||
| }) | ||
|
|
||
| overrideFunctions(web3, providers) | ||
| overrideFunctions(web3.eth, providers) | ||
| web3.subscriptionProvider = subscriptionProvider | ||
|
|
||
| const web3 = new Web3Http(config.httpApiUrls) | ||
| return web3 | ||
| } |
There was a problem hiding this comment.
The createWeb3 function is creating a new instance of Web3Http and returning it, but there's no validation of the config parameter. If config.httpApiUrls is not provided or is not in the expected format, this could lead to unexpected behavior or errors. It's recommended to add validation for the config parameter and its properties before using them.
| createWeb3, | ||
| destroyWeb3, |
There was a problem hiding this comment.
The exported functions createWeb3 and destroyWeb3 are not documented. While this is not a critical issue, it's a good practice to document exported functions, especially in a module like this that could be used by other parts of the application. This can help other developers understand what these functions do, what parameters they expect, and what they return. Consider adding JSDoc comments or similar documentation for these functions.
| (provider) => | ||
| new Web3.providers.HttpProvider(provider, { | ||
| agent: new https.Agent({ | ||
| rejectUnauthorized: false, // Set to false if your HTTPS node endpoint uses a self-signed certificate |
There was a problem hiding this comment.
The rejectUnauthorized option is set to false which means that the application will not reject any unauthorized certificates. This can lead to potential security risks such as Man-in-the-Middle (MitM) attacks where an attacker can intercept and possibly alter the communication between the client and the server. It's recommended to set rejectUnauthorized to true in production environments to ensure that only authorized certificates are accepted. If you're using self-signed certificates for development, consider using a process environment variable to switch between settings.
| setProviderOptions(options) { | ||
| this.currentProvider.host = options.host || this.currentProvider.host | ||
| this.currentProvider.timeout = | ||
| options.timeout || this.currentProvider.timeout | ||
| } |
There was a problem hiding this comment.
The setProviderOptions method directly modifies the properties of this.currentProvider. This could potentially lead to unexpected behavior if other parts of the code rely on the original values of these properties. Instead of directly modifying these properties, consider creating a new provider with the updated options and setting it as the current provider. This way, you ensure that the original provider remains unchanged and can be reused if necessary.
| function createWeb3(config) { | ||
| // debug.enabled = config.debug | ||
|
|
||
| providers = config.httpApiUrls.map((url) => { | ||
| return new Web3.providers.HttpProvider(url, { | ||
| agent: new https.Agent({ | ||
| rejectUnauthorized: false, // Set to false if your HTTPS node endpoint uses a self-signed certificate | ||
| }), | ||
| }) | ||
| }) | ||
|
|
||
| const web3 = new Web3(providers[0], { | ||
| agent: new https.Agent({ | ||
| rejectUnauthorized: false, // Set to false if your HTTPS node endpoint uses a self-signed certificate | ||
| }), | ||
| }) | ||
|
|
||
| overrideFunctions(web3, providers) | ||
| overrideFunctions(web3.eth, providers) | ||
| web3.subscriptionProvider = subscriptionProvider | ||
|
|
||
| const web3 = new Web3Http(config.httpApiUrls) | ||
| return web3 | ||
| } |
There was a problem hiding this comment.
The createWeb3 function creates a new instance of Web3Http but does not use the Web3 module imported at line 5. If the Web3 module is not used elsewhere in the code, it should be removed to avoid confusion and unnecessary imports. If it is used, consider renaming the function or the variable to avoid confusion between the Web3 module and the web3 variable.
| }) | ||
|
|
||
| object.setProvider = originalSetProvider | ||
| web3.currentProvider?.disconnect() |
There was a problem hiding this comment.
The destroyWeb3 function assumes that web3.currentProvider is always defined. If web3.currentProvider is undefined, calling disconnect() on it will throw an error. Consider adding error handling or a check to ensure that web3.currentProvider is defined before calling disconnect() on it.
| overrideFunctions(web3.eth, providers) | ||
| web3.subscriptionProvider = subscriptionProvider | ||
|
|
||
| const web3 = new Web3Http(config.httpApiUrls) |
There was a problem hiding this comment.
The createWeb3 function creates a new instance of Web3Http using config.httpApiUrls. There is no validation of config.httpApiUrls before it's used. If it's not provided or is not in the expected format, this could lead to errors. Consider adding validation for config.httpApiUrls before using it.
| (provider) => | ||
| new Web3.providers.HttpProvider(provider, { | ||
| agent: new https.Agent({ | ||
| rejectUnauthorized: false, // Set to false if your HTTPS node endpoint uses a self-signed certificate |
There was a problem hiding this comment.
The rejectUnauthorized option is set to false which means that the application will accept and establish a connection even if the server's SSL certificate is not trusted. This is a security risk as it makes the application vulnerable to man-in-the-middle attacks. The recommended solution is to use trusted certificates and set rejectUnauthorized to true. If self-signed certificates are necessary for development or internal use, consider using a configuration option to toggle this behavior, but ensure that in production environments, only trusted certificates are used.
| this.currentProvider.send = (payload, callback) => { | ||
| originalSend(payload, (error, response) => { | ||
| if (error) { | ||
| // Avoid infinite loop | ||
| if (this.retryCount >= this.providers.length) { | ||
| callback(error, null) | ||
| this.retryCount = 0 | ||
| return; | ||
| } | ||
| // If the request fails, switch to the next provider and try again | ||
| this.currentIndex = (this.currentIndex + 1) % this.providers.length | ||
| this.setCustomProvider(this.providers[this.currentIndex]) | ||
| logger.error( | ||
| `Switched to provider: ${this.providers[this.currentIndex].host}` | ||
| ) | ||
| this.retryCount += 1 | ||
| this.currentProvider.send(payload, callback) // Retry the request | ||
| } else { | ||
| this.retryCount = 0 | ||
| callback(null, response) | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
The retry mechanism implemented here could lead to a potential infinite loop if the error persists across all providers. Although there is a check to avoid an infinite loop if the retry count exceeds the number of providers, the retry count is reset to 0 before the callback is called with the error. If the callback function in turn calls the send method again, it could lead to an infinite loop. To avoid this, consider implementing a more robust retry mechanism that includes a maximum retry limit across all providers and does not reset the retry count before calling the error callback.
| setProviderOptions(options) { | ||
| this.currentProvider.host = options.host || this.currentProvider.host | ||
| this.currentProvider.timeout = | ||
| options.timeout || this.currentProvider.timeout | ||
| } |
There was a problem hiding this comment.
The setProviderOptions method directly modifies the properties of this.currentProvider. This could potentially lead to unexpected behavior if the currentProvider object is used elsewhere in the application. Instead of directly modifying the properties of currentProvider, consider creating a new provider with the updated options and setting it as the current provider.
No description provided.