Add missing proto files for indexing #8754
Conversation
mattverse
added
the
V:state/compatible/no_backport
mattverse
requested review from
p0mvn,
czarcas7ic and
AlpinYukseloglu
as code owners
WalkthroughThe pull request updates the Changes
Possibly related PRs
Suggested labels
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: 1
🧹 Outside diff range and nitpick comments (4)
proto/osmosis/lockup/tx.proto (3)
122-122: LGTM. Consider adding documentation.The uncommented
MsgSetRewardReceiverAddressResponsemessage looks good. It's a simple and appropriate structure for a response message.Consider adding a comment to explain the purpose of this message and when it's used in the protocol. This will improve the overall documentation of the protobuf file.
124-129: LGTM. Consider enhancing deprecation documentation.The addition of the
MsgUnlockPeriodLockmessage is appropriate. It's correctly marked as deprecated and kept for indexing purposes.To improve clarity for developers, consider adding more detailed documentation:
- Explain why this message is deprecated.
- Provide information on what should be used instead.
- If possible, include a timeline or version for when this message might be removed.
This additional context will help maintainers and users understand the status and future of this message type.
131-133: LGTM, but consider improving consistency and clarity.The addition of the
MsgUnlockTokensmessage is appropriate for maintaining indexing capabilities. However, there are a few points to consider:
This message is implicitly deprecated due to its position under the DEPRECATED comment. For consistency with
MsgUnlockPeriodLock, consider adding an explicit deprecation comment to this message as well.Unlike
MsgUnlockPeriodLock, this message doesn't include anIDfield. If this is intentional, it might be worth adding a comment explaining the difference.As with the previous message, consider adding more detailed documentation about its deprecated status, alternatives, and potential removal timeline.
Apply these suggestions to improve consistency and clarity:
// DEPRECATED // Following messages are deprecated but kept to support indexing. message MsgUnlockPeriodLock { string owner = 1 [ (gogoproto.moretags) = "yaml:\"owner\"" ]; uint64 ID = 2; } +// DEPRECATED message MsgUnlockTokens { string owner = 1 [ (gogoproto.moretags) = "yaml:\"owner\"" ]; }Also, consider adding comments to explain why
MsgUnlockTokensdoesn't have anIDfield if this is intentional.CHANGELOG.md (1)
Line range hint
1-85: Suggestions for improving the changelogWhile the changelog is comprehensive, consider the following improvements:
- Add a brief explanation or context for some of the more technical changes, especially in the API breaks and state breaking sections.
- Include links to relevant documentation or upgrade guides for major features like SuperCharged Liquidity and CosmWasm Pool Module.
- Provide more details on the potential impact of the ProtoRev changes on existing users.
- Consider adding a "Upgrade Instructions" section with a link to detailed upgrade documentation for node operators and developers.
These additions would make the changelog more informative and user-friendly, especially for those who may not be familiar with all aspects of Osmosis development.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
x/lockup/types/tx.pb.gois excluded by!**/*.pb.go,!**/*.pb.gox/protorev/types/tx.pb.gois excluded by!**/*.pb.go,!**/*.pb.go
📒 Files selected for processing (3)
- CHANGELOG.md (1 hunks)
- proto/osmosis/lockup/tx.proto (1 hunks)
- proto/osmosis/protorev/v1beta1/tx.proto (1 hunks)
🔇 Additional comments (8)
proto/osmosis/lockup/tx.proto (1)
122-133: Overall changes look good, with minor suggestions for improvement.The changes in this file are primarily focused on maintaining backwards compatibility for indexing purposes. The uncommented
MsgSetRewardReceiverAddressResponseand the addition of deprecated messagesMsgUnlockPeriodLockandMsgUnlockTokensare appropriate for this goal.Key points:
- The changes don't introduce any significant issues or alterations to the existing protocol.
- The deprecated messages are clearly marked and separated, which is good for maintainability.
- There are some minor inconsistencies in documentation and structure between the deprecated messages.
To improve the overall quality of the file:
- Consider adding more detailed documentation for all new and uncommented messages.
- Ensure consistency in how deprecated messages are marked and documented.
- If possible, provide more context about the deprecation status, alternatives, and potential removal timeline for deprecated messages.
These changes align well with the PR objectives of adding missing proto files for indexing and don't introduce any new features or user-facing behavior changes.
CHANGELOG.md (7)
Line range hint
1-5: Major release with significant changes and new featuresThis release (v19.0.0) introduces several new modules and features, including:
- SuperCharged Liquidity
- CosmWasm Pool Module
- ProtoRev changes
- TokenFactory before send hooks
These additions significantly enhance the functionality of Osmosis, but users and developers should be aware of the potential impact on existing systems.
Line range hint
7-9: Security update: Upgraded wasmvm to address vulnerabilityThe upgrade to wasmvm 1.2.3 addresses the security vulnerability CWA-2023-002. This is a critical security update that all users should apply.
Line range hint
11-20: New features and modules addedThe release includes several major new features:
- x/concentrated-liquidity module for SuperCharged Liquidity
- x/cosmwasmpool module for custom liquidity pools
- Changes to ProtoRev, including payment schedule modifications
- TokenFactory before send hooks
These additions provide new capabilities for Osmosis, but users should be cautious and thoroughly test their integrations with these new features.
Line range hint
22-38: API breaks and potential migration requiredThis release includes several API breaks that may require updates to existing integrations:
- Changes to query endpoints and responses
- Modifications to message structures
- Updates to keeper interfaces
Developers should carefully review these changes and update their code accordingly. It's recommended to test thoroughly in a non-production environment before upgrading.
Line range hint
40-54: State breaking changesThe release includes several state breaking changes, such as:
- Changes to ProtoRev logic
- Updates to x/gamm and x/poolmanager modules
- Modifications to x/mint and x/incentives modules
These changes may affect the blockchain's state and require careful consideration during the upgrade process. Node operators should follow the upgrade instructions closely.
Line range hint
56-60: Dependency updatesThe release includes updates to key dependencies:
- wasmd upgraded to 0.31
- Cosmwasm security patch
- IBC-go update
Ensure that your environment is compatible with these updated dependencies before upgrading.
Line range hint
62-85: Miscellaneous improvements and bug fixesThe release includes various improvements and bug fixes, such as:
- CLI enhancements
- Performance optimizations
- Bug fixes in multiple modules
These changes should improve the overall stability and usability of Osmosis, but users should still test thoroughly after upgrading.
| message MsgSetPoolWeights { | ||
| // admin is the account that is authorized to set the pool weights. | ||
| string admin = 1; | ||
| // pool_weights is the list of pool weights to set. | ||
| PoolWeights pool_weights = 2; | ||
| } |
There was a problem hiding this comment.
Add missing options and field annotations to MsgSetPoolWeights
The new message MsgSetPoolWeights lacks the option declarations and field annotations that are present in other message definitions within the file. This may lead to inconsistent behavior or serialization issues. For consistency and correctness, please add the amino.name and cosmos.msg.v1.signer options, and annotate the fields appropriately.
Here is a suggested fix:
message MsgSetPoolWeights {
+ option (amino.name) = "osmosis/MsgSetPoolWeights";
+ option (cosmos.msg.v1.signer) = "admin";
// admin is the account that is authorized to set the pool weights.
- string admin = 1;
+ string admin = 1 [
+ (gogoproto.moretags) = "yaml:\"admin\"",
+ (cosmos_proto.scalar) = "cosmos.AddressString"
+ ];
// pool_weights is the list of pool weights to set.
- PoolWeights pool_weights = 2;
+ PoolWeights pool_weights = 2 [
+ (gogoproto.moretags) = "yaml:\"pool_weights\"",
+ (gogoproto.nullable) = false
+ ];
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // MsgSetPoolWeights defines the Msg/SetPoolWeights request type. | |
| message MsgSetPoolWeights { | |
| // admin is the account that is authorized to set the pool weights. | |
| string admin = 1; | |
| // pool_weights is the list of pool weights to set. | |
| PoolWeights pool_weights = 2; | |
| } | |
| // MsgSetPoolWeights defines the Msg/SetPoolWeights request type. | |
| message MsgSetPoolWeights { | |
| option (amino.name) = "osmosis/MsgSetPoolWeights"; | |
| option (cosmos.msg.v1.signer) = "admin"; | |
| // admin is the account that is authorized to set the pool weights. | |
| string admin = 1 [ | |
| (gogoproto.moretags) = "yaml:\"admin\"", | |
| (cosmos_proto.scalar) = "cosmos.AddressString" | |
| ]; | |
| // pool_weights is the list of pool weights to set. | |
| PoolWeights pool_weights = 2 [ | |
| (gogoproto.moretags) = "yaml:\"pool_weights\"", | |
| (gogoproto.nullable) = false | |
| ]; | |
| } |
|
Oh were not backporting so my comment isn't important. Forgot were doing a release soon |
Closes: #XXX
What is the purpose of the change
Re: #8600
After some investigation, I've found out that the proto changes in gamm will require separate migration code ( see https://github.com/osmosis-labs/osmosis/blob/main/proto/osmosis/gamm/v1beta1/balancerPool.proto#L2-L5) for more details.
Will change the proto packages , run up a node to confirm that changing gamm package for proto in subsequent PR
Testing and Verifying
This change is a trivial rework / code cleanup without any test coverage.
Documentation and Release Note
Unreleasedsection ofCHANGELOG.md?Where is the change documented?
x/{module}/README.md)