-
Notifications
You must be signed in to change notification settings - Fork 68
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: added an option for retrieving transaction records from either the Mirror Node or the Consensus Node. #2782
feat: added an option for retrieving transaction records from either the Mirror Node or the Consensus Node. #2782
Conversation
36f73a3
to
ad5e1c7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks really good. I think we should run the Hbar Limiter tests two times - once with GET_RECORD_DEFAULT_TO_CONSENSUS_NODE=true
and once with false
. Ideally when it is using the Mirror node the total untracked spent amount by the operator should be less. In those tests there is a delta
variable, currently set to 0.02
. Try decreasing it and see if we are closer to 0 when using the MN as compared to CN
Yes I agree! It is a good point! Pushing updates soon! Thanks! |
@Ivo-Yankov updated new changes to run hbarLimiter with another round with calls being made to mirror node. Delta just went down a little not too much though but still the new solution did its job. Please take a look when you have a chance! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unblocking the requested changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job on the improvements; the code is certainly cleaner than before. However, passing the TransactionService
around within methods is still creating unnecessary coupling between classes, which we should address.
To avoid this and simplify the dependency structure, we can take the following steps:
-
Follow the dependency tree:
EthImpl
->HapiService
->AccountService
,TransactionService
,ContractService
,FeeService
->SdkClient
,MirrorNodeClient
. We can consider opening separate issues to extract logic for accounts, contracts, and fees into distinct services. -
Keep the logic for initializing
SdkClient
intoHapiService
. This way,HapiService
will manage theSdkClient
and be the main point of interaction. -
Remove the direct dependency on
TransactionService
fromEthImpl
. Instead, initializeTransactionService
insideHapiService
, and haveHapiService
pass the initializedSdkClient
to it. -
Add methods such as
createFile
,deleteFile
,appendFile
, etc., directly inHapiService
. This should be our main entry point for operations (e.g., instead of callinghapiService.getSdkClient().createFile()
, we should directly callhapiService.createFile()
). -
The wrapper logic for
executeQuery
andexecuteTransaction
should also reside withinHapiService
.HapiService
can then use these wrappers and delegate the actual execution to theSdkClient
as needed. -
With
HapiService
callingTransactionService
to fetch and extract transaction details, it remains agnostic to whetherSdkClient
orMirrorNodeClient
is being used.
This approach untangles the dependencies between classes, resulting in a clear and linear dependency tree.
packages/relay/src/lib/services/transactionService/transactionService.ts
Outdated
Show resolved
Hide resolved
packages/relay/src/lib/services/transactionService/transactionService.ts
Outdated
Show resolved
Hide resolved
packages/relay/src/lib/services/transactionService/transactionService.ts
Outdated
Show resolved
Hide resolved
@victor-yanev absolute amazing points! However, the PR is getting bigger and it would be a little out of scope for this particular ticket, given that couple other tickets are being blocked by this PR. Could you kindly open a new ticket and drop all these amazing suggestions there along with more ideas of reorganizing the codebase? |
Signed-off-by: Logan Nguyen <[email protected]>
Signed-off-by: Logan Nguyen <[email protected]>
…tion() 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]>
…Metrrics 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]>
Signed-off-by: Logan Nguyen <[email protected]> Co-authored-by: Victor Yanev <[email protected]> Signed-off-by: Logan Nguyen <[email protected]>
Signed-off-by: Logan Nguyen <[email protected]> Co-authored-by: Victor Yanev <[email protected]> Signed-off-by: Logan Nguyen <[email protected]>
Signed-off-by: Logan Nguyen <[email protected]> Co-authored-by: Victor Yanev <[email protected]> Signed-off-by: Logan Nguyen <[email protected]>
Signed-off-by: Logan Nguyen <[email protected]> Signed-off-by: Logan Nguyen <[email protected]>
…er module Signed-off-by: Logan Nguyen <[email protected]>
Signed-off-by: Logan Nguyen <[email protected]>
d760f61
to
192a3a2
Compare
Signed-off-by: Logan Nguyen <[email protected]>
d009a4f
to
f6381ac
Compare
…mirror node Signed-off-by: Logan Nguyen <[email protected]>
Quality Gate failedFailed conditions |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2782 +/- ##
==========================================
+ Coverage 77.77% 83.29% +5.51%
==========================================
Files 43 48 +5
Lines 3312 3513 +201
Branches 702 738 +36
==========================================
+ Hits 2576 2926 +350
+ Misses 492 346 -146
+ Partials 244 241 -3
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Description:
added GET_RECORD_DEFAULT_TO_CONSENSUS_NODE to configuration docs. This configuration allows the Relay to retrieve transaction record by making calls to Consensus Node (CN) or Mirror Node (MN) based on its input. Default to "false" for MN calls.
added new wrapper class
TransactionService
withgetTransactionStatusAndMetrrics()
method. This method is the implementation for the logic mentioned above. If GET_RECORD_DEFAULT_TO_CONSENSUS_NODE is set to "true", calls will be redirected to CN. Otherwise, calls will be redirected to MN. MirrorNodeClient is injected frometh.ts level
. Unit tests are included.modified getTransferAmountSumForAccount so it can be compatible with transfers array returned from MN.
added getTransactionStatusAndMetrrics to executeTransaction() and executeTransaction().
removed unused methods and added JSDoc to methods in SDKClient, sdkClient.executeGetTransactionRecord() and sdkClient.getTransactionMetrics() as they are now replaced by getTransactionStatusAndMetrrics(). Also added JSDoc comments for all methods in SDKClient.
Modified and added new tests in sdkClient.spec.ts reflect the new logic.
added a new IMirrorNode.ts type file and host all interfaces and types that are related to Mirror Node.
Related issue(s):
Fixes #2706
Notes for reviewer:
Checklist