chore: rename ClientKeeper interface functions
#6017
Conversation
crodriguezvega
requested review from
colin-axner,
AdityaSripal,
damiannolan,
chatton,
charleenfei and
DimitrisJim
as code owners
WalkthroughWalkthroughThe recent updates focus on renaming specific functions within the IBC core modules to more accurately reflect their purpose related to client operations. Functions dealing with retrieving the latest height and timestamp for a client have been renamed to include 'Client' in their names, enhancing clarity and consistency across the codebase. This change affects both the core client and channel modules, as well as their corresponding test suites and interfaces, ensuring a unified approach to naming conventions. Changes
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (8)
- modules/core/02-client/keeper/keeper.go (2 hunks)
- modules/core/02-client/keeper/keeper_test.go (3 hunks)
- modules/core/04-channel/keeper/packet.go (1 hunks)
- modules/core/04-channel/keeper/packet_test.go (2 hunks)
- modules/core/04-channel/keeper/timeout.go (1 hunks)
- modules/core/04-channel/keeper/upgrade.go (1 hunks)
- modules/core/04-channel/types/expected_keepers.go (1 hunks)
- testing/chain.go (1 hunks)
Additional comments: 11
modules/core/04-channel/types/expected_keepers.go (1)
- 17-18: Renaming
GetLatestHeighttoGetClientLatestHeightandGetTimestampAtHeighttoGetClientTimestampAtHeightimproves clarity by indicating these methods relate to a client. Ensure all references to these methods are updated across the codebase for consistency.modules/core/04-channel/keeper/timeout.go (1)
- 66-66: The update from
GetTimestampAtHeighttoGetClientTimestampAtHeightaligns with the renamed method in theClientKeeperinterface. Consider adding a comment explaining the reason for this change to aid future maintainability.modules/core/04-channel/keeper/packet.go (1)
- 71-72: Updating method calls from
GetLatestHeighttoGetClientLatestHeightand fromGetTimestampAtHeighttoGetClientTimestampAtHeightin theSendPacketfunction aligns with the renamed methods in theClientKeeperinterface. Ensure the functionality remains unchanged and verify that all references to these methods are correctly updated across the codebase.modules/core/02-client/keeper/keeper.go (2)
- 480-482: The function
GetClientLatestHeighthas been correctly renamed fromGetLatestHeightto align with the PR's objective of enhancing naming consistency within theClientKeeperinterface. This change makes the function's purpose clearer, specifically that it retrieves the latest height of a client state for a given client identifier.- 500-501: The function
GetClientTimestampAtHeighthas been correctly renamed fromGetTimestampAtHeighttoGetClientTimestampAtHeight, which is in line with the PR's goal of improving naming consistency. This renaming clarifies that the function returns the timestamp of the consensus state at a given height for a specific client.testing/chain.go (1)
- 617-617: The update from
GetLatestHeighttoGetClientLatestHeightaligns with the PR's objective of improving naming consistency. Ensure that all references to the old function name across the codebase have been updated accordingly.Verification successful
The occurrences of
GetLatestHeightfound outsidetesting/chain.goare in contexts not directly related to theClientKeeperinterface renaming effort. These include a test function, a function within a different context (solomachine.go), and a protobuf-generated method, indicating that the renaming toGetClientLatestHeightwithin theClientKeeperinterface does not conflict with these instances. The original review comment remains valid within its specific context of improving naming consistency within theClientKeeperinterface.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the old function name is no longer used in the codebase. rg --type go "GetLatestHeight" --glob '!testing/chain.go'Length of output: 459
modules/core/02-client/keeper/keeper_test.go (2)
- 455-461: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [458-502]
The renaming of
TestGetLatestHeighttoTestGetClientLatestHeightaligns with the PR's objective to improve naming consistency within theClientKeeperinterface. This change makes the function's purpose clearer, indicating it specifically relates to client operations. The test logic remains unchanged, focusing on validating theGetClientLatestHeightfunction's behavior under various conditions. The test cases are well-structured, covering success and failure scenarios effectively.
- 574-574: The renaming of
TestGetTimestampAtHeighttoTestGetClientTimestampAtHeightis consistent with the PR's goal of enhancing naming clarity. This adjustment specifies that the function deals with obtaining timestamps related to client heights, which is more descriptive. The test scenarios provided are comprehensive, testing the function's response to different client and consensus state conditions. This thorough testing approach ensures the renamed function's reliability across various scenarios.modules/core/04-channel/keeper/upgrade.go (1)
- 748-748: The change from
GetTimestampAtHeighttoGetClientTimestampAtHeightaligns with the PR's objective to enhance naming consistency within theClientKeeperinterface. This modification ensures that the function's purpose is more explicitly communicated, indicating that it retrieves a timestamp associated with a client at a specific height. This change is consistent with the broader goal of improving code readability and maintainability without altering the underlying functionality.modules/core/04-channel/keeper/packet_test.go (2)
- 161-161: The method
GetTimestampAtHeighthas been correctly updated toGetClientTimestampAtHeightin line with the PR's objective to rename certain functions for consistency. This change is accurately reflected in theTestSendPacketfunction.- 178-178: The method
GetTimestampAtHeighthas been correctly updated toGetClientTimestampAtHeightin theTestSendPacketfunction for the solomachine case. This change aligns with the PR's objective and ensures consistency across the interface.
|
I don't feel v strongly about this either way, as i don't think it adds clarity as both functions are being called on the |
|
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (1)
- modules/core/02-client/keeper/keeper.go (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- modules/core/02-client/keeper/keeper.go
Co-authored-by: Charly <[email protected]>
Description
I thought this is more consistent with the other functions in the interface. See my comment here. Happy to close the PR if people think it's ok as is.
closes: #XXXX
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/).godoccomments.Files changedin the GitHub PR explorer.SonarCloud Reportin the comment section below once CI passes.Summary by CodeRabbit