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.

Sign up for GitHub

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

Jump to bottom

Add Rate Limit Support #481

Merged
merged 3 commits into from
Sep 17, 2022
Merged

Add Rate Limit Support #481

merged 3 commits into from
Sep 17, 2022

Conversation

georgi-l95
Copy link
Collaborator

@georgi-l95 georgi-l95 commented Aug 30, 2022 •
edited
Loading

Description:
Adds application level rate limiting with support for method based rate limiting with a tighter limit on relay account costing methods. Custom solution implemented in koa-jsonrpc module.

Related issue(s):

Fixes #426

Notes for reviewer:
Some rate limit values in methodConfiguration module, perhaps should be modified.

Checklist

  • Documented (Code comments, README, etc.)
  • Tested (unit, integration, etc.)

@georgi-l95 georgi-l95 self-assigned this Aug 30, 2022
@georgi-l95 georgi-l95 linked an issue Aug 30, 2022 that may be closed by this pull request
Add rate limit support #426
Closed
@georgi-l95 georgi-l95 added this to the 0.8.0 milestone Aug 30, 2022
@georgi-l95 georgi-l95 added enhancement New feature or request limechain P2 labels Aug 30, 2022
@georgi-l95 georgi-l95 marked this pull request as ready for review August 30, 2022 15:21
Nana-EC
Nana-EC requested changes Aug 31, 2022
View reviewed changes
Copy link
Collaborator

@Nana-EC Nana-EC left a comment

Choose a reason for hiding this comment

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

Nice progress.
Please add documentation, .env level configuration and streamline the rate limiting logic removing parts that aren't necessary and capturing in a future ticket.
Also note HBAR comment

packages/server/src/koaJsonRpc/lib/RpcError.ts Outdated Show resolved Hide resolved
packages/server/src/koaJsonRpc/index.ts Outdated Show resolved Hide resolved
packages/server/src/koaJsonRpc/lib/methodConfiguration.ts Show resolved Hide resolved
packages/server/src/koaJsonRpc/lib/methodConfiguration.ts Outdated Show resolved Hide resolved
packages/server/src/ratelimit/index.ts Show resolved Hide resolved
packages/server/src/ratelimit/index.ts Show resolved Hide resolved
packages/server/tests/acceptance/rpc.spec.ts Outdated Show resolved Hide resolved
packages/server/src/koaJsonRpc/index.ts Show resolved Hide resolved
@georgi-l95 georgi-l95 marked this pull request as draft September 1, 2022 11:17
@codecov-commenter
Copy link

codecov-commenter commented Sep 2, 2022 •
edited
Loading

Codecov Report

Base: 76.38% // Head: 76.38% // No change to project coverage 👍

Coverage data is based on head (e1e84fb) compared to base (c3d3fb8).
Patch has no changes to coverable lines.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main     #481   +/-   ##
=======================================
  Coverage   76.38%   76.38%           
=======================================
  Files          12       12           
  Lines         923      923           
  Branches      144      144           
=======================================
  Hits          705      705           
  Misses        165      165           
  Partials       53       53
Impacted Files Coverage Δ
packages/relay/src/lib/errors/JsonRpcError.ts 75.00% <ø> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@georgi-l95
some progress
3478c49 
Signed-off-by: georgi-l95 <glazarov95@gmail.com>

ts fix

Signed-off-by: georgi-l95 <glazarov95@gmail.com>

add test

Signed-off-by: georgi-l95 <glazarov95@gmail.com>

fix test and code smell

Signed-off-by: georgi-l95 <glazarov95@gmail.com>

remove comments and auth support

Signed-off-by: georgi-l95 <glazarov95@gmail.com>

test

Signed-off-by: georgi-l95 <glazarov95@gmail.com>

test

Signed-off-by: georgi-l95 <glazarov95@gmail.com>

test

Signed-off-by: georgi-l95 <glazarov95@gmail.com>

test

Signed-off-by: georgi-l95 <glazarov95@gmail.com>

test

Signed-off-by: georgi-l95 <glazarov95@gmail.com>

test

Signed-off-by: georgi-l95 <glazarov95@gmail.com>

test

Signed-off-by: georgi-l95 <glazarov95@gmail.com>

Exposed limit configuration and added comment

Signed-off-by: georgi-l95 <glazarov95@gmail.com>

add comment on test

Signed-off-by: georgi-l95 <glazarov95@gmail.com>

add rate limit docs

Signed-off-by: georgi-l95 <glazarov95@gmail.com>

revert some changes

Signed-off-by: georgi-l95 <glazarov95@gmail.com>
@georgi-l95
add one more test
0eea7b9 
Signed-off-by: georgi-l95 <glazarov95@gmail.com>
@georgi-l95
revert
e1e84fb 
Signed-off-by: georgi-l95 <glazarov95@gmail.com>
@sonarcloud
Copy link

sonarcloud bot commented Sep 15, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@georgi-l95 georgi-l95 marked this pull request as ready for review September 15, 2022 12:35
@Nana-EC
Copy link
Collaborator

Nana-EC commented Sep 16, 2022

@rustyShacklefurd heads up as this contains some additional configs.
Defaults are probably okay but please advise on any concerns regarding ranges or deployment impacts

Nana-EC
Nana-EC reviewed Sep 16, 2022
View reviewed changes
packages/server/src/server.ts
const relay: Relay = new RelayImpl(logger, register);
const cors = require('koa-cors');
const app = new Koa();
const rpc = koaJsonRpc();
const rpc = new KoaJsonRpc();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does using the custom implementation make any of the controller logic in this file moveable?
That is should anything here move to koaJsonRPc/index.ts?
Don't have to do it in this PR but it's be good to know so we track that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think that's doable. I've opened a separate issue to take care of this. Issue #530

Nana-EC
Nana-EC approved these changes Sep 16, 2022
View reviewed changes
Copy link
Collaborator

@Nana-EC Nana-EC left a comment

Choose a reason for hiding this comment

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

LG

@georgi-l95 georgi-l95 merged commit bed9aa9 into main Sep 17, 2022
@georgi-l95 georgi-l95 deleted the 426-add-rate-limit-support branch September 17, 2022 14:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Nana-EC Nana-EC Nana-EC approved these changes

@natanasow natanasow Awaiting requested review from natanasow

@lukelee-sl lukelee-sl Awaiting requested review from lukelee-sl lukelee-sl is a code owner

@shemnon shemnon Awaiting requested review from shemnon

@rustyShacklefurd rustyShacklefurd Awaiting requested review from rustyShacklefurd

Assignees

@georgi-l95 georgi-l95

Labels
enhancement New feature or request limechain P2
Projects
None yet
Milestone
0.8.0
Development

Successfully merging this pull request may close these issues.

Add rate limit support
3 participants
@georgi-l95 @codecov-commenter @Nana-EC