-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
refactor(types)!: removed txEncoder from global config #18695
Conversation
WalkthroughThe cosmos-sdk repository has undergone a significant update, removing the global configuration for Changes
Assessment against linked issues
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 X ? TipsChat with CodeRabbit Bot (
|
CHANGELOG.md
Outdated
* (crypto | x/auth) [#14372](https://github.com/cosmos/cosmos-sdk/pull/18194) Key checks on signatures antehandle. | ||
* (staking) [#18506](https://github.com/cosmos/cosmos-sdk/pull/18506) Detect the length of the ed25519 pubkey in CreateValidator to prevent panic. | ||
* (types) [#18372](https://github.com/cosmos/cosmos-sdk/pull/18372) Removed global configuration for coin type and purpose. Setters and getters should be removed and access directly to defined types. | ||
* (types) [#18695](https://github.com/cosmos/cosmos-sdk/pull/18695) Removed global configuration for txEncoder. | ||
|
||
### Bug Fixes | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [15-15]
The changelog mentions a security vulnerability identified in the x/bank module in the v0.41.x series, which is critical and should be highlighted for users running non-Cosmos Hub chains.
CHANGELOG.md
Outdated
@@ -65,6 +65,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ | |||
* (crypto | x/auth) [#14372](https://github.com/cosmos/cosmos-sdk/pull/18194) Key checks on signatures antehandle. | |||
* (staking) [#18506](https://github.com/cosmos/cosmos-sdk/pull/18506) Detect the length of the ed25519 pubkey in CreateValidator to prevent panic. | |||
* (types) [#18372](https://github.com/cosmos/cosmos-sdk/pull/18372) Removed global configuration for coin type and purpose. Setters and getters should be removed and access directly to defined types. | |||
* (types) [#18695](https://github.com/cosmos/cosmos-sdk/pull/18695) Removed global configuration for txEncoder. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we always precise the alternative when we remove something from the config?
Maybe it would be nice to add an UPGRADING.md section on the global config removals, so there will be one place to look at it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one has not been used anywhere, I think it is deprecated, thats why I didn't add an alternative, TxEncoder is used on other parts but not the one dfined as a global config. I dont see any use cases where someone would want some sort of global txEncoder
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A quick search here: https://sourcegraph.com/search?q=context%3Aglobal+.GetTxEncoder%28%29&patternType=standard&sm=1&groupBy=repo shows indeed that it is only used in v0.45 chains / fork. Then I guess it is fine indeed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you!! looks good other than the comment from julian
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK. However, we need the changelog to be under API breaking instead of improvements. Same for the line just above about #18372 btw.
I still have the same comment about the changelog. |
Description
Closes: #18696
ref #7448
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.md
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.
I have...