Refactor Remove AcceptedErrorCodes from MirrorNodeClient - GET_ACCOUNTS_BY_ID_ENDPOINT #1649
Conversation
AlfredoG87
requested review from
georgi-l95,
Ivo-Yankov,
a team,
ebadiere and
lukelee-sl
as code owners
| * @param {string} idOrAliasOrEvmAddress commonly an evm address | ||
| * @param {string} requestIdPrefix optional request id prefix | ||
| */ | ||
| public async getAccountOrNull(idOrAliasOrEvmAddress: string, requestIdPrefix?: string) { |
There was a problem hiding this comment.
Q: why the OrNull method overload?
Should services calling this handle the null?
There was a problem hiding this comment.
Yes, methods handle the null, this overload is to return null whenever a 404 exception is thrown.
Handling the Exception on each service was my original approach, but then realized that this methods are used by many different services (eth.ts, precheck.ts and relay.ts) and that I had to handle basically the same logic in all 3 services, so I needed to share the handleAccountNotFound logic in all 3 classes, so Instead of copy pasting (or creating a new helper class for the mirrorNodeClient), and then modify all the reference where the logic already expects a null to add a try catch block, did not seem like progress or improvement to the code.
The difference between this approach and current one of having a collection of acceptedErrorCodes is that there you don't have the option to get exceptions once the status is added to the collection, and it applies to all the different methods that might use the same endpoint, so there is no intentionality, and handles the same, any code such as 400 that in these cases, it should throw errors.
| return this.get(`${MirrorNodeClient.GET_ACCOUNTS_BY_ID_ENDPOINT}${idOrAliasOrEvmAddress}?limit=${constants.MIRROR_NODE_QUERY_LIMIT}`, | ||
| MirrorNodeClient.GET_ACCOUNTS_BY_ID_ENDPOINT, | ||
| requestIdPrefix); | ||
| public async getAccountPageLimitOrNull(idOrAliasOrEvmAddress: string, requestIdPrefix?: string) { |
There was a problem hiding this comment.
Q: why the OrNull method overload?
Should services calling this handle the null?
…NDPOINT`: getAccount, getAccountLatestEthereumTransactionsByTimestamp, getAccountPageLimit to stop accepting error codes 400 and 404, but instead to throw the error and moved the handling of the error to be specific of those methods and not as a general handler for all errors on the mirror node requests Signed-off-by: Alfredo Gutierrez <[email protected]>
AlfredoG87
force-pushed
the
1276-refactor-GetAccountsById-remove-accepted-code
branch
from
81f36cf to
53b8cd0
Compare
|
Kudos, SonarCloud Quality Gate passed!
|
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #1649 +/- ##
==========================================
+ Coverage 77.29% 77.46% +0.17%
==========================================
Files 36 36
Lines 2739 2778 +39
Branches 554 565 +11
==========================================
+ Hits 2117 2152 +35
- Misses 447 449 +2
- Partials 175 177 +2
☔ View full report in Codecov by Sentry. |
|
Closed in favor of #1651 |
AlfredoG87
deleted the
1276-refactor-GetAccountsById-remove-accepted-code
branch
Description:
Refactor methods that use mirror node endpoint
GET_ACCOUNTS_BY_ID_ENDPOINT: getAccount, getAccountLatestEthereumTransactionsByTimestamp, getAccountPageLimit to stop accepting error codes 400 and 404, but instead to throw the error and moved the handling of the error to be specific of those methods and not as a general handler for all errors on the mirror node requestsRelated issue(s)
Addresses #1276
Notes for reviewer:
This replaces this other PR
Checklist