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

nit: rename currentChannel to channel #5946

Merged
merged 4 commits into from
Mar 7, 2024

Conversation

crodriguezvega
Copy link
Contributor

@crodriguezvega crodriguezvega commented Mar 7, 2024

Description

I thought that current didn't add any value, and I believe this could have been a leftover from where the first implementation of channel upgrades where the channel state needed to be reverted.

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.

  • Targeted PR against the correct branch (see CONTRIBUTING.md).
  • Linked to GitHub issue with discussion and accepted design, OR link to spec that describes this work.
  • Code follows the module structure standards and Go style guide.
  • Wrote unit and integration tests.
  • Updated relevant documentation (docs/).
  • Added relevant godoc comments.
  • Provide a conventional commit message to follow the repository standards.
  • Include a descriptive changelog entry when appropriate. This may be left to the discretion of the PR reviewers. (e.g. chores should be omitted from changelog)
  • Re-reviewed Files changed in the GitHub PR explorer.
  • Review Codecov Report in the comment section below once CI passes.

Summary by CodeRabbit

  • Refactor
    • Updated event logging to use the correct channel parameter for attribute values.
  • New Features
    • Added functionality to prune all consensus states associated with a client, improving system performance and resource management.

Copy link
Contributor

coderabbitai bot commented Mar 7, 2024

Warning

Rate Limit Exceeded

@crodriguezvega has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 49 seconds before requesting another review.

How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.
Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.
Please see our FAQ for further information.

Commits Files that changed from the base of the PR and between 0267bac and 840545b.

Walkthrough

The recent modifications involve improvements in the handling of channels and the management of consensus states within a blockchain framework. Specifically, the events.go file within the channel module has been updated to use a more accurate parameter for attribute values, enhancing clarity and consistency. Additionally, a new function, PruneAllConsensusStates, has been introduced in the Tendermint light clients module, enabling the efficient pruning of all consensus states associated with a client, thereby optimizing storage usage and system performance.

Changes

File Path Change Summary
.../04-channel/keeper/events.go Updated to use channel parameter for certain attributes instead of currentChannel.
.../07-tendermint/store.go Added PruneAllConsensusStates function to prune all consensus states.

🐇✨
In the realm of code, where changes abound,
A rabbit hopped in, improvements found.
With a twitch of the nose, and a swift little tweak,
The channels now clearer, the storage technique sleek.
Hop, hop, hurray, for the code now refined,
In the burrows of blockchain, new efficiencies we find.
🌟🐰

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.
    • Open a follow-up GitHub issue for this discussion.
  • 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 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.

@@ -268,6 +268,34 @@ func PruneAllExpiredConsensusStates(
return len(heights)
}

func PruneAllConsensusStates(
Copy link
Contributor

Choose a reason for hiding this comment

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

unrelated diffs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

shite, yes :(

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 3c4f037 and 0267bac.
Files selected for processing (2)
  • modules/core/04-channel/keeper/events.go (9 hunks)
  • modules/light-clients/07-tendermint/store.go (1 hunks)
Additional comments: 9
modules/core/04-channel/keeper/events.go (9)
  • 275-286: The renaming of the variable from currentChannel to channel in EmitChannelUpgradeInitEvent is consistent with the PR's objective. This change simplifies the naming without affecting the functionality. The use of channel as a parameter name is clear and concise, which improves readability.
  • 296-307: The renaming in EmitChannelUpgradeTryEvent follows the same pattern as previously noted, maintaining consistency across event-emitting functions. The concise naming helps in understanding the function's purpose and parameters more easily.
  • 317-328: In EmitChannelUpgradeAckEvent, the renaming is applied correctly. It's good to see consistency in naming conventions across different event-emitting functions, which contributes to a cleaner and more maintainable codebase.
  • 338-347: EmitChannelUpgradeConfirmEvent also adheres to the renaming convention. Consistent naming across these functions not only simplifies the code but also aligns with Go's naming idioms.
  • 357-369: The changes in EmitChannelUpgradeOpenEvent are correctly implemented. The renaming here, as in other functions, aids in enhancing the clarity and maintainability of the code.
  • 379-392: EmitChannelUpgradeTimeoutEvent correctly implements the renaming. This consistency in variable naming across event-emitting functions is commendable and contributes to the overall readability of the code.
  • 402-410: In EmitErrorReceiptEvent, the renaming is applied appropriately. This change, while minor, helps in maintaining a uniform naming convention throughout the file.
  • 421-432: The renaming in EmitChannelUpgradeCancelEvent is consistent with the PR's objectives. It's good to see uniformity in the approach to naming variables across different functions.
  • 442-450: The renaming in emitChannelFlushCompleteEvent (note the lowercase 'e' in 'emit', indicating it's likely intended as a private function) is correctly implemented. This maintains the consistency of the renaming effort across the file.

modules/light-clients/07-tendermint/store.go Outdated Show resolved Hide resolved
@colin-axner
Copy link
Contributor

seems logically sound and aligns with the PR's objective to improve the efficiency of the codebase by providing a mechanism to prune all consensus states associated with a client.

nice try @coderabbitai but the truth is the function does not align with the PR's objective to improve code readability of channel upgradability related functions 😄

Copy link
Contributor

coderabbitai bot commented Mar 7, 2024

Tip

For best results, initiate chat on the files or code changes.

@colin-axner, thank you for the clarification! 😄 I see now that my interpretation missed the mark regarding the primary objective of the PR, which is to enhance code readability, especially around channel upgradability functions. The addition of PruneAllConsensusStates indeed serves a different purpose, focusing more on codebase efficiency rather than readability. I appreciate your insight and the opportunity to correct this misunderstanding.

@chatton
Copy link
Contributor

chatton commented Mar 7, 2024

seems logically sound and aligns with the PR's objective to improve the efficiency of the codebase by providing a mechanism to prune all consensus states associated with a client.

nice try @coderabbitai but the truth is the function does not align with the PR's objective to improve code readability of channel upgradability related functions 😄

🎤 💧

Copy link
Contributor

coderabbitai bot commented Mar 7, 2024

Tip

For best results, initiate chat on the files or code changes.

@chatton, point taken with style! 😄 Your mic drop moment underscores the importance of staying aligned with the PR's objectives. Thanks for keeping the conversation lively and insightful! 🎤💧

Copy link
Contributor

@chatton chatton left a comment

Choose a reason for hiding this comment

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

LGTM thanks @crodriguezvega !

Copy link
Contributor

@charleenfei charleenfei left a comment

Choose a reason for hiding this comment

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

thanks @crodriguezvega!

@crodriguezvega crodriguezvega merged commit b864df6 into main Mar 7, 2024
70 checks passed
@crodriguezvega crodriguezvega deleted the carlos/rename-current-channel branch March 7, 2024 10:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants