chore: bump sdk and cometbft #8743
Conversation
PaddyMc
added
V:state/compatible/backport
|
Important Notice This PR modifies an in-repo Go module. It is one of:
The dependent Go modules, especially the root one, will have to be Please follow the instructions below:
Please let us know if you need any help. |
WalkthroughThe pull request introduces multiple updates to the Osmosis project, including new features, bug fixes, and breaking changes. Notable enhancements involve additional queries in the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (9)
CHANGELOG.md (9)
Line range hint
62-63: State Breaking: Change LiquidityNetInDirection return type to sdk mathThis change in return type could affect existing code that relies on the previous return type.
Ensure that all dependent code is updated to handle the new return type. Consider providing migration examples or guidelines for developers.
Line range hint
66-67: State Breaking: Taker fee shareThe introduction of a taker fee share could have significant implications for the economics of the system.
Please provide more details on how this taker fee share works and its potential impact on users and liquidity providers. Consider adding documentation or links to more comprehensive explanations.
Line range hint
72-73: State Breaking: Increase unauthenticated gas to fix fee token issueThis change could affect the economics of transactions on the network.
Please provide more context on the fee token issue and how this increase in unauthenticated gas addresses it. Consider explaining the potential impact on users and validators.
Line range hint
74-75: State Breaking: Param changes for block and cost per byteChanges to these parameters can have significant performance and economic implications.
It would be helpful to provide more details on the specific changes made and their expected impact on the network's performance and transaction costs.
Line range hint
76-77: State Breaking: ExemptUnrestrictedPoolCreatorWhitelistaddresses from pool creation feeThis change introduces a new privilege for certain addresses.
Please clarify how addresses get added to this whitelist and the governance process (if any) for managing it. Consider the potential centralization implications of this feature.
Line range hint
78-81: State Breaking: Upgrade SDK, CometBFT, and Wasmd versionsThese are significant dependency upgrades that could introduce new features, bug fixes, and potential breaking changes.
It's crucial to review the changelogs of these upgraded dependencies and highlight any significant changes that might affect Osmosis users or developers. Consider adding links to the relevant changelogs or upgrade guides.
Line range hint
86-87: State Breaking: Bump cosmwasm-optimizerUpgrading the cosmwasm-optimizer could affect contract compilation and deployment.
Please specify the version bump and any notable changes or improvements that come with this upgrade. Consider mentioning if any action is required from contract developers.
Line range hint
89-91: Config: Disable SQS by default in app.tomlThis change in default configuration could affect existing setups.
Please provide more context on why SQS is being disabled by default and how users who rely on it can re-enable it if needed.
Line range hint
58-108: Overall impression of v26.0.0 releaseThis is a significant release with many state-breaking changes and improvements. While the changelog provides a good overview of the changes, there are several areas where more detailed information would be beneficial:
- For state-breaking changes, consider adding more context on why the change was made and its potential impact.
- Provide links to upgrade guides or migration documentation for complex changes.
- Explain the implications of economic changes (e.g., taker fee share, gas cost changes) on users and validators.
- For dependency upgrades (SDK, CometBFT, Wasmd), highlight key changes or link to their respective changelogs.
- Consider grouping related changes together for better readability.
- For new features or queries, provide brief examples or use cases to illustrate their benefits.
Overall, this release brings many improvements and new features, but the changelog could be enhanced to better guide users and developers through the upgrade process.
Consider expanding the changelog with more detailed information and links to relevant documentation to ensure a smooth upgrade experience for all stakeholders.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (9)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sumosmomath/go.modis excluded by!**/*.modosmomath/go.sumis excluded by!**/*.sum,!**/*.sumosmoutils/go.modis excluded by!**/*.modx/epochs/go.modis excluded by!**/*.modx/epochs/go.sumis excluded by!**/*.sum,!**/*.sumx/ibc-hooks/go.modis excluded by!**/*.modx/ibc-hooks/go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (1)
- CHANGELOG.md (1 hunks)
🔇 Additional comments (6)
CHANGELOG.md (6)
Line range hint
64-65: State Breaking: Prevent Setting Invalid Before Send HookThis is a good security measure to prevent potential vulnerabilities.
Line range hint
68-69: State Breaking: Add additional events in x/lockup, x/superfluid, x/concentratedliquidityAdding new events is generally good for improving observability and integration capabilities.
However, ensure that all event consumers (e.g., indexers, analytics tools) are aware of these new events to prevent any potential issues.
Line range hint
70-71: State Breaking: Add ledger signing to smart account moduleThis is a significant feature addition that enhances security for users with hardware wallets.
Consider providing user documentation or guides on how to use this new feature with ledger devices.
Line range hint
82-83: State Breaking: Fix protorev throws a nil pointerFixing a nil pointer issue is important for stability.
However, it would be helpful to provide more context on where this issue occurred and its potential impact before the fix.
Line range hint
84-85: State Breaking: Update enforce sub-authenticator to be greater than 1 error messageImproving error messages is generally good for developer experience.
Consider providing an example of the new error message for clarity.
Line range hint
93-108: State Compatible ChangesThese changes are compatible with the current state but introduce new features or improvements.
The additions of OTEL wiring, performance improvements, and new queries are all positive changes. However, it would be beneficial to provide more details on some of these changes, such as:
- The impact of the "Minor speedup to CalcExitCFMM shares"
- The purpose and benefits of the new "EstimateTradeBasedOnPriceImpact" query
- The implications of changing the commit timeout and timeout propose values
| * [#8743](https://github.com/osmosis-labs/osmosis/pull/8743) chore: bump sdk and cometbft | ||
|
|
There was a problem hiding this comment.
State Breaking Changes: SDK v50 and Comet v0.38 upgrade
This is a significant upgrade that may require careful consideration during the upgrade process.
Please ensure that all node operators are aware of this major upgrade and have proper upgrade procedures in place. Consider providing detailed upgrade instructions or linking to them in the changelog.
| ### State Machine Breaking | ||
|
|
There was a problem hiding this comment.
State Breaking: Enforce sub-authenticator to be greater than 1
This change could potentially break existing integrations or scripts that rely on the current behavior.
It's important to communicate this change clearly to all developers and users who might be affected. Consider adding a brief explanation of why this change was made and how to adapt to it.
|
Shouldn't we be updating https://github.com/osmosis-labs/cometbft/tree/osmo/v0.38.x instead of creating an entirely new branch? |
|
I am a bit confused on this, everything below 1c07c7c is newly added, there is no way all of these changes are Osmosis specific changes that are not upstreamed right? For reference, this is what it looked like in osmo/v0.38.x https://github.com/osmosis-labs/cometbft/blob/osmo/v0.38.x/CHANGELOG.md#osmosis-specific-info |
|
It seems we are using an osmo/v0.38.12 branch for our comet fork, but are still using the osmo/v0.50.x branch for the sdk. Can we keep the same standard between these repos? EDIT: Actually, I take this back, it seems like osmo/v0.50.x is behind in compared to the tag that you made, so I am a bit confused what the base is that we are using now for the sdk. |
|
Reading the changelog in the sdk fork that you tagged: This commit is from 5 months ago, so I assume this was not updated to match where we forked off the sdk. |
This wasn't as easy as expected, I think creating a new branch is actually the best way to do this. e.g => https://github.com/osmosis-labs/cometbft/blob/osmo/v0.38.12/CHANGELOG.md This is a branch for cometbft now, I think you're right tho, I'll create a branch for both v26 and v27 with the prefix for each, to make it easier to know which is which. && https://github.com/osmosis-labs/cosmos-sdk/tree/osmo-v27/0.50.10 => this is the commit for the cosmos-sdk |
Ah sorry I forgot to update the changelog there will do that now 😓 Commits are recent: |
|
So updated the branches and changelogs of each! Lmk what you think, I think this is the pattern we should follow going forward e.g && https://github.com/osmosis-labs/cometbft/tree/osmo-v26/0.38.12 |
* chore: bump sdk and cometbft * chore: add CHANGELOG * chore: bump sdk and cometbft (cherry picked from commit 6f64b71) # Conflicts: # go.mod # go.sum # osmomath/go.mod # osmomath/go.sum # osmoutils/go.mod # x/epochs/go.sum
* chore: bump sdk and cometbft (#8743) * chore: bump sdk and cometbft * chore: add CHANGELOG * chore: bump sdk and cometbft (cherry picked from commit 6f64b71) # Conflicts: # go.mod # go.sum # osmomath/go.mod # osmomath/go.sum # osmoutils/go.mod # x/epochs/go.sum * chore: fix merge conflicts --------- Co-authored-by: PaddyMc <[email protected]>
What is the purpose of the change
Bumping cometbft and cosmos sdk to the lastest versions:
comet => https://github.com/osmosis-labs/cometbft/releases/tag/v0.38.12-v26-osmo-2
cosmossdk => https://github.com/osmosis-labs/cosmos-sdk/releases/tag/v0.50.10-v26-osmo-2