Skip to content
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

test: create e2e tests for basic spending plan limit #3094

Open
wants to merge 52 commits into
base: main
Choose a base branch
from

Conversation

konstantinabl
Copy link
Collaborator

@konstantinabl konstantinabl commented Oct 11, 2024

Description:

This PR adds acceptance tests to verify the scenarios listed in the issue that is linked

Related issue(s):

Fixes #3065

Notes for reviewer:

Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com>
Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com>
Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com>
Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com>
Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com>
Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com>
Some queries like getAccountInfo, getBalanceInfo, FileContentsQuery, etc. also add expense to remainingBalance but don't necessarily need to have an originalCaller (ethAddress).

Therefore, addExpense can accept nullable ethAddress value.

Only when ethAddress or ipAddress is valid, utilize spendingPlan logic. Otherwise, skip completely.

Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com>
Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com>
…and IExecuteQueryEventPayload

Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com>
Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com>
Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com>
Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com>
Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com>
Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com>
Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com>
Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com>
Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com>
Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com>
Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com>
Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com>
This reverts commit fd9e119 and updated related tests

Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com>
Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com>
Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com>
Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com>
This reverts commit e45c321.

Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com>
Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com>
Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com>
Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com>
Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com>
Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com>
Copy link

github-actions bot commented Oct 12, 2024

🚨 Memory Leak Detected 🚨

A potential memory leak has been detected in the test titled validates enforcement of request id. This may impact the application's performance and stability.

Details

📊 Memory Leak Detection Report 📊

GC Type: MarkSweepCompact
Cost: 25,194.7 ms

Heap Statistics (before vs after executing the test):

  • Total Heap Size: increased with 1.46 MB
  • Total Heap Size Executable: no changes
  • Total Physical Size: decreased with 348.16 KB
  • Total Available Size: decreased with 5.48 MB
  • Total Global Handles Size: no changes
  • Used Global Handles Size: decreased with 64.00 bytes
  • Used Heap Size: decreased with 3.48 MB
  • Heap Size Limit: no changes
  • Malloced Memory: no changes
  • External Memory: no changes
  • Peak Malloced Memory: no changes

Heap Space Statistics (before vs after executing the test):

  • Old Space:

    • Space Size: increased with 1.84 MB
    • Space Used Size: increased with 2.03 MB
    • Space Available Size: decreased with 8.57 MB
    • Physical Space Size: increased with 1.84 MB
  • Large Object Space:

    • Space Size: increased with 835.58 KB
    • Space Used Size: increased with 813.50 KB
    • Space Available Size: no changes
    • Physical Space Size: increased with 835.58 KB

Recommendations

Please investigate the memory allocations in this test, focusing on objects that are not being properly deallocated.

@quiet-node quiet-node force-pushed the 3065-2.0-create-e2e-tests-for-basic-spending-plan-limit branch from 504eedb to 6d559c8 Compare October 12, 2024 01:44
Copy link

github-actions bot commented Oct 12, 2024

Tests

       3 files     397 suites   18s ⏱️
1 412 tests 1 411 ✔️ 1 💤 0
1 421 runs  1 420 ✔️ 1 💤 0

Results for commit ccdf203.

♻️ This comment has been updated with latest results.

@quiet-node quiet-node force-pushed the 3065-2.0-create-e2e-tests-for-basic-spending-plan-limit branch from 6d559c8 to 2d6eb18 Compare October 12, 2024 02:23
Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com>
Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com>
Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com>
@quiet-node quiet-node force-pushed the 3065-2.0-create-e2e-tests-for-basic-spending-plan-limit branch 2 times, most recently from 203f001 to 8f70db4 Compare October 12, 2024 02:49
Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com>
@quiet-node quiet-node force-pushed the 3065-2.0-create-e2e-tests-for-basic-spending-plan-limit branch from 22e4ca3 to 69192f0 Compare October 12, 2024 03:18
const remainingHbars = Number(await metrics.get(testConstants.METRICS.REMAINING_HBAR_LIMIT));

// Check that the BASIC spending plan is created after the first deployment only
if (i === 0) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@quiet-node Why is this deleted? I wanted to have this check here to ensure we create a basic plan as intended?

}
const hbarPlan = await hbarSpendingPlanRepository.findByIdWithDetails(spendingPlanId, requestDetails);

expect(hbarPlan.amountSpent).to.be.gt(spentToday);
Copy link
Collaborator Author

@konstantinabl konstantinabl Oct 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@quiet-node And why remove this? I think it is still separate than what you are doing in the catch?
I want to ensure that in each call we calculate the amount spent correctly and we increase it and there was a problem with amount spent actually that i wanted to investigate?

konstantinabl and others added 4 commits October 14, 2024 14:23
Signed-off-by: Konstantina Blazhukova <konstantina.blajukova@gmail.com>
Signed-off-by: Konstantina Blazhukova <konstantina.blajukova@gmail.com>
Signed-off-by: Konstantina Blazhukova <konstantina.blajukova@gmail.com>
Signed-off-by: Victor Yanev <victor.yanev@limechain.tech>

it('should eventually exhaust the hbar limit for a BASIC user after multiple deployments of large contracts', async function () {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@victor-yanev why is this whole test deleted??

[SubscriptionTier.BASIC],
requestDetails,
);
for (const plan of basicPlans) {
Copy link
Collaborator Author

@konstantinabl konstantinabl Oct 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@victor-yanev Why delete all the plans :?

@quiet-node quiet-node force-pushed the 3065-2.0-create-e2e-tests-for-basic-spending-plan-limit branch from 6cb081b to 2396bea Compare October 14, 2024 21:10
Signed-off-by: Konstantina Blazhukova <konstantina.blajukova@gmail.com>
Signed-off-by: Konstantina Blazhukova <konstantina.blajukova@gmail.com>
Signed-off-by: Konstantina Blazhukova <konstantina.blajukova@gmail.com>
Signed-off-by: Konstantina Blazhukova <konstantina.blajukova@gmail.com>
@konstantinabl konstantinabl force-pushed the 3065-2.0-create-e2e-tests-for-basic-spending-plan-limit branch from 2396bea to ccdf203 Compare October 14, 2024 21:52
Copy link

sonarcloud bot commented Oct 14, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

Copy link

codecov bot commented Oct 14, 2024

Codecov Report

Attention: Patch coverage is 78.18182% with 12 lines in your changes missing coverage. Please review.

Project coverage is 82.66%. Comparing base (1c8579b) to head (ccdf203).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...hbarLimiter/ipAddressHbarSpendingPlanRepository.ts 0.00% 10 Missing ⚠️
packages/relay/src/lib/clients/sdkClient.ts 90.90% 1 Missing ⚠️
packages/relay/src/lib/constants.ts 66.66% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3094      +/-   ##
==========================================
- Coverage   83.16%   82.66%   -0.51%     
==========================================
  Files          63       62       -1     
  Lines        4236     4187      -49     
  Branches      830      821       -9     
==========================================
- Hits         3523     3461      -62     
- Misses        470      483      +13     
  Partials      243      243              
Flag Coverage Δ
relay 84.99% <78.18%> (-0.61%) ⬇️
server 83.48% <ø> (ø)
ws-server 33.91% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
packages/relay/src/lib/relay.ts 87.32% <100.00%> (+0.18%) ⬆️
.../relay/src/lib/services/hapiService/hapiService.ts 80.00% <100.00%> (ø)
...s/relay/src/lib/services/hbarLimitService/index.ts 95.79% <100.00%> (-1.69%) ⬇️
...ay/src/lib/services/metricService/metricService.ts 92.68% <100.00%> (+0.18%) ⬆️
packages/relay/src/utils.ts 100.00% <100.00%> (ø)
packages/relay/src/lib/clients/sdkClient.ts 58.22% <90.90%> (-0.19%) ⬇️
packages/relay/src/lib/constants.ts 90.56% <66.66%> (-1.44%) ⬇️
...hbarLimiter/ipAddressHbarSpendingPlanRepository.ts 75.51% <0.00%> (-19.37%) ⬇️

): number {
const fileCreateTransactions = 1;
const fileCreateFeeInCents = constants.NETWORK_FEES_IN_CENTS.FILE_CREATE_PER_5_KB;

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to validate that fileChunkSize is not a zero value to prevent divide by zero issues in the next line?

  if (fileChunkSize <= 0) {
    throw new Error('fileChunkSize must be greater than zero.');
  }

const fileCreateFeeInCents = constants.NETWORK_FEES_IN_CENTS.FILE_CREATE_PER_5_KB;

// The first chunk goes in with FileCreateTransaciton, the rest are FileAppendTransactions
const fileAppendTransactions = Math.floor(callDataSize / fileChunkSize) - 1;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps I am missing something here, but I think we want to use Math.ceil here instead of .floor Using .floor will round down to the nearest integer, possibly missing the last chunk. .ceil will round up to the nearest integer catching the last chunk even if the callDataSize is not an exact multiple of the fileChunkSize.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[HBAR Rate Limit Redesign] Create e2e tests for basic spending plan limit
4 participants