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: multisig account failed on threshold encode after send tx #1611

Merged
merged 1 commit into from
Sep 27, 2024

Conversation

mmsqe
Copy link
Collaborator

@mmsqe mmsqe commented Sep 27, 2024

for more info, https://github.com/crypto-org-chain/ethermint/pull/532/files

👮🏻👮🏻👮🏻 !!!! 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

  • Bug Fixes

    • Resolved an issue with multisig accounts failing on threshold encoding after transaction processing.
  • Chores

    • Updated dependency versions for various modules to ensure compatibility and stability.

@mmsqe mmsqe requested a review from a team as a code owner September 27, 2024 06:12
@mmsqe mmsqe requested review from yihuang and leejw51crypto and removed request for a team September 27, 2024 06:12
Copy link
Contributor

coderabbitai bot commented Sep 27, 2024

Walkthrough

The pull request introduces a bug fix for multisig accounts, specifically addressing an issue with threshold encoding after transaction processing. Additionally, it updates several dependencies in the go.mod and gomod2nix.toml files to newer versions, reflecting recent changes in the Cosmos SDK components. The CHANGELOG.md has been updated to document these changes.

Changes

File Change Summary
CHANGELOG.md Updated to include a bug fix for multisig account threshold encoding after sending a transaction.
go.mod Updated dependency versions for several packages related to the Cosmos SDK.
gomod2nix.toml Updated version numbers and hashes for modules related to the Cosmos SDK.

Possibly related PRs

Suggested reviewers

  • calvinaco
  • thomas-nguy
  • yihuang

🐰 In the code, I hop and play,
Fixing bugs along the way.
Multisig accounts now shine bright,
With encoded thresholds done just right!
Dependencies updated, all is well,
In the land of code, we bid farewell! 🌟


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between e1cf56b and a4a43ca.

⛔ Files ignored due to path filters (1)
  • go.sum is excluded by !**/*.sum
📒 Files selected for processing (3)
  • CHANGELOG.md (1 hunks)
  • go.mod (2 hunks)
  • gomod2nix.toml (5 hunks)
🔇 Additional comments (12)
go.mod (1)

258-261: Verify compatibility and stability of updated dependencies

The replace directives have been updated to point to very recent commits for several critical dependencies:

  1. cosmos-sdk related packages (lines 258-261)
  2. ethermint (line 278)

While using specific commits ensures reproducibility, it's important to consider the following:

  1. These updates are very recent. Ensure that they have been thoroughly tested with your codebase to avoid introducing new bugs or breaking changes.
  2. The cosmos-sdk updates seem coordinated (same timestamp), while the ethermint update has a different timestamp. Verify that these updates are compatible with each other and address the specific issue mentioned in the PR title ("multisig account failed on threshold encode after send tx").
  3. Consider adding comments explaining why these specific commits were chosen, especially if they're addressing known issues.

To ensure these updates resolve the mentioned issue and don't introduce new problems, please run the following verification script:

Also applies to: 278-278

gomod2nix.toml (6)

26-26: Approved: Minor version update for cosmossdk.io/client/v2

The update from v2.0.0-20240926054726-1675d17877a6 to v2.0.0-20240927054437-c2a1e0678f4b appears to be a routine minor version bump. This likely includes bug fixes or small improvements.


51-51: Approved: Minor version update for cosmossdk.io/store

The update from v0.0.0-20240926054726-1675d17877a6 to v0.0.0-20240927054437-c2a1e0678f4b appears to be a routine minor version bump. This likely includes bug fixes or small improvements.


64-65: Approved: Minor version update for cosmossdk.io/x/tx

The update from v0.0.0-20240926054726-1675d17877a6 to v0.0.0-20240927054437-c2a1e0678f4b appears to be a routine minor version bump. The hash update is expected with this change. This likely includes bug fixes or small improvements.


177-177: Approved: Minor version update for github.com/cosmos/cosmos-sdk

The update from v0.50.6-0.20240926054726-1675d17877a6 to v0.50.6-0.20240927054437-c2a1e0678f4b appears to be a routine minor version bump. This likely includes bug fixes or small improvements.


Line range hint 1-1: Summary of dependency updates

This change updates several key dependencies:

  1. cosmossdk.io/client/v2
  2. cosmossdk.io/store
  3. cosmossdk.io/x/tx
  4. github.com/cosmos/cosmos-sdk
  5. github.com/evmos/ethermint

Most of these are minor version bumps, likely including bug fixes and small improvements. The update to github.com/evmos/ethermint appears more substantial and may require additional attention.

These updates are generally beneficial for keeping the project current and secure. However, it's important to:

  1. Ensure all tests pass with these new versions.
  2. Check for any breaking changes, especially in the ethermint update.
  3. Verify that these updates address or don't interfere with the PR's main objective of fixing the multisig account issue.

To ensure compatibility and proper functionality after these updates, consider running the following verification steps:

#!/bin/bash
# Run all tests
go test ./...

# Check for any new deprecation warnings or errors
go vet ./...

# If there's a specific test for multisig functionality, run it explicitly
go test -v -run TestMultisigAccount ./...

266-267: Approved with caution: Significant version update for github.com/evmos/ethermint

The update from v0.6.1-0.20240925024103-f2a562ba9b9f to v0.6.1-0.20240927061036-33e3cc6ed365 appears to be a more substantial change compared to the other updates in this file. While this update is approved, it's recommended to:

  1. Check the changelog for any breaking changes or significant new features.
  2. Ensure that this update aligns with the project's objectives, especially considering the PR's focus on fixing a multisig account issue.
  3. Verify that all dependent code is compatible with this new version.

To verify the changes and potential impacts, you can run the following script:

CHANGELOG.md (5)

Line range hint 1-24: LGTM: Unreleased changes are well documented

The unreleased section of the changelog is well-structured and informative. It includes important bug fixes and improvements:

  1. A fix for multisig account failure on threshold encode after sending a transaction.
  2. Support for specifying channel ID for send-to-ibc events in case of source token.
  3. Enlargement of the max block gas limit in the new version.
  4. Disabling of the gravity module in the app.
  5. Support for IBC callback.

These changes seem significant and are clearly explained. The format is consistent with the rest of the changelog.

🧰 Tools
🪛 Markdownlint

16-16: null
Emphasis used instead of a heading

(MD036, no-emphasis-as-heading)


Line range hint 26-51: Important updates in v1.1.0-rc1

This release candidate includes several significant changes:

  1. State Machine Breaking changes:

    • Updates to Cosmos SDK, IBC-Go, and other dependencies.
    • Support for turnbridge transaction.
    • Implementation of bidirectional token mapping.
    • Integration of IBC fee middleware.
  2. Bug Fixes:

    • Fixed issues with JSON-RPC APIs for failed transactions.
    • Addressed problems with init and validate-genesis commands.
  3. Improvements:

    • Enabled jemalloc memory allocator.
    • Replaced IBC-hook with IBC middleware.
    • Support for basic JSON-RPC APIs on pruned nodes.

These changes appear to be well-documented and significant for the project.

🧰 Tools
🪛 Markdownlint

16-16: null
Emphasis used instead of a heading

(MD036, no-emphasis-as-heading)


Line range hint 53-66: Notable changes in v0.8.0 and v0.7.1

v0.8.0 includes a state machine breaking change related to the selfdestruct operation in smart contracts.

v0.7.1 contains several important bug fixes and improvements:

  • Fixes for JSON-RPC APIs and transaction hash issues.
  • Addition of commands to handle unlucky transactions and reindex duplicated transactions.
  • Improvement in memory allocation and database performance.

These changes address critical issues and improve the overall stability of the system.

🧰 Tools
🪛 Markdownlint

16-16: null
Emphasis used instead of a heading

(MD036, no-emphasis-as-heading)


Line range hint 68-141: Comprehensive update in v0.7.0

This version includes numerous state machine breaking changes, improvements, and bug fixes:

  1. State Machine Breaking:

    • Major updates to Ethermint, IBC-Go, and Cosmos SDK.
    • Changes to EVM hooks, nonce increment, and base fee calculation.
  2. Improvements:

    • Re-enabling of gravity bridge conditionally.
    • Updates to min-gas-price handling for EVM transactions.
    • Enhanced panic information display in query results.
  3. Bug Fixes:

    • Several fixes related to upgrade handlers, OOM issues, and ethermint bugs.
    • Improvements to websocket functionality and transaction gas reporting.

This update seems to be a major overhaul of the system, addressing various issues and introducing new features.

🧰 Tools
🪛 Markdownlint

16-16: null
Emphasis used instead of a heading

(MD036, no-emphasis-as-heading)


Line range hint 143-1000: LGTM: Earlier versions are well documented

The changelog entries for versions 0.6.5 and earlier are well-structured and provide clear information about bug fixes, improvements, and breaking changes. The format is consistent throughout, making it easy for users and developers to understand the evolution of the project.

Some notable points:

  1. Regular updates to dependencies like Ethermint, Cosmos SDK, and IBC-Go.
  2. Consistent attention to bug fixes and performance improvements.
  3. Clear marking of consensus-breaking changes.
  4. Detailed explanations of new features and improvements.

The changelog provides a comprehensive history of the project's development, which is valuable for both developers and users.

🧰 Tools
🪛 Markdownlint

16-16: null
Emphasis used instead of a heading

(MD036, no-emphasis-as-heading)


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

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 36.12%. Comparing base (e1cf56b) to head (a4a43ca).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1611   +/-   ##
=======================================
  Coverage   36.12%   36.12%           
=======================================
  Files          97       97           
  Lines        7725     7725           
=======================================
  Hits         2791     2791           
  Misses       4585     4585           
  Partials      349      349           

@mmsqe mmsqe added this pull request to the merge queue Sep 27, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 27, 2024
@mmsqe mmsqe added this pull request to the merge queue Sep 27, 2024
Merged via the queue into crypto-org-chain:main with commit bbdd407 Sep 27, 2024
43 of 44 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