feat: multi-ibc-relayer supported #205
Conversation
WalkthroughWalkthroughThis update primarily focuses on enhancing the permission management framework in the IBC module. It includes renaming functions and variables for clarity, introducing new functions for handling multiple relayers, and restructuring various components to support lists instead of single entities. The changes streamline the process of managing lists of permissioned relayers, adding flexibility and scalability to the permission system. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant MsgServer as MsgServer
participant Keeper as Keeper
participant RelayerList as RelayerList
Client->>MsgServer: MsgSetPermissionedRelayers
MsgServer->>Keeper: SetPermissionedRelayers(req)
Keeper->>RelayerList: Update list with new relayers
RelayerList-->>Keeper: Confirmation
Keeper-->>MsgServer: Success message
MsgServer-->>Client: Response with status
Poem
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? TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Additionally, you can add CodeRabbit Configration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #205 +/- ##
==========================================
+ Coverage 38.94% 39.01% +0.06%
==========================================
Files 251 252 +1
Lines 24131 24221 +90
==========================================
+ Hits 9399 9451 +52
- Misses 13219 13255 +36
- Partials 1513 1515 +2
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (5)
x/ibc/perm/types/genesis.pb.gois excluded by!**/*.pb.go,!**/*.pb.gox/ibc/perm/types/query.pb.gois excluded by!**/*.pb.go,!**/*.pb.gox/ibc/perm/types/query.pb.gw.gois excluded by!**/*.pb.gw.go,!**/*.pb.gw.gox/ibc/perm/types/tx.pb.gois excluded by!**/*.pb.go,!**/*.pb.gox/ibc/perm/types/types.pb.gois excluded by!**/*.pb.go,!**/*.pb.go
Files selected for processing (21)
- app/hook/bridge_hook.go (4 hunks)
- app/hook/bridge_hook_test.go (2 hunks)
- proto/ibc/applications/perm/v1/genesis.proto (1 hunks)
- proto/ibc/applications/perm/v1/query.proto (1 hunks)
- proto/ibc/applications/perm/v1/tx.proto (2 hunks)
- proto/ibc/applications/perm/v1/types.proto (1 hunks)
- x/ibc/perm/ibc_module.go (2 hunks)
- x/ibc/perm/keeper/genesis.go (1 hunks)
- x/ibc/perm/keeper/genesis_test.go (1 hunks)
- x/ibc/perm/keeper/grpc_query.go (1 hunks)
- x/ibc/perm/keeper/grpc_query_test.go (3 hunks)
- x/ibc/perm/keeper/keeper.go (4 hunks)
- x/ibc/perm/keeper/keeper_test.go (3 hunks)
- x/ibc/perm/keeper/msg_server.go (4 hunks)
- x/ibc/perm/keeper/msg_server_test.go (1 hunks)
- x/ibc/perm/types/codec.go (1 hunks)
- x/ibc/perm/types/events.go (1 hunks)
- x/ibc/perm/types/extended_types.go (1 hunks)
- x/ibc/perm/types/genesis.go (1 hunks)
- x/ibc/perm/types/key.go (1 hunks)
- x/ibc/perm/types/tx.go (2 hunks)
Additional comments not posted (41)
x/ibc/perm/types/events.go (1)
5-5: The new event typeEventTypeAddPermissionedRelayercorrectly reflects the addition of multiple relayers functionality.proto/ibc/applications/perm/v1/genesis.proto (1)
12-13: The protobuf definition forpermissioned_relayer_setsis correctly updated with appropriate field options to ensure data integrity and compatibility with YAML configurations.proto/ibc/applications/perm/v1/types.proto (1)
9-12: The new protobuf message typesPermissionedRelayersSetandPermissionedRelayerListare well-defined and align with the PR's objectives to handle multiple relayers efficiently.Also applies to: 15-17
x/ibc/perm/types/genesis.go (1)
4-6: The functionsNewGenesisStateandDefaultGenesisStateare correctly updated to use the newPermissionedRelayersSetdata structure, aligning with the changes to handle multiple relayers.Also applies to: 13-13
x/ibc/perm/types/key.go (2)
19-21: LGTM! The new constants for relayer prefixes align with the multi-relayer functionality.
26-26: The function correctly utilizes the newPermissionedRelayersPrefixKeyto generate keys for permissioned relayers.x/ibc/perm/types/codec.go (2)
14-14: The message type registration has been updated to align with the new functionality for handling multiple relayers.
20-20: Proper registration of the newMsgSetPermissionedRelayersmessage type ensures compatibility with the interface system.x/ibc/perm/types/extended_types.go (4)
8-15: TheContainsfunction efficiently checks for the presence of a relayer address, which is crucial for permission checks.
16-18: TheAddfunction correctly manages the dynamic addition of relayers to the list.
20-33: TheToAccAddressfunction correctly converts relayer addresses from strings, handling errors appropriately. This is essential for internal SDK operations.
35-41: TheToRelayerListfunction effectively converts a list ofsdk.AccAddressto aPermissionedRelayerList, facilitating reverse conversions within the module.x/ibc/perm/keeper/genesis_test.go (1)
30-39: The test forInitGenesisandExportGenesiscorrectly checks the consistency of the genesis state, reflecting the new structure for handling multiple relayers.x/ibc/perm/keeper/genesis.go (1)
31-41: Ensure consistent error handling strategy across the application.This function uses a panic for error handling similar to
InitGenesis. If changing one to a more graceful error handling, consider applying it here as well.x/ibc/perm/keeper/msg_server_test.go (1)
26-45: The test logic correctly handles multiple relayers. Consider adding more edge cases.This test checks basic functionality. You might want to add tests for cases where duplicate or invalid addresses are added, or when the list is empty.
x/ibc/perm/keeper/grpc_query.go (2)
24-35: The implementation of querying relayers for one channel is correct.Consider enhancing the error messages to provide more context about the failure.
40-59: The pagination and retrieval logic for all channels is well-implemented.Ensure that there are comprehensive tests covering various pagination scenarios and edge cases.
proto/ibc/applications/perm/v1/query.proto (1)
14-20: The updated gRPC service definitions correctly reflect the new functionality.Ensure that the implementation in the gRPC server matches these definitions accurately.
proto/ibc/applications/perm/v1/tx.proto (4)
16-19: The addition ofMsgSetPermissionedRelayersandMsgAddPermissionedRelayerscorrectly aligns with the new functionality to handle multiple relayers. Ensure that these methods are appropriately integrated into the service handlers.
22-36: The messageMsgSetPermissionedRelayersis well-defined with appropriate fields for handling multiple relayers. The use ofPermissionedRelayerListis consistent with the changes in other parts of the system.
Line range hint
43-57: The messageMsgAddPermissionedRelayersis correctly structured to support the addition of multiple relayers to a channel. The fields and options are consistent with the system's requirements for handling permissions.
60-61: The response messages for setting and adding permissioned relayers are minimal, which is suitable since no data needs to be returned. This is a good practice for simple acknowledgment messages.x/ibc/perm/types/tx.go (4)
18-27: The constructorNewMsgSetPermissionedRelayerscorrectly initializes the message with the necessary fields to handle multiple relayers. This is crucial for the new functionality of managing multiple relayers per channel.
29-37: The constructorNewMsgAddPermissionedRelayersis well-implemented, ensuring that it can handle the addition of multiple relayers. This aligns with the system's expansion to support more dynamic relayer management.
Line range hint
45-67: TheValidatemethod forMsgSetPermissionedRelayersis comprehensive, checking all necessary fields and ensuring that relayer addresses are valid. This is critical for maintaining data integrity and preventing configuration errors.
69-89: TheValidatemethod forMsgAddPermissionedRelayersmirrors the validation logic ofMsgSetPermissionedRelayers, which is appropriate given their similar functionalities. Consistency in validation logic across similar message types is a good practice.x/ibc/perm/keeper/grpc_query_test.go (2)
Line range hint
18-38: The testTest_QueryPermissionedRelayersOneChanneleffectively checks the retrieval of permissioned relayers for a single channel. It properly tests both the successful retrieval and the error scenario where the channel does not exist.
Line range hint
46-90: The testTest_QueryPermissionedRelayersAllChannelis comprehensive, covering scenarios for multiple channels. It checks that the correct data is returned for each channel, and the use of conditional assertions based on channel IDs is a good practice to confirm the accuracy of the data.x/ibc/perm/keeper/msg_server.go (2)
Line range hint
27-67: The methodSetPermissionedRelayersis well-implemented, with comprehensive validation and error handling. The logging and event emission are appropriate for tracking operations and debugging.
70-110: The methodAddPermissionedRelayersfollows a similar pattern toSetPermissionedRelayers, ensuring consistency in handling similar operations. The thorough validation and detailed logging are commendable.x/ibc/perm/keeper/keeper.go (6)
27-27: The updatedPermissionedRelayersmap now correctly usestypes.PermissionedRelayerListto accommodate multiple relayers. This change aligns with the PR's goal to support multi-IBC relayers.
45-45: Initialization ofPermissionedRelayersmap with the correct key and value codecs is crucial for ensuring data consistency and type safety. Good use ofcollections.NewMaphere.
65-70: This method effectively iterates over all permissioned relayers, which now correctly handles a list of relayers per channel. The use oftypes.PermissionedRelayersSetensures that each entry is processed as a set, which is appropriate for the new multi-relayer feature.
76-79: The method to set permissioned relayers is correctly implemented to handle an array ofsdk.AccAddress. This change is necessary for the support of multiple relayers per channel.
99-108: Retrieving permissioned relayers has been updated to return a list of addresses. This is a crucial update for supporting multiple relayers. The error handling and conversion tosdk.AccAddressare done properly.
113-125: The method to check if a relayer has permission now iterates over a list of relayers, which aligns with the updated data structure. This is a necessary update for the new functionality.app/hook/bridge_hook.go (3)
31-33: The updatedPermKeeperinterface now supports operations on multiple relayers, which is essential for the new functionality of handling multiple IBC relayers.
65-65: Good handling of the error scenario where the permissioned relayers cannot be retrieved. The use ofcollections.ErrNotFoundto decide the flow of logic is appropriate.
72-72: Setting permissioned relayers in different bridge lifecycle events ensures that the relayer management is consistent across different states of the bridge. This is crucial for maintaining the integrity of the permissioned channels.Also applies to: 100-100, 158-158
x/ibc/perm/keeper/keeper_test.go (1)
Line range hint
81-155: The test cases have been updated to reflect the handling of multiple relayers. This includes tests for getting, setting, and adding relayers, which are crucial for validating the changes made in the keeper logic.app/hook/bridge_hook_test.go (1)
67-71: The methodGetPermissionedRelayersproperly handles the case where no relayers are found for the given port and channel, returning an appropriate error. This is a good use of defensive programming.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- app/hook/bridge_hook_test.go (2 hunks)
- x/ibc/perm/keeper/keeper.go (4 hunks)
Files skipped from review as they are similar to previous changes (2)
- app/hook/bridge_hook_test.go
- x/ibc/perm/keeper/keeper.go
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- x/ibc/perm/ibc_module.go (2 hunks)
- x/ibc/perm/keeper/grpc_query_test.go (3 hunks)
- x/ibc/perm/keeper/keeper.go (4 hunks)
- x/ibc/perm/keeper/keeper_test.go (3 hunks)
- x/ibc/perm/keeper/msg_server.go (4 hunks)
- x/ibc/perm/keeper/msg_server_test.go (1 hunks)
- x/ibc/perm/types/extended.go (1 hunks)
Files skipped from review as they are similar to previous changes (5)
- x/ibc/perm/ibc_module.go
- x/ibc/perm/keeper/keeper.go
- x/ibc/perm/keeper/keeper_test.go
- x/ibc/perm/keeper/msg_server.go
- x/ibc/perm/keeper/msg_server_test.go
Additional comments not posted (4)
x/ibc/perm/types/extended.go (2)
16-18: LGTM! Efficient use of list append inAddRelayer.
35-41: LGTM! Good use of helper functionAddRelayerinToRelayerList.x/ibc/perm/keeper/grpc_query_test.go (2)
Line range hint
18-38: Ensure proper setup and querying inTest_QueryPermissionedRelayersOneChannel.The test covers the scenario of querying a single channel's relayers effectively.
Line range hint
46-90: Ensure comprehensive testing inTest_QueryPermissionedRelayersAllChannel.The test effectively handles multiple channels and validates the correct retrieval and ordering of relayers.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- x/ibc/perm/types/extended.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- x/ibc/perm/types/extended.go
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (5)
x/ibc/perm/types/genesis.pb.gois excluded by!**/*.pb.go,!**/*.pb.gox/ibc/perm/types/query.pb.gois excluded by!**/*.pb.go,!**/*.pb.gox/ibc/perm/types/query.pb.gw.gois excluded by!**/*.pb.gw.go,!**/*.pb.gw.gox/ibc/perm/types/tx.pb.gois excluded by!**/*.pb.go,!**/*.pb.gox/ibc/perm/types/types.pb.gois excluded by!**/*.pb.go,!**/*.pb.go
Files selected for processing (16)
- app/hook/bridge_hook.go (4 hunks)
- proto/ibc/applications/perm/v1/genesis.proto (1 hunks)
- proto/ibc/applications/perm/v1/query.proto (1 hunks)
- proto/ibc/applications/perm/v1/tx.proto (2 hunks)
- proto/ibc/applications/perm/v1/types.proto (1 hunks)
- x/ibc/perm/keeper/genesis.go (1 hunks)
- x/ibc/perm/keeper/genesis_test.go (1 hunks)
- x/ibc/perm/keeper/grpc_query.go (1 hunks)
- x/ibc/perm/keeper/grpc_query_test.go (4 hunks)
- x/ibc/perm/keeper/keeper.go (4 hunks)
- x/ibc/perm/keeper/keeper_test.go (3 hunks)
- x/ibc/perm/keeper/msg_server.go (3 hunks)
- x/ibc/perm/types/events.go (1 hunks)
- x/ibc/perm/types/extended.go (1 hunks)
- x/ibc/perm/types/genesis.go (2 hunks)
- x/ibc/perm/types/tx.go (2 hunks)
Files skipped from review as they are similar to previous changes (14)
- app/hook/bridge_hook.go
- proto/ibc/applications/perm/v1/genesis.proto
- proto/ibc/applications/perm/v1/query.proto
- proto/ibc/applications/perm/v1/tx.proto
- proto/ibc/applications/perm/v1/types.proto
- x/ibc/perm/keeper/genesis.go
- x/ibc/perm/keeper/genesis_test.go
- x/ibc/perm/keeper/grpc_query_test.go
- x/ibc/perm/keeper/keeper.go
- x/ibc/perm/keeper/keeper_test.go
- x/ibc/perm/types/events.go
- x/ibc/perm/types/extended.go
- x/ibc/perm/types/genesis.go
- x/ibc/perm/types/tx.go
Additional comments not posted (3)
x/ibc/perm/keeper/msg_server.go (1)
Line range hint
27-67: ### Review ofSetPermissionedRelayersMethod
- Validation and Error Handling (Lines 27-29): The validation of request parameters is correctly implemented using
req.Validate(). Consider adding more specific error messages for better user guidance.- Authority Verification (Lines 31-34): The authority check is crucial for security, ensuring that only authorized users can set relayers. This is well implemented.
- Relayer Conversion (Lines 36-38): The conversion of relayer addresses uses
types.ToRelayerAccAddr. Ensure that this function handles all expected address formats correctly.- Setting Relayers (Lines 42-42): The actual setting of relayers is performed here. It's important to ensure that the
SetPermissionedRelayersmethod in the keeper handles errors appropriately and performs all necessary checks.- Logging (Lines 50-50): Logging is well-placed, providing traceability for this operation.
- Event Emission (Lines 56-59): Events are crucial for observers on the blockchain. The event details are correctly populated, but ensure that the event types and attributes are consistent with other parts of the system.
- Response (Line 67): The method returns an empty response after successful execution. Consider if any additional information might be useful for the caller.
Overall, the method is well-structured but ensure thorough testing, especially for error handling paths.
x/ibc/perm/keeper/grpc_query.go (2)
23-35: ### Review ofPermissionedRelayersByChannelMethod
- Context Unwrapping (Line 24): Correctly obtaining the SDK context from the gRPC context.
- Data Retrieval (Lines 26-26): The method retrieves the list of relayers using a composite key. Ensure that the
Getmethod correctly handles different edge cases, such as non-existent keys.- Response Construction (Lines 31-35): The response is well-constructed, ensuring that all necessary data is returned. However, consider adding error handling or logging if the relayers list is unexpectedly empty.
40-57: ### Review ofAllPermissionedRelayersMethod
- Pagination Handling (Lines 41-44): The use of
query.CollectionPaginateis appropriate for handling large datasets with pagination. Ensure that the pagination parameters are validated before use.- Data Transformation (Lines 46-49): The lambda function used for transforming the data is succinct and clear. However, ensure that the transformation logic is consistent with other parts of the application.
- Response Construction (Lines 57-57): The response is appropriately constructed, returning both the data and pagination results. Consider adding more detailed logging for monitoring and debugging purposes.
[APROVED]
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- app/hook/bridge_hook_test.go (2 hunks)
- x/ibc/perm/keeper/keeper.go (4 hunks)
- x/ibc/perm/keeper/msg_server.go (3 hunks)
- x/ibc/perm/types/codec.go (1 hunks)
- x/ibc/perm/types/extended.go (1 hunks)
- x/ibc/perm/types/key.go (1 hunks)
Files skipped from review as they are similar to previous changes (5)
- app/hook/bridge_hook_test.go
- x/ibc/perm/keeper/keeper.go
- x/ibc/perm/types/codec.go
- x/ibc/perm/types/extended.go
- x/ibc/perm/types/key.go
Additional comments not posted (1)
x/ibc/perm/keeper/msg_server.go (1)
Line range hint
27-66: Review ofSetPermissionedRelayersfunction.
- Validation and Error Handling: The function begins by validating the request using
req.Validate. This is good practice as it ensures that the request meets expected criteria before proceeding.- Authority Verification: It checks if the
authorityfrom the request matches an expected value, returning an error if not. This is crucial for security, ensuring that only authorized entities can modify relayers.- Relayer Address Conversion: Converts relayer identifiers to account addresses. This is necessary for internal consistency and further processing.
- Setting Permissioned Relayers: The core functionality where the relayers are actually set in the keeper based on the provided port and channel IDs.
- Logging: Logs the operation, which is useful for audit trails and debugging.
- Event Emission: Emits relevant events for the operation, which is essential for monitoring and reactive architectures.
Overall, the function is well-structured with clear separation of concerns and robust error handling. However, ensure that the logging level and details are appropriate and consider adding more specific error messages for better troubleshooting.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- x/ibc/perm/keeper/genesis.go (1 hunks)
- x/ibc/perm/keeper/genesis_test.go (2 hunks)
- x/ibc/perm/keeper/keeper.go (4 hunks)
- x/ibc/perm/keeper/keeper_test.go (3 hunks)
- x/ibc/perm/types/extended.go (1 hunks)
- x/ibc/perm/types/key.go (1 hunks)
Files skipped from review as they are similar to previous changes (6)
- x/ibc/perm/keeper/genesis.go
- x/ibc/perm/keeper/genesis_test.go
- x/ibc/perm/keeper/keeper.go
- x/ibc/perm/keeper/keeper_test.go
- x/ibc/perm/types/extended.go
- x/ibc/perm/types/key.go
Since the interface has changed, it may affect other projects and dependencies, so any warnings or notifications are appreciated.