-
Notifications
You must be signed in to change notification settings - Fork 727
feat: rate limit module #8268
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: rate limit module #8268
Conversation
641539e
to
26cc7cd
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8268 +/- ##
==========================================
+ Coverage 1.13% 57.57% +56.43%
==========================================
Files 14 317 +303
Lines 2019 22673 +20654
==========================================
+ Hits 23 13053 +13030
- Misses 1995 9012 +7017
- Partials 1 608 +607
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
0013e4f
to
defbff0
Compare
6cec140
to
d8959b3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Starting to come together, but looks like we're not quite done yet :)
k.transferKeeper = transferKeeper | ||
} | ||
|
||
// SetICS4Wrapper sets the ICS4 Wrapper to pass packets downstream. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain why this was added now, and why it worked without it in the past?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I understand this PR may not be the best place to add this, but the reason was, I wanted to all the middleware to have similar setup style.
Something like,
RateLimit(PFM(Transfer(Chan)))
then
RateLim.ICS4(Chan)
PFM.ICS4(RateLim)
Transfer.ICS4(PFM)
But all the functionality still works if we remove this function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not add it if it's not used now, it will be added in #8528
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed here 83e7ce2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aparently this is required here https://github.com/cosmos/ibc-go/blob/feature/rate-limit/modules/apps/rate-limiting/keeper/packet_test.go#L743
So I am re introducing SetICS4Wrapper
for PFM
eaf5d3c
to
fd27174
Compare
e16a2d6
to
4378fa5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
YES! Now I think we are there 💪
I'll merge this now, and after we get ibc v2 e2e tests in, I will ask @AdityaSripal to review both middlewares holistically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces a new rate-limiting module to the IBC-go codebase. Key changes include new middleware and keeper implementations for enforcing throughput limits on IBC transfers, comprehensive proto definitions for the rate limit APIs, and extensive unit and integration tests ensuring proper behavior.
Reviewed Changes
Copilot reviewed 63 out of 72 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
modules/apps/rate-limiting/types/keys.go | Defines key prefixes used for storing module state; review of prefix value |
modules/apps/rate-limiting/keeper/*.go | New keeper logic with repeated KV store adapter instantiation for state access |
Comments suppressed due to low confidence (1)
modules/apps/rate-limiting/types/keys.go:30
- The key prefix for whitelisted addresses is set to 'address-blacklist', which appears inconsistent with its purpose. Consider renaming it to 'address-whitelist' to accurately reflect its usage.
AddressWhitelistKeyPrefix = bytes("address-blacklist")
Description
ref: #7974 [Github]
closes: IBCGO-11 [Linear]
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/
).godoc
comments.Files changed
in the GitHub PR explorer.SonarCloud Report
in the comment section below once CI passes.