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

[CORE-834] Wire x/ratelimit methods to IBC transfer logic #962

Merged
merged 8 commits into from
Jan 26, 2024

Conversation

teddyding
Copy link
Contributor

@teddyding teddyding commented Jan 12, 2024

Changelist

  • Implement x/ratelimit IBC Middleware which wraps around native IBC transfer module
  • Implement additional keeper methods for dealing with IBC packets

The overall architecture of the IBC middleware resembles closely to Stride's x/ratelimit implementation. The x/ratelimit module spec includes a high level summary.

Many functions were re-used or re-written from Stride implementation. I left TODO items to for giving generally better attributions.

Test Plan

Some unit tests.
More tests to come in below tickets:

Author/Reviewer Checklist

  • If this PR has changes that result in a different app state given the same prior state and transaction list, manually add the state-breaking label.
  • If the PR has breaking postgres changes to the indexer add the indexer-postgres-breaking label.
  • If this PR isn't state-breaking but has changes that modify behavior in PrepareProposal or ProcessProposal, manually add the label proposal-breaking.
  • If this PR is one of many that implement a specific feature, manually label them all feature:[feature-name].
  • If you wish to for mergify-bot to automatically create a PR to backport your change to a release branch, manually add the label backport/[branch-name].
  • Manually add any of the following labels: refactor, chore, bug.

Copy link
Contributor

coderabbitai bot commented Jan 12, 2024

Walkthrough

The recent update introduces a rate-limiting module to an existing blockchain protocol, enhancing the control over transaction flow within the network. It integrates the module within the application logic, extends the IBC middleware for packet handling with rate-limiting features, and updates the keeper logic to manage these new functionalities. Utility functions and tests are added to support parsing and handling IBC packets, while new message types and constants are introduced to facilitate internal communication and metrics within the rate-limiting context.

Changes

Files Change Summary
protocol/app/app.go Integrated ratelimitmodule with capabilities, created RatelimitKeeper, and applied middleware to IBC Transfer module.
protocol/x/ratelimit/ibc_middleware.go, protocol/x/ratelimit/types/expected_keepers.go Added IBC middleware for rate limiting, including lifecycle event handling and packet processing. Expanded ICS4Wrapper interface for middleware.
protocol/x/ratelimit/keeper/keeper.go, protocol/x/ratelimit/keeper/package_test.go, protocol/x/ratelimit/keeper/packet.go, protocol/x/ratelimit/util/ibc.go, protocol/x/ratelimit/util/ibc_test.go Enhanced keeper functionality with ics4Wrapper, added pending packet tests, and introduced utility functions for IBC packet processing.
protocol/x/ratelimit/types/ibc.go, protocol/x/ratelimit/types/keys.go Defined types and constants for IBC packet handling and key generation functions.
protocol/x/ratelimit/util/capacity_test.go, protocol/x/ratelimit/util/parse_denom.go, protocol/x/ratelimit/util/parse_denom_test.go Renamed capacity test function, added denomination parsing utilities, and associated tests for IBC packets.
protocol/lib/ante/internal_msg.go, protocol/lib/metrics/constants.go Added new internal message types for rate limiting and introduced a new constant UndoWithdrawAmount.

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share

Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit-tests for this file.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit tests for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository from git and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit tests.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • The JSON schema for the configuration file is available here.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

CodeRabbit Discord Community

Join our Discord Community to get help, request features, and share feedback.

@teddyding teddyding force-pushed the td/ratelimit-ibc branch 2 times, most recently from a21c886 to 1b010c3 Compare January 12, 2024 19:49
@teddyding teddyding changed the title wip [CORE-834] Wire x/ratelimit methods to IBC transfer logic Jan 12, 2024
Copy link

linear bot commented Jan 12, 2024

@teddyding teddyding marked this pull request as ready for review January 12, 2024 19:56
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 3

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between e0fd276 and 1b010c3.
Files selected for processing (10)
  • protocol/app/app.go (5 hunks)
  • protocol/x/ratelimit/ibc_middleware.go (1 hunks)
  • protocol/x/ratelimit/keeper/keeper.go (3 hunks)
  • protocol/x/ratelimit/keeper/package_test.go (1 hunks)
  • protocol/x/ratelimit/keeper/packet.go (1 hunks)
  • protocol/x/ratelimit/types/expected_keepers.go (2 hunks)
  • protocol/x/ratelimit/types/ibc.go (1 hunks)
  • protocol/x/ratelimit/types/keys.go (2 hunks)
  • protocol/x/ratelimit/util/ibc.go (1 hunks)
  • protocol/x/ratelimit/util/ibc_test.go (1 hunks)
Additional comments: 38
protocol/x/ratelimit/types/keys.go (3)
  • 3-3: The addition of the "fmt" package import is necessary for the new function GetPendingSendPacketKey which uses fmt.Sprintf.

  • 20-21: The new constant PendingSendPacketPrefix is introduced for key-value store prefixing, which is a common pattern for namespacing in key-value stores to avoid collisions.

  • 27-29: The new function GetPendingSendPacketKey generates a key for a pending send packet. It uses the fmt.Sprintf function to concatenate the channel ID and sequence number with an underscore. This is a simple and effective way to create unique keys for each packet.

protocol/x/ratelimit/types/ibc.go (4)
  • 5-10: The PacketDirection type is defined as an int32 which is a common practice for enum-like types in Go. This will be used to indicate the direction of an IBC packet.

  • 12-18: The IBCTransferPacketInfo struct is well-defined with clear field names indicating the information it holds about an IBC transfer packet. This will be useful for rate-limiting logic that needs to know about the packet's channel, denomination, and amount.

  • 20-28: The AckResponseStatus type and its associated constants provide a clear way to represent the status of an IBC packet acknowledgment. This is a good use of Go's iota for creating a series of related constants.

  • 30-34: The AcknowledgementResponse struct is introduced to encapsulate the status and potential error of an IBC transfer packet acknowledgment. This is a straightforward approach to handling acknowledgments.

protocol/x/ratelimit/types/expected_keepers.go (2)
  • 8-9: The addition of capabilitytypes and clienttypes imports is necessary for the new ICS4Wrapper interface, which uses types from these packages.

  • 22-40: The ICS4Wrapper interface is introduced with methods that are essential for interacting with IBC packets. This interface will be implemented by the middleware to handle packet sending and acknowledgments. The methods are well-defined and include all necessary parameters for IBC operations.

protocol/x/ratelimit/keeper/package_test.go (1)
  • 10-50: The test TestPendingPacket is added to ensure the correct behavior of the pending packet functionality. It tests setting, checking, and removing pending packets. The use of require from the testify package is appropriate for asserting conditions in tests.
protocol/x/ratelimit/ibc_middleware.go (7)
  • 23-28: The IBCMiddleware struct is defined with fields for the underlying IBC module and the keeper. This struct will act as middleware for IBC, handling channel lifecycle events and packet processing.

  • 30-36: The NewIBCMiddleware function is correctly implemented to initialize the IBCMiddleware struct. It takes a keeper and an IBC module as parameters and returns a new instance of IBCMiddleware.

  • 38-58: The OnChanOpenInit method correctly implements the IBCMiddleware interface, delegating the call to the underlying IBC module. This pattern is consistent across all channel lifecycle event handlers in this file.

  • 141-153: The OnAcknowledgementPacket method handles the acknowledgment of an IBC packet. It calls the keeper's AcknowledgeIBCTransferPacket method and logs an error if it fails. This is a critical part of the IBC packet lifecycle.

  • 155-165: The OnTimeoutPacket method handles the timeout of an IBC packet. It calls the keeper's TimeoutIBCTransferPacket method and logs an error if it fails. This ensures that the state is correctly reverted if a packet times out.

  • 168-188: The SendPacket and WriteAcknowledgement methods implement the ICS4Wrapper interface. They delegate the work to the keeper, which contains the rate-limiting logic. This separation of concerns is good for modularity.

  • 200-203: The GetAppVersion method provides the application version for the underlying application. This is a simple getter method that is part of the ICS4Wrapper interface.

protocol/x/ratelimit/keeper/packet.go (7)
  • 22-35: The RemovePendingSendPacket and SetPendingSendPacket methods are correctly implemented to manage the state of pending IBC packets. They use a prefix store for namespacing and simple byte slices for values.

  • 37-43: The HasPendingSendPacket method checks if a packet sequence number is pending. It uses the same prefix store pattern as the other methods, ensuring consistency in how packet state is managed.

  • 46-75: The AcknowledgeIBCTransferPacket and TimeoutIBCTransferPacket methods handle the acknowledgment and timeout of IBC packets, respectively. They include logic to update the state based on the success or failure of the packet transfer.

  • 91-106: The UndoSendPacket method is idempotent and reverts the capacity decrease that happened during the send if a packet fails or times out. This is an important feature for maintaining correct state in the event of failures.

  • 108-145: The SendPacket method wraps the IBC ChannelKeeper's SendPacket function and includes rate-limiting logic. It ensures that packets are sent according to the rate limits set for the channel.

  • 148-165: The SendRateLimitedPacket method checks if the rate limit has been exceeded before sending the packet. This is a critical part of the rate-limiting functionality.

  • 168-181: The WriteAcknowledgement and GetAppVersion methods wrap corresponding methods from the IBC ChannelKeeper. They are simple pass-through methods that allow the keeper to interact with the IBC module.

protocol/x/ratelimit/util/ibc_test.go (3)
  • 42-95: The tests TestParseDenomFromRecvPacket and TestParseDenomFromSendPacket ensure that the utility functions for parsing denominations from IBC packets work as expected. These tests are comprehensive and cover different scenarios.

  • 130-173: The TestParsePacketInfo test checks the ParsePacketInfo utility function, ensuring it correctly parses packet information for both send and receive directions. This is an important test for verifying packet parsing logic.

  • 176-231: The TestUnpackAcknowledgementResponseForTransfer test ensures that the unpacking of IBC transfer packet acknowledgments works correctly and handles both success and error cases. This is crucial for the correct processing of acknowledgments.

protocol/x/ratelimit/util/ibc.go (4)
  • 21-44: The ParseDenomFromSendPacket function is implemented to parse the denomination from a send packet. It correctly handles both native and non-native tokens, returning the appropriate denomination for rate-limiting purposes.

  • 46-106: The ParseDenomFromRecvPacket function handles parsing the denomination from a receive packet. It accounts for whether the chain is acting as a source or sink and adjusts the denomination accordingly.

  • 109-147: The ParsePacketInfo function extracts information from an IBC packet and constructs an IBCTransferPacketInfo struct. It handles both send and receive directions and performs necessary parsing and conversion of the amount.

  • 150-191: The UnpackAcknowledgementResponseForTransfer function unmarshals acknowledgments for IBC transfers and determines the status. It handles both result and error acknowledgments, providing a structured response.

protocol/x/ratelimit/keeper/keeper.go (3)
  • 29-29: The addition of the ics4Wrapper field to the Keeper struct is necessary for the keeper to interact with the IBC module. This allows the keeper to send packets and write acknowledgments.

  • 43-43: The NewKeeper function is updated to accept an ICS4Wrapper as a parameter, which is then set on the Keeper struct. This change is necessary for the keeper to perform IBC operations.

  • 57-58: The expanded documentation for the ProcessWithdrawal function provides clarity on the error handling. It specifies that an error will result in the transaction failing upstream, which is important for developers to understand the implications of capacity inefficiencies.

protocol/app/app.go (4)
  • 153-153: The import of the ratelimitmodule and its sub-packages is correctly added to integrate the new rate-limiting functionality.

  • 543-549: Capabilities for the new ratelimitmodule are being granted here. It's important to ensure that the capability is correctly scoped and that no other module can accidentally or maliciously claim the same capability.

Verification successful

The verification process has confirmed that the ratelimitmodule capability is scoped uniquely within the codebase, as no other instances of ScopeToModule using ratelimitmoduletypes.ModuleName were found outside of the one in app.go. This addresses the concern raised in the review comment.

Scripts Executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Verify that the ratelimitmodule's capability is unique and not claimed elsewhere.
ast-grep --lang go --pattern $'app.CapabilityKeeper.ScopeToModule("ratelimitmoduletypes.ModuleName")'

Length of output: 102



Script:

#!/bin/bash
# Broaden the search to find any instances of ScopeToModule being used with the ratelimitmodule's capability
rg 'ScopeToModule\(.*ratelimitmoduletypes.ModuleName.*\)'

Length of output: 148

* 596-596: The `TransferKeeper` is being modified to include the `RatelimitKeeper` as an `ICS4Wrapper`. This change is crucial for the middleware to function correctly. It's important to ensure that the `TransferKeeper` is correctly updated everywhere it's used.
Verification successful

---



---



---

The output from the script shows the instantiation of `app.TransferKeeper` with `ibctransferkeeper.NewKeeper`. The parameters passed to this function include `app.RatelimitKeeper`, which is commented as `ICS4Wrapper`. This confirms that the `RatelimitKeeper` is indeed being passed as a parameter where expected in the instantiation of `TransferKeeper`.

Based on this information, the review comment stating that the `TransferKeeper` is being modified to include the `RatelimitKeeper` as an `ICS4Wrapper` is consistent with the codebase. The verification process has confirmed that the `TransferKeeper` is correctly updated in the `app.go` file.



</details>

<details>
<summary>Scripts Executed</summary>

The following scripts were executed for the analysis:



---

Script:
```shell
#!/bin/bash
# Verify that TransferKeeper is updated to include RatelimitKeeper as ICS4Wrapper wherever used.
ast-grep --lang go --pattern $'ibctransferkeeper.NewKeeper($_, $_, $_, app.RatelimitKeeper, $_, $_, $_)'

Length of output: 105



Script:

#!/bin/bash
# Search for instances of TransferKeeper being instantiated or referenced to verify the inclusion of RatelimitKeeper.
ast-grep --lang go --pattern $'ibctransferkeeper.NewKeeper($$$)'

Length of output: 65



Script:

#!/bin/bash
# Broaden the search to find any reference to TransferKeeper in the codebase.
ast-grep --lang go --pattern $'TransferKeeper'

Length of output: 45



Script:

#!/bin/bash
# Use ripgrep to perform a text-based search for "TransferKeeper" across the codebase.
rg 'TransferKeeper'

Length of output: 482



Script:

#!/bin/bash
# Extract the instantiation of TransferKeeper to check the parameters passed to NewKeeper.
rg 'NewKeeper' protocol/app/app.go

Length of output: 1505



Script:

#!/bin/bash
# Display the lines around the instantiation of TransferKeeper to check the parameters passed to NewKeeper.
cat protocol/app/app.go | grep -C 5 'app.TransferKeeper = ibctransferkeeper.NewKeeper'

Length of output: 387

* 607-610: The `ratelimitmodule` middleware is being wrapped over the IBC Transfer module. This is a critical integration point and must be done correctly to ensure that the middleware functions as expected.

@@ -0,0 +1,232 @@
package util_test
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -0,0 +1,191 @@
package util
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you separate these util functions into two files:

  1. exact copy from Stride
  2. methods that made some modifications

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done - moved exact copies and tests in to file parse_denom.go and parse_denom_test.go

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 1b010c3 and bf33a43.
Files selected for processing (2)
  • protocol/app/app.go (4 hunks)
  • protocol/x/ratelimit/util/capacity_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
  • protocol/app/app.go
Additional comments: 5
protocol/x/ratelimit/util/capacity_test.go (5)
  • 16-16: The test function TestCalculateNewCapacityList has been renamed appropriately to reflect its functionality. Ensure that all references to this function are updated accordingly.

  • 16-16: The test cases within TestCalculateNewCapacityList are comprehensive and cover a wide range of scenarios, including edge cases. The use of table-driven tests is a good practice for clarity and maintainability.

  • 16-16: The error handling in the test cases is done correctly using require.Error and require.NoError. The assertions are appropriate for the conditions being tested.

  • 16-16: The expected results in the test cases appear to match the logic of the CalculateNewCapacityList function. It's important to ensure that the function's logic is sound and that the test cases accurately reflect the expected behavior.

  • 16-16: The error case where the length of the capacity list does not match the length of the limiters is handled correctly, and the error message provides sufficient information to diagnose the issue.

@@ -0,0 +1,191 @@
package util
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you separate these util functions into two files:

  1. exact copy from Stride
  2. methods that made some modifications

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 9

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between bf33a43 and 35038c3.
Files selected for processing (7)
  • protocol/x/ratelimit/ibc_middleware.go (1 hunks)
  • protocol/x/ratelimit/keeper/keeper.go (2 hunks)
  • protocol/x/ratelimit/keeper/packet_test.go (1 hunks)
  • protocol/x/ratelimit/util/ibc.go (1 hunks)
  • protocol/x/ratelimit/util/ibc_test.go (1 hunks)
  • protocol/x/ratelimit/util/parse_denom.go (1 hunks)
  • protocol/x/ratelimit/util/parse_denom_test.go (1 hunks)

return
}
// Undo'ing a withdrawal is equivalent to processing a deposit.
k.ProcessDeposit(ctx, denom, amount)
Copy link
Contributor

Choose a reason for hiding this comment

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

Does calling this function have any negative downstream effects? For example logging or metrics that indicate a deposit happened?

Wonder if it's better to have an explicit UndoWithdrawal function to prevent these issues (now or in the future if code is modified/refactored)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I renamed ProcessDeposit to IncrementCapacitiesForDenom and wrapped UndoWithdrawal around it (and added metrics)

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 35038c3 and 1a44bad.
Files selected for processing (1)
  • protocol/x/ratelimit/util/parse_denom_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
  • protocol/x/ratelimit/util/parse_denom_test.go

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 1a44bad and 3f47889.
Files selected for processing (6)
  • protocol/lib/ante/internal_msg.go (2 hunks)
  • protocol/lib/metrics/constants.go (1 hunks)
  • protocol/x/ratelimit/ibc_middleware.go (1 hunks)
  • protocol/x/ratelimit/keeper/keeper.go (3 hunks)
  • protocol/x/ratelimit/keeper/packet.go (1 hunks)
  • protocol/x/ratelimit/util/ibc.go (1 hunks)
Files skipped from review as they are similar to previous changes (3)
  • protocol/x/ratelimit/ibc_middleware.go
  • protocol/x/ratelimit/keeper/keeper.go
  • protocol/x/ratelimit/util/ibc.go
Additional comments: 12
protocol/lib/ante/internal_msg.go (2)
  • 25-25: The import of ratelimit is correctly placed alphabetically among other imports.
  • 102-103: The addition of MsgSetLimitParams and MsgSetLimitParamsResponse to the IsInternalMsg function is consistent with the pattern of adding message types to this function. Ensure that these message types are indeed intended to be internal and not exposed to external users or systems.
protocol/x/ratelimit/keeper/packet.go (9)
  • 24-28: The RemovePendingSendPacket function correctly deletes a key from the store, which is used to track pending send packets. This is a standard pattern for managing state in a key-value store.
  • 31-35: The SetPendingSendPacket function correctly sets a key in the store to indicate a packet is pending. Using a single bit as the value is efficient for this purpose.
  • 39-43: The HasPendingSendPacket function correctly checks for the existence of a key in the store. This is a standard existence check in a key-value store.
  • 49-74: The AcknowledgeIBCTransferPacket function handles the acknowledgement of an IBC transfer packet. It checks the status of the acknowledgement and either removes the pending packet or undoes the send packet. This logic is consistent with the expected behavior of acknowledging a packet.
  • 81-88: The TimeoutIBCTransferPacket function correctly handles the timeout of an IBC transfer packet by undoing the send packet. This is the expected behavior when a packet times out.
  • 93-106: The UndoSendPacket function is idempotent, as it checks for the existence of the pending packet before attempting to undo it. This is a good practice to prevent unintended side effects.
  • 110-145: The SendPacket function sends a packet and attempts to send a rate-limited packet. It logs an error if the rate-limited send fails. Ensure that the error logging is sufficient for debugging purposes and consider if any additional error handling is necessary.
  • 150-165: The TrySendRateLimitedPacket function processes a withdrawal and sets the packet as pending if successful. Ensure that the ProcessWithdrawal function has appropriate error handling and that the pending packet state is correctly managed.
  • 168-181: The WriteAcknowledgement and GetAppVersion functions are simple wrappers around the ics4Wrapper interface methods. Ensure that the ics4Wrapper interface is correctly implemented and that these functions are used appropriately in the middleware.
protocol/lib/metrics/constants.go (1)
  • 405-408: The addition of the UndoWithdrawAmount constant is consistent with the naming and categorization of other constants in this file. Ensure that this constant is used appropriately wherever metrics related to undoing a withdrawal amount are recorded.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 1

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between a27e96a and d1a7930.
Files selected for processing (14)
  • protocol/app/app.go (4 hunks)
  • protocol/lib/metrics/constants.go (1 hunks)
  • protocol/x/ratelimit/ibc_middleware.go (1 hunks)
  • protocol/x/ratelimit/keeper/keeper.go (3 hunks)
  • protocol/x/ratelimit/keeper/packet.go (1 hunks)
  • protocol/x/ratelimit/keeper/packet_test.go (1 hunks)
  • protocol/x/ratelimit/types/expected_keepers.go (2 hunks)
  • protocol/x/ratelimit/types/ibc.go (1 hunks)
  • protocol/x/ratelimit/types/keys.go (2 hunks)
  • protocol/x/ratelimit/util/capacity_test.go (1 hunks)
  • protocol/x/ratelimit/util/ibc.go (1 hunks)
  • protocol/x/ratelimit/util/ibc_test.go (1 hunks)
  • protocol/x/ratelimit/util/parse_denom.go (1 hunks)
  • protocol/x/ratelimit/util/parse_denom_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (9)
  • protocol/lib/metrics/constants.go
  • protocol/x/ratelimit/ibc_middleware.go
  • protocol/x/ratelimit/keeper/keeper.go
  • protocol/x/ratelimit/keeper/packet.go
  • protocol/x/ratelimit/keeper/packet_test.go
  • protocol/x/ratelimit/util/ibc.go
  • protocol/x/ratelimit/util/ibc_test.go
  • protocol/x/ratelimit/util/parse_denom.go
  • protocol/x/ratelimit/util/parse_denom_test.go
Additional comments: 9
protocol/x/ratelimit/types/keys.go (3)
  • 3-3: The import of the "fmt" package is added to support the new GetPendingSendPacketKey function.
  • 20-21: The new constant PendingSendPacketPrefix is introduced for key-value store operations.
  • 27-29: The GetPendingSendPacketKey function constructs a key for pending send packets. Ensure that the format used here is consistent with other key generation patterns in the system.
protocol/x/ratelimit/types/expected_keepers.go (2)
  • 8-10: New imports for capabilitytypes, clienttypes, and ibcexported are added to support the new ICS4Wrapper interface methods.
  • 22-40: The ICS4Wrapper interface is expanded with new methods for middleware. Ensure that the new methods are implemented wherever this interface is used and that the method signatures align with the expected behavior.
protocol/x/ratelimit/util/capacity_test.go (1)
  • 16-16: The test function TestUpdateAllCapacitiesEndBlocker has been renamed to TestCalculateNewCapacityList. This change should be reflected in any test suite or documentation that references this test.
protocol/app/app.go (3)
  • 633-644: The TransferKeeper is correctly modified to incorporate the RatelimitKeeper as an ICS4Wrapper. This change aligns with the PR objectives to integrate rate-limiting into the IBC transfer logic.
  • 650-650: The middleware stack is correctly updated to wrap the ratelimitmodule.NewIBCMiddleware around the transferStack. This change is consistent with the PR's goal of adding rate-limiting functionality to the IBC transfer module.
  • 656-656: The ibcRouter is updated to include the transferStack with the rate-limiting middleware. This ensures that the IBC transfer module will use the new middleware for packet handling.

Comment on lines +572 to +578
// grant capabilities for the ibc, ibc-transfer, ICAHostKeeper and ratelimit modules

scopedIBCKeeper := app.CapabilityKeeper.ScopeToModule(ibcexported.ModuleName)
scopedIBCTransferKeeper := app.CapabilityKeeper.ScopeToModule(ibctransfertypes.ModuleName)
scopedICAHostKeeper := app.CapabilityKeeper.ScopeToModule(icahosttypes.SubModuleName)
// scopedRatelimitKeeper is not used as an input to any other module.
app.CapabilityKeeper.ScopeToModule(ratelimitmoduletypes.ModuleName)
Copy link
Contributor

Choose a reason for hiding this comment

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

The addition of the ratelimit module to the capabilities grant is noted. However, the scoped keeper for ratelimit is created but not stored or used. This could potentially be an oversight.

Ensure that the scoped keeper for the ratelimit module is stored and used appropriately, or remove it if it's unnecessary.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between a13958a and b700060.
Files selected for processing (14)
  • protocol/app/app.go (4 hunks)
  • protocol/lib/metrics/constants.go (1 hunks)
  • protocol/x/ratelimit/ibc_middleware.go (1 hunks)
  • protocol/x/ratelimit/keeper/keeper.go (3 hunks)
  • protocol/x/ratelimit/keeper/packet.go (1 hunks)
  • protocol/x/ratelimit/keeper/packet_test.go (1 hunks)
  • protocol/x/ratelimit/types/expected_keepers.go (2 hunks)
  • protocol/x/ratelimit/types/ibc.go (1 hunks)
  • protocol/x/ratelimit/types/keys.go (2 hunks)
  • protocol/x/ratelimit/util/capacity_test.go (1 hunks)
  • protocol/x/ratelimit/util/ibc.go (1 hunks)
  • protocol/x/ratelimit/util/ibc_test.go (1 hunks)
  • protocol/x/ratelimit/util/parse_denom.go (1 hunks)
  • protocol/x/ratelimit/util/parse_denom_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (14)
  • protocol/app/app.go
  • protocol/lib/metrics/constants.go
  • protocol/x/ratelimit/ibc_middleware.go
  • protocol/x/ratelimit/keeper/keeper.go
  • protocol/x/ratelimit/keeper/packet.go
  • protocol/x/ratelimit/keeper/packet_test.go
  • protocol/x/ratelimit/types/expected_keepers.go
  • protocol/x/ratelimit/types/ibc.go
  • protocol/x/ratelimit/types/keys.go
  • protocol/x/ratelimit/util/capacity_test.go
  • protocol/x/ratelimit/util/ibc.go
  • protocol/x/ratelimit/util/ibc_test.go
  • protocol/x/ratelimit/util/parse_denom.go
  • protocol/x/ratelimit/util/parse_denom_test.go

@teddyding teddyding merged commit f1825ea into main Jan 26, 2024
17 checks passed
@teddyding teddyding deleted the td/ratelimit-ibc branch January 26, 2024 19:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants