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

refactor(vesting): fix build #19539

Merged
merged 8 commits into from
Feb 24, 2024
Merged

refactor(vesting): fix build #19539

merged 8 commits into from
Feb 24, 2024

Conversation

julienrbrt
Copy link
Member

@julienrbrt julienrbrt commented Feb 23, 2024

Description

Follow-up of #19535


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • reviewed "Files changed" and left comments if necessary
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • updated the relevant documentation or specification, including comments for documenting Go code
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic, API design and naming, documentation is accurate, tests and test coverage

Summary by CodeRabbit

  • Refactor
    • Updated consensus version handling in the vesting module.
  • Chores
    • Deprecated the x/auth/vesting module in favor of x/accounts.
    • Simplified the build workflow logic by removing conditional checks.
    • Adjusted the AppModule within the vesting package.
    • Ensured backward compatibility by maintaining the x/auth/vesting module for existing chains.

@julienrbrt julienrbrt requested a review from a team as a code owner February 23, 2024 18:06
Copy link
Contributor

coderabbitai bot commented Feb 23, 2024

Walkthrough

Walkthrough

The changes encompass streamlining the vesting module by removing unnecessary gRPC service registrations and adjusting the ConsensusVersion function in the AppModule. This update aims to simplify the architecture and enhance efficiency by deprecating certain functionality and workflows while maintaining backward compatibility.

Changes

File(s) Change Summary
x/auth/vesting/module.go Removed RegisterServices function, gRPC imports; adjusted ConsensusVersion in AppModule.
.github/workflows/build.yml Simplified workflow by eliminating get-diff-action step, conditional checks based on Git differences.
x/auth/vesting/README.md Deprecated x/auth/vesting module in favor of x/accounts; vesting accounts creation not supported from v0.51. Importing x/auth/vesting still needed for backward compatibility.
tests/integration/rapidgen/rapidgen.go Removed declarations for vesting account creation messages.

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.

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.yml

Commits Files that changed from the base of the PR and between 4b73e31 and 38bb9ef.
Files selected for processing (1)
  • x/auth/vesting/module.go (2 hunks)
Additional comments: 2
x/auth/vesting/module.go (2)
  • 2-7: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-48]

The removal of the RegisterServices function and related imports, as mentioned in the summary, is not directly visible in the provided code snippet. However, this change is significant as it alters how module services are registered, moving away from gRPC-based service registration. This could have implications for how clients interact with the module and might necessitate updates in documentation, client code, and tests to reflect the new registration mechanism. Ensure that all necessary updates are made and consider the impact on downstream modules or clients that might rely on the previous gRPC-based service registration.

  • 2-7: > 📝 NOTE

This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [44-44]

The ConsensusVersion function has been adjusted to return a hardcoded value of 1. This change seems straightforward but is crucial for the module's compatibility with the consensus mechanism. Ensure that this version increment aligns with the overall versioning strategy of the application and that any implications of this change on consensus and state migration processes are thoroughly evaluated. It's also important to verify that all necessary documentation and tests reflect this version change to avoid any confusion or issues during upgrades.

@julienrbrt julienrbrt changed the title chore: fix build chore(vesting): fix build Feb 23, 2024
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.yml

Commits Files that changed from the base of the PR and between 38bb9ef and 585c816.
Files selected for processing (1)
  • x/auth/vesting/module.go (2 hunks)
Files skipped from review as they are similar to previous changes (1)
  • x/auth/vesting/module.go

@julienrbrt julienrbrt changed the title chore(vesting): fix build refactor(vesting): fix build Feb 23, 2024
x/auth/vesting/README.md Show resolved Hide resolved
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: 2

Configuration used: .coderabbit.yml

Commits Files that changed from the base of the PR and between 585c816 and e2a6622.
Files selected for processing (2)
  • .github/workflows/build.yml (3 hunks)
  • x/auth/vesting/README.md (1 hunks)
Additional comments: 1
.github/workflows/build.yml (1)
  • 27-38: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-39]

The changes in the build.yml workflow remove conditional steps based on Git differences, simplifying the workflow. Ensure that the removal of these conditions does not impact the ability to build specific changesets or branches, especially for larger projects where build optimization and specificity are crucial.

Verification successful

The search results indicate that get-diff-action is extensively used in other workflows (test.yml and lint.yml), but its removal from build.yml is specific to that workflow and does not directly impact the usage of the action in other workflows. The original concern about ensuring the removal does not impact the ability to build specific changesets or branches remains valid but is isolated to the build.yml workflow. The review comment's concern about potential impacts due to the removal of conditional steps based on Git differences in build.yml is acknowledged, but there's no direct evidence of interdependency with other workflows that would be affected by this change.

Scripts Executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Verify if any other workflows or scripts depend on the outputs of the removed steps.
rg --type yaml 'get-diff-action' .github/workflows/

Length of output: 3132

x/auth/vesting/README.md Show resolved Hide resolved
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.yml

Commits Files that changed from the base of the PR and between e2a6622 and a9e2f0f.
Files selected for processing (1)
  • tests/integration/rapidgen/rapidgen.go (1 hunks)
Additional comments: 1
tests/integration/rapidgen/rapidgen.go (1)
  • 218-223: > 📝 NOTE

This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-184]

The removal of vesting account creation messages (MsgCreateVestingAccount, MsgCreatePermanentLockedAccount, and MsgCreatePeriodicVestingAccount) aligns with the deprecation of the x/auth/vesting module in favor of x/accounts. It's important to ensure that any tests or functionality that depended on these messages are either updated or removed to reflect this change. Additionally, verify that the deprecation and removal have been communicated clearly in the project's documentation to inform users and developers of the change.

@julienrbrt julienrbrt enabled auto-merge February 23, 2024 20:31
auto-merge was automatically disabled February 24, 2024 13:41

Merge queue setting changed

@tac0turtle tac0turtle merged commit 619e0da into main Feb 24, 2024
60 of 61 checks passed
@tac0turtle tac0turtle deleted the julien/fix-build branch February 24, 2024 13:41
@coderabbitai coderabbitai bot mentioned this pull request Aug 8, 2024
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants