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(authority): message update chain info to update single chains #2890

Merged
merged 8 commits into from
Sep 20, 2024

Conversation

kingpinXD
Copy link
Contributor

@kingpinXD kingpinXD commented Sep 17, 2024

Description

  • The update_chain_info message has been refactored to update/add a single chain ,instead of providing the entire list as ChainInfo
    • The user is now required to provide a json file with the details of the chain to be updated/added.
      • If the chain already exists, the details will be updated.
      • If the chain does not exist, it will be added to the list of chains and saved to the store.
  • A new transaction type RemoveChainInfo has also been added to remove a chain from the list of chains.
    • It accepts the chain-id of the chain to be removed as a parameter.

Closes #2815

How Has This Been Tested?

  • Tested CCTX in localnet
  • Tested in development environment
  • Go unit tests
  • Go integration tests
  • Tested via GitHub Actions

Summary by CodeRabbit

  • New Features

    • Introduced a command to remove chain information via the zetacored CLI.
    • Added a new message type, MsgRemoveChainInfo, for removing chain data.
    • Enhanced the MsgUpdateChainInfo functionality to streamline updates for individual chains.
  • Bug Fixes

    • Improved error handling for unauthorized removal attempts and missing chain information.
  • Documentation

    • Updated CLI documentation to include new command options.
    • Enhanced API documentation with definitions for new message types and responses.
  • Tests

    • Added comprehensive tests for the new MsgRemoveChainInfo functionality and updated existing tests for clarity.

Copy link
Contributor

coderabbitai bot commented Sep 17, 2024

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

Walkthrough

The pull request introduces significant modifications to the chain management functionality within the ZetaChain framework. It refactors the MsgUpdateChainInfo to accept a single chain, introduces a new message type MsgRemoveChainInfo for chain deletions, and updates related documentation and tests. These changes aim to streamline the process of managing chain information, enhancing the clarity and functionality of the system.

Changes

Files Change Summary
changelog.md, docs/releases/v21_breaking_changes.md Added MsgRemoveChainInfo and modified MsgUpdateChainInfo to accept a single chain.
docs/cli/zetacored/cli.md, x/authority/client/cli/tx.go, x/authority/client/cli/tx_remove_chain_info.go Introduced CLI command remove-chain-info for removing chain information and updated related command handling.
docs/openapi/openapi.swagger.yaml, proto/zetachain/zetacore/authority/tx.proto Added MsgRemoveChainInfoResponse to OpenAPI spec and updated service definitions for chain management.
x/authority/keeper/msg_server_remove_chain_info.go, x/authority/keeper/msg_server_remove_chain_info_test.go Implemented and tested the logic for removing chain information based on chain ID.
x/authority/types/message_remove_chain_info.go, x/authority/types/message_remove_chain_info_test.go Defined MsgRemoveChainInfo structure and its associated methods, along with unit tests for validation.
x/authority/types/message_update_chain_info.go, x/authority/types/message_update_chain_info_test.go Updated MsgUpdateChainInfo to handle a single chain and revised tests accordingly.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant CLI
    participant MsgServer
    participant Keeper

    User->>CLI: Execute remove-chain-info [chain-id]
    CLI->>MsgServer: Send MsgRemoveChainInfo
    MsgServer->>Keeper: RemoveChainInfo(chain-id)
    Keeper-->>MsgServer: Confirmation of removal
    MsgServer-->>CLI: Response success
    CLI-->>User: Display success message
Loading

Assessment against linked issues

Objective Addressed Explanation
Update MsgUpdateChainInfo to only provide one chain (2815)
Add a new message to remove an existing chain (2815)

Possibly related PRs

Suggested labels

chain:solana


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>, please review it.
    -- Generate unit testing code 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 testing code 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 gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    -- @coderabbitai read src/utils.ts and generate unit testing code.
    -- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    -- @coderabbitai help me debug CodeRabbit configuration file.

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 using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

codecov bot commented Sep 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.20%. Comparing base (9b704a5) to head (cbb96cb).
Report is 1 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2890      +/-   ##
===========================================
+ Coverage    67.12%   67.20%   +0.07%     
===========================================
  Files          378      380       +2     
  Lines        21121    21170      +49     
===========================================
+ Hits         14178    14227      +49     
  Misses        6276     6276              
  Partials       667      667              
Files with missing lines Coverage Δ
x/authority/keeper/msg_server_remove_chain_info.go 100.00% <100.00%> (ø)
x/authority/keeper/msg_server_update_chain_info.go 100.00% <100.00%> (ø)
x/authority/types/authorization_list.go 100.00% <ø> (ø)
x/authority/types/message_remove_chain_info.go 100.00% <100.00%> (ø)
x/authority/types/message_update_chain_info.go 100.00% <100.00%> (ø)

@kingpinXD kingpinXD changed the title refactor: message update chain infor refactor: message update chain info Sep 17, 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.

Actionable comments posted: 8

Outside diff range and nitpick comments (4)
docs/spec/authority/messages.md (2)

16-24: The changes to MsgUpdateChainInfo enhance clarity and align with the PR objectives.

The modifications to the message documentation provide a clearer understanding of how the message operates regarding the management of chain information. The changes are consistent with the PR objectives and the linked issue, which aim to streamline the process of managing chain information and enhance the clarity and functionality of the system.

Suggestion: Fix the formatting issues.

The static analysis hints indicate some formatting issues related to the use of hard tabs. Consider replacing the hard tabs with spaces to improve the consistency and readability of the documentation.

Tools
Markdownlint

22-22: Column: 1
Hard tabs

(MD010, no-hard-tabs)


23-23: Column: 1
Hard tabs

(MD010, no-hard-tabs)


27-34: The introduction of MsgRemoveChainInfo enhances the functionality and aligns with the PR objectives.

The addition of the MsgRemoveChainInfo message expands the functionality related to chain management by allowing for the removal of chain information based on a specified chain ID. This change reflects a more comprehensive approach to handling chain information, enabling both updates and removals. The message is consistent with the PR objectives and the linked issue, which emphasize the necessity of introducing a new message type that allows for the removal of an existing chain.

Suggestion: Fix the formatting issues.

The static analysis hints indicate some formatting issues related to the use of hard tabs. Consider replacing the hard tabs with spaces to improve the consistency and readability of the documentation.

Tools
Markdownlint

33-33: Column: 1
Hard tabs

(MD010, no-hard-tabs)


34-34: Column: 1
Hard tabs

(MD010, no-hard-tabs)

x/authority/keeper/msg_server_update_chain_info.go (1)

Line range hint 12-49: Consider refactoring the function to separate the update and add logic.

The UpdateChainInfo function currently handles both updating an existing chain and adding a new chain. To improve readability and maintainability, consider refactoring the function to separate these two responsibilities into separate methods.

For example, you can create two separate methods:

func (k msgServer) updateExistingChain(ctx sdk.Context, chainInfo types.ChainInfo, chain types.Chain) (types.ChainInfo, error) {
    for i, existingChain := range chainInfo.Chains {
        if existingChain.ChainId == chain.ChainId {
            chainInfo.Chains[i] = chain
            return chainInfo, nil
        }
    }
    return chainInfo, errors.Wrap(types.ErrChainNotFound, "chain not found")
}

func (k msgServer) addNewChain(ctx sdk.Context, chainInfo types.ChainInfo, chain types.Chain) types.ChainInfo {
    chainInfo.Chains = append(chainInfo.Chains, chain)
    return chainInfo
}

Then, you can update the UpdateChainInfo function to use these methods:

func (k msgServer) UpdateChainInfo(goCtx context.Context, msg *types.MsgUpdateChainInfo) (*types.MsgUpdateChainInfoResponse, error) {
    // ...

    chainInfo := types.ChainInfo{}
    if found {
        chainInfo = cf
        updatedChainInfo, err := k.updateExistingChain(ctx, chainInfo, msg.Chain)
        if err != nil && !errors.Is(err, types.ErrChainNotFound) {
            return nil, err
        }
        if err == nil {
            chainInfo = updatedChainInfo
        }
    }

    if len(chainInfo.Chains) == 0 || errors.Is(err, types.ErrChainNotFound) {
        chainInfo = k.addNewChain(ctx, chainInfo, msg.Chain)
    }

    k.SetChainInfo(ctx, chainInfo)
    return &types.MsgUpdateChainInfoResponse{}, nil
}

This refactoring makes the code more readable and maintainable by separating the update and add logic into separate methods.

x/authority/keeper/msg_server_update_chain_info_test.go (1)

106-106: Fix the typo in the test case name.

Please change "dos" to "does" in the test case name for clarity.

Apply this diff to fix the typo:

-t.Run("add chain to chain info if chain dos not exist", func(t *testing.T) {
+t.Run("add chain to chain info if chain does not exist", func(t *testing.T) {
Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 77cf636 and 6906d53.

Files ignored due to path filters (2)
  • typescript/zetachain/zetacore/authority/tx_pb.d.ts is excluded by !**/*_pb.d.ts
  • x/authority/types/tx.pb.go is excluded by !**/*.pb.go, !**/*.pb.go
Files selected for processing (23)
  • changelog.md (1 hunks)
  • docs/cli/zetacored/cli.md (2 hunks)
  • docs/openapi/openapi.swagger.yaml (1 hunks)
  • docs/releases/v21_breaking_changes.md (1 hunks)
  • docs/spec/authority/messages.md (1 hunks)
  • proto/zetachain/zetacore/authority/tx.proto (3 hunks)
  • testutil/testdata/types/chain.json (1 hunks)
  • x/authority/client/cli/tx.go (1 hunks)
  • x/authority/client/cli/tx_remove_chain_info.go (1 hunks)
  • x/authority/client/cli/tx_update_chain_info.go (3 hunks)
  • x/authority/client/cli/tx_update_chain_info_test.go (1 hunks)
  • x/authority/keeper/msg_server_remove_chain_info.go (1 hunks)
  • x/authority/keeper/msg_server_remove_chain_info_test.go (1 hunks)
  • x/authority/keeper/msg_server_update_chain_info.go (2 hunks)
  • x/authority/keeper/msg_server_update_chain_info_test.go (4 hunks)
  • x/authority/types/authorization_list.go (1 hunks)
  • x/authority/types/authorization_list_test.go (1 hunks)
  • x/authority/types/codec.go (1 hunks)
  • x/authority/types/errors.go (1 hunks)
  • x/authority/types/message_remove_chain_info.go (1 hunks)
  • x/authority/types/message_remove_chain_info_test.go (1 hunks)
  • x/authority/types/message_update_chain_info.go (2 hunks)
  • x/authority/types/message_update_chain_info_test.go (3 hunks)
Additional context used
Path-based instructions (17)
x/authority/client/cli/tx.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

x/authority/types/codec.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

x/authority/client/cli/tx_update_chain_info_test.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

x/authority/client/cli/tx_remove_chain_info.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

x/authority/types/errors.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

x/authority/types/message_remove_chain_info.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

x/authority/types/message_update_chain_info.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

x/authority/client/cli/tx_update_chain_info.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

x/authority/keeper/msg_server_remove_chain_info.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

x/authority/keeper/msg_server_update_chain_info.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

x/authority/types/message_update_chain_info_test.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

x/authority/types/message_remove_chain_info_test.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

proto/zetachain/zetacore/authority/tx.proto (1)

Pattern **/*.proto: Review the Protobuf definitions, point out issues relative to compatibility, and expressiveness.

x/authority/keeper/msg_server_update_chain_info_test.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

x/authority/types/authorization_list.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

x/authority/keeper/msg_server_remove_chain_info_test.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

x/authority/types/authorization_list_test.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

Markdownlint
docs/releases/v21_breaking_changes.md

4-4: Expected: h2; Actual: h3
Heading levels should only increment by one level at a time

(MD001, heading-increment)


7-7: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)


8-8: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)


9-9: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)


10-10: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)


11-11: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)

docs/spec/authority/messages.md

22-22: Column: 1
Hard tabs

(MD010, no-hard-tabs)


23-23: Column: 1
Hard tabs

(MD010, no-hard-tabs)


33-33: Column: 1
Hard tabs

(MD010, no-hard-tabs)


34-34: Column: 1
Hard tabs

(MD010, no-hard-tabs)

docs/cli/zetacored/cli.md

8177-8177: Column: 88
Hard tabs

(MD010, no-hard-tabs)


8338-8338: Column: 52
Hard tabs

(MD010, no-hard-tabs)


8291-8291: null
Fenced code blocks should have a language specified

(MD040, fenced-code-language)


8297-8297: null
Fenced code blocks should have a language specified

(MD040, fenced-code-language)


8328-8328: null
Fenced code blocks should have a language specified

(MD040, fenced-code-language)

Additional comments not posted (41)
docs/openapi/openapi.swagger.yaml (1)

56841-56843: LGTM!

The addition of the authorityMsgRemoveChainInfoResponse object definition enhances the API documentation by providing a clear description of the MsgRemoveChainInfoResponse service. The change is well-structured, follows the existing naming conventions, and improves the overall clarity and usability of the API specification.

testutil/testdata/types/chain.json (1)

1-9: LGTM! Consider updating with real-world configurations before deployment.

The JSON object provides a well-structured configuration for a blockchain entity, encapsulating essential parameters for its operation. The fields and their assigned values appear to be placeholders or test values, which is appropriate for a test data file.

However, before deploying this configuration to a production environment, ensure that the values are updated with real-world configurations that align with the intended blockchain network and its associated components.

docs/releases/v21_breaking_changes.md (1)

6-11: The content accurately describes the breaking changes.

The content in this section clearly and concisely describes the changes to the update_chain_info message and the introduction of the new RemoveChainInfo transaction type. The description provides sufficient details about the changes and their implications for users.

Tools
Markdownlint

7-7: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)


8-8: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)


9-9: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)


10-10: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)


11-11: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)

x/authority/client/cli/tx.go (1)

25-25: LGTM: The addition of the CmdRemoveChainInfo() command enhances the chain management functionality.

The inclusion of the CmdRemoveChainInfo() command in the GetTxCmd() function aligns with the PR objectives and the linked issue, which emphasize the need for a new message type to remove an existing chain. This addition contributes to a more comprehensive chain management capability, allowing users to remove chain information through the command-line interface.

The placement and syntax of the command are correct, and no issues are apparent.

x/authority/types/codec.go (2)

13-13: LGTM!

The registration of MsgRemoveChainInfo as a concrete type in the codec is done correctly, following the same pattern as other message types. This is necessary for proper serialization and deserialization of the message.


20-20: LGTM!

The registration of MsgRemoveChainInfo as an implementation of the sdk.Msg interface is done correctly, following the same pattern as other message types. This is necessary for the message to be recognized as a valid message type in the SDK.

x/authority/client/cli/tx_update_chain_info_test.go (1)

13-36: Excellent restructuring of the test function.

The changes made to the TestReadChainInfoFromFile function significantly improve the clarity and organization of the test cases. By utilizing t.Run() to create sub-tests, the code clearly delineates the different scenarios being tested, making it easier to understand and maintain.

The first sub-test, "successfully read file," effectively verifies that the cli.ReadChainFromFile function correctly reads a valid chain configuration from the specified JSON file. It compares the returned chain against an expected chains.Chain structure, ensuring the accuracy of the reading functionality.

The second sub-test, "file not found," appropriately tests the error handling of the cli.ReadChainFromFile function when attempting to read from a non-existent file. It asserts that an error is returned, confirming the function's ability to handle this edge case gracefully.

Overall, the restructured test function covers both positive and negative scenarios, enhancing the robustness and reliability of the cli.ReadChainFromFile function. The changes adhere to best practices for writing clear and comprehensive test cases.

x/authority/client/cli/tx_remove_chain_info.go (1)

14-39: Excellent implementation of the CLI command for removing chain information.

The CmdRemoveChainInfo function is well-structured, follows best practices, and effectively implements the desired functionality. The command usage, description, and arguments are clearly defined, and the error handling is appropriate.

The function correctly retrieves the transaction context, parses the chain ID, constructs a message of type MsgRemoveChainInfo, and generates or broadcasts the transaction using the CLI's transaction handling capabilities. The command flags are also properly added to the command.

Overall, the implementation is clean, concise, and production-grade.

x/authority/types/errors.go (1)

15-16: LGTM!

The new error variable ErrChainInfoNotFound is declared and registered correctly, following the existing code style and conventions in the file. The error code is unique and the error message is clear and concise, indicating that the chain info was not found.

The addition of this error variable enhances the error handling capabilities of the module by providing a specific error for cases where chain information is not found, improving the clarity and specificity of error reporting.

x/authority/types/message_update_chain_info.go (3)

7-8: LGTM!

The import statement is necessary and correctly added to use the chains.Chain type in the file.


15-18: LGTM!

The modification to the NewMsgUpdateChainInfo function signature aligns with the objective of refactoring the MsgUpdateChainInfo to accept a single chain. The usage of the chains.Chain type provides a more specific representation of chain information.


48-51: LGTM!

The update to the validation logic in the ValidateBasic method correctly reflects the change in the MsgUpdateChainInfo structure to use the chains.Chain type. The validation is performed by calling the Validate method on the Chain field, ensuring that the chain adheres to its own validation rules.

x/authority/client/cli/tx_update_chain_info.go (2)

24-24: LGTM!

The function has been updated to use the new ReadChainFromFile function, which aligns with the refactored functionality. The logic is correct, and the implementation is accurate.


46-54: LGTM!

The function has been appropriately renamed and refactored to align with the updated functionality. The logic is correct, and the implementation is accurate. The changes in the function name and return type provide clarity and consistency with the broader refactor.

x/authority/keeper/msg_server_remove_chain_info.go (2)

12-33: LGTM!

The RemoveChainInfo function is well-structured and follows the expected flow:

  1. It checks the authorization and returns an error if unauthorized.
  2. It retrieves the existing chain info and returns an error if not found.
  3. It calls RemoveChain to remove the specified chain from the chain info.
  4. It saves the updated chain info back to the store.

The implementation is correct and aligns with the PR objectives.


35-45: LGTM!

The RemoveChain function is a clean and concise implementation for removing a chain from the ChainInfo object:

  1. It iterates through the existing chains.
  2. It only adds the chains that don't match the specified chain ID to the new ChainInfo object.
  3. If the specified chain ID doesn't exist, the original chain info is returned unchanged.

The implementation is correct and aligns with the PR objectives.

x/authority/types/message_update_chain_info_test.go (6)

22-22: LGTM!

The test case is creating a valid message using the NewMsgUpdateChainInfo function with a sample address and chain. The usage of the sample.Chain function to create a sample chain with an ID of 42 is appropriate.


26-26: LGTM!

The test case is correctly testing the scenario where an invalid creator address is provided to the NewMsgUpdateChainInfo function. The usage of the sample.Chain function to create a sample chain with an ID of 42 is appropriate, and the expected error containing the string "invalid creator address" is being checked.


30-32: LGTM!

The test case is correctly testing the scenario where an invalid chain with a negative ID is provided to the NewMsgUpdateChainInfo function. The expected error containing the string "invalid chain" is being checked.


57-57: LGTM!

The test case is correctly testing the scenario where a valid signer is provided to the NewMsgUpdateChainInfo function. The usage of the sample.Chain function to create a sample chain with an ID of 42 is appropriate, and the test case expects no panic when calling the GetSigners method on the message.


62-62: LGTM!

The test case is correctly testing the scenario where an invalid signer is provided to the NewMsgUpdateChainInfo function. The usage of the sample.Chain function to create a sample chain with an ID of 42 is appropriate, and the test case expects a panic when calling the GetSigners method on the message.


82-82: LGTM!

The remaining changes in the TestMsgUpdateChainInfo_Type, TestMsgUpdateChainInfo_Route, and TestMsgUpdateChainInfo_GetSignBytes test functions are using the NewMsgUpdateChainInfo function with a sample address and chain to create a valid message for testing the Type, Route, and GetSignBytes methods of the MsgUpdateChainInfo message. The changes are appropriate and there are no apparent issues.

Also applies to: 87-87, 92-92

x/authority/types/message_remove_chain_info_test.go (4)

13-40: LGTM!

The test function TestMsgRemoveChainInfo_ValidateBasic is well-structured and covers the essential scenarios for testing the ValidateBasic method of MsgRemoveChainInfo. The use of table-driven tests and the sample.AccAddress() function for generating valid addresses is a good practice. The test cases also check for specific error messages when an error is expected, which is a thorough approach.


42-73: LGTM!

The test function TestMsgRemoveChainInfo_GetSigners is well-structured and covers the essential scenarios for testing the GetSigners method of MsgRemoveChainInfo. The use of table-driven tests and the sample.AccAddress() function for generating valid addresses is a good practice. The test cases also check for panics when an invalid signer is provided, which is a good way to ensure the expected behavior.


75-78: LGTM!

The test function TestMsgRemoveChainInfo_Type is simple and straightforward. It checks if the returned message type matches the expected constant value, which is sufficient for testing the Type method of MsgRemoveChainInfo.


80-83: LGTM!

The test function TestMsgRemoveChainInfo_Route is simple and straightforward. It checks if the returned route key matches the expected constant value, which is sufficient for testing the Route method of MsgRemoveChainInfo.

proto/zetachain/zetacore/authority/tx.proto (5)

7-7: Import statement looks good.

The import statement for chains.proto follows the expected convention and suggests a dependency on chain-related definitions, which aligns with the objectives of this pull request.


16-16: RPC method for removing chain information looks good.

The RemoveChainInfo RPC method follows the naming convention, enhances the functionality of the authority service, and has a consistent signature with the other methods in the service.


58-58: Improved chain information representation in MsgUpdateChainInfo.

The change from ChainInfo to pkg.chains.Chain suggests a more structured and detailed representation of chain data, which aligns with the objectives of this pull request. The updated field name chain is more concise and consistent with the message name. The (gogoproto.nullable) = false option ensures that the field is required, preventing ambiguity and potential errors.


64-68: New MsgRemoveChainInfo message type for chain removal.

The MsgRemoveChainInfo message type follows the naming convention and includes the essential fields required for chain removal: creator (the address of the user initiating the removal) and chain_id (the identifier of the chain to be removed). The message structure is simple and focused, which aligns with the objectives of this pull request and enhances the clarity of the chain removal process.


70-71: New MsgRemoveChainInfoResponse message type for confirmation.

The MsgRemoveChainInfoResponse message type follows the naming convention and serves as a confirmation of the chain removal operation initiated by the MsgRemoveChainInfo message. Although the message is empty, its presence maintains consistency with the other message types in the service and provides a clear indication of the successful completion of the chain removal process.

x/authority/keeper/msg_server_update_chain_info_test.go (3)

Line range hint 32-62: LGTM!

The test case is well-structured and correctly verifies the behavior of setting a new chain info when it doesn't exist. The assertions are appropriate and cover the necessary conditions.


81-103: LGTM!

The test case effectively verifies the behavior of updating an existing chain info. It sets up the initial state, updates the chain name, and correctly asserts that the stored chain info reflects the updated name.


Line range hint 106-136: LGTM!

The test case correctly verifies the behavior of adding a new chain to the existing chain info. It sets up the initial state, creates a new chain with a different ID, and asserts that the stored chain info is updated and contains the new chain.

x/authority/types/authorization_list.go (1)

42-42: LGTM!

The addition of the new message URL "/zetachain.zetacore.authority.MsgRemoveChainInfo" to the AdminPolicyMessages slice is appropriate and consistent with the existing code structure. It suggests a new functionality to remove chain info, which aligns with the admin policy's responsibilities.

x/authority/keeper/msg_server_remove_chain_info_test.go (2)

15-155: LGTM!

The TestMsgServer_RemoveChainInfo function thoroughly tests the RemoveChainInfo method of the MsgServer under various conditions. The test cases cover the important scenarios, and the assertions are correct. The test function is well-structured and follows the Arrange-Act-Assert pattern.


157-184: LGTM!

The TestMsgServer_RemoveChain function thoroughly tests the RemoveChain helper function using a table-driven testing approach. The test cases cover the important scenarios, and the assertions are correct. The test function is well-structured and easy to understand.

x/authority/types/authorization_list_test.go (1)

433-433: LGTM!

The addition of the MsgRemoveChainInfo message type to the admin policy message list is consistent with the PR objective and the linked issue. The code change is straightforward and logically sound.

changelog.md (1)

19-19: Changelog entry looks good.

The entry accurately describes the refactoring of MsgUpdateChainInfo and the addition of MsgRemoveChainInfo. The pull request number is provided for traceability.

docs/cli/zetacored/cli.md (2)

8177-8177: LGTM!

The addition of the new zetacored tx authority remove-chain-info command in the table of contents is valid and consistent with the surrounding entries.

Tools
Markdownlint

8177-8177: Column: 88
Hard tabs

(MD010, no-hard-tabs)


8287-8339: LGTM!

The addition of the new zetacored tx authority remove-chain-info command section is valid and follows the structure of other command sections in the document. The description, usage, options, and options inherited from parent commands are clearly documented.

Tools
Markdownlint

8338-8338: Column: 52
Hard tabs

(MD010, no-hard-tabs)


8291-8291: null
Fenced code blocks should have a language specified

(MD040, fenced-code-language)


8297-8297: null
Fenced code blocks should have a language specified

(MD040, fenced-code-language)


8328-8328: null
Fenced code blocks should have a language specified

(MD040, fenced-code-language)

docs/releases/v21_breaking_changes.md Show resolved Hide resolved
docs/releases/v21_breaking_changes.md Outdated Show resolved Hide resolved
docs/cli/zetacored/cli.md Show resolved Hide resolved
docs/cli/zetacored/cli.md Show resolved Hide resolved
docs/cli/zetacored/cli.md Show resolved Hide resolved
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
@kingpinXD kingpinXD changed the title refactor: message update chain info refactor(authority): message update chain info to update single chains Sep 17, 2024
@lumtis lumtis added this pull request to the merge queue Sep 20, 2024
Merged via the queue into develop with commit bb62bc1 Sep 20, 2024
31 checks passed
@lumtis lumtis deleted the refactor-msgupdatechaininfo branch September 20, 2024 16:20
@coderabbitai coderabbitai bot mentioned this pull request Oct 17, 2024
5 tasks
@coderabbitai coderabbitai bot mentioned this pull request Nov 1, 2024
5 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.

Update MsgUpdateChainInfo to only provide one chain
5 participants