Use Bisq API to fix scalability issues #20
Conversation
There was a problem hiding this comment.
Thank you for initiating this; it is long overdue and much needed to improve performance.
Although I am not familiar with Typescript nor the implementation details of this project, I reviewed this as best I could and provided feedback where I deemed necessary. Apologies if some of my comments are due to my lack of knowledge or understanding.
I found the lack of documentation a hindrance since the behavior of some methods were not entirely obvious due to misleading method names or non-obvious parameter names. But a lack of documentation seems to be consistent with the project as a whole, so its at least consistent in that regard. Perhaps some of my struggle is due to unfamiliarity with the project. I am not suggesting that documentation needs to be added, but at least improving method and parameter names would help to improve comprehension, particularly in areas that I have provided comments.
I feel that logging should be reviewed in terms of level, context, consistency, and judiciousness.
I found some methods somewhat complex and hard to grok, and wonder if there are more efficient paradigms to be used. Again, maybe that is just due to my lack of familiarity with Typescript. But I do have concerns around some implemented behaviors and highly recommend adding tests to confirm behavior is as expected, particularly in bisq.ts.
Out of curiosity, have you tried this on a running instance?
Feel free to provide clarification to my comments and I can re-review this again after any changes. Once I feel satisfied my concerns are resolved, I can try running this.
| @@ -96,7 +96,8 @@ | |||
| }, | |||
| "BISQ": { | |||
| "ENABLED": true, | |||
| "DATA_PATH": "__BISQ_DATA_PATH__" | |||
| "HOST": "127.0.0.1", | |||
| "PORT": 8081 | |||
There was a problem hiding this comment.
You should also update this in the production config file.
https://github.com/bisq-network/mempool/blob/main/production/mempool-config.bisq.json#L36
| public startBisqService(): void { | ||
| logger.info('start bisq service (start)'); | ||
| this.$pollForNewData(); // obtains the current block height | ||
| logger.info('start bisq service (end)'); |
There was a problem hiding this comment.
Are these log messages wrapping the call to pollForNewData more for debugging than informational purposes?
If debugging, then could change to debug level or remove if no longer necessary.
If informational, the text content could be more informative. There is no Bisq service being started anymore, despite the method name remaining unchanged. This method now just initiates the data polling, so could have a single log statement before the call indicating it is starting the polling.
| getTransaction(txId: string): BisqTransaction | undefined { | ||
| return this.transactionIndex[txId]; | ||
| public async $getTransaction(txId: string): Promise<BisqTransaction | undefined> { | ||
| logger.info("getTransaction called from frontend"); |
There was a problem hiding this comment.
Perhaps this log message would be more suitable at a debug level. Perhaps also including the requested txId would be beneficial. And likely not necessary to include "from frontend".
There was a problem hiding this comment.
This suggestion also applies to numerous other log statements throughout this file. Rather than having to comment on each individually, I will leave it up to you to identify them.
| }); | ||
| private isBisqConnected() : boolean { | ||
| if (this.stats.height > 0) | ||
| return true; |
There was a problem hiding this comment.
So this method just verifies whether data was retrieved at least once, not whether a connection is active or can be made? Perhaps connection verification would be beneficial, but at least a more appropriate method name would be beneficial.
| genesisHeight: 0, | ||
| _bsqPrice: 0, | ||
| _usdPrice: 0, | ||
| _marketCap: 0 |
There was a problem hiding this comment.
Out of curiosity, since I am not familiar with Typescript style, is the leading underscore used to indicate anything?
| private async $lookupBsqTx2(start: number, limit: number, types: string) : Promise<BisqTransaction[]> { | ||
| const customPromise = this.makeApiCall('transactions/query-txs-paginated', [String(start), String(limit), types]); | ||
| var buffer = await customPromise; | ||
| const txs: BisqTransaction[] = JSON.parse(buffer) |
There was a problem hiding this comment.
I assume should have some exception handling here?
| private async $lookupBsqTxForAddr(addr: string) : Promise<BisqTransaction[]> { | ||
| const customPromise = this.makeApiCall('transactions/get-bsq-tx-for-addr', [addr]); | ||
| var buffer = await customPromise; | ||
| const txs: BisqTransaction[] = JSON.parse(buffer) |
There was a problem hiding this comment.
I assume should have some exception handling here?
| private async $lookupBsqBlockByHeight(height: number) : Promise<BisqBlock> { | ||
| const customPromise = this.makeApiCall('blocks/get-bsq-block-by-height', [String(height)]); | ||
| var buffer = await customPromise; | ||
| const block: BisqBlock = JSON.parse(buffer) |
There was a problem hiding this comment.
I assume should have some exception handling here?
| logger.err('Error updating Bisq market price: ' + (e instanceof Error ? e.message : e)); | ||
| await setDelay(config.MEMPOOL.EXTERNAL_RETRY_INTERVAL); | ||
| retry++; | ||
| this.blocks = this.allBlocks; |
There was a problem hiding this comment.
What is the difference between blocks and allBlocks?
| var tradesPre = this.tradesData.length; | ||
| trades.forEach((trade) => { | ||
| // ignore trade stats older than two years (performance reasons) | ||
| var oneYear = 1000*60*60*24*365*2; |
There was a problem hiding this comment.
Is this not two years, rather than one year as the name suggests.
Resubmitting this PR (previously #13) after CI fix.
Original version of BSQ explorer had Bisq statsnode periodically export large JSON files to disk which were read into memory arrays. This became inefficient as the blockchain grew.
This version uses a Bisq REST API as direct source of BSQ blockchain & markets data, therefore no longer duplicating the entire data set in the explorer's memory. Is a change to the Bisq portion of the mempool backend service only.