Draft: Feat/eth http poll #78
Conversation
![codereviewbot-ai[bot]](https://avatars.githubusercontent.com/in/309638?s=60&v=4){kind=small}
…etCore into feat/eth-http-poll
| async start() { | ||
| await this.refreshContracts() | ||
| const lastBlock = await this.web3.eth.getBlockNumber() | ||
| this.watcher.startWatching(this.updateContract.bind(this), (e) => { | ||
| logger.error(`WatcherPolling error: ${e}`) | ||
| this.eventBus.emit('wallet-error', { | ||
| inner: e, | ||
| message: 'Could not update contract state', | ||
| meta: { plugin: 'contracts' }, | ||
| }) | ||
| }, lastBlock) | ||
| } |
There was a problem hiding this comment.
The start method starts a watcher without any mechanism to stop it in case of an error or exception. This could lead to resource leaks if the watcher continues to run indefinitely. It's important to ensure that resources are properly cleaned up to avoid potential memory leaks or other related issues.
I recommend adding error handling or a try-catch block around the watcher's startWatching method. In the catch block, you should stop the watcher and handle the error appropriately. This could involve logging the error, notifying the user, or even re-throwing the error after cleanup, depending on your application's error handling strategy.
| async refreshContracts() { | ||
| this.eventBus.emit('contracts-scan-started') | ||
| const contractIDs = await this.cloneFactory.methods.getContractList().call(); | ||
|
|
||
| // cannot use .revese() because it mutates the original array, which is not allowed in readonly {contractIDs} | ||
| const reversedContracts = []; | ||
| for (let i = contractIDs.length - 1; i >= 0; i--) { | ||
| reversedContracts.push(contractIDs[i]); | ||
| } | ||
|
|
||
| const chunkSize = 5; | ||
| for (let i = 0; i < reversedContracts.length; i += chunkSize) { | ||
| const chunk = reversedContracts.slice(i, i + chunkSize); | ||
| await Promise.all(chunk.map((id) => this.updateContract(id))) | ||
| } | ||
| this.eventBus.emit("contracts-scan-finished") | ||
| } |
There was a problem hiding this comment.
The refreshContracts method retrieves all contracts and updates them one by one. This could be inefficient if there are a large number of contracts, as it would result in a large number of sequential network requests.
Consider using a more efficient approach to update contracts in parallel. For example, you could divide the contracts into smaller chunks and update each chunk in parallel. This would significantly reduce the total time taken to update all contracts. However, be careful not to make too many parallel requests as this could overwhelm the network or the server. You might need to experiment with different chunk sizes to find the optimal balance between speed and resource usage.
| const watcher = new WatcherPolling(web3, config.walletAddress, cloneFactory ,config.pollingIntervalMs) | ||
| eventsController = new EventsController( | ||
| web3, eventBus, watcher, config.walletAddress, cloneFactory | ||
| ) |
There was a problem hiding this comment.
The WatcherPolling and EventsController objects are created without any error handling. If the creation of these objects fails due to any reason (like invalid parameters), it will cause the application to crash. It's recommended to wrap these object creations in a try-catch block and handle any potential errors gracefully. This could involve logging the error and stopping the execution of the function, or throwing a custom error that can be caught and handled by the calling function.
| startWatching: eventsController.start.bind(eventsController), | ||
| stopWatching: eventsController.stop.bind(eventsController), | ||
| refreshContracts: eventsController.refreshContracts.bind(eventsController), | ||
| getContractHistory: (contractAddr) => getContractHistory(web3, contractAddr, config.walletAddress), | ||
| createContract: createContract(web3, cloneFactory), | ||
| cancelContract: cancelContract(web3, cloneFactory), | ||
| purchaseContract: purchaseContract(web3, cloneFactory, lumerin), | ||
| editContract: editContract(web3, cloneFactory, lumerin), | ||
| editContract: editContract(web3, cloneFactory), | ||
| getMarketplaceFee: getMarketplaceFee(cloneFactory), | ||
| setContractDeleteStatus: setContractDeleteStatus( | ||
| web3, | ||
| cloneFactory, | ||
| onUpdate, | ||
| eventsController.updateContract.bind(eventsController), | ||
| ), |
There was a problem hiding this comment.
The api object is directly exposing methods from the eventsController object. This tightly couples the api object with the eventsController object, making it harder to change or replace eventsController in the future without also having to modify the api object. Instead, consider creating wrapper functions within the api object that call the corresponding eventsController methods. This way, if eventsController needs to be changed in the future, you only need to update the wrapper functions, not the entire api object.
| web3.currentProvider?.disconnect().catch(() => { | ||
| logger.error('Web3 disconnect error') | ||
| }) |
There was a problem hiding this comment.
The error handling in the destroyWeb3 function is not sufficient. When an error occurs during the disconnect operation, it's simply logged and then ignored. This could lead to unexpected behavior if the disconnect operation fails for some reason, as the rest of the code would continue to execute as if nothing had happened. It would be better to propagate the error up to the caller, so they can decide how to handle it. This could be done by removing the .catch block and letting the error bubble up, or by rethrowing the error inside the .catch block.
| getBlockFunc() | ||
| const interval = setInterval(getBlockFunc, updateInterval); |
There was a problem hiding this comment.
The getBlockFunc function is called immediately before setting up the interval. This could potentially lead to a race condition if the function takes longer than the updateInterval to complete, as it could be called again before the first call has finished. This could lead to unexpected behavior and potential issues with the event stream.
To avoid this, consider setting up the interval first and then immediately calling the function within the interval. This ensures that the function is not called again before a previous call has completed.
| stream.removeAllListeners() | ||
| if (interval) { | ||
| clearInterval(interval) | ||
| } |
There was a problem hiding this comment.
The stop function removes all listeners from the stream and clears the interval. However, it does not check if there are any listeners before trying to remove them. This could potentially lead to errors if the function is called when there are no listeners.
To avoid this, consider checking if there are any listeners before trying to remove them. Additionally, it would be good to nullify the interval after clearing it to avoid any potential issues with multiple calls to stop.
| function start({ config, eventBus, plugins }) { | ||
| // debug.enabled = config.debug; | ||
| const { lmrTokenAddress } = config; | ||
|
|
||
| const web3 = new Web3(plugins.eth.web3Provider); | ||
| /** @type { import('web3').default } */ | ||
| const web3 = plugins.eth.web3 | ||
|
|
||
| const web3Subscribable = new Web3(plugins.eth.web3SubscriptionProvider); | ||
| const lumerin = Lumerin(web3, config.lmrTokenAddress); | ||
| const cf = CloneFactory(web3, config.cloneFactoryAddress); | ||
|
|
||
| const eventsRegistry = createEventsRegistry(); | ||
| const queue = createQueue(config, eventBus, web3); | ||
| const lumerin = Lumerin(web3Subscribable, lmrTokenAddress); | ||
|
|
||
| const explorer = createExplorer(config.explorerApiURLs, web3, lumerin, eventBus); | ||
|
|
||
| syncer = createTransactionSyncer( | ||
| config, | ||
| eventBus, | ||
| web3, | ||
| queue, | ||
| eventsRegistry, | ||
| explorer | ||
| ); | ||
| explorer = createExplorer(config.explorerApiURLs, web3, lumerin, cf, config.pollingIntervalMs); | ||
|
|
||
| logger.debug('Initiating blocks stream'); | ||
| const streamData = createStream(web3, config.blocksUpdateMs); | ||
| blocksStream = streamData.stream; | ||
| interval = streamData.interval; | ||
| blocksStream = createBlockStream(web3, config.blocksUpdateMs); | ||
|
|
||
| blocksStream.on('data', function ({ hash, number, timestamp }) { | ||
| blocksStream.stream.on('data', function ({ hash, number, timestamp }) { | ||
| logger.debug('New block', hash, number); | ||
| eventBus.emit('coin-block', { hash, number, timestamp }); | ||
| }); | ||
| blocksStream.on('error', function (err) { | ||
| eventBus.emit('web3-connection-status-changed', { connected: true }); | ||
| }).on('error', function (err) { | ||
| logger.debug('Could not get latest block'); | ||
| eventBus.emit('wallet-error', { | ||
| inner: err, | ||
| message: 'Could not get latest block', | ||
| meta: { plugin: 'explorer' } | ||
| }); | ||
| eventBus.emit('web3-connection-status-changed', { connected: false }); | ||
| }); |
There was a problem hiding this comment.
The start function does not handle the case where the web3 object is not available in the plugins.eth object. This could lead to a runtime error if plugins.eth.web3 is undefined. To avoid this, you should add a check to ensure that web3 is defined before using it. If it's not defined, you should handle this appropriately, for example by logging an error message and stopping the execution of the function.
| const Web3 = require('web3') | ||
| const { SubscriptionPolling } = require('./polling') | ||
|
|
||
| const web3 = new Web3('http://localhost:8545') |
There was a problem hiding this comment.
The URL for the Web3 instance is hardcoded to 'http://localhost:8545'. This could lead to issues when the code is deployed in a different environment where the Ethereum node might be running on a different URL or port. It's recommended to make this configurable through environment variables or configuration files.
| if (!lumerinAddr || !walletAddr || !cfAddr) { | ||
| throw new Error('LUMERIN_ADDRESS or WALLET_ADDRESS or CLONEFACTORY_ADDRESS env variables must be set') | ||
| } |
There was a problem hiding this comment.
The code throws an error if any of the environment variables LUMERIN_ADDRESS, WALLET_ADDRESS, or CLONEFACTORY_ADDRESS are not set. However, it does not provide a clear message about which specific variable is missing. This could make debugging more difficult. It's recommended to check each variable individually and throw an error with a specific message for each one.
| function decodeEvent(contractAbi, eventData) { | ||
| const [eventSignature, ...restTopics] = eventData.topics | ||
| const eventAbi = decodeAbiSignature(contractAbi, eventSignature) | ||
| if (!eventAbi) { | ||
| throw new Error(`Event ${eventSignature} not found`) | ||
| } | ||
| try { | ||
| const data = abi.decodeLog(eventAbi.inputs, eventData.data, restTopics) | ||
| return { | ||
| eventName: eventAbi.name, | ||
| ...data | ||
| } | ||
| } catch (err) { | ||
| throw new Error(`Event ${eventAbi.name} decode error: ${err}`) | ||
| } | ||
| } |
There was a problem hiding this comment.
The decodeEvent function does not handle errors gracefully. If the event signature is not found in the contract ABI or if there is an error while decoding the log, it throws an error which might cause the application to crash if not properly handled. Instead of throwing an error, consider returning a specific error object or null, and handle this case in the calling function. This way, the application can continue to run even if one event fails to decode.
| function sleep(ms) { | ||
| return new Promise(resolve => setTimeout(resolve, ms)); | ||
| } |
There was a problem hiding this comment.
The sleep function is a blocking operation that will halt the execution of your code for a specified amount of time. This could lead to performance issues if used excessively or with large time values. Consider using non-blocking alternatives or design patterns like async/await or Promises to manage delays or waiting times in your code.
| // cannot use .revese() because it mutates the original array, which is not allowed in readonly {contractIDs} | ||
| const reversedContracts = []; | ||
| for (let i = contractIDs.length - 1; i >= 0; i--) { | ||
| reversedContracts.push(contractIDs[i]); | ||
| } |
There was a problem hiding this comment.
The code is manually reversing an array which can be inefficient and verbose. Instead of using a loop to create a reversed copy of the array, it would be more efficient and concise to use the built-in array method slice() to create a shallow copy of the array and then apply reverse() on that copy. This would avoid mutating the original array and provide a cleaner solution.
| for (let i = 0; i < reversedContracts.length; i += chunkSize) { | ||
| const chunk = reversedContracts.slice(i, i + chunkSize); | ||
| await Promise.all(chunk.map((id) => this.updateContract(id))) |
There was a problem hiding this comment.
The code is processing chunks of promises sequentially within a loop, which could lead to performance issues if there are many contracts to update. Instead of awaiting each chunk one after the other, it would be more efficient to collect all the chunks into an array of promise arrays and then use Promise.all() to wait for all of them to resolve. This would allow for concurrent processing of contract updates, potentially reducing the overall time taken for the refreshContracts method to complete.
| createContract: createContract(web3, cloneFactory), | ||
| cancelContract: cancelContract(web3, cloneFactory), | ||
| purchaseContract: purchaseContract(web3, cloneFactory, lumerin), | ||
| editContract: editContract(web3, cloneFactory, lumerin), | ||
| editContract: editContract(web3, cloneFactory), | ||
| getMarketplaceFee: getMarketplaceFee(cloneFactory), |
There was a problem hiding this comment.
The createContract, cancelContract, purchaseContract, editContract, and getMarketplaceFee functions are being called incorrectly. These functions are likely intended to be factory functions that return new functions, but they are being invoked directly without arguments, which would result in passing undefined to the API methods that require a web3 instance and other parameters. This will cause runtime errors when these API methods are called.
Recommended solution: Refactor the code to correctly pass the required arguments to these functions so that they return the appropriate handler functions. Ensure that the functions are invoked with the necessary parameters to create the handler functions that can be used by the API.
No description provided.