feat: validate that operator account has positive balance #3930
Conversation
Test Results 20 files ±0 277 suites ±0 18m 2s ⏱️ +5s Results for commit c1224f4. ± Comparison against base commit f82f220. This pull request removes 1 test.♻️ This comment has been updated with latest results. |
Signed-off-by: Luis Mastrangelo <[email protected]>
Signed-off-by: Luis Mastrangelo <[email protected]>
Signed-off-by: Luis Mastrangelo <[email protected]>
Signed-off-by: Luis Mastrangelo <[email protected]>
acuarica
force-pushed
the
3929-operator-has-balance-in-read-write-mode
branch
from
8fa49fb to
c1224f4
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅ @@ Coverage Diff @@
## main #3930 +/- ##
==========================================
- Coverage 86.86% 86.75% -0.11%
==========================================
Files 87 87
Lines 5039 5045 +6
Branches 1020 1022 +2
==========================================
Hits 4377 4377
- Misses 400 407 +7
+ Partials 262 261 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
| @@ -50,6 +50,7 @@ jobs: | |||
| -e MIRROR_NODE_URL='http://127.0.0.1:5551' \ | |||
| -e OPERATOR_ID_MAIN='0.0.1002' \ | |||
| -e OPERATOR_KEY_MAIN='302e020100300506032b65700422042077d69b53642d0000000000000000000000000000000000000000000000000000' \ | |||
| -e READ_ONLY='true' \ | |||
There was a problem hiding this comment.
qq why the need to add READ_ONLY it already has operator key and ID right?
There was a problem hiding this comment.
Because otherwise it fails to properly start due to balance check (there is no MN behind http://127.0.0.1:5551).
There was a problem hiding this comment.
Ah I see thanks for the explanation
|
|
||
| async ensureOperatorHasBalance() { | ||
| if (ConfigService.get('READ_ONLY')) return; | ||
|
|
||
| const operator = this.clientMain.operatorAccountId!.toString(); | ||
| const balance = BigInt(await this.ethImpl.getBalance(operator, 'latest', {} as RequestDetails)); | ||
| if (balance === BigInt(0)) { | ||
| throw new Error(`Operator account '${operator}' has no balance`); | ||
| } else { | ||
| this.logger.info(`Operator account '${operator}' has balance: ${balance}`); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
I feel like the ensureOperatorHasBalance() function could be done a little differently as it seems to pull in quite a few components. Like relying on both clientMain and ethImpl makes it feel somewhat fragile, since it depends on both being properly set up. On top of that, using ethImpl.getBalance, an app-level API that itself relies on MirrorNodeClient, cacheService, Logger, eventEmitter, etc. being correctly initialized — just to validate a config value feels a bit circular.
So would it make more sense, or be possible, to:
• Get the operator ID directly from ConfigService via ConfigService.get('OPERATOR_ID')
• Fetch the balance straight from the Mirror Node using the /balances endpoint via ConfigService.get('MIRROR_NODE_URL')
That way, we could skip involving ethImpl altogether and avoid any extra logic or validation tied to eth_getBalance. I know it’s a lightweight call, but since it’s an app-level API and could be programmatically updated, it makes the function feel a bit less stable for this job.
At the end I just think the fewer moving parts we rely on, the better at this stage of validation.
There was a problem hiding this comment.
Also when I first heard about validating the operator balance, I initially thought it could be done inside hapiService during client initialization or some sort since that’s where we set up the operator keys and IDs. It felt like a natural place to include a balance check as part of that process.
Maybe it’s not feasible because it would require the function to be async or maybe we could just use .then() or something along those lines. Either way, it feels like the balance validation belongs there. We’re initializing the SDK client, and if the operator doesn’t have enough balance, it makes sense to catch that early and fail right at that level.
There was a problem hiding this comment.
an app-level API that itself relies on MirrorNodeClient, cacheService, Logger, eventEmitter, etc. being correctly initialized — just to validate a config value feels a bit circular.
The ensureOperatorHasBalance is just part of the public API of the Relay. There is nothing circular here, that's why it's being called in main.
• Get the operator ID directly from ConfigService via ConfigService.get('OPERATOR_ID')
• Fetch the balance straight from the Mirror Node using the /balances endpoint via ConfigService.get('MIRROR_NODE_URL')
Well, the idea is to avoid duplication. If we have an API to make requests to the MN, we should use it, regardless if it's for RPC calls or initial checks. The same for the operator, usingoperatorAccountId ensures that we are reusing any logic that is already put in clientMain.
There was a problem hiding this comment.
It felt like a natural place to include a balance check as part of that process.
In that case now you have a dependency from hapiService to the Mirror Node client, which to me does not seem right. MN client and HAPI service should be independent.
On the other hand, I understand that maybe this validation could have been put somewhere else. That's where async comes in. Having async constructors will cause more problems than solutions.
There was a problem hiding this comment.
Cool thanks for the explanation it makes sense! However, I still think it might be more stable and decoupled if we used configService to get the operator ID and called the /balances endpoint directly. That way, we can avoid relying on all the extra properties coming from hapiService and ethImpl. Just a thought I wanted to put out there not opposed to your solution at all, especially since it shouldn’t affect the app’s performance either way.
There was a problem hiding this comment.
Not sure what's the problem on relying on an object being properly constructed. Note that given this check is async, we need a proper Relay constructed anyways.
Regarding the /balances/ endpoint, there is some additional logic in AccountService.getBalance like converting to weibars, formatting, and deconstructing the balance result. This would have to be repeated if we call the endpoint directly.
Description:
This PR validates that the operator account
OPERATOR_ID_MAINhas positive balance when starting up the Relay/WebSocket server in Read-Write mode. If the operator account has no balance (or does not exist), the Relay/WebSocket will fail on start up.Related issue(s):
Fixes #3929.
Notes for reviewer:
Note
This PR introduces an
asyncstart up check. It needs to beasyncbecause it needs to make a request to the Mirror Node to get the operator's balance. This check needs to be placed directly inmain(for both Relay and WebSocket) because theConfigServiceloading process is sync (noPromises involved). It cannot be placed in theRelay's constructor either because this will lead to anasyncconstructor.Note
Initial discussion can be found here #3896 (comment).
Note
In
packages/relay/tests/lib/eth/eth-helpers.ts, some@ts-ignorecomments were removed because they were not exercised. We should use@ts-expect-errorinstead.Note
In
packages/relay/tests/lib/eth/eth_getBalance.spec.ts, code related toSDKClientwas removed because it's not used anymore.Checklist