chore: re-add IntProto and DecProto and deprecate msg #22925
Conversation
julienrbrt
added
the
backport/v0.52.x
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces updates to the Cosmos SDK, focusing on enhancing the framework's functionality and addressing technical debt. The changes span multiple components, including the changelog, protobuf definitions, and type implementations. Key modifications involve adding new message types, deprecating certain methods, improving documentation, and standardizing code formatting. The updates aim to provide more clarity, improve type handling, and prepare the SDK for future developments. Changes
📜 Recent review detailsConfiguration used: .coderabbit.yml 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
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
Documentation and Community
|
julienrbrt
requested review from
facundomedica,
testinginprod,
JulianToledano,
ziscky and
a team
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
CHANGELOG.md (3)
Line range hint
1-2500: Improve version number formatting consistencyThe version numbers in headers are formatted inconsistently throughout the document. Some use
[vX.Y.Z]format while others use## vX.Y.Z. Consider standardizing to one format across the entire changelog for better readability.- # [v0.47.16] + ## [v0.47.16]
Line range hint
1-2500: Standardize changelog entry formattingSome changelog entries start with PR references while others end with them. Consider standardizing the format to always have PR references at the end of entries for better readability.
For example:
- [#13891](https://github.com/cosmos/cosmos-sdk/pull/13891) Introduced a new `DenomOwnersByQuery` query method + Introduced a new `DenomOwnersByQuery` query method [#13891](https://github.com/cosmos/cosmos-sdk/pull/13891)
Line range hint
1-2500: Fix typos and formatting issuesThere are several minor typos and formatting inconsistencies throughout the document:
- Line 167: Extra tilde character "~" should be removed
- Some bullet points use "*" while others use "-", should standardize
- Some code blocks are missing language identifiers
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
types/coin.pb.gois excluded by!**/*.pb.gox/staking/types/staking.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (6)
CHANGELOG.md(1 hunks)api/cosmos/base/v1beta1/coin.pulsar.go(5 hunks)api/cosmos/benchmark/module/v1/module.pulsar.go(1 hunks)proto/cosmos/base/v1beta1/coin.proto(1 hunks)tools/benchmark/proto/cosmos/benchmark/module/v1/module.proto(2 hunks)types/coin.go(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- tools/benchmark/proto/cosmos/benchmark/module/v1/module.proto
- api/cosmos/benchmark/module/v1/module.pulsar.go
- types/coin.go
🧰 Additional context used
📓 Path-based instructions (2)
api/cosmos/base/v1beta1/coin.pulsar.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
CHANGELOG.md (1)
Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
🔇 Additional comments (8)
api/cosmos/base/v1beta1/coin.pulsar.go (6)
986-995: Reintroduction of IntProto with proper deprecation
The IntProto type has been reintroduced and is correctly marked as deprecated with clear guidance to use math.Int directly. This ensures backward compatibility without encouraging new usage.
997-1003: Implementation of ProtoReflect for IntProto
The ProtoReflect method for IntProto is appropriately implemented, facilitating proper message reflection as required by the protobuf standards.
1933-1971: Deprecation comments for IntProto are clear and conform to style guidelines
The deprecation notice for IntProto is clearly stated in the comments, and the formatting aligns with the Go conventions, aiding developers in understanding the change.
2074-2080: Updated message types to include IntProto and DecProto
The file_cosmos_base_v1beta1_coin_proto_msgTypes array has been updated to include the new message types, ensuring they are registered correctly within the protobuf framework.
2119-2142: Exporter functions for IntProto and DecProto are correctly implemented
The exporter functions for both IntProto and DecProto are properly defined, which is necessary for accurate message state management in protobuf.
2150-2151: Message count updated appropriately
The NumMessages field is incremented to accommodate the new message types, maintaining consistency in the file descriptor.
proto/cosmos/base/v1beta1/coin.proto (2)
43-53: Reintroduction of IntProto with appropriate deprecation notice
The IntProto message has been reintroduced and properly marked as deprecated. The deprecation directive is clear, advising users to prefer using math.Int directly, which supports binary marshaling and unmarshaling.
55-65: Reintroduction of DecProto with appropriate deprecation notice
The DecProto message has been reintroduced and correctly marked as deprecated. Users are guided to use math.LegacyDec instead, aligning with the intended migration path.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| x.Int = value.Interface().(string) | ||
| default: | ||
| if fd.IsExtension() { | ||
| panic(fmt.Errorf("proto3 declared messages do not support extensions: cosmos.base.v1beta1.IntProto")) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
| if fd.IsExtension() { | ||
| panic(fmt.Errorf("proto3 declared messages do not support extensions: cosmos.base.v1beta1.IntProto")) | ||
| } | ||
| panic(fmt.Errorf("message cosmos.base.v1beta1.IntProto does not contain field %s", fd.FullName())) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
| x.Dec = value.Interface().(string) | ||
| default: | ||
| if fd.IsExtension() { | ||
| panic(fmt.Errorf("proto3 declared messages do not support extensions: cosmos.base.v1beta1.DecProto")) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
| if fd.IsExtension() { | ||
| panic(fmt.Errorf("proto3 declared messages do not support extensions: cosmos.base.v1beta1.DecProto")) | ||
| } | ||
| panic(fmt.Errorf("message cosmos.base.v1beta1.DecProto does not contain field %s", fd.FullName())) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
| @@ -164,7 +164,7 @@ Every module contains its own CHANGELOG.md. Please refer to the module you are i | |||
| ### API Breaking Changes | |||
|
|
|||
| * (baseapp) [#16244](https://github.com/cosmos/cosmos-sdk/pull/16244) `SetProtocolVersion` has been renamed to `SetAppVersion`. It now updates the consensus params in baseapp's `ParamStore`. | |||
| * (types) [#16918](https://github.com/cosmos/cosmos-sdk/pull/16918) Remove `IntProto` and `DecProto`. Instead, `math.Int` and `math.LegacyDec` should be used respectively. Both types support `Marshal` and `Unmarshal` which should be used for binary marshaling. | |||
| * (types) [#16918](https://github.com/cosmos/cosmos-sdk/pull/16918), [#22925](https://github.com/cosmos/cosmos-sdk/pull/22925) Deprecate `IntProto` and `DecProto`. Instead, `math.Int` and `math.LegacyDec` should be used respectively. Both types support `Marshal` and `Unmarshal` which should be used for binary marshaling. | |||
There was a problem hiding this comment.
This PR link should be updated as well
| @@ -16,16 +16,16 @@ message Module { | |||
| // GenesisParams defines the genesis parameters for the benchmark module. | |||
| message GeneratorParams { | |||
| // seed is the seed for the random number generator. | |||
| uint64 seed = 1; | |||
| uint64 seed = 1; | |||
There was a problem hiding this comment.
doesn't need to be committed
There was a problem hiding this comment.
It does. make proto-gen runs linting on proto files. This means when this file was committed linting wasn't run on it. The entirety of the result of make proto-gen should always be committed.
| @@ -35,3 +35,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ | |||
| # Changelog | |||
|
|
|||
| ## [Unreleased] | |||
|
|
|||
| ## [v0.1.0](https://github.com/cosmos/cosmos-sdk/releases/tag/indexer/postgres/v0.1.0) | |||
There was a problem hiding this comment.
does this need to be included?
There was a problem hiding this comment.
Theoretically it didn't but it saved me a PR to do it here directly.
I don't like to open PRs with one line change, especially when it is docs.
(cherry picked from commit 0a89178) # Conflicts: # api/cosmos/base/v1beta1/coin.pulsar.go # api/cosmos/benchmark/module/v1/module.pulsar.go # indexer/postgres/CHANGELOG.md # x/staking/types/staking.pb.go
… (#22928) Co-authored-by: Julien Robert <[email protected]>
Description
reverts half of: #16918
We shouldn't have removed the type, but only deprecated, as it was proto breaking and it is needed for migrating away from it.
catched by @damiannolan during IBC integration.
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...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers 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...
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
API Breaking Changes
IntProtoandDecProto, and renamedSetProtocolVersiontoSetAppVersion.Documentation
GeneratorParamsstruct.