fix: Allow the HBar Rate Limiter to be disabled. #3252
Conversation
Signed-off-by: Eric Badiere <[email protected]>
Test Results 4 files + 3 421 suites +414 27s ⏱️ -3s Results for commit 1938973. ± Comparison against base commit ce43b76. This pull request removes 5 and adds 1493 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Signed-off-by: Eric Badiere <[email protected]>
ebadiere
requested review from
AlfredoG87,
Nana-EC,
a team and
lukelee-sl
as code owners
| @@ -145,7 +145,7 @@ export default { | |||
| // @ts-ignore | |||
| HBAR_RATE_LIMIT_DURATION: parseInt(ConfigService.get('HBAR_RATE_LIMIT_DURATION') || '86400000'), // 1 day | |||
| // @ts-ignore | |||
| HBAR_RATE_LIMIT_TOTAL: BigNumber(ConfigService.get('HBAR_RATE_LIMIT_TINYBAR') || '800000000000'), // 8000 HBARs | |||
| HBAR_RATE_LIMIT_TOTAL: BigNumber(ConfigService.get('HBAR_RATE_LIMIT_TINYBAR') ?? '800000000000'), // 8000 HBARs | |||
There was a problem hiding this comment.
Q: why this change and does it affect all the other configs?
There was a problem hiding this comment.
While working on the acceptance test I noticed that the zero value for the HBAR_RATE_LIMIT_TINYBAR was not getting picked up. Most likely because the logical OR operator || returns the first truthy value and 0 is falsy. The nullish coalescing operator ?? falls back to the default value when the left-hand operand is null or undefined, not when it's 0 or any other falsy value.
| @@ -90,6 +96,10 @@ export class HbarLimitService implements IHbarLimitService { | |||
| this.reset = this.getResetTimestamp(); | |||
| this.remainingBudget = this.totalBudget; | |||
|
|
|||
| if (this.remainingBudget.toTinybars().isZero()) { | |||
There was a problem hiding this comment.
nit: as a guard I'd say zero or less
| ); | ||
| it('should return false if the rate limiter is disabled by setting HBAR_RATE_LIMIT_TINYBAR to zero', async function () { | ||
| // hbarLimitServiceDisabled.disableRateLimiter(); | ||
| const result = await hbarLimitServiceDisabled.shouldLimit( |
There was a problem hiding this comment.
This actually doesn't really test that the rate limiter is working.
For instance if a future code change was made and the return was false for some other reason that a 0 value this logic would not catch it.
I think we should reconsider the layout on the product side.
There was a problem hiding this comment.
Updated test to include a condition that would normally trigger rate limiting, but verifies that shouldLimit returns false since rate limiting is not enabled.
| @@ -181,6 +191,10 @@ export class HbarLimitService implements IHbarLimitService { | |||
| requestDetails: RequestDetails, | |||
| estimatedTxFee: number = 0, | |||
| ): Promise<boolean> { | |||
| if (this.disableRateLimiter) { | |||
There was a problem hiding this comment.
I would suggest adding an isEnabled() function to IHbarLimitService.
Then this method can call it and it can be tested.
| @@ -887,5 +914,77 @@ describe('@hbarlimiter HBAR Limiter Acceptance Tests', function () { | |||
| }); | |||
| }); | |||
| }); | |||
|
|
|||
| describe('@hbarlimiter-batch3 BASIC Tier', () => { | |||
There was a problem hiding this comment.
nit: not related to PR focus but thanks for adding more coverage
There was a problem hiding this comment.
I had to add it to another batch because of the order of loading environment variables in the acceptance tests. The relay.ts reads and sets the HBAR_RATE_LIMIT_TINYBAR at boot time.
There was a problem hiding this comment.
I believe he is addressing the scenario in which the limiter is turned off, allowing an account to make calls without being HBAR rate limited.
@ebadiere, if that's true and the suite is needed, I suggest renaming the test suite from "BASIC Tier" to something more descriptive, such as "Unlimited Limit" or "Limiter Off" or something like that. The term "BASIC Tier" is confusing, as we have another suite with the same name.
There was a problem hiding this comment.
@quiet-node Done.
| @@ -887,5 +914,77 @@ describe('@hbarlimiter HBAR Limiter Acceptance Tests', function () { | |||
| }); | |||
| }); | |||
| }); | |||
|
|
|||
| describe('@hbarlimiter-batch3 BASIC Tier', () => { | |||
There was a problem hiding this comment.
I believe he is addressing the scenario in which the limiter is turned off, allowing an account to make calls without being HBAR rate limited.
@ebadiere, if that's true and the suite is needed, I suggest renaming the test suite from "BASIC Tier" to something more descriptive, such as "Unlimited Limit" or "Limiter Off" or something like that. The term "BASIC Tier" is confusing, as we have another suite with the same name.
trigger rate limiting, and added isEnabled() method. Signed-off-by: Eric Badiere <[email protected]>
Signed-off-by: Eric Badiere <[email protected]>
Signed-off-by: Eric Badiere <[email protected]>
Signed-off-by: Eric Badiere <[email protected]>
the workflow Signed-off-by: Eric Badiere <[email protected]>
| const basicPlans = await hbarSpendingPlanRepository.findAllActiveBySubscriptionTier( | ||
| [SubscriptionTier.BASIC], | ||
| requestDetails, | ||
| ); |
There was a problem hiding this comment.
Doesn't seem like this is used anywhere
There was a problem hiding this comment.
Correct, thanks. Removed.
| const neededAccounts: number = 1; | ||
| accounts.push( | ||
| ...(await Utils.createMultipleAliasAccounts( | ||
| mirrorNode, | ||
| initialAccount, | ||
| neededAccounts, | ||
| initialBalance, | ||
| requestDetails, | ||
| )), | ||
| ); |
There was a problem hiding this comment.
looks like you created an extra account here but in the test below you use the initialAccount, accounts[0]. Guess we don't need to create extra account here
There was a problem hiding this comment.
Yes. Removed and just using global.accounts[0]. Thanks.
| isEnabled(): boolean { | ||
| return !this.disableRateLimiter; | ||
| } | ||
|
|
There was a problem hiding this comment.
looks like you attempted to add this for testing purposes, right? I guess we can remove it and can only use disableRateLimiter to determine if it should throw limit or not
There was a problem hiding this comment.
This was recommended by @Nana-EC in one of the earlier comments. It makes sense to me.
There was a problem hiding this comment.
Yeah, I think it's cleaner interface wise also since the service isn't required to be on. I'm open to debate
| if (!this.isEnabled()) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
| if (!this.isEnabled()) { | |
| return false; | |
| } | |
| if (this.disableRateLimiter) { | |
| return false; | |
| } |
| @@ -37,6 +37,12 @@ export class HbarLimitService implements IHbarLimitService { | |||
| PRIVILEGED: Hbar.fromTinybars(constants.HBAR_RATE_LIMIT_PRIVILEGED), | |||
| }; | |||
|
|
|||
| /** | |||
| * Disables the rate limiter. | |||
There was a problem hiding this comment.
This is quite misleading. It doesn't actually disables the limiter but it's more like a flag to turn the limiter on and off, right?
There was a problem hiding this comment.
Updated comments to be more descriptive.
| @@ -31,4 +31,5 @@ export interface IHbarLimitService { | |||
| estimatedTxFee?: number, | |||
| ): Promise<boolean>; | |||
| addExpense(cost: number, ethAddress: string, requestDetails: RequestDetails, ipAddress?: string): Promise<void>; | |||
| isEnabled(): boolean; | |||
There was a problem hiding this comment.
It was recommended by @Nana-EC
Signed-off-by: Eric Badiere <[email protected]> fix: clean up of experimental code. Signed-off-by: Eric Badiere <[email protected]>
ebadiere
force-pushed
the
3251-habr-rate-limiter-can-be-disabled
branch
from
240a06c
to
1938973
Compare
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3252 +/- ##
==========================================
+ Coverage 77.85% 78.40% +0.55%
==========================================
Files 66 69 +3
Lines 4453 4627 +174
Branches 993 1040 +47
==========================================
+ Hits 3467 3628 +161
- Misses 613 640 +27
+ Partials 373 359 -14
Flags with carried forward coverage won't be shown. Click here to find out more.
|
This fix allows the HBar Rate Limiter to be disabled by setting the
HBAR_RATE_LIMIT_TINYBARto0Related issue(s):
Fixes #3251
Notes for reviewer:
Checklist