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

Problem: sender check for MsgStoreBlockList is not in CheckTx #1613

Merged
merged 8 commits into from
Oct 8, 2024

Conversation

mmsqe
Copy link
Collaborator

@mmsqe mmsqe commented Sep 30, 2024

👮🏻👮🏻👮🏻 !!!! REFERENCE THE PROBLEM YOUR ARE SOLVING IN THE PR TITLE AND DESCRIBE YOUR SOLUTION HERE !!!! DO NOT FORGET !!!! 👮🏻👮🏻👮🏻

PR Checklist:

  • Have you read the CONTRIBUTING.md?
  • Does your PR follow the C4 patch requirements?
  • Have you rebased your work on top of the latest master?
  • Have you checked your code compiles? (make)
  • Have you included tests for any non-trivial functionality?
  • Have you checked your code passes the unit tests? (make test)
  • Have you checked your code formatting is correct? (go fmt)
  • Have you checked your basic code style is fine? (golangci-lint run)
  • If you added any dependencies, have you checked they do not contain any known vulnerabilities? (go list -json -m all | nancy sleuth)
  • If your changes affect the client infrastructure, have you run the integration test?
  • If your changes affect public APIs, does your PR follow the C4 evolution of public contracts?
  • If your code changes public APIs, have you incremented the crate version numbers and documented your changes in the CHANGELOG.md?
  • If you are contributing for the first time, please read the agreement in CONTRIBUTING.md now and add a comment to this pull request stating that your PR is in accordance with the Developer's Certificate of Origin.

Thank you for your code, it's appreciated! :)

Summary by CodeRabbit

  • New Features

    • Introduced a mechanism to check sender authorization for MsgStoreBlockList messages.
    • Added support for a new message type, MsgUpdateTokenMapping.
  • Bug Fixes

    • Enhanced validation logic for the MsgStoreBlockList message type.
    • Various bug fixes implemented, including updates to the ethermint API and corrections to account queries and multisig account handling.
  • Tests

    • Added comprehensive test cases for validating the MsgStoreBlockList, including scenarios for valid inputs and error handling.

@mmsqe mmsqe requested a review from a team as a code owner September 30, 2024 01:49
@mmsqe mmsqe requested review from JayT106 and thomas-nguy and removed request for a team September 30, 2024 01:49
Copy link
Contributor

coderabbitai bot commented Sep 30, 2024

Walkthrough

The changes introduced in this pull request include updates to the CHANGELOG.md to document various improvements and bug fixes. The memiavl module's version logic is modified for compatibility with iavl version 1.2.0, and the default parallelism of block-stm is adjusted. The ethermint API is updated to prevent unnecessary block results, and support for pebbledb is fixed. Additionally, the x/cronos/types/messages.go file sees the removal of several message type constants, the introduction of maximum length validation for blob messages, and new tests added to validate these changes.

Changes

Files Change Summary
CHANGELOG.md Updated to reflect changes, including compatibility updates and bug fixes.
app/app.go Modified setAnteHandler to include app.CronosKeeper.GetParams in NewBlockAddressesDecorator.
app/block_address.go Updated NewBlockAddressesDecorator to include a new parameter for retrieving parameters; enhanced authorization checks in AnteHandle.
x/cronos/types/messages.go Removed several message type constants; added TypeMsgUpdateTokenMapping and updated ValidateBasic for MsgStoreBlockList.
x/cronos/types/messages_test.go Introduced TestValidateMsgStoreBlockList to validate the new message type logic and edge cases.

Possibly related PRs

Suggested reviewers

  • calvinaco
  • thomas-nguy

Poem

In the meadow where bunnies hop,
A new rule for blobs, we cheer and stop!
Lengths now checked, no more will they stray,
With validation in place, we dance and play!
Hopping with joy, our code's now so bright,
Thank you, dear devs, for making it right! 🐇✨


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between f7405ce and a7e864c.

📒 Files selected for processing (1)
  • app/block_address.go (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • app/block_address.go

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.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • 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/schema.v2.json

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 30, 2024

Codecov Report

Attention: Patch coverage is 28.57143% with 5 lines in your changes missing coverage. Please review.

Project coverage is 36.31%. Comparing base (fea4dec) to head (a7e864c).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
app/block_address.go 28.57% 5 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1613      +/-   ##
==========================================
+ Coverage   36.00%   36.31%   +0.31%     
==========================================
  Files         123      123              
  Lines        9791     9715      -76     
==========================================
+ Hits         3525     3528       +3     
+ Misses       5851     5773      -78     
+ Partials      415      414       -1     
Files with missing lines Coverage Δ
x/cronos/types/messages.go 48.95% <ø> (+22.94%) ⬆️
x/e2ee/types/msg.go 71.42% <ø> (+29.76%) ⬆️
app/block_address.go 21.05% <28.57%> (-2.03%) ⬇️

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: 1

🧹 Outside diff range and nitpick comments (5)
x/cronos/types/messages_test.go (1)

66-69: Suggestion: Improve error handling in key parsing.

The current implementation uses log.Fatalf which will terminate the program if the key parsing fails. In a test context, it might be more appropriate to use t.Fatalf to fail the test instead.

Consider replacing the error handling with:

-	if err != nil {
-		log.Fatalf("Failed to parse public key %q: %v", publicKey, err)
-	}
+	require.NoError(t, err, fmt.Sprintf("Failed to parse public key %q", publicKey))

This change will provide more consistent error reporting within the test framework.

x/cronos/types/messages.go (3)

338-339: LGTM. Consider adding a comment explaining the chosen value.

The introduction of MaximumBlobLength is a good practice to limit the size of the Blob field. However, it would be beneficial to add a comment explaining why 20480 (20 KB) was chosen as the maximum length. This helps future maintainers understand the reasoning behind this limit.

Consider adding a comment like:

// MaximumBlobLength defines the maximum allowed length for the Blob field.
// It is set to 20 KB to [explain the reason for this specific limit].
const MaximumBlobLength = 20480

347-350: LGTM. Consider a minor optimization.

The new validation logic for the blob length is correct and well-implemented. It properly enforces the MaximumBlobLength limit and provides a clear error message.

For a minor optimization, consider storing the length in a variable to avoid calling len(msg.Blob) twice:

blobLength := len(msg.Blob)
if blobLength > MaximumBlobLength {
    return errors.Wrapf(sdkerrors.ErrInvalidRequest, "block length %d must not exceed %d", blobLength, MaximumBlobLength)
}

This change would slightly improve readability and prevent redundant length calculations.


355-356: LGTM. Consider clarifying the comment slightly.

The added comment provides valuable context for the use of errDummyIdentity in the Decrypt operation. It's helpful in explaining the rationale behind this error handling approach.

To improve clarity, consider slightly rewording the comment:

// Use errDummyIdentity for early return to skip heavy decryption operation.
// This approach is based on the implementation in:
// https://github.com/FiloSottile/age/blob/v1.1.1/age.go#L197

This rewording makes the purpose of using errDummyIdentity more explicit and separates the explanation from the reference link.

CHANGELOG.md (1)

Line range hint 13-29: Consider adding release date for v1.0.7

For better version tracking and historical reference, it would be helpful to include the release date for v1.0.7 in the changelog.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 3d3cfed and 5935f05.

📒 Files selected for processing (3)
  • CHANGELOG.md (1 hunks)
  • x/cronos/types/messages.go (1 hunks)
  • x/cronos/types/messages_test.go (2 hunks)
🧰 Additional context used
🪛 Gitleaks
x/cronos/types/messages_test.go

65-65: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.

(generic-api-key)

🔇 Additional comments (5)
x/cronos/types/messages_test.go (2)

17-17: LGTM: Bech32 prefix configuration added.

The addition of cmdcfg.SetBech32Prefixes(sdk.GetConfig()) ensures consistent address formatting across tests, which is a good practice for maintaining test reliability.


62-135: LGTM: Comprehensive test cases for MsgStoreBlockList validation.

The new test function TestValidateMsgStoreBlockList effectively addresses the PR objective of validating maximum length for blob messages. The test cases cover various scenarios, including valid input, exceeding maximum blob length, invalid sender address, and decryption errors. The use of age encryption adds a layer of security to the blob storage mechanism.

🧰 Tools
🪛 Gitleaks

65-65: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.

(generic-api-key)

x/cronos/types/messages.go (1)

338-356: Overall, the changes look good and address the PR objectives.

The modifications to x/cronos/types/messages.go successfully implement max length validation for blob messages in the MsgStoreBlockList struct. The changes are well-implemented, consistent with the existing code style, and improve the robustness of the system.

Key improvements:

  1. Introduction of MaximumBlobLength constant.
  2. Implementation of blob length validation in ValidateBasic.
  3. Addition of a helpful comment explaining the errDummyIdentity usage.

The suggestions provided in the review are minor and aimed at further improving code clarity and efficiency. Overall, these changes effectively address the issue of "no max length validation for blob msg" as described in the PR objectives.

CHANGELOG.md (2)

11-11: LGTM: New feature added to changelog

The addition of max length validation for blob messages is correctly documented in the changelog. This aligns with the PR objectives mentioned earlier.


Line range hint 1-1023: LGTM: Well-structured and informative changelog

The changelog is well-organized, providing a clear history of changes across multiple versions. It consistently categorizes changes (e.g., "State Machine Breaking", "Bug Fixes", "Features", "Improvements") and often includes links to relevant GitHub issues or pull requests. This structure makes it easy for users and developers to understand the project's evolution.

x/cronos/types/messages.go Outdated 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.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 5935f05 and cc638bf.

📒 Files selected for processing (3)
  • CHANGELOG.md (1 hunks)
  • x/cronos/types/messages.go (1 hunks)
  • x/cronos/types/messages_test.go (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • CHANGELOG.md
  • x/cronos/types/messages.go
🧰 Additional context used
🪛 Gitleaks
x/cronos/types/messages_test.go

65-65: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.

(generic-api-key)

🔇 Additional comments (5)
x/cronos/types/messages_test.go (5)

4-11: LGTM: New imports added for the test function.

The new imports are correctly added to support the functionality of the new test function TestValidateMsgStoreBlockList.


17-17: LGTM: Proper test environment setup.

The addition of cmdcfg.SetBech32Prefixes(sdk.GetConfig()) ensures that the Bech32 prefixes are correctly set for the test environment, which is a good practice for maintaining consistent test conditions.


62-135: LGTM: Comprehensive test coverage for MsgStoreBlockList validation.

The new test function TestValidateMsgStoreBlockList provides good coverage of various scenarios:

  1. Valid message
  2. Blob exceeding maximum length
  3. Invalid sender address
  4. Decryption error

The test structure is clear and follows good practices. The use of the age package for encryption testing is appropriate.

🧰 Tools
🪛 Gitleaks

65-65: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.

(generic-api-key)


115-124: LGTM: Well-implemented encryption logic for test cases.

The encryption logic is correctly implemented using the age package. It's applied conditionally based on the noEncrypt flag, which allows for testing both encrypted and unencrypted scenarios. The use of a buffer to store encrypted data is efficient, and error handling is properly done with require.NoError.


126-132: LGTM: Thorough validation and error checking.

The validation and error checking logic is well-implemented. It correctly calls ValidateBasic() on the message and checks for expected errors, including their specific content. The differentiation between expected errors and unexpected successes ensures that the test cases are properly validated.

x/cronos/types/messages_test.go 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.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (2)
x/cronos/types/messages.go (2)

203-215: Approve changes to ValidateBasic method with a minor suggestion.

The modifications to the ValidateBasic method of MsgStoreBlockList are well-implemented:

  1. The length check using MaximumBlobLength is correctly added to enforce the blob size limit.
  2. The added comment explains the purpose of the early return with errDummyIdentity, improving code readability.

These changes enhance the robustness of the validation process for blob messages.

Consider extracting the error message for the length check into a constant for better maintainability:

+const errMsgBlobTooLarge = "block length %d must not exceed %d"

 func (msg *MsgStoreBlockList) ValidateBasic() error {
 	length := len(msg.Blob)
 	if length > MaximumBlobLength {
-		return errors.Wrapf(sdkerrors.ErrInvalidRequest, "block length %d must not exceed %d", length, MaximumBlobLength)
+		return errors.Wrapf(sdkerrors.ErrInvalidRequest, errMsgBlobTooLarge, length, MaximumBlobLength)
 	}
 	// ... rest of the method
 }

This minor change would make it easier to update the error message in the future if needed.


Line range hint 1-215: Summary of changes and their impact

This PR successfully implements the max length validation for blob messages as stated in the objectives. The changes include:

  1. Introduction of MaximumBlobLength constant (20KB limit).
  2. Implementation of length validation in MsgStoreBlockList.ValidateBasic().

Additionally, several significant changes were made that were not explicitly mentioned in the PR objectives:

  1. Removal of multiple message type constants (TypeMsgConvertVouchers, TypeMsgTransferTokens, TypeMsgTurnBridge, TypeMsgUpdatePermissions, TypeMsgStoreBlockList).
  2. Removal of associated methods for these message types.

These removals represent a substantial change to the module's API and functionality. Please ensure that:

  • These changes are thoroughly documented in the CHANGELOG.
  • Any dependent modules or external systems are updated accordingly.
  • The impact of these removals on the overall system architecture is well understood and communicated to the team.

Consider creating a migration guide or deprecation notice for users of the removed message types, if applicable. This will help ensure a smooth transition for any systems relying on the removed functionality.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between cc638bf and dd51c58.

📒 Files selected for processing (3)
  • CHANGELOG.md (1 hunks)
  • x/cronos/types/messages.go (2 hunks)
  • x/e2ee/types/msg.go (0 hunks)
💤 Files with no reviewable changes (1)
  • x/e2ee/types/msg.go
🚧 Files skipped from review as they are similar to previous changes (1)
  • CHANGELOG.md
🧰 Additional context used
🔇 Additional comments (2)
x/cronos/types/messages.go (2)

194-195: Approve the introduction of MaximumBlobLength constant.

The addition of MaximumBlobLength constant with a value of 20480 (20KB) is a good practice to enforce a size limit on blob messages. This helps prevent potential issues related to processing or storing excessively large blobs.

Regarding the past discussion about whether 20KB is sufficient:

  1. The chosen limit seems reasonable for most use cases.
  2. As mentioned in the previous comments, there's already a max transaction size limit in place, which provides an additional safeguard.

However, to ensure this limit is appropriate:

  1. Verify that 20KB is sufficient for all valid use cases of blob messages in your system.
  2. Consider making this value configurable (e.g., through governance or a module parameter) to allow for future adjustments if needed.

To help determine if the 20KB limit is appropriate, you can analyze the current blob sizes in your system:

#!/bin/bash
# Analyze blob sizes in the codebase
rg --type go -A 10 "MsgStoreBlockList" | rg "Blob:" | awk '{print length($2)}' | sort -n | uniq -c

This command will show the distribution of blob sizes, helping you confirm if the 20KB limit is suitable for your use case.


15-15: Verify the impact of removed message type constants.

The removal of several message type constants (TypeMsgConvertVouchers, TypeMsgTransferTokens, TypeMsgTurnBridge, TypeMsgUpdatePermissions, TypeMsgStoreBlockList) suggests significant changes in the module's functionality. Please ensure that:

  1. These message types are no longer used anywhere in the codebase.
  2. Any external systems or modules depending on these message types have been updated accordingly.
  3. The removal of these constants is documented in the CHANGELOG.

To confirm that the removed constants are not used elsewhere in the codebase, run the following command:

If this command returns any results, those occurrences need to be addressed.

@mmsqe mmsqe marked this pull request as draft October 7, 2024 13:32
@mmsqe mmsqe changed the title Problem: no max length validation for blob msg Problem: sender check for MsgStoreBlockList is not in CheckTx Oct 8, 2024
@mmsqe mmsqe marked this pull request as ready for review October 8, 2024 01:32
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: 2

🧹 Outside diff range and nitpick comments (5)
x/cronos/types/messages.go (3)

205-210: Approve changes with suggestion for additional validation

The added validation logic improves the security of the MsgStoreBlockList by ensuring the blob is properly encrypted. The comment provides clarity on the purpose of using errDummyIdentity.

However, consider adding a maximum length check for the blob to prevent potential performance issues with extremely large blobs.

Consider adding a maximum length check for the blob before attempting decryption. For example:

func (msg *MsgStoreBlockList) ValidateBasic() error {
	_, err := sdk.AccAddressFromBech32(msg.From)
	if err != nil {
		return errors.Wrapf(sdkerrors.ErrInvalidAddress, "invalid sender address (%s)", err)
	}
+	if len(msg.Blob) > MaxBlobSize {
+		return errors.Wrapf(sdkerrors.ErrInvalidRequest, "blob size exceeds maximum allowed (%d > %d)", len(msg.Blob), MaxBlobSize)
+	}
	// skip heavy operation in Decrypt by early return with errDummyIdentity in
	// https://github.com/FiloSottile/age/blob/v1.1.1/age.go#L197
	_, err = age.Decrypt(bytes.NewBuffer(msg.Blob), new(dummyIdentity))
	if err != nil && err != errDummyIdentity {
		return err
	}
	return nil
}

Make sure to define MaxBlobSize as a constant at the package level.


Line range hint 1-210: Summary of changes and recommendation for thorough testing

This file has undergone significant changes, including the removal of several message types and the addition of new validation logic. While the visible changes appear appropriate, the extent of the removals suggests a larger refactoring effort.

Given the scope of these changes:

  1. Ensure that all affected parts of the codebase have been updated to reflect these changes.
  2. Update any relevant documentation to reflect the removed message types and new validation logic.
  3. Conduct thorough testing, including integration tests, to verify that these changes don't introduce any regressions or unexpected behavior.
  4. Consider updating the CHANGELOG.md to reflect these significant changes.

Consider creating a migration guide if these changes affect external integrations or APIs.


Issues Found with Removed Message Types and Methods

The removal of several message type constants and their associated methods has led to multiple references remaining in the codebase. This will likely cause compilation errors and disrupt existing functionality.

Affected Files:

  • x/cronos/types/codec.go
  • x/cronos/types/messages_test.go
  • x/cronos/types/messages.go
  • x/cronos/types/tx.pb.go
  • x/cronos/keeper/msg_server.go
  • x/cronos/handler_test.go
  • x/cronos/client/cli/tx.go
  • app/block_address.go

Please address these references to ensure the codebase remains consistent and functional.

🔗 Analysis chain

Line range hint 1-210: Verify impact of removed message types and methods

The AI-generated summary indicates that several message type constants and their associated methods have been removed from this file. While these removals are not directly visible in the provided code, it's crucial to ensure that they don't negatively impact the existing functionality or cause issues in other parts of the codebase.

Please run the following script to check for any remaining references to the removed message types:

This script will help identify any remaining references to the removed message types and structs in the codebase. If any references are found, they may need to be updated or removed to maintain consistency with these changes.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for references to removed message types in the codebase

# List of removed message types
removed_types=(
  "TypeMsgConvertVouchers"
  "TypeMsgTransferTokens"
  "TypeMsgUpdateParams"
  "TypeMsgTurnBridge"
  "TypeMsgUpdatePermissions"
  "TypeMsgStoreBlockList"
)

# Search for references to removed types
for type in "${removed_types[@]}"; do
  echo "Searching for references to $type:"
  rg --type go "$type"
  echo
done

# Search for uses of removed message structs
echo "Searching for uses of removed message structs:"
rg --type go "MsgConvertVouchers|MsgTransferTokens|MsgTurnBridge|MsgUpdatePermissions|MsgStoreBlockList"

Length of output: 32680

CHANGELOG.md (2)

Line range hint 27-36: Multiple improvements and bug fixes in dependencies.

This update includes several important changes:

  1. Upgrade to Cosmos SDK v0.47.5 and IBC-Go v7.2.0
  2. Support for stateful precompiled contracts for relayer and ICA
  3. Removal of the Gravity module
  4. Increase in max block gas limit
  5. Support for IBC callback

These changes represent significant improvements in functionality and performance. However, they also introduce breaking changes that may require updates to dependent applications.

Consider providing a migration guide or upgrade instructions for users and developers to ensure a smooth transition to this new version.


Line range hint 1-1185: Overall assessment of v1.1.0-rc0 changes

The changes introduced in v1.1.0-rc0 represent a significant update to the Cronos blockchain. Key improvements include:

  1. Enhanced security with admin sender checks for MsgStoreBlockList
  2. Critical bug fixes for genesis migration, state synchronization, and data integrity
  3. Major dependency upgrades (Cosmos SDK, IBC-Go)
  4. Introduction of stateful precompiled contracts for relayer and ICA
  5. Removal of the Gravity module
  6. Increased max block gas limit
  7. Support for IBC callback

These changes should improve the overall performance, security, and functionality of the Cronos blockchain. However, the breaking changes, particularly the removal of the Gravity module and the introduction of new features, may require significant updates to dependent applications and tools.

  1. Provide comprehensive upgrade instructions and migration guides for node operators and developers.
  2. Consider a phased rollout or testnet period to ensure smooth transition and identify any potential issues before mainnet deployment.
  3. Communicate clearly with the community about the implications of these changes, especially the removal of the Gravity module and how it might affect existing integrations.
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between dd51c58 and e250f69.

📒 Files selected for processing (5)
  • CHANGELOG.md (1 hunks)
  • app/app.go (1 hunks)
  • app/block_address.go (2 hunks)
  • x/cronos/types/messages.go (2 hunks)
  • x/cronos/types/messages_test.go (2 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
app/block_address.go

[warning] 42-46: app/block_address.go#L42-L46
Added lines #L42 - L46 were not covered by tests

🪛 Gitleaks
x/cronos/types/messages_test.go

65-65: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.

(generic-api-key)

🔇 Additional comments (13)
x/cronos/types/messages_test.go (4)

17-17: LGTM: Improved test setup with Bech32 prefixes.

The addition of cmdcfg.SetBech32Prefixes(sdk.GetConfig()) enhances the test environment by configuring the correct Bech32 address prefixes. This change ensures that the test conditions more accurately reflect the real blockchain environment.


65-65: Consider moving the hardcoded public key to a test constant.

While the current implementation works for testing purposes, it's generally a good practice to avoid hardcoding sensitive information directly in the code. Consider moving this public key to a separate test constant or configuration file to improve maintainability and reduce the risk of accidentally exposing sensitive information.

🧰 Tools
🪛 Gitleaks

65-65: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.

(generic-api-key)


62-125: LGTM: Well-structured test function with good coverage.

The TestValidateMsgStoreBlockList function is well-implemented and covers important test cases:

  1. Valid message scenario
  2. Invalid sender address scenario
  3. Decryption error scenario

The use of the age package for encryption testing is appropriate, and the test structure allows for easy addition of more test cases if needed in the future.

🧰 Tools
🪛 Gitleaks

65-65: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.

(generic-api-key)


Line range hint 1-125: Summary: Improved test coverage and environment setup.

The changes in this file enhance the test suite in two ways:

  1. The existing TestValidateMsgUpdateTokenMapping function now includes proper Bech32 prefix setup, ensuring a more accurate test environment.
  2. The new TestValidateMsgStoreBlockList function provides comprehensive testing for the MsgStoreBlockList message type, including encryption scenarios.

These improvements contribute to a more robust and reliable test suite for the Cronos blockchain types. The only minor suggestion is to consider moving the hardcoded public key to a constant for better maintainability.

🧰 Tools
🪛 Gitleaks

65-65: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.

(generic-api-key)

x/cronos/types/messages.go (1)

15-15: LGTM: New constant added for MsgUpdateTokenMapping

The addition of TypeMsgUpdateTokenMapping constant is consistent with the existing code structure and naming conventions.

CHANGELOG.md (4)

18-18: New feature: Check admin sender for MsgStoreBlockList in check tx.

This change adds a security check for the admin sender when processing MsgStoreBlockList transactions. This is an important security improvement that helps prevent unauthorized modifications to the block list.


Line range hint 22-23: Bug fix: Fix genesis migrate for feeibc, evm, feemarket and gravity.

This fix addresses migration issues for several modules during genesis. It's crucial for ensuring proper initialization of the chain state after upgrades.


Line range hint 24-24: Bug fix: Fix lock issues when state-sync with memiavl.

Resolving lock issues during state synchronization is important for maintaining data consistency and preventing potential deadlocks.


Line range hint 25-25: Bug fix: Fix memiavl's unsafe retain of the root hashes.

Addressing unsafe retention of root hashes in memiavl is crucial for maintaining data integrity and preventing potential security vulnerabilities.

app/app.go (1)

1109-1109: LGTM. Verify impact on transaction processing.

The addition of app.CronosKeeper.GetParams to the NewBlockAddressesDecorator initialization looks good. This change allows the decorator to access dynamic parameters from the Cronos module, which could enhance the flexibility of blocked address handling.

To ensure this change doesn't introduce unexpected behavior, please run the following verification:

✅ Verification successful

Verified: NewBlockAddressesDecorator correctly integrates GetParams, ensuring blocked addresses are properly handled in transaction processing.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any changes in transaction processing related to blocked addresses

# Test: Search for usage of GetParams in BlockAddressesDecorator
rg --type go 'func.*BlockAddressesDecorator' -A 10 | grep 'GetParams'

# Test: Look for any changes in transaction processing logic related to blocked addresses
rg --type go 'func.*checkBlockedAddress' -A 20

Length of output: 118


Script:

#!/bin/bash
# Description: Search for all usages of GetParams in the codebase

rg --type go 'GetParams'

# Description: Retrieve all instances where NewBlockAddressesDecorator is initialized
rg --type go 'NewBlockAddressesDecorator' -A 5

# Description: Search for any changes in transaction processing functions related to blocked addresses
rg --type go 'checkBlockedAddress' -A 20

Length of output: 2704


Script:

#!/bin/bash
# Description: Search for functions related to blocked address validation with varied naming

rg --type go 'func.*(validate|is|check).*BlockedAddress' -A 20

# Description: Retrieve all usages of BlockAddressesDecorator to ensure it's integrated correctly
rg --type go 'BlockAddressesDecorator' -A 10

# Description: Search for middleware or handlers that incorporate decorators
rg --type go 'middleware' -A 50 | grep 'BlockAddressesDecorator'

# Description: Look for any changes in transaction processing pipelines
rg --type go 'handler' -A 30 | grep -E 'BlockAddressesDecorator|BlockedMap|GetParams'

Length of output: 2671

app/block_address.go (3)

16-16: Addition of getParams field to BlockAddressesDecorator

The getParams function is added to the BlockAddressesDecorator struct to retrieve parameters from the context. This enhances the decorator's ability to access dynamic configuration.


19-22: Update NewBlockAddressesDecorator constructor to include getParams

The constructor now accepts getParams as a parameter, ensuring the decorator is properly initialized with access to the necessary parameters.


25-25: Assign getParams in the struct initialization

The assignment of getParams within the constructor correctly links the parameter retrieval function to the decorator.

app/block_address.go Show resolved Hide resolved
app/block_address.go Show resolved Hide resolved
app/block_address.go Outdated 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.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (2)
app/block_address.go (2)

16-16: Consider adding documentation for the getParams field

Adding a comment to explain the purpose of the getParams function within the BlockAddressesDecorator struct will improve code readability and maintainability.


19-22: Update function documentation to include the new parameter

The NewBlockAddressesDecorator function now accepts a getParams parameter. Please update the function's documentation to describe this parameter and its role in the decorator's behavior.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between e250f69 and f7405ce.

📒 Files selected for processing (1)
  • app/block_address.go (2 hunks)
🧰 Additional context used
🔇 Additional comments (1)
app/block_address.go (1)

43-50: Authorization logic correctly restricts access to MsgStoreBlockList messages

The added authorization check ensures that only the CronosAdmin can submit MsgStoreBlockList messages. This implementation properly enforces the required access control.

This reverts commit f7405ce.
@mmsqe mmsqe enabled auto-merge October 8, 2024 01:59
@mmsqe mmsqe added this pull request to the merge queue Oct 8, 2024
Merged via the queue into crypto-org-chain:main with commit 7c3ac87 Oct 8, 2024
45 of 46 checks passed
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.

2 participants