feat: replaced HbarLimit module with the new HbarLimitService class #3024
Conversation
quiet-node
requested review from
AlfredoG87,
ebadiere,
Nana-EC,
a team and
lukelee-sl
as code owners
There was a problem hiding this comment.
Please, avoid introducing unnecessary changes unrelated to what is asked from the issue:
- it makes the PR harder to review, because it is cluttered with too many empty / unrelated changes
- makes it easier to slip in bugs, because reviewers might overlook an important change among all the "empty" changes which surround it
- wastes other people's time because it creates unnecessary conflicts with other PRs
| if (this.remainingBudget <= 0 || this.remainingBudget - estimatedTxFee < 0) { | ||
| this.hbarLimitCounter.labels(mode, methodName).inc(1); | ||
| this.logger.warn( | ||
| `${requestIdPrefix} HBAR rate limit incoming call: remainingBudget=${this.remainingBudget}, totalBudget=${this.totalBudget}, estimatedTxFee=${estimatedTxFee}, resetTimestamp=${this.reset}`, | ||
| `${requestIdPrefix} Daily HBAR rate limit incoming call: remainingBudget=${this.remainingBudget}, totalBudget=${ |
There was a problem hiding this comment.
| `${requestIdPrefix} Daily HBAR rate limit incoming call: remainingBudget=${this.remainingBudget}, totalBudget=${ | |
| `${requestIdPrefix} HBAR limit reached: remainingBudget=${this.remainingBudget}, totalBudget=${ |
There was a problem hiding this comment.
explained in this #3024 (comment)
There was a problem hiding this comment.
As explained in my previous comment, daily should be removed everywhere from the logs, as the limit duration will be flexible and is no longer going to be on a daily basis:
| `${requestIdPrefix} Daily HBAR rate limit incoming call: remainingBudget=${this.remainingBudget}, totalBudget=${ | |
| `${requestIdPrefix} Total HBAR rate limit reached: remainingBudget=${this.remainingBudget}, totalBudget=${ |
| * @param {string} [ethAddress] - The eth address to get the spending plan for. | ||
| * @param {string} [ipAddress] - The ip address to get the spending plan for. | ||
| * @returns {Promise<IDetailedHbarSpendingPlan | null>} - A promise that resolves with the spending plan or null if none exists. | ||
| * @private | ||
| */ | ||
| private async getSpendingPlan(ethAddress: string, ipAddress?: string): Promise<IDetailedHbarSpendingPlan | null> { | ||
| private async getSpendingPlan(ethAddress?: string, ipAddress?: string): Promise<IDetailedHbarSpendingPlan | null> { |
There was a problem hiding this comment.
It should NEVER be possible to call this method without arguments, please revert this change.
There was a problem hiding this comment.
If the statement is definitive, then there is a conflict in the implementation of the method itself. If the method is not supposed to be callable without arguments, doesn’t the return false statement contradict your assertion? Returning false implies that both arguments, ethAddress and ipAddress, are invalid, leading to a null return.
Regard to the update, this change altered the validity of the ethAddress as it can potentially be read as null in cases where it is used for helper queries that do not necessarily involve originalCallerAddress, as explained in the PR's description.
There was a problem hiding this comment.
Let’s move the logic which is related only to the totalLimit/remainingLimit and doesn’t require IP/ETH addresses into a separate shouldLimit method that accepts no arguments, we can call this method in those situations and the current shouldLimit would still be calling it from its implementation, but would extend its logic and require ETH and IP address to be passed to it.
There was a problem hiding this comment.
This change doesn't affect the implementation of shouldLimit(), but rather impacts addExpense(). Additionally, introducing another method would result in repetitive code and an additional instance to maintain.
I've pushed a fix that reverts ethAddress back to a required parameter for both getSpendingPlan() and createBasicSpendingPlan(). As a result, in the executeQuery, originalCaller is turned into an empty string if it's undefined before passing it as a parameter to addExpense(). Look into this comment for more details: 5066ec9.
| * @param {string} [ethAddress] - The eth address to create the spending plan for. | ||
| * @param {string} [ipAddress] - The ip address to create the spending plan for. | ||
| * @returns {Promise<IDetailedHbarSpendingPlan>} - A promise that resolves with the created spending plan. | ||
| * @private | ||
| */ | ||
| private async createBasicSpendingPlan(ethAddress: string, ipAddress?: string): Promise<IDetailedHbarSpendingPlan> { | ||
| private async createBasicSpendingPlan(ethAddress?: string, ipAddress?: string): Promise<IDetailedHbarSpendingPlan> { |
There was a problem hiding this comment.
Same here, we don't EVER want to create a spending plan without a linked ETH/IP address to it
There was a problem hiding this comment.
This change has altered the validity of the ethAddress, as it can potentially be read as null in cases where it is used for helper queries that do not necessarily involve originalCallerAddress, as explained in the PR's description.
The fallback logic for cases where neither ethAddress nor ipAddress is valid is handled within the first block of the method:
if (!ethAddress && !ipAddress) {
throw new Error('Cannot create a spending plan without an associated eth address or ip address');
}There was a problem hiding this comment.
Still, we don’t ever expect createBasicPlan to be called in those situations, if no IP/ETH address is passed to shouldLimit we want to check only against the totalLimit of the HBAR limiter and we don’t include the logic with the spending plans. Creating an empty spending plan without an IP/ETH address linked to it would only fill the memory in cache with unnecessary entries which will be used only once (for the request in which they were created) and don’t really provide any additional value.
Please, revert this change.
There was a problem hiding this comment.
Creating an empty spending plan without an IP/ETH address linked to it would only fill the memory in cache with unnecessary entries which will be used only once (for the request in which they were created) and don’t really provide any additional value.
Again, in cases where both ethAddress and ipAddress are undefined, the program will throw the "Cannot create a spending plan without an associated eth address or ip address" error.
if (!ethAddress && !ipAddress) {
throw new Error('Cannot create a spending plan without an associated eth address or ip address');
}As a result, the program cannot create an empty spending plan on its own under these conditions.
That said, I acknowledge your concern and it is a valid point. A fix has been pushed to revert ethAddress back to be required parameter, rather than optional. The fix is found here: 5066ec9.
| @@ -37,4 +38,5 @@ export interface IExecuteQueryEventPayload { | |||
| interactingEntity: string; | |||
| status: string; | |||
| requestId: string; | |||
| originalCallerAddress: string | undefined; | |||
There was a problem hiding this comment.
QQ: When is this going to be undefined?
There was a problem hiding this comment.
When requests are made through queries that do not always involve originalCaller, as explained in the PR's description.
There was a problem hiding this comment.
Is this used only for logs?
There was a problem hiding this comment.
No it's the actual ethAddress we use for the spanding plans
There was a problem hiding this comment.
I was thinking, instead of tracking the remaining total budget in-memory inside HbarLimitService (we would have to divide it by the number of Kubernetes instances, etc - it is creating unnecessary complexity in the configurations), we could create a new subscription tier (OPERATOR) and add a pre-configured spending plan which links the ETH address of the relay operator account to it.
In this way we can only specify the total limit for the OPERATOR tier and it will be shared by all instances of the relay.
What do you think?
Ofc, this could be handled in a separate PR as an improvement. We can discuss it today in the parking lot of the daily meeting.
There was a problem hiding this comment.
Also, here we would be passing the ETH address of the operator account instead of undefined for these queries executed by the operator.
quiet-node
force-pushed
the
2896-hbar-rate-limit-redesign-replace-usages-of-hbarlimit-with-the-new-hbarlimitservice-and-delete-it
branch
from
8523fe9
to
3a20364
Compare
quiet-node
force-pushed
the
2896-hbar-rate-limit-redesign-replace-usages-of-hbarlimit-with-the-new-hbarlimitservice-and-delete-it
branch
2 times, most recently
from
e04b43b
to
349eb2d
Compare
quiet-node
force-pushed
the
2896-hbar-rate-limit-redesign-replace-usages-of-hbarlimit-with-the-new-hbarlimitservice-and-delete-it
branch
from
5066ec9
to
ebc1b72
Compare
🚨 Memory Leak Detected 🚨A potential memory leak has been detected in the test titled Details📊 Memory Leak Detection Report 📊 GC Type: MarkSweepCompact Heap Statistics (before vs after executing the test):
Heap Space Statistics (before vs after executing the test):
RecommendationsPlease investigate the memory allocations in this test, focusing on objects that are not being properly deallocated. |
| const user = `(ethAddress=${ethAddress}, ipAddress=${ipAddress})`; | ||
| this.logger.trace(`${requestDetails.formattedRequestId} Checking if ${user} should be rate limited...`); |
There was a problem hiding this comment.
Please, revert this change - we don't want to be logging the IP address anywhere.
There was a problem hiding this comment.
Also, rate limited is not really the correct term here, as we previously discussed.
There was a problem hiding this comment.
Also, rate limited is not really the correct term here, as we previously discussed.
hmm good catch and sorry I know we've had this conversation couple of times and I've fixed it. Well must've slipped back in during fixing merge conflicts. Updated.
| @@ -251,21 +269,33 @@ export class HbarLimitService implements IHbarLimitService { | |||
| private async isDailyBudgetExceeded( | |||
There was a problem hiding this comment.
isDailyBudgetExceeded -> isTotalBudgetExceeded
| @@ -37,4 +38,5 @@ export interface IExecuteQueryEventPayload { | |||
| interactingEntity: string; | |||
| status: string; | |||
| requestId: string; | |||
| originalCallerAddress: string | undefined; | |||
There was a problem hiding this comment.
I was thinking, instead of tracking the remaining total budget in-memory inside HbarLimitService (we would have to divide it by the number of Kubernetes instances, etc - it is creating unnecessary complexity in the configurations), we could create a new subscription tier (OPERATOR) and add a pre-configured spending plan which links the ETH address of the relay operator account to it.
In this way we can only specify the total limit for the OPERATOR tier and it will be shared by all instances of the relay.
What do you think?
Ofc, this could be handled in a separate PR as an improvement. We can discuss it today in the parking lot of the daily meeting.
| callDataSize: number, | ||
| fileChunkSize: number, | ||
| currentNetworkExchangeRateInCents: number, | ||
| ): number { |
There was a problem hiding this comment.
QQ: instead of returning a flat number here, wouldn't it be better if we returned an Hbar object here?
| ): number { | |
| ): Hbar { |
| const estimatedTxFee = Math.round( | ||
| (totalTxFeeInCents / currentNetworkExchangeRateInCents) * constants.HBAR_TO_TINYBAR_COEF, | ||
| ); |
There was a problem hiding this comment.
| const estimatedTxFee = Math.round( | |
| (totalTxFeeInCents / currentNetworkExchangeRateInCents) * constants.HBAR_TO_TINYBAR_COEF, | |
| ); | |
| const estimatedTxFee = Hbar.fromTinybars(Math.round( | |
| (totalTxFeeInCents / currentNetworkExchangeRateInCents) * constants.HBAR_TO_TINYBAR_COEF, | |
| )); |
There was a problem hiding this comment.
That's a valid point. However, after trying it out, I found that it doesn't seem to provide much benefit and only adds complexity. Since estimatedTxFee is primarily used in two places—comparing it against the remaining budget and comparing it against spendingPlan.amountSpent—both deal with numbers. So, we would eventually need to convert it back to tinybars anyway.
- const exceedsLimit = ... spendingLimit.lt(spendingPlan.amountSpent + estimatedTxFee);
+ const exceedsLimit = ... spendingLimit.lt(spendingPlan.amountSpent + estimatedTxFee.toTinybars().toNumber());
- if (... this.remainingBudget.toTinybars().sub(estimatedTxFee).lt(0))
+ if (... this.remainingBudget.toTinybars().sub(estimatedTxFee.toTinybars()).lt(0))Additionally, all the log statements would require conversions as well:
- this.logger.trace(`... estimatedTxFee=${estimatedTxFee}, ...`)
+ this.logger.trace(`... estimatedTxFee=${estimatedTxFee.toTinybars().toNumber()}, ...`)So, for simplicity's sake, I believe keeping it as a number for now would be more efficient.
…and IExecuteQueryEventPayload 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]>
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]>
Signed-off-by: Logan Nguyen <[email protected]>
Signed-off-by: Logan Nguyen <[email protected]>
This reverts commit fd9e119 and updated related tests 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]>
This reverts commit e45c321. 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]>
Signed-off-by: Logan Nguyen <[email protected]>
Signed-off-by: Logan Nguyen <[email protected]>
Signed-off-by: Victor Yanev <[email protected]>
Signed-off-by: Logan Nguyen <[email protected]>
Signed-off-by: Logan Nguyen <[email protected]>
quiet-node
force-pushed
the
2896-hbar-rate-limit-redesign-replace-usages-of-hbarlimit-with-the-new-hbarlimitservice-and-delete-it
branch
from
d846e4d
to
9f7c0eb
Compare
Signed-off-by: Logan Nguyen <[email protected]>
quiet-node
force-pushed
the
2896-hbar-rate-limit-redesign-replace-usages-of-hbarlimit-with-the-new-hbarlimitservice-and-delete-it
branch
from
9f7c0eb
to
99603f7
Compare
|
Description:
HbarLimitmodule with the newHbarLimitServiceclass in theSdkClientandMetricServiceclasses.HbarLimitmodule from the codebase.addExpenselogic in the newHbarLimitServicehas been updated to accept a nullableethAddressvalue. Certain queries, such asgetAccountInfo,getBalanceInfo,FileContentsQuery, etc., also add expenses toremainingBalancebut don’t always require anoriginalCaller(ethAddress). Therefore,addExpensecan now handle a nullableethAddressvalue. If eitherethAddressoripAddressis valid, thespendingPlanlogic is applied. Otherwise, it is skipped entirely.sdkClient.createFile()method.HbarLimitServiceclass.Related issue(s):
Fixes #2896
Notes for reviewer:
Checklist