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

build(deps): upgrade to [email protected] #22436

Merged
merged 5 commits into from
Nov 6, 2024
Merged

Conversation

kocubinski
Copy link
Member

@kocubinski kocubinski commented Nov 5, 2024

Description

Part of: #22435

IAVL changes diff are from cosmos/iavl#1003, introducing support for saving at version 0.


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, you can find examples of the prefixes below:
  • 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.

Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.

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

  • New Features
    • Updated handling of the initialVersion parameter in the store initialization logic, ensuring it defaults to 1 if set to 0.
  • Dependency Updates
    • Incremented the version of github.com/cosmos/iavl from v1.3.0 to v1.3.1 across multiple modules, ensuring compatibility and potential bug fixes.
    • Introduced new indirect dependencies in some modules, enhancing functionality and testing capabilities.

@kocubinski kocubinski requested a review from a team as a code owner November 5, 2024 15:07
Copy link
Contributor

coderabbitai bot commented Nov 5, 2024

Caution

Review failed

The pull request is closed.

📝 Walkthrough

Walkthrough

The pull request includes updates to multiple go.mod files across various modules within the Cosmos SDK. The primary change involves updating the version of the github.com/cosmos/iavl dependency from v1.3.0 to v1.3.1, which is reflected in several modules. Additionally, some modules have introduced new indirect dependencies or modified existing replace directives. A specific change in the store/iavl/store.go file alters the handling of the initialVersion parameter in the LoadStoreWithInitialVersion function.

Changes

File Path Change Summary
client/v2/go.mod Updated github.com/cosmos/iavl from v1.3.0 to v1.3.1.
go.mod Updated github.com/cosmos/iavl from v1.3.0 to v1.3.1, modified replace directive for github.com/99designs/keyring, updated github.com/syndtr/goleveldb, added retract section.
runtime/v2/go.mod Updated github.com/cosmos/iavl from v1.3.0 to v1.3.1.
server/v2/cometbft/go.mod Updated github.com/cosmos/iavl from v1.3.0 to v1.3.1.
server/v2/go.mod Updated github.com/cosmos/iavl from v1.3.0 to v1.3.1.
simapp/go.mod Updated github.com/cosmos/iavl from v1.3.0 to v1.3.1.
simapp/v2/go.mod Updated github.com/cosmos/iavl from v1.3.0 to v1.3.1.
store/go.mod Updated github.com/cosmos/iavl from v1.0.0-beta.1.0.20240813194616-eb5078efcf9e to v1.3.1, added new indirect dependencies github.com/golang/snappy v0.0.4 and github.com/google/btree v1.1.2.
store/v2/go.mod Updated github.com/cosmos/iavl from v1.0.0-beta.1.0.20240813194616-eb5078efcf9e to v1.3.1.
tests/go.mod Updated github.com/cosmos/iavl from v1.3.0 to v1.3.1, added new dependencies.
x/accounts/defaults/base/go.mod Updated github.com/cosmos/iavl from v1.3.0 to v1.3.1.
x/accounts/defaults/lockup/go.mod Updated github.com/cosmos/iavl from v1.3.0 to v1.3.1.
x/accounts/defaults/multisig/go.mod Updated github.com/cosmos/iavl from v1.3.0 to v1.3.1.
x/accounts/go.mod Updated github.com/cosmos/iavl from v1.3.0 to v1.3.1.
x/authz/go.mod Updated github.com/cosmos/iavl from v1.3.0 to v1.3.1, added new indirect dependency github.com/cosmos/cosmos-db.
x/bank/go.mod Updated github.com/cosmos/iavl from v1.3.0 to v1.3.1.
x/circuit/go.mod Updated github.com/cosmos/iavl from v1.3.0 to v1.3.1.
x/consensus/go.mod Updated github.com/cosmos/iavl from v1.3.0 to v1.3.1.
x/distribution/go.mod Updated github.com/cosmos/iavl from v1.3.0 to v1.3.1.
x/epochs/go.mod Updated github.com/cosmos/iavl from v1.3.0 to v1.3.1.
x/evidence/go.mod Updated github.com/cosmos/iavl from v1.3.0 to v1.3.1.
x/feegrant/go.mod Updated github.com/cosmos/iavl from v1.3.0 to v1.3.1.
x/gov/go.mod Updated github.com/cosmos/iavl from v1.3.0 to v1.3.1.
x/group/go.mod Updated github.com/cosmos/iavl from v1.3.0 to v1.3.1.
x/mint/go.mod Updated github.com/cosmos/iavl from v1.3.0 to v1.3.1.
x/nft/go.mod Updated github.com/cosmos/iavl from v1.3.0 to v1.3.1.
x/params/go.mod Updated github.com/cosmos/iavl from v1.3.0 to v1.3.1.
x/protocolpool/go.mod Updated github.com/cosmos/iavl from v1.3.0 to v1.3.1.
x/slashing/go.mod Updated github.com/cosmos/iavl from v1.3.0 to v1.3.1.
x/staking/go.mod Updated github.com/cosmos/iavl from v1.3.0 to v1.3.1.
x/upgrade/go.mod Updated github.com/cosmos/iavl from v1.3.0 to v1.3.1.
store/iavl/store.go Modified LoadStoreWithInitialVersion to set initialVersion to 1 if it is 0.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant Store
    User->>Store: LoadStoreWithInitialVersion(initialVersion=0)
    Store-->>User: Store initialized with initialVersion=1
Loading

Possibly related PRs

  • build(deps): bump x/tx #22327: This PR updates the cosmossdk.io/x/tx dependency from v0.13.3 to v1.0.0-alpha.1, which is relevant as it involves a significant version change of a dependency that is also updated in the main PR.
  • build(deps): use cosmossdk.io/core v1.0.0-alpha.5 #22369: This PR updates the cosmossdk.io/core dependency from v1.0.0-alpha.4 to v1.0.0-alpha.5, which is related to the overall dependency management context of the main PR, although it does not directly involve the iavl dependency.

Suggested labels

C:orm, C:collections, C:core/testing, C:server/v2 stf, C:server/v2 appmanager

Suggested reviewers

  • kocubinski
  • sontrinh16
  • julienrbrt

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
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 (1)
store/v2/go.mod (1)

Line range hint 3-3: Invalid Go version specified

The Go version 1.23 is incorrect as it does not exist yet. The latest stable version of Go is 1.22.

-go 1.23
+go 1.22
📜 Review details

Configuration used: .coderabbit.yml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 8c24b6b and 05ffc7d.

⛔ Files ignored due to path filters (31)
  • client/v2/go.sum is excluded by !**/*.sum
  • go.sum is excluded by !**/*.sum
  • runtime/v2/go.sum is excluded by !**/*.sum
  • server/v2/cometbft/go.sum is excluded by !**/*.sum
  • server/v2/go.sum is excluded by !**/*.sum
  • simapp/go.sum is excluded by !**/*.sum
  • simapp/v2/go.sum is excluded by !**/*.sum
  • store/go.sum is excluded by !**/*.sum
  • store/v2/go.sum is excluded by !**/*.sum
  • tests/go.sum is excluded by !**/*.sum
  • x/accounts/defaults/base/go.sum is excluded by !**/*.sum
  • x/accounts/defaults/lockup/go.sum is excluded by !**/*.sum
  • x/accounts/defaults/multisig/go.sum is excluded by !**/*.sum
  • x/accounts/go.sum is excluded by !**/*.sum
  • x/authz/go.sum is excluded by !**/*.sum
  • x/bank/go.sum is excluded by !**/*.sum
  • x/circuit/go.sum is excluded by !**/*.sum
  • x/consensus/go.sum is excluded by !**/*.sum
  • x/distribution/go.sum is excluded by !**/*.sum
  • x/epochs/go.sum is excluded by !**/*.sum
  • x/evidence/go.sum is excluded by !**/*.sum
  • x/feegrant/go.sum is excluded by !**/*.sum
  • x/gov/go.sum is excluded by !**/*.sum
  • x/group/go.sum is excluded by !**/*.sum
  • x/mint/go.sum is excluded by !**/*.sum
  • x/nft/go.sum is excluded by !**/*.sum
  • x/params/go.sum is excluded by !**/*.sum
  • x/protocolpool/go.sum is excluded by !**/*.sum
  • x/slashing/go.sum is excluded by !**/*.sum
  • x/staking/go.sum is excluded by !**/*.sum
  • x/upgrade/go.sum is excluded by !**/*.sum
📒 Files selected for processing (31)
  • client/v2/go.mod (1 hunks)
  • go.mod (1 hunks)
  • runtime/v2/go.mod (1 hunks)
  • server/v2/cometbft/go.mod (1 hunks)
  • server/v2/go.mod (1 hunks)
  • simapp/go.mod (1 hunks)
  • simapp/v2/go.mod (1 hunks)
  • store/go.mod (2 hunks)
  • store/v2/go.mod (1 hunks)
  • tests/go.mod (1 hunks)
  • x/accounts/defaults/base/go.mod (1 hunks)
  • x/accounts/defaults/lockup/go.mod (1 hunks)
  • x/accounts/defaults/multisig/go.mod (1 hunks)
  • x/accounts/go.mod (1 hunks)
  • x/authz/go.mod (1 hunks)
  • x/bank/go.mod (1 hunks)
  • x/circuit/go.mod (1 hunks)
  • x/consensus/go.mod (1 hunks)
  • x/distribution/go.mod (1 hunks)
  • x/epochs/go.mod (1 hunks)
  • x/evidence/go.mod (1 hunks)
  • x/feegrant/go.mod (1 hunks)
  • x/gov/go.mod (1 hunks)
  • x/group/go.mod (1 hunks)
  • x/mint/go.mod (1 hunks)
  • x/nft/go.mod (1 hunks)
  • x/params/go.mod (1 hunks)
  • x/protocolpool/go.mod (1 hunks)
  • x/slashing/go.mod (1 hunks)
  • x/staking/go.mod (1 hunks)
  • x/upgrade/go.mod (1 hunks)
✅ Files skipped from review due to trivial changes (22)
  • client/v2/go.mod
  • server/v2/cometbft/go.mod
  • simapp/go.mod
  • tests/go.mod
  • x/accounts/defaults/base/go.mod
  • x/accounts/defaults/multisig/go.mod
  • x/accounts/go.mod
  • x/authz/go.mod
  • x/bank/go.mod
  • x/circuit/go.mod
  • x/consensus/go.mod
  • x/distribution/go.mod
  • x/epochs/go.mod
  • x/evidence/go.mod
  • x/feegrant/go.mod
  • x/gov/go.mod
  • x/group/go.mod
  • x/params/go.mod
  • x/protocolpool/go.mod
  • x/slashing/go.mod
  • x/staking/go.mod
  • x/upgrade/go.mod
🔇 Additional comments (13)
store/go.mod (2)

36-37: LGTM: New indirect dependencies are appropriate.

The new indirect dependencies github.com/golang/snappy v0.0.4 and github.com/google/btree v1.1.2 are stable versions of commonly used libraries, likely pulled in by the iavl upgrade.


15-15: Verify the iavl version upgrade impact.

The upgrade from a beta version (v1.0.0-beta.1.0.20240813194616-eb5078efcf9e) to a stable release (v1.3.1) is a significant change. While this aligns with the PR objective, the version jump could introduce breaking changes.

✅ Verification successful

The iavl v1.3.1 upgrade appears safe to proceed

Based on the code analysis:

  1. The API usage patterns remain consistent across the codebase, with no breaking changes in the way IAVL trees are initialized and used
  2. The core interfaces and methods (NewMutableTree, NewIavlTree, etc.) maintain the same signatures and parameters
  3. The upgrade is part of a coordinated update across multiple modules in the ecosystem
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any breaking changes in the iavl changelog between the versions

# Test: Look for breaking changes or migration notes in the iavl repository
gh repo clone cosmos/iavl /tmp/iavl
cd /tmp/iavl
git log --oneline v1.0.0-beta.1.0.20240813194616-eb5078efcf9e..v1.3.1 | grep -i "breaking\|migration"

Length of output: 478


Script:

#!/bin/bash
# Let's try a different approach to check the impact of iavl changes

# 1. Find all files that import iavl
rg --type go "github.com/cosmos/iavl" -l

# 2. Check for any iavl-specific interfaces or types being used
rg --type go "iavl\." -A 2 -B 2

# 3. Look for any migration or upgrade related files
fd -e go -e md migration
fd -e go -e md upgrade

Length of output: 28496


Script:

#!/bin/bash
# Let's check the changelog and release notes for iavl v1.3.1

# 1. Get the release notes for v1.3.1
gh release view v1.3.1 --repo cosmos/iavl

# 2. Check for any breaking changes in the code usage patterns
rg --type go -B 3 -A 3 "iavl\.New" 

Length of output: 17185

store/v2/go.mod (2)

Line range hint 38-39: LGTM on indirect dependencies

The addition of indirect dependencies github.com/golang/snappy and github.com/google/btree appears to be correct, likely pulled in by the iavl upgrade.


13-13: Verify compatibility with the new iavl version

The upgrade from v1.0.0-beta.1 to v1.3.1 is a significant change. Let's verify the compatibility with other dependencies and check for any breaking changes.

✅ Verification successful

IAVL v1.3.1 upgrade appears compatible with current usage

The verification shows that the IAVL upgrade is safe because:

  • The codebase uses IAVL primarily through wrapper types in store/v2/commitment/iavl/ that abstract the underlying implementation
  • Usage is limited to core tree operations (NewIavlTree, MutableTree, Importer, Exporter) which are stable APIs
  • No deprecated or removed features are being used
  • The implementation follows IAVL's standard configuration patterns and default settings
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for potential compatibility issues with the iavl upgrade

# Test 1: Search for direct iavl imports to identify affected code
echo "Searching for direct iavl imports..."
rg -l 'github.com/cosmos/iavl'

# Test 2: Check if there are any version constraints in other go.mod files
echo "Checking version constraints in other modules..."
fd -e go.mod --exec grep -l "github.com/cosmos/iavl"

# Test 3: Look for iavl-specific type usage that might be affected
echo "Checking for iavl type usage..."
rg "iavl\.(Tree|Node|Proof|Version)"

Length of output: 2194


Script:

#!/bin/bash
# Let's check the specific usage of IAVL in the codebase and version constraints

# Check IAVL usage in store/v2 directory which contains the modified file
echo "Checking IAVL usage in store/v2..."
rg -A 5 "github.com/cosmos/iavl" store/v2/

# Check version constraints in go.mod files
echo "Checking IAVL version constraints in all go.mod files..."
fd -e go.mod --exec sh -c 'echo "=== {} ==="; cat {} | grep -A 2 -B 2 "github.com/cosmos/iavl"'

# Check for breaking changes in CHANGELOG
echo "Checking for any migration notes or breaking changes..."
rg -i "breaking|migration|upgrade" store/v2/CHANGELOG.md || true

# Check the actual IAVL implementation usage
echo "Checking IAVL implementation details..."
rg -t go "iavl\." store/v2/

Length of output: 6541

runtime/v2/go.mod (1)

44-44: LGTM: iavl dependency upgrade.

The upgrade of github.com/cosmos/iavl to v1.3.1 aligns with the PR objective.

Let's verify the changelog of iavl v1.3.1 to ensure there are no breaking changes:

server/v2/go.mod (1)

56-56: LGTM: Clean iavl dependency upgrade

The update from iavl v1.3.0 to v1.3.1 is a minor version bump and aligns with the PR objectives. Since this is an indirect dependency, the change appears safe.

Let's verify that this version is consistent across all go.mod files:

x/accounts/defaults/lockup/go.mod (1)

49-49: Verify the necessity of this indirect dependency.

The update from github.com/cosmos/iavl v1.3.0 to v1.3.1 is marked as indirect. Let's verify if this module actually requires this dependency.

✅ Verification successful

Based on the results, I can generate the final response as we have sufficient information from the shell script output.

The indirect IAVL dependency is necessary and version is consistent with the codebase

The dependency is justified because:

  • IAVL is a core dependency used extensively in the store package (store/v2/commitment/iavl/, store/iavl/, etc.)
  • The version v1.3.1 is consistently used across most modules, with only a few tool-specific exceptions
  • It's marked as indirect because it's pulled in through the store module, which is a fundamental dependency
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check if this module uses iavl directly or indirectly
# and verify version consistency across the codebase

# Test 1: Check for direct usage of iavl in this module
echo "Checking for direct iavl usage in lockup module..."
rg "github.com/cosmos/iavl" --type go

# Test 2: Check iavl versions across all go.mod files
echo "Checking iavl versions across all modules..."
fd -e mod -x grep -l "github.com/cosmos/iavl" {} \; | \
  xargs grep "github.com/cosmos/iavl"

# Test 3: Check which direct dependencies might be pulling in iavl
echo "Checking which dependencies require iavl..."
go mod graph | grep "github.com/cosmos/iavl"

Length of output: 3299

x/nft/go.mod (1)

57-57: LGTM: iavl version bump looks good.

The update of the indirect dependency github.com/cosmos/iavl from v1.3.0 to v1.3.1 aligns with the PR objectives.

Let's verify the version consistency across other modules:

x/mint/go.mod (1)

56-56: LGTM: iavl dependency successfully upgraded to v1.3.1

The change correctly implements the intended upgrade of the iavl library from v1.3.0 to v1.3.1 as an indirect dependency.

Let's verify the consistency of the iavl version across related modules:

go.mod (3)

Line range hint 186-207: LGTM: Well-organized replace directives

The replace directives are clearly organized into three categories (short-lived, module-specific, and long-lived) with proper documentation explaining their purpose.


Line range hint 209-225: LGTM: Well-documented retract directives

Each retracted version or version range includes a clear explanation of why it was retracted, which is essential for maintainers and users.


87-87: LGTM: iavl upgrade to v1.3.1

The upgrade from v1.3.0 to v1.3.1 is a minor version bump, suggesting backward compatibility is maintained.

Let's verify the compatibility with other dependencies:

simapp/v2/go.mod (1)

100-100: LGTM: iavl version bump to v1.3.1

The update from v1.3.0 to v1.3.1 aligns with the PR objectives. As this is an indirect dependency, the impact should be minimal.

Let's verify the version consistency across other modules:

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

📜 Review details

Configuration used: .coderabbit.yml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 8c24b6b and 05ffc7d.

⛔ Files ignored due to path filters (31)
  • client/v2/go.sum is excluded by !**/*.sum
  • go.sum is excluded by !**/*.sum
  • runtime/v2/go.sum is excluded by !**/*.sum
  • server/v2/cometbft/go.sum is excluded by !**/*.sum
  • server/v2/go.sum is excluded by !**/*.sum
  • simapp/go.sum is excluded by !**/*.sum
  • simapp/v2/go.sum is excluded by !**/*.sum
  • store/go.sum is excluded by !**/*.sum
  • store/v2/go.sum is excluded by !**/*.sum
  • tests/go.sum is excluded by !**/*.sum
  • x/accounts/defaults/base/go.sum is excluded by !**/*.sum
  • x/accounts/defaults/lockup/go.sum is excluded by !**/*.sum
  • x/accounts/defaults/multisig/go.sum is excluded by !**/*.sum
  • x/accounts/go.sum is excluded by !**/*.sum
  • x/authz/go.sum is excluded by !**/*.sum
  • x/bank/go.sum is excluded by !**/*.sum
  • x/circuit/go.sum is excluded by !**/*.sum
  • x/consensus/go.sum is excluded by !**/*.sum
  • x/distribution/go.sum is excluded by !**/*.sum
  • x/epochs/go.sum is excluded by !**/*.sum
  • x/evidence/go.sum is excluded by !**/*.sum
  • x/feegrant/go.sum is excluded by !**/*.sum
  • x/gov/go.sum is excluded by !**/*.sum
  • x/group/go.sum is excluded by !**/*.sum
  • x/mint/go.sum is excluded by !**/*.sum
  • x/nft/go.sum is excluded by !**/*.sum
  • x/params/go.sum is excluded by !**/*.sum
  • x/protocolpool/go.sum is excluded by !**/*.sum
  • x/slashing/go.sum is excluded by !**/*.sum
  • x/staking/go.sum is excluded by !**/*.sum
  • x/upgrade/go.sum is excluded by !**/*.sum
📒 Files selected for processing (31)
  • client/v2/go.mod (1 hunks)
  • go.mod (1 hunks)
  • runtime/v2/go.mod (1 hunks)
  • server/v2/cometbft/go.mod (1 hunks)
  • server/v2/go.mod (1 hunks)
  • simapp/go.mod (1 hunks)
  • simapp/v2/go.mod (1 hunks)
  • store/go.mod (2 hunks)
  • store/v2/go.mod (1 hunks)
  • tests/go.mod (1 hunks)
  • x/accounts/defaults/base/go.mod (1 hunks)
  • x/accounts/defaults/lockup/go.mod (1 hunks)
  • x/accounts/defaults/multisig/go.mod (1 hunks)
  • x/accounts/go.mod (1 hunks)
  • x/authz/go.mod (1 hunks)
  • x/bank/go.mod (1 hunks)
  • x/circuit/go.mod (1 hunks)
  • x/consensus/go.mod (1 hunks)
  • x/distribution/go.mod (1 hunks)
  • x/epochs/go.mod (1 hunks)
  • x/evidence/go.mod (1 hunks)
  • x/feegrant/go.mod (1 hunks)
  • x/gov/go.mod (1 hunks)
  • x/group/go.mod (1 hunks)
  • x/mint/go.mod (1 hunks)
  • x/nft/go.mod (1 hunks)
  • x/params/go.mod (1 hunks)
  • x/protocolpool/go.mod (1 hunks)
  • x/slashing/go.mod (1 hunks)
  • x/staking/go.mod (1 hunks)
  • x/upgrade/go.mod (1 hunks)
✅ Files skipped from review due to trivial changes (22)
  • client/v2/go.mod
  • runtime/v2/go.mod
  • server/v2/cometbft/go.mod
  • simapp/go.mod
  • simapp/v2/go.mod
  • x/accounts/defaults/base/go.mod
  • x/accounts/defaults/lockup/go.mod
  • x/accounts/defaults/multisig/go.mod
  • x/accounts/go.mod
  • x/bank/go.mod
  • x/circuit/go.mod
  • x/consensus/go.mod
  • x/distribution/go.mod
  • x/epochs/go.mod
  • x/gov/go.mod
  • x/group/go.mod
  • x/mint/go.mod
  • x/nft/go.mod
  • x/params/go.mod
  • x/protocolpool/go.mod
  • x/slashing/go.mod
  • x/upgrade/go.mod
🧰 Additional context used
📓 Path-based instructions (1)
tests/go.mod (1)

Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"

🔇 Additional comments (13)
store/go.mod (2)

36-37: LGTM: New indirect dependencies

The new indirect dependencies github.com/golang/snappy and github.com/google/btree are automatically added by Go's dependency resolution, likely pulled in by IAVL v1.3.1. Both are well-maintained libraries commonly used in Go projects.


15-15: Verify IAVL v1.3.1 compatibility

The update from a beta version to v1.3.1 is significant. Let's verify the changelog and compatibility.

✅ Verification successful

Let me gather more specific information about the IAVL usage to verify compatibility.


IAVL v1.3.1 upgrade appears safe to proceed

Based on the codebase analysis:

  • The IAVL usage is primarily through stable interfaces (MutableTree, ImmutableTree, Importer)
  • All method calls (Set, Get, Remove, Hash, etc.) are core IAVL functionality that has remained stable
  • No deprecated or beta-specific APIs are being used

The upgrade from beta to v1.3.1 is safe as:

  • The codebase uses standard IAVL interfaces that are part of the stable API
  • New indirect dependencies (snappy, btree) are common libraries used for optimization
  • No breaking changes in the method signatures used by the store module
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check IAVL's changelog and Go API compatibility

# Get the changelog between versions
gh api repos/cosmos/iavl/releases | jq -r '.[].body' | grep -A 10 "v1.3.1"

# Check for potential breaking changes in function signatures
rg -A 2 "github.com/cosmos/iavl" --type go

Length of output: 1642


Script:

#!/bin/bash
# Check IAVL interface usage in the codebase
ast-grep --pattern 'type $_ struct {
  $$$
  tree *iavl.$_
  $$$
}'

# Check IAVL method calls
ast-grep --pattern '$_.tree.$_($$$)'

# Check imported IAVL types and interfaces
rg "import.*iavl" -A 5 --type go

Length of output: 5154

store/v2/go.mod (1)

13-13: Verify compatibility with IAVL v1.3.1

The upgrade from a beta version to v1.3.1 is significant. While the change aligns with the coordinated upgrade across modules, we should verify:

  1. Breaking changes between versions
  2. Impact on consensus-critical operations

Let's check the changelog and release notes:

server/v2/go.mod (1)

56-56: LGTM: iavl version upgrade looks good.

The update from v1.3.0 to v1.3.1 is consistent with the PR objectives. Since iavl is a critical dependency for state management, let's verify there are no breaking changes.

x/staking/go.mod (2)

54-54: LGTM: iavl version update looks correct.

The update from v1.3.0 to v1.3.1 aligns with the PR objectives.


54-54: Verify consistent iavl version across modules.

Let's ensure all modules are updated to use the same iavl version.

x/evidence/go.mod (1)

59-59: Verify IAVL upgrade impact.

The version bump from v1.3.0 to v1.3.1 looks correct. Let's verify the changelog to ensure no breaking changes.

✅ Verification successful

IAVL v1.3.1 upgrade is safe to proceed

The upgrade from IAVL v1.3.0 to v1.3.1 introduces a single non-breaking change that allows saving version 0. This feature addition is backward compatible and doesn't affect existing functionality. The codebase uses IAVL primarily in store-related components, and this change won't impact their behavior.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check IAVL changelog between v1.3.0 and v1.3.1 for breaking changes
# and verify its usage in the codebase

# Get the changelog diff between versions
gh api repos/cosmos/iavl/compare/v1.3.0...v1.3.1 --jq '.commits[].commit.message'

# Find direct IAVL imports in the codebase
rg --type go 'github.com/cosmos/iavl'

Length of output: 780

x/authz/go.mod (1)

56-56: LGTM: iavl version upgrade to v1.3.1

The update of the indirect dependency github.com/cosmos/iavl from v1.3.0 to v1.3.1 aligns with the PR objectives.

Let's verify the version consistency across other modules:

go.mod (3)

Line range hint 4-86: LGTM: Clean dependency management

The PR maintains a focused scope by only updating the IAVL version without introducing any unexpected dependency changes.

Also applies to: 88-185


Line range hint 186-204: LGTM: Replace directives maintained

The existing replace directives are properly maintained and well-documented, with clear separation between temporary and long-lived replacements.


Line range hint 206-224: LGTM: Retract section properly maintained

The retract section remains unchanged and continues to provide clear documentation for versions that should not be used.

tests/go.mod (2)

99-99: LGTM: iavl version update to v1.3.1

The upgrade of the iavl dependency to version 1.3.1 is correctly implemented and properly marked as indirect.


Line range hint 1-24: LGTM: Dependency structure maintains testing integrity

The module configuration properly maintains the testing environment by:

  1. Using appropriate Go version
  2. Correctly organizing replace directives
  3. Ensuring tests run against local SDK modules

Also applies to: 234-289

@@ -64,7 +64,7 @@ require (
github.com/cosmos/crypto v0.1.2 // indirect
github.com/cosmos/go-bip39 v1.0.0 // indirect
github.com/cosmos/gogogateway v1.2.0 // indirect
github.com/cosmos/iavl v1.3.0 // indirect
github.com/cosmos/iavl v1.3.1 // indirect
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

IAVL version inconsistency requires attention

The following modules need to be updated to use IAVL v1.3.1 for consistency:

  • tests/systemtests/go.mod (currently using v1.1.4)
  • tools/confix/go.mod (currently using v1.2.0)
  • tools/cosmovisor/go.mod (currently using v1.2.0)
  • tools/hubl/go.mod (currently using v1.2.0)

While the feegrant module's update is correct, these inconsistencies in test and tool modules should be addressed to maintain uniform dependency versions across the codebase.

🔗 Analysis chain

LGTM: iavl version upgrade to v1.3.1

The update from v1.3.0 to v1.3.1 aligns with the PR objectives. This is an indirect dependency (marked with // indirect), suggesting it's pulled in through another module.

Let's verify the consistency of this upgrade across related modules:


Inconsistent IAVL versions detected across modules

The verification revealed multiple inconsistent versions of github.com/cosmos/iavl across different go.mod files:

  • Most modules use v1.3.1 as intended
  • However, some modules still reference older versions:
    • v1.1.4
    • v1.2.0

Let's identify the specific modules using older versions:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any inconsistent iavl versions across go.mod files
# Expected: All modules should reference the same version (v1.3.1)

echo "Checking iavl versions across all go.mod files..."
fd --type f --glob 'go.mod' --exec rg --no-line-number 'github.com/cosmos/iavl'

Length of output: 1667


Script:

#!/bin/bash
# Find which modules are using older IAVL versions
echo "Modules using older IAVL versions:"
fd --type f --glob 'go.mod' --exec sh -c 'if grep -q "github.com/cosmos/iavl v1.[12]" "{}"; then echo -e "\nFile: {}"; grep "github.com/cosmos/iavl v1." "{}"; fi'

Length of output: 548

@@ -84,7 +84,7 @@ require (
github.com/cockroachdb/redact v1.1.5 // indirect
github.com/cockroachdb/tokenbucket v0.0.0-20230807174530-cc333fc44b06 // indirect
github.com/cometbft/cometbft-db v0.15.0 // indirect
github.com/cosmos/iavl v1.3.0 // indirect
github.com/cosmos/iavl v1.3.1 // indirect
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

Warning: IAVL v1.3.1 is not released yet

While tag v1.3.1 exists in the repository, it has not been officially released. The latest stable release is v1.3.0 from September 6, 2024. Consider using v1.3.0 instead until v1.3.1 is officially released to ensure stability.

🔗 Analysis chain

LGTM: IAVL version upgrade to v1.3.1

The version upgrade matches the PR objective.

Let's verify this is the latest stable version:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check if v1.3.1 is the latest stable release of IAVL
gh release list --repo cosmos/iavl --limit 5

Length of output: 234


Script:

#!/bin/bash
# Let's verify if v1.3.1 exists in the repository tags
gh api repos/cosmos/iavl/tags --paginate | jq -r '.[].name' | grep "v1.3"

Length of output: 87

@julienrbrt
Copy link
Member

CI isn't happy here

@kocubinski
Copy link
Member Author

CI isn't happy here

Yea.. there are some unexpected assumptions in BaseApp around height 0, I should have checked integration with app/v1 first, but was too focused on app/v2 working, sorry about that.

Looking at the logs it may be easier if IAVL v1 aligns with app/v1 assumptions, which is that if SetInitialVersion is not called then the first saved version will be 1. Right now it's 0.

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 (1)
store/iavl/store.go (1)

54-54: Consider updating documentation.

The comment explains the rationale, but consider updating the function's documentation to explicitly mention this behavior for better visibility.

Add this to the function's documentation:

// LoadStoreWithInitialVersion returns an IAVL Store as a CommitKVStore setting its initialVersion
// to the one given. Internally, it will load the store's version (id) from the
// provided DB. An error is returned if the version fails to load, or if called with a positive
// version on an empty tree.
+// Note: If initialVersion is 0, it will be automatically set to 1 to maintain compatibility
+// with store/v1 and app/v1 assumptions.
func LoadStoreWithInitialVersion(...)
📜 Review details

Configuration used: .coderabbit.yml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 03ba454 and c9c31b2.

📒 Files selected for processing (2)
  • baseapp/abci.go (1 hunks)
  • store/iavl/store.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • baseapp/abci.go
🧰 Additional context used
📓 Path-based instructions (1)
store/iavl/store.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

🔇 Additional comments (1)
store/iavl/store.go (1)

54-57: LGTM! Verify impact on existing systems.

The change correctly handles the initial version compatibility between app/v1 and IAVL v1. This aligns with the existing assumptions and fixes the CI issues.

Let's verify if there are any direct usages of version 0:

✅ Verification successful

Change is safe and consistent with existing behavior

The verification shows this change is well-aligned with the codebase:

  • The default LoadStore already uses initialVersion=0, which gets converted to 1
  • Tests handle version initialization properly and don't rely on version 0
  • Height 0 handling is separate from version handling and remains unchanged
  • The change maintains consistency with the existing SetInitialVersion interface
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for potential version 0 usages in tests or initialization code
rg -A 3 "SetInitialVersion\(0\)" || rg -A 3 "initialVersion.*:.*0"

Length of output: 67


Script:

#!/bin/bash
# Let's try a broader search for version-related patterns
rg -i "version.*0" store/iavl/

# Also search for test cases that might be affected
rg -i "initialversion" store/iavl/

# Look for any IAVL tree initialization in tests
rg -i "newtree|newmutabletree" store/iavl/

# Check for any height 0 references
rg "height.*0" store/iavl/

Length of output: 3417

@kocubinski
Copy link
Member Author

kocubinski commented Nov 5, 2024

Added these lines to store/v1 to fix integration, we'll need a store/v1 release after this. simapp/v1 is working because of the replace directive.

// store/v1 and app/v1 flows never require an initial version of 0
if initialVersion == 0 {
initialVersion = 1
}

@tac0turtle tac0turtle added this pull request to the merge queue Nov 6, 2024
@tac0turtle tac0turtle added the backport/v0.52.x PR scheduled for inclusion in the v0.52's next stable release label Nov 6, 2024
Merged via the queue into main with commit 9611c5a Nov 6, 2024
92 of 93 checks passed
@tac0turtle tac0turtle deleted the kocu/iavl-v1.3.1 branch November 6, 2024 09:40
mergify bot pushed a commit that referenced this pull request Nov 6, 2024
(cherry picked from commit 9611c5a)

# Conflicts:
#	go.mod
#	runtime/v2/go.mod
#	runtime/v2/go.sum
#	server/v2/go.mod
#	server/v2/go.sum
#	store/go.mod
#	store/go.sum
#	store/iavl/store.go
#	store/v2/go.mod
#	store/v2/go.sum
alpe added a commit that referenced this pull request Nov 6, 2024
* main: (24 commits)
  build(deps): upgrade to [email protected] (#22436)
  docs: Update tendermint validators query pagination documentation (#22412)
  refactor(client/v2): offchain uses client/v2/factory (#22344)
  feat: wire new handlers to grpc (#22333)
  fix(x/group): proper address rendering in error (#22425)
  refactor(serevr/v2/cometbft): update `RegisterQueryHandlers` and GRPC queries (#22403)
  docs: update ADR 59 (#22423)
  build(deps): Bump github.com/fsnotify/fsnotify from 1.7.0 to 1.8.0 in /tools/cosmovisor (#22407)
  docs: Module account address documentation (#22289)
  feat(baseapp): add per message telemetry (#22175)
  docs: Update Twitter Links to X in Documentation (#22408)
  docs: redirect the remote generation page (#22404)
  refactor(serverv2): remove unused interface methods, honuor context  (#22394)
  fix(server/v2): return ErrHelp (#22399)
  feat(indexer): implement `schema.HasModuleCodec` interface in the `bank` module (#22349)
  refactor(math): refactor ApproxRoot for readality (#22263)
  docs: fix KWallet Handbook (#22395)
  feat(client/v2): broadcast logic (#22282)
  refactor(server/v2): eager config loading (#22267)
  test(system): check feePayers signature (#22389)
  ...
julienrbrt added a commit that referenced this pull request Nov 7, 2024
Co-authored-by: Matt Kocubinski <[email protected]>
Co-authored-by: marbar3778 <[email protected]>
Co-authored-by: Julien Robert <[email protected]>
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.

7 participants