chore: clean up code by removing unused sections and formatting #732
Conversation
WalkthroughThe changes involve updates to several Go files across different packages, primarily focusing on marking various components as deprecated in the Changes
Possibly related PRs
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 (10)
server/grpc/gasprice/legacy/v2/gas_price.go (3)
12-12: Approved: Clearer deprecation commentThe updated comment format provides more clarity about the deprecated entity. This change aligns well with the PR objective of cleaning up the code.
Consider adding a brief explanation of why this type is deprecated and, if applicable, what should be used instead. For example:
// Deprecated: Querier is no longer supported. Use NewQuerier() from the v3 package instead.
17-17: Approved: Consistent deprecation commentThe updated comment for
GetGasPriceis consistent with the previous change and clearly indicates which method is deprecated.Similar to the previous suggestion, consider adding a brief explanation and alternative if available:
// Deprecated: GetGasPrice will be removed in the next major version. Use GetGasPriceV3() from the v3 package for improved functionality.
Line range hint
27-32: Approved with suggestions: Consistent deprecation comment and error handling concernThe updated comment for
RegisterGRPCGatewayRoutesis consistent with the previous changes and clearly indicates which function is deprecated.Consider adding a brief explanation and alternative if available:
// Deprecated: RegisterGRPCGatewayRoutes will be removed in the next major version. Use RegisterGRPCGatewayRoutesV3() from the v3 package for improved routing.The function still uses a panic for error handling, which might not be ideal for a deprecated function. Consider updating the error handling to return an error instead of panicking:
-func RegisterGRPCGatewayRoutes(clientConn grpc.ClientConn, mux *runtime.ServeMux) { +func RegisterGRPCGatewayRoutes(clientConn grpc.ClientConn, mux *runtime.ServeMux) error { if err := RegisterQueryHandlerClient(context.Background(), mux, NewQueryClient(clientConn)); err != nil { - panic(fmt.Sprintf("failed to %s register grpc gateway routes: %s", "legacy gas price", err.Error())) + return fmt.Errorf("failed to register legacy gas price grpc gateway routes: %w", err) } + return nil }This change would allow callers to handle the error more gracefully, especially important for a deprecated function where unexpected behavior might occur.
x/ibc/middleware/types/packet.go (3)
25-35: LGTM: Pointer receiver change and formatting improvements forValidateBasicThe change to a pointer receiver is consistent with the other methods and improves potential performance. The formatting changes enhance readability.
Consider using
errors.Joinfor combining multiple validation errors, which could make the code more maintainable if additional validations are added in the future.
Line range hint
43-49: LGTM: Pointer receiver change forMustGetData, with a suggestionThe change to a pointer receiver is consistent with the other methods and aligns with the PR objectives. This change potentially improves performance without altering the method's functionality.
Consider renaming this method to
GetDataand returning an error instead of using a panic. This would allow for more graceful error handling:func (m *IbcCallEvmPacket) GetData() ([]byte, error) { return hex.DecodeString(m.Data) }This change would make the method safer to use and more consistent with Go's error handling conventions.
Line range hint
21-49: Summary: Consistent optimization ofIbcCallEvmPacketmethodsThe changes in this file consistently update the method receivers for
IbcCallEvmPacketfrom value receivers to pointer receivers. This optimization can potentially improve performance by avoiding unnecessary struct copying.Key points:
- All four methods (
GetType,ValidateBasic,GetToAddress, andMustGetData) now use pointer receivers.- The functionality of the methods remains unchanged.
- These changes align well with the PR objectives of cleaning up and optimizing the code.
Consider reviewing the entire
IbcCallEvmPacketstruct and its usage throughout the codebase to ensure consistency with these changes. It might be beneficial to update any remaining methods or function parameters that still use value semantics for this struct.x/gov/types/params.go (1)
Line range hint
73-88: LGTM: Correct adaptation toNewCustomParamschange with a minor suggestion.The update to the
NewCustomParamscall innewOtherCustomParamscorrectly reflects the change in theNewCustomParamsfunction signature. The removal of the dereferencing operator*is appropriate and maintains the intended functionality.Consider renaming the
defaultParamsvariable todefaultParam(singular) to better reflect that it's now a value type rather than a pointer. This might improve code clarity:- defaultParams := NewCustomParams(DefaultCustomParamDepositRatio.String(), DefaultCustomParamVotingPeriod, DefaultCustomParamQuorum25.String()) + defaultParam := NewCustomParams(DefaultCustomParamDepositRatio.String(), DefaultCustomParamVotingPeriod, DefaultCustomParamQuorum25.String()) customMsgTypes := []string{ // erc20 proposal sdk.MsgTypeURL(&erc20types.MsgRegisterCoin{}), sdk.MsgTypeURL(&erc20types.MsgRegisterERC20{}), sdk.MsgTypeURL(&erc20types.MsgToggleTokenConversion{}), sdk.MsgTypeURL(&erc20types.MsgUpdateDenomAlias{}), // evm proposal sdk.MsgTypeURL(&evmtypes.MsgCallContract{}), } params := make([]InitGenesisCustomParams, 0, len(customMsgTypes)) for _, msgType := range customMsgTypes { - params = append(params, NewInitGenesisCustomParams(msgType, defaultParams)) + params = append(params, NewInitGenesisCustomParams(msgType, defaultParam)) }x/gov/keeper/msg_server.go (3)
Line range hint
70-72: Enhance Error Message for Invalid Store SpaceIn the
UpdateStoremethod, when an invalid store space is encountered, the error message could be more informative by including the invalid space name. This improves debugging and user experience.Apply this change to include the invalid store space in the error message:
if !ok { - return nil, sdkerrors.ErrInvalidRequest.Wrap("invalid store space") + return nil, sdkerrors.ErrInvalidRequest.Wrapf("invalid store space: %s", updateStore.Space) }
Line range hint
100-106: Incorrect Struct Comparison inUpdateCustomParamsIn
UpdateCustomParams, the comparisonreq.GetCustomParams() == (types.CustomParams{})may not work as intended. In Go, struct comparisons require that all fields are comparable. IfCustomParamscontains non-comparable fields (e.g., slices, maps), this comparison will result in a compilation error or unexpected behavior.To fix this, consider implementing an
IsEmpty()method onCustomParamsthat checks if all fields are zero values, or compare each field individually. For example:func (cp CustomParams) IsEmpty() bool { // Replace with actual field checks return cp.Field1 == "" && cp.Field2 == 0 && len(cp.Field3) == 0 }Then update the condition:
- if req.GetCustomParams() == (types.CustomParams{}) { + if req.GetCustomParams().IsEmpty() {
Line range hint
180-188: Use Case-Sensitive Comparison for Message Type URLsIn the
checkProposalMsgsfunction,strings.EqualFoldis used to compare message type URLs in a case-insensitive manner. SinceMsgTypeURLvalues are case-sensitive identifiers, using a case-insensitive comparison may result in incorrect validations.Replace
strings.EqualFoldwith a case-sensitive comparison to ensure accurate validation:if msgType != "" && !strings.EqualFold(msgType, sdk.MsgTypeURL(pMsg)) { + if msgType != "" && msgType != sdk.MsgTypeURL(pMsg) { return govtypes.ErrInvalidProposalType.Wrapf("proposal MsgTypeURL is different") }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- server/grpc/gasprice/legacy/v1/gas_price.go (2 hunks)
- server/grpc/gasprice/legacy/v2/gas_price.go (2 hunks)
- x/gov/keeper/deposit.go (1 hunks)
- x/gov/keeper/msg_server.go (1 hunks)
- x/gov/types/params.go (3 hunks)
- x/ibc/middleware/types/events.go (1 hunks)
- x/ibc/middleware/types/packet.go (1 hunks)
✅ Files skipped from review due to trivial changes (3)
- server/grpc/gasprice/legacy/v1/gas_price.go
- x/gov/keeper/deposit.go
- x/ibc/middleware/types/events.go
🧰 Additional context used
🔇 Additional comments (5)
x/ibc/middleware/types/packet.go (2)
21-23: LGTM: Pointer receiver change forGetTypeThe change from a value receiver to a pointer receiver is a good optimization. It avoids unnecessary copying of the struct, potentially improving performance without altering the method's functionality.
38-41: LGTM: Pointer receiver change forGetToAddressThe change from a value receiver to a pointer receiver is consistent with the other methods and aligns with the PR objectives. This change potentially improves performance without altering the method's functionality.
x/gov/types/params.go (3)
68-68: LGTM: Correct adaptation toNewCustomParamschange.The update to the
NewCustomParamscall innewEGFCustomParamscorrectly reflects the change in theNewCustomParamsfunction signature. The removal of the dereferencing operator*is appropriate and maintains the intended functionality.
Line range hint
50-55: Approve the change toNewCustomParamsfunction.The modification from returning a pointer to returning a value is a good improvement. It simplifies memory management and potentially improves performance for small structs. This change aligns well with the PR objective of cleaning up the code.
To ensure this change doesn't introduce any issues, let's verify its usage across the codebase:
#!/bin/bash # Description: Check for any remaining dereferences of NewCustomParams calls # Test: Search for any remaining dereferences of NewCustomParams calls # Expect: No results, as all calls should now use the value directly rg --type go '&NewCustomParams\(' # Test: Verify all calls to NewCustomParams have been updated # Expect: All calls should now use the value directly without dereferencing rg --type go 'NewCustomParams\('
Line range hint
1-108: Note on potential removals mentioned in the AI summary.The AI-generated summary mentioned the removal of the
ExtractMsgTypeURLfunction and changes to import statements, particularly the removal ofstringsandcodectypespackages. These changes are not visible in the provided code snippet, possibly because removed lines are not shown.To confirm these changes, please run the following script:
If these elements have indeed been removed, it aligns with the PR objective of cleaning up the code by removing unused sections. However, please ensure that the removal of the
ExtractMsgTypeURLfunction doesn't negatively impact any other parts of the codebase that might have depended on it.✅ Verification successful
Removal of
ExtractMsgTypeURLand Specified Imports ConfirmedThe
ExtractMsgTypeURLfunction and the imports for"strings"and"github.com/cosmos/cosmos-sdk/codec/types"have been successfully removed fromx/gov/types/params.go.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the removal of ExtractMsgTypeURL function and import changes # Test: Check for the presence of ExtractMsgTypeURL function # Expect: No results if the function has been removed rg --type go 'func ExtractMsgTypeURL' x/gov/types/params.go # Test: Check for the presence of strings and codectypes imports # Expect: No results if these imports have been removed rg --type go '^import \($| "strings"| "github.com/cosmos/cosmos-sdk/codec/types"' x/gov/types/params.goLength of output: 603
Script:
#!/bin/bash # Description: Verify the removal of ExtractMsgTypeURL function and import changes # Test: Check for the presence of ExtractMsgTypeURL function # Expect: No results if the function has been removed rg --type go 'func ExtractMsgTypeURL' x/gov/types/params.go # Test: Check for the presence of "strings" and "codec/types" imports # Expect: No results if these imports have been removed rg --type go 'import \(\n\s+"strings"\n\s+"github.com/cosmos/cosmos-sdk/codec/types"\n\)' x/gov/types/params.goLength of output: 367
Script:
#!/bin/bash # Description: Verify the removal of ExtractMsgTypeURL function and import changes with multiline support # Test: Check for the presence of ExtractMsgTypeURL function # Expect: No results if the function has been removed rg --type go 'func ExtractMsgTypeURL' x/gov/types/params.go # Test: Check for the presence of "strings" and "codec/types" imports # Expect: No results if these imports have been removed rg -U --type go 'import \(\n\s+"strings"\n\s+"github.com/cosmos/cosmos-sdk/codec/types"\n\)' x/gov/types/params.goLength of output: 178
Summary by CodeRabbit
New Features
SubmitProposalmethod in the msgServer for enhanced proposal submission functionality.Deprecations
Improvements
CustomParamsby changing function return types.Removals
AttributeKeyRecvErrorconstant from the IBC middleware.