feat: replaced getRecord network calls with calls to mirror node to avoid SDK costs #2737
Conversation
Signed-off-by: Logan Nguyen <[email protected]>
Signed-off-by: Logan Nguyen <[email protected]>
Signed-off-by: Logan Nguyen <[email protected]>
Signed-off-by: Logan Nguyen <[email protected]>
quiet-node
requested review from
AlfredoG87,
ebadiere,
lukelee-sl and
a team
as code owners
|
| this.mirrorNodeClient = new MirrorNodeClient( | ||
| process.env.MIRROR_NODE_URL || '', | ||
| logger.child({ name: `mirror-node` }), | ||
| register, | ||
| this.cacheService, | ||
| undefined, | ||
| process.env.MIRROR_NODE_URL_WEB3 || process.env.MIRROR_NODE_URL || '', | ||
| ); |
There was a problem hiding this comment.
Please, let's inject this MirrorNodeClient instead of instantiating it here, I see no point in having multiple copies of this client in the heap space. Same for other places where MirrorNodeClient is used, we should inject the same object instance for those classes.
In general, it's a good practice to apply dependency injection to make each class independent of its dependencies and have a loosely coupled program.
| @@ -98,9 +100,22 @@ export class SDKClient { | |||
| private operatorAccountGauge; | |||
| private maxChunks; | |||
| private fileAppendChunkSize; | |||
| private mirrorNodeClient; | |||
There was a problem hiding this comment.
| private mirrorNodeClient; | |
| private readonly mirrorNodeClient: MirrorNodeClient; |
There was a problem hiding this comment.
Same for other lines above (98-102):
- please add types to those variables
- change them to
readonlyif we never reassign them anywhere in the class
| } | ||
|
|
||
| // Ensure that the calldata file is not empty | ||
| if (fileId) { | ||
| const fileSize = await (await new FileInfoQuery().setFileId(fileId).execute(client)).size; | ||
| // @todo: figure out the relationship between query.getCost() with the actual HBAR operator get charged as they are different => capture in metrics | ||
| const fileSize = (await new FileInfoQuery().setFileId(fileId).execute(client)).size; |
There was a problem hiding this comment.
QQ: Why not use await this.executeQuery here?
| .setFileId(fileId) | ||
| .setMaxTransactionFee(new Hbar(2)) | ||
| .freezeWith(this.clientMain); | ||
|
|
||
| // execute fileDeleteTx | ||
| const fileDeleteTxResponse = await fileDeleteTx.execute(this.clientMain); | ||
| await fileDeleteTx.execute(this.clientMain); |
There was a problem hiding this comment.
Same here - don't we want to use executeTransaction here and also for all other transactions executed inside SdkClient?
There was a problem hiding this comment.
We can also add a method executeAllTransactions and reuse it for cases where we want to call executeAll.
In this way we can only define this try/catch/finally logic only in one place and we don't have to duplicate it everywhere, just have to make sure all transactions and queries are using executeTransaction/executeAllTransactions and executeQuery.
| private readonly MirrorNodeRetries = parseNumericEnvVar( | ||
| 'MIRROR_NODE_GET_CONTRACT_RESULTS_RETRIES', | ||
| 'MIRROR_NODE_GET_CONTRACT_RESULTS_DEFAULT_RETRIES', | ||
| ); |
There was a problem hiding this comment.
This sounds like something that's a responsibility of the MirrorNodeClient, let's move it there.
Description:
Added an instance of Mirror Node client to the SDKClient class.
Added the
retrieveAndCaptureChargedTxFee()method to retrieve charged transaction fees by making repeated calls to the Mirror Node using the appropriate transactionId. This method is intended to replace thegetRecord()methods called by the SDK to reduce costs.Refactored the createFile() and deleteFile() method to implement the new
retrieveAndCaptureChargedTxFeelogic.Related issue(s):
Fixes #2706
Notes for reviewer:
Checklist