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

feat: Implement HbarLimitService#addExpense #2902

Merged
merged 12 commits into from
Sep 2, 2024

Conversation

victor-yanev
Copy link
Contributor

@victor-yanev victor-yanev commented Aug 29, 2024

Description

This PR implements the addExpense method in the HbarLimitService class and adds corresponding unit tests. The changes include:

  • Implementing the addExpense method to handle expense addition for ETH addresses.
  • Adding unit tests for the addExpense method to ensure correct functionality.
  • Adding unit tests for other methods in HbarLimitService to improve test coverage.

Changes

  1. Implementation of addExpense Method:

    • Added logic to create a basic spending plan if none exists for the provided ETH or IP address.
    • Updated the remaining budget and spending history after adding an expense.
    • Added error handling for cases where neither ETH nor IP address is provided.
  2. Unit Tests:

    • Added tests for the addExpense method to cover various scenarios, including:
      • Adding an expense when both ETH and IP addresses are provided.
      • Adding an expense when only ETH address is provided.
      • Handling errors when adding an expense fails.
    • Added tests for other methods in HbarLimitService to ensure comprehensive test coverage.

Related Issues

Fixes #2901

Notes for Reviewer

  • Ensure to implement the remaining TODOs related to IP address handling in future PRs.
  • Review the added unit tests to confirm they cover all edge cases and scenarios.

@victor-yanev victor-yanev added enhancement New feature or request P1 labels Aug 29, 2024
@victor-yanev victor-yanev added this to the 0.55.0 milestone Aug 29, 2024
@victor-yanev victor-yanev self-assigned this Aug 29, 2024
Copy link

github-actions bot commented Aug 29, 2024

Tests

       3 files     263 suites   20s ⏱️
1 232 tests 1 231 ✔️ 1 💤 0
1 244 runs  1 243 ✔️ 1 💤 0

Results for commit af07187.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Aug 29, 2024

Acceptance Tests

  21 files  304 suites   32m 24s ⏱️
597 tests 576 ✔️ 4 💤 17
979 runs  955 ✔️ 6 💤 18

Results for commit af07187.

♻️ This comment has been updated with latest results.

@victor-yanev victor-yanev requested a review from a team August 29, 2024 13:22
Copy link
Collaborator

@ebadiere ebadiere left a comment

Choose a reason for hiding this comment

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

A couple of nits, but I have a more general question.

I assume based on the description of issue #2896 that the plan will be to simply swap out the old HBar Rate Limiter with the new HBar Rate Limiter in the sdkClient.ts. If that is the case what happens if the Tier 3 general users are limited to 10K HBar per day, and a given partner is limited to 20K per day, and the general users use up all of the 10K budget before the partner has used up all of their allocated 20K. Is the partner rate limited as well, even though the partner may have only used up 12K Hbar that day?

Should we have a test for this?

@victor-yanev
Copy link
Contributor Author

A couple of nits, but I have a more general question.

I assume based on the description of issue #2896 that the plan will be to simply swap out the old HBar Rate Limiter with the new HBar Rate Limiter in the sdkClient.ts. If that is the case what happens if the Tier 3 general users are limited to 10K HBar per day, and a given partner is limited to 20K per day, and the general users use up all of the 10K budget before the partner has used up all of their allocated 20K. Is the partner rate limited as well, even though the partner may have only used up 12K Hbar that day?

Should we have a test for this?

I have also been thinking about this, but I couldn't come up with something else than eliminating the total daily limit and only keeping the tiered limits per user/partner, let's discuss it on the parking lot today.

Signed-off-by: Victor Yanev <[email protected]>
Signed-off-by: Victor Yanev <[email protected]>

# Conflicts:
#	package-lock.json
Copy link

sonarcloud bot commented Aug 30, 2024

Copy link

🚨 Memory Leak Detected 🚨

A potential memory leak has been detected in the test titled should execute "eth_chainId". This may impact the application's performance and stability.

Details

📊 Memory Leak Detection Report 📊

GC Type: Scavenge
Cost: 4,112.56 ms

Heap Statistics (before vs after executing the test):

  • Total Heap Size: increased with 2.71 MB
  • Total Heap Size Executable: no changes
  • Total Physical Size: increased with 2.71 MB
  • Total Available Size: increased with 12.62 MB
  • Total Global Handles Size: no changes
  • Used Global Handles Size: decreased with 352.00 bytes
  • Used Heap Size: decreased with 12.00 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 2.88 MB
    • Space Used Size: increased with 2.10 MB
    • Space Available Size: increased with 732.09 KB
    • Physical Space Size: increased with 2.88 MB
  • Large Object Space:

    • Space Size: increased with 507.90 KB
    • Space Used Size: increased with 496.30 KB
    • Space Available Size: no changes
    • Physical Space Size: increased with 507.90 KB

Recommendations

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

Copy link
Collaborator

@Ivo-Yankov Ivo-Yankov left a comment

Choose a reason for hiding this comment

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

LGTM

@victor-yanev victor-yanev merged commit d0b0851 into main Sep 2, 2024
45 checks passed
@victor-yanev victor-yanev deleted the 2901-Implement-add-expense branch September 2, 2024 09:59
Copy link

codecov bot commented Sep 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.71%. Comparing base (b03ed7a) to head (af07187).
Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2902      +/-   ##
==========================================
+ Coverage   83.48%   83.71%   +0.22%     
==========================================
  Files          55       55              
  Lines        3676     3715      +39     
  Branches      765      770       +5     
==========================================
+ Hits         3069     3110      +41     
+ Misses        367      365       -2     
  Partials      240      240              
Flag Coverage Δ
relay 83.63% <100.00%> (+0.28%) ⬆️
server 83.05% <ø> (ø)
ws-server 97.87% <ø> (ø)

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

Files with missing lines Coverage Δ
...s/relay/src/lib/services/hbarLimitService/index.ts 92.59% <100.00%> (+11.64%) ⬆️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request P1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[HBAR Rate Limit Redesign] Implement addExpense
3 participants