feat!: introduce MsgTryUpgrade #2880
Conversation
WalkthroughWalkthroughThe Celestia blockchain has introduced a new RPC method 'TryUpgrade' to its upgrade module, allowing votes to be tallied and triggering upgrades at a specified height. This change replaces the previous 'EndBlock' logic with a direct method call that can be invoked via an endpoint. The upgrade process now involves a new message type for initiating upgrade requests and a corresponding response type. Additionally, the upgrade module's internal logic and test cases have been updated to reflect this new approach. Changes
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
x/upgrade/types/msgs.go
Outdated
| func (msg *MsgTryUpgrade) GetSigners() []sdk.AccAddress { | ||
| valAddr, err := sdk.ValAddressFromBech32(msg.Signer) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| return []sdk.AccAddress{sdk.AccAddress(valAddr)} |
There was a problem hiding this comment.
The GetSigners() method for MsgTryUpgrade incorrectly converts the Signer from a Bech32 string to a ValAddress instead of an AccAddress. This should be corrected to avoid potential runtime errors.
- valAddr, err := sdk.ValAddressFromBech32(msg.Signer)
+ accAddr, err := sdk.AccAddressFromBech32(msg.Signer)
- return []sdk.AccAddress{sdk.AccAddress(valAddr)}
+ return []sdk.AccAddress{accAddr}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.
| func (msg *MsgTryUpgrade) GetSigners() []sdk.AccAddress { | |
| valAddr, err := sdk.ValAddressFromBech32(msg.Signer) | |
| if err != nil { | |
| panic(err) | |
| } | |
| return []sdk.AccAddress{sdk.AccAddress(valAddr)} | |
| func (msg *MsgTryUpgrade) GetSigners() []sdk.AccAddress { | |
| accAddr, err := sdk.AccAddressFromBech32(msg.Signer) | |
| if err != nil { | |
| panic(err) | |
| } | |
| return []sdk.AccAddress{accAddr} |
| @@ -13,7 +13,7 @@ import ( | |||
|
|
|||
| // Keeper implements the MsgServer and QueryServer interfaces | |||
| var ( | |||
| _ types.MsgServer = Keeper{} | |||
| _ types.MsgServer = &Keeper{} | |||
| _ types.QueryServer = Keeper{} | |||
There was a problem hiding this comment.
The Keeper struct should consistently implement interfaces either by pointer or by value. The QueryServer interface is still implemented by value, which is inconsistent with the MsgServer interface implementation. Consider updating the QueryServer implementation to use a pointer receiver for consistency.
- _ types.QueryServer = Keeper{}
+ _ types.QueryServer = &Keeper{}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.
| _ types.QueryServer = Keeper{} | |
| _ types.QueryServer = &Keeper{} |
| if hasQuorum { | ||
| k.quorumVersion = version |
There was a problem hiding this comment.
The quorumVersion is set within the TryUpgrade method without checking if it has already been set during the same block, which could lead to overwriting the quorumVersion if TryUpgrade is called multiple times. Consider adding a check to prevent overwriting an already established quorumVersion within the same block.
x/upgrade/types/msgs.go
Outdated
| } | ||
|
|
||
| func (msg *MsgTryUpgrade) GetSigners() []sdk.AccAddress { | ||
| valAddr, err := sdk.ValAddressFromBech32(msg.Signer) |
There was a problem hiding this comment.
[blocking question]
why not just use a normal account address?
There was a problem hiding this comment.
Yeah good catch. This was just copied over from the previous message type. Let me fix it
|
Yes I thought about implementing an upgrade daemon that continually queries the chain and sends the upgrade message when the quorum has been reached |
Addresses: #2877
Introduces a crank message for tallying all the signal messages