refactor!: turn MsgsV2 into ReflectMessages to make it less confusing #19839
Conversation
WalkthroughWalkthroughThe changes aim to standardize message signer retrieval methods to Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: .coderabbit.yml
Files selected for processing (12)
- baseapp/baseapp.go (5 hunks)
- codec/codec.go (2 hunks)
- codec/proto_codec.go (2 hunks)
- go.mod (1 hunks)
- server/mock/tx.go (2 hunks)
- types/mempool/mempool_test.go (3 hunks)
- types/mempool/signer_extraction_adapater_test.go (2 hunks)
- types/tx/types.go (3 hunks)
- types/tx_msg.go (2 hunks)
- x/auth/go.mod (1 hunks)
- x/auth/tx/gogotx.go (2 hunks)
- x/tx/decode/decode.go (2 hunks)
Additional comments: 20
x/tx/decode/decode.go (2)
- 18-18: The update to use
[]protoreflect.Messagein theDecodedTxstruct aligns with the PR's objectives to transition toprotoreflectfor message handling. This change is consistent and appropriate.- 101-108: The logic for appending
protoreflect.Messageinstances tomsgsand extracting signers is correctly implemented. Ensure that all message types used in the application are compatible with this new approach to avoid runtime errors.server/mock/tx.go (1)
- 107-108: The implementation of
GetReflectMessagesin theKVStoreTxstruct correctly returns a slice ofprotoreflect.Message, which is a mock for testing purposes. This aligns with the PR's objectives. Ensure that the comment about this being a hack for tests is addressed or clarified if this is intended to be a permanent solution.Consider adding a detailed comment explaining why this approach is chosen and if it's intended to be temporary or permanent.
types/tx_msg.go (1)
- 55-61: The transition from
GetMsgsV2toGetReflectMessagesin theTxinterface, returning[]protoreflect.Message, is consistent with the PR's objectives. This change enhances clarity and aligns with the use ofprotoreflectfor dynamic read operations.codec/codec.go (3)
- 27-30: The update to
GetMsgAnySignersto returnprotoreflect.Messageis consistent with the PR's objectives. This change enhances the use of reflection and aligns with the transition toprotoreflect.- 32-36: The method
GetMsgSignerscorrectly transitions to returningprotoreflect.Message, facilitating the use of reflection in context where needed. This change is appropriate and aligns with the refactor's goals.- 38-39: The introduction of
GetReflectMsgSignersis a clear and direct approach to obtaining signers from aprotoreflect.Message. This method simplifies the interface for cases where the message is already in the reflected form.types/tx/types.go (1)
- 97-114: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [100-136]
The update to the
GetSignersfunction to includeprotoreflect.Messagein its return values aligns with the PR's objectives, facilitating the use of reflection for dynamic operations. Ensure that all message types used are compatible with this approach to avoid runtime issues.x/auth/go.mod (1)
- 177-177: The addition of the replace directive for
cosmossdk.io/x/txis appropriate for local development and testing. Ensure that this directive is intended for temporary use and is planned to be removed before merging into a production branch.Consider documenting the temporary nature of this replace directive to avoid confusion.
x/auth/tx/gogotx.go (2)
- 9-9: The import of
protoreflectis correctly added to support the newGetReflectMessagesfunctionality.- 120-120: The implementation of
GetReflectMessagescorrectly aligns with the PR's objectives to utilizeprotoreflect.Message. Ensure thatw.decodedTx.Messagesis of the correct type and consider consistent error handling across the codebase.go.mod (1)
- 190-190: The addition of the replace directive for
cosmossdk.io/x/txis noted. Please ensure its intention is clear and document its purpose if it's meant to be temporary or for local development.codec/proto_codec.go (3)
- 299-305: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [302-312]
The update to
GetMsgAnySignersto returnprotoreflect.Messagealigns with the refactor's goals. Ensure that the returned types and error handling are consistent across the codebase.
- 315-316: The implementation of
GetReflectMsgSignerscorrectly utilizesprotoreflect.Message, aligning with the refactor's objectives. Ensure that error handling and the use ofpc.interfaceRegistry.SigningContext()are consistent with the codebase's standards.- 319-322: The update to
GetMsgSignersto returnprotoreflect.Messageand handle bothproto.Messageandgogoproto.Messagetypes aligns with the refactor's goals. Ensure that the handling of different message types and error handling are consistent and clear across the codebase.baseapp/baseapp.go (5)
- 18-18: The import of
protoreflectaligns with the PR's objective to transition fromprotov2toprotoreflectfor message handling. This change is necessary for the new functionality.- 942-944: The introduction of
GetReflectMessagesmethod in the transaction processing aligns with the PR's objective. However, it's crucial to ensure that all transactions processed throughrunTxare compatible with this new method. Additionally, consider adding error handling for scenarios whereGetReflectMessagesmight fail due to the transaction not implementing the required interface.- 990-990: The addition of
reflectMsgsas a parameter torunMsgsis a significant change that supports the transition to usingprotoreflect. Ensure that all calls torunMsgsthroughout the codebase have been updated to include this new parameter.- 1012-1012: Creating events based on
protoreflect.Messageinstances is a key part of the refactor. It's important to ensure that the event creation process correctly handles the dynamic nature ofprotoreflect.Messageand that the events generated are consistent with the application's requirements.- 1063-1063: The use of
GetReflectMsgSignersto dynamically retrieve signers from messages is a critical part of leveragingprotoreflectfor dynamic operations. Ensure that this method robustly handles various message types and correctly interprets the signer information.Verification successful
Given the lack of results from the initial
ast-grepcommand, let's attempt a different approach to verify the implementation ofGetReflectMsgSigners. We'll userg(ripgrep) to search for the function definition and any relevant implementation details that could indicate how it handles various message types.
The search using
rgsuccessfully located theGetReflectMsgSignersfunction withincodec/proto_codec.go. The function takes aprotoreflect.Messageas an argument and returns a slice of byte slices ([][]byte) and an error. It delegates the task of retrieving signers topc.interfaceRegistry.SigningContext().GetSigners(msg.Interface()), indicating that the actual logic for handling various message types and extracting signers is encapsulated within theGetSignersmethod of theSigningContextobtained from theinterfaceRegistry.This implementation suggests that
GetReflectMsgSignersis designed to work with a variety of message types by leveraging theinterfaceRegistryand itsSigningContext. However, without further details on theGetSignersmethod and theSigningContext, it's not possible to fully verify the robust handling of various message types directly from this snippet alone.Given the information available, the implementation of
GetReflectMsgSignersappears to be consistent with the expectation of dynamically retrieving signers from messages usingprotoreflect. The method's reliance on theinterfaceRegistry'sSigningContextto abstract the details of signer extraction suggests a design that could accommodate various message types, assuming theSigningContextandGetSignersmethod are appropriately implemented.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the implementation of GetReflectMsgSigners to ensure it handles various message types correctly. ast-grep --lang go --pattern $'func ($_ codec.Codec) GetReflectMsgSigners($_ protoreflect.Message) ([]sdk.AccAddress, error) { $$$ }'Length of output: 134
Script:
#!/bin/bash # Use rg to search for the GetReflectMsgSigners function and its implementation details rg "func .*GetReflectMsgSigners.*protoreflect.Message" -A 10Length of output: 779
| @@ -20,7 +20,7 @@ func (n nonVerifiableTx) GetMsgs() []sdk.Msg { | |||
| panic("not implemented") | |||
| } | |||
|
|
|||
| func (n nonVerifiableTx) GetMsgsV2() ([]proto.Message, error) { | |||
| func (n nonVerifiableTx) GetReflectMessages() ([]protoreflect.Message, error) { | |||
There was a problem hiding this comment.
The method GetReflectMessages is correctly updated to return []protoreflect.Message, aligning with the PR's objectives. However, it's essential to ensure that the panic statement is only a placeholder and that actual implementation or tests are planned.
Would you like assistance in implementing the actual logic for GetReflectMessages or creating meaningful tests for this method?
types/mempool/mempool_test.go
Outdated
|
|
||
| func (tx testTx) GetMsgs() []sdk.Msg { return nil } | ||
|
|
||
| func (tx testTx) GetMsgsV2() ([]protov2.Message, error) { return nil, nil } | ||
| func (tx testTx) GetReflectMsgs() ([]protoreflect.Message, error) { return nil, nil } |
There was a problem hiding this comment.
The addition of the GetReflectMsgs method in the testTx struct, returning nil, nil, is a placeholder for tests. Ensure that actual tests for this method are implemented to verify its functionality.
Would you like assistance in creating tests for the GetReflectMsgs method?
| @@ -93,7 +94,7 @@ | |||
|
|
|||
| func (sigErrTx) GetMsgs() []sdk.Msg { return nil } | |||
|
|
|||
| func (sigErrTx) GetMsgsV2() ([]protov2.Message, error) { return nil, nil } | |||
| func (sigErrTx) GetReflectMessages() ([]protoreflect.Message, error) { return nil, nil } | |||
There was a problem hiding this comment.
Similarly, the GetReflectMessages method in the sigErrTx struct returns nil, nil as a placeholder. It's important to implement tests that verify the functionality of this method.
Would you like assistance in creating tests for the GetReflectMessages method?
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (1)
- codec/proto_codec_test.go (2 hunks)
Additional comments: 3
codec/proto_codec_test.go (3)
- 194-197: The use of
protov2.Equalfor comparing messages is appropriate here, ensuring that the messages are equal in terms of their serialized content. However, it's important to ensure thatmsgSendV2Copyis correctly obtained frommsgSendV1through thecdc.GetMsgSignersfunction, as this involves a type conversion that should be carefully handled to avoid runtime errors.Ensure that the conversion process in
cdc.GetMsgSignerscorrectly handles different message types and preserves all necessary information for an accurate comparison.
- 199-199: Renaming
GetMsgV2SignerstoGetReflectMsgSignersaligns with the PR's objective to make function names more descriptive and intuitive. This change should help clarify the use of reflection in obtaining message signers. Ensure that all references to the old function name are updated across the codebase to prevent any broken dependencies.- 208-208: The comparison using
protov2.Equalafter retrieving signers from atypes.Anymessage is consistent with the earlier comparison. This consistency is good for maintainability. Again, ensure that the conversion and retrieval logic incdc.GetMsgAnySignersis robust and error-free.Double-check the implementation of
cdc.GetMsgAnySignersto confirm that it correctly handles the conversion and retrieval of message signers, especially when dealing with different message types.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (2)
- x/auth/tx/builder.go (1 hunks)
- x/auth/tx/gogotx.go (5 hunks)
Additional comments: 5
x/auth/tx/gogotx.go (4)
- 9-9: The import of
"google.golang.org/protobuf/reflect/protoreflect"is correctly added to support the newGetReflectMessagesfunction. Ensure that all references toprotoreflectare used appropriately throughout the file.- 34-34: The reference to
decodeMsgsV1suggests that the functionGetMsgsV1might have been replaced or its functionality altered. SinceGetMsgsV1is not directly shown in the provided code, ensure that its replacement or modification aligns with the PR objectives of enhancing clarity and efficiency in message handling.- 77-82: The implementation of
GetReflectMessagesusingprotoreflect.Messageis aligned with the PR objectives. However, ensure that the loop correctly iterates overdecodedTx.Messagesand that each message'sProtoReflectmethod is appropriately called without errors.- 129-130: The
GetReflectMessagesfunction correctly returns the slice ofprotoreflect.Message. This change is consistent with the PR's goal of improving message handling. Ensure that error handling is considered if any operations within this function could potentially fail in future enhancements.x/auth/tx/builder.go (1)
- 59-59: The update to assign
decoded.msgsto themsgsfield in thenewBuilderFromDecodedTxfunction aligns with the PR objectives of enhancing message handling. Ensure that thedecoded.msgsfield is correctly populated and represents the intended message format after the refactor.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: .coderabbit.yml
Files selected for processing (15)
- baseapp/baseapp.go (5 hunks)
- server/mock/tx.go (2 hunks)
- store/database.go (1 hunks)
- store/internal/encoding/changeset_test.go (1 hunks)
- store/storage/pebbledb/batch.go (1 hunks)
- store/storage/pebbledb/db.go (2 hunks)
- store/storage/sqlite/batch.go (1 hunks)
- store/storage/sqlite/db_test.go (1 hunks)
- types/mempool/mempool_test.go (3 hunks)
- x/accounts/keeper.go (3 hunks)
- x/accounts/utils_test.go (2 hunks)
- x/auth/tx/gogotx.go (5 hunks)
- x/authz/keeper/keeper.go (1 hunks)
- x/gov/keeper/proposal.go (1 hunks)
- x/group/keeper/proposal_executor.go (1 hunks)
Files skipped from review due to trivial changes (2)
- store/internal/encoding/changeset_test.go
- store/storage/sqlite/db_test.go
Files skipped from review as they are similar to previous changes (4)
- baseapp/baseapp.go
- server/mock/tx.go
- types/mempool/mempool_test.go
- x/auth/tx/gogotx.go
Additional comments: 15
store/storage/pebbledb/batch.go (2)
- 60-60: The change to consolidate
storeKeyandkeyinto a singlestoreKeyparameter in theSetmethod simplifies the method signature. Ensure that theb.setmethod correctly handles the consolidatedstoreKey.- 64-64: The change to consolidate
storeKeyandkeyinto a singlestoreKeyparameter in theDeletemethod simplifies the method signature. Ensure that theb.setmethod correctly handles the consolidatedstoreKeywhen marking an entry as deleted.store/storage/sqlite/batch.go (2)
- 65-65: The change to accept multiple byte slices in the
Setmethod enhances flexibility. Verify the overall logic and performance implications of this change.- 71-71: The change to accept multiple byte slices in the
Deletemethod enhances flexibility. Verify the overall logic and performance implications of this change.x/group/keeper/proposal_executor.go (1)
- 59-59: The change from
GetMsgV1SignerstoGetMsgSignersaligns with the PR's objectives to enhance clarity in message handling. Ensure that theGetMsgSignersmethod is correctly implemented and used throughout the codebase.x/accounts/utils_test.go (1)
- 70-70: The change to use
protoreflect.Messagein theGetMsgV1Signersfunction signature aligns with the PR's objectives to transition toprotoreflectfor message handling. Ensure compatibility ofprotoreflect.Messagewith the rest of the codebase.store/database.go (3)
- 21-21: The consolidation of
storeKeyandkeyinto a single parameter in theGetmethod of theReaderinterface simplifies the method signature. Ensure that the implementation of theGetmethod correctly handles the consolidated parameter.- 29-29: The consolidation of
storeKeyandkeyinto a single parameter in theSetmethod of theWriterinterface simplifies the method signature. Ensure that the implementation of theSetmethod correctly handles the consolidated parameter.- 34-34: The consolidation of
storeKeyandkeyinto a single parameter in theDeletemethod of theWriterinterface simplifies the method signature. Ensure that the implementation of theDeletemethod correctly handles the consolidated parameter.x/gov/keeper/proposal.go (1)
- 73-73: The change from
GetMsgV1SignerstoGetMsgSignersaligns with the PR's objectives to enhance clarity in message handling. Ensure that theGetMsgSignersmethod is correctly implemented and used throughout the codebase.store/storage/pebbledb/db.go (2)
- 322-322: The separation of
storeKeyandkeyinto separate parameters in theprependStoreKeyfunction enhances clarity and maintainability. Ensure compatibility of this change with the rest of the codebase.- 365-365: The separation of
storeKeyandkeyinto separate parameters in thegetMVCCSlicefunction enhances clarity and maintainability. Ensure that the implementation of thegetMVCCSlicefunction correctly handles the separated parameters.x/authz/keeper/keeper.go (1)
- 95-95: The change from
GetMsgV1SignerstoGetMsgSignersaligns with the PR's objectives to enhance clarity in message handling. Ensure that theGetMsgSignersmethod is correctly implemented and used throughout the codebase.x/accounts/keeper.go (2)
- 12-12: The import of
protoreflectaligns with the PR's objective to transition fromprotov2toprotoreflectfor message handling. This change is necessary for the new functionality and does not appear to introduce any issues.- 366-366: The call to
GetMsgSignerswithinsendModuleMessagecorrectly handles the updated method signature. This usage demonstrates proper integration of the changes with existing code. However, ensure that the error handling and the logic for checking the sender's authorization are thoroughly tested, especially since the method signature change could affect how messages and signers are processed.Verification successful
The provided context confirms that the
sendModuleMessagefunction correctly usesGetMsgSigners, including proper error handling and sender authorization checks. This aligns with the expectations set in the original review comment, ensuring that the changes integrate well with the existing code and adhere to best practices.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify thorough testing of GetMsgSigners usage in sendModuleMessage, focusing on error handling and sender authorization. rg --type go -C 5 'GetMsgSigners' | grep 'sendModuleMessage'Length of output: 300
Script:
#!/bin/bash # Inspect the sendModuleMessage function in x/accounts/keeper.go for the usage of GetMsgSigners and its error handling. rg --type go -C 10 'func \(k Keeper\) sendModuleMessage' x/accounts/keeper.goLength of output: 1696
x/accounts/keeper.go
Outdated
| @@ -53,7 +53,7 @@ type MsgRouter interface { | |||
| // SignerProvider defines an interface used to get the expected sender from a message. | |||
| type SignerProvider interface { | |||
| // GetMsgV1Signers returns the signers of the message. | |||
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (3)
- CHANGELOG.md (1 hunks)
- baseapp/baseapp.go (5 hunks)
- codec/proto_codec_test.go (2 hunks)
Additional Context Used
Path-based Instructions (3)
codec/proto_codec_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"baseapp/baseapp.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 not posted (5)
codec/proto_codec_test.go (1)
Line range hint
194-208: Ensure consistent use ofprotoreflectinGetReflectMsgSigners.Verification successful
The review comment's concern about ensuring the consistent use of
protoreflectinGetReflectMsgSignersacross the codebase has been verified. All instances ofGetReflectMsgSignerscorrectly useprotoreflect.Messageas per the new expected usage. Here are the relevant files and lines:
codec/proto_codec_test.go: Usage withmsgSendV2.ProtoReflect()baseapp/baseapp.go: Usage withreflectMsgx/accounts/defaults/multisig/utils_test.go: Mock implementation usingprotoreflect.Messagecodec/proto_codec.go: Function definitioncodec/codec.go: Interface declarationScripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `GetReflectMsgSigners` is consistently used with `protoreflect` across the codebase. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type go $'GetReflectMsgSigners'Length of output: 624
baseapp/baseapp.go (2)
Line range hint
952-1022: Ensure thatrunMsgsproperly integratesprotoreflectfor message handling.Verification successful
The verification process confirms that the
runMsgsfunction inbaseapp.goproperly integratesprotoreflectfor message handling. The function signature includesprotoreflect.Message, and references across the codebase maintain consistency with this integration. No issues were found regarding the use of an outdated function signature or incorrect integration.
- File:
baseapp/baseapp.go- Function:
runMsgsScripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `runMsgs` is properly integrating `protoreflect` for message handling across the codebase. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type go $'runMsgs'Length of output: 1308
1068-1073: Check the implementation ofcreateEventsfor correct handling ofprotoreflectmessages.Verification successful
The implementation of the
createEventsfunction in handlingprotoreflectmessages has been verified across the codebase. The function is consistently used with the correct signature as shown in the provided snippet. There are no discrepancies or multiple versions of the function, ensuring uniformity and correctness in its application.
- Location Verified:
baseapp/baseapp.goScripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `createEvents` is correctly handling `protoreflect` messages across the codebase. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type go $'createEvents'Length of output: 273
CHANGELOG.md (2)
178-179: Adjust the indentation of the unordered list items to maintain consistency and readability in the document.- * Every module has the codec already, passing it created an unneeded dependency. - * Additionally, to reflect this change, the module manager does not take a codec either. + * Every module has the codec already, passing it created an unneeded dependency. + * Additionally, to reflect this change, the module manager does not take a codec either.
181-181: Correct the typographical error in the description to enhance clarity.- These API changes clear up confusin as to the use and purpose of these methods. + These API changes clear up confusion as to the use and purpose of these methods.
There was a problem hiding this comment.
Actionable comments posted: 1
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- x/accounts/keeper.go (4 hunks)
Additional Context Used
Path-based Instructions (1)
x/accounts/keeper.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (1)
x/accounts/keeper.go (1)
349-349: Ensure that thecodecimplementation correctly handlesprotoreflect.MessageinGetMsgSignerscalls.Also applies to: 372-372
x/accounts/keeper.go
Outdated
| // SignerProvider defines an interface used to get the expected sender from a message. | ||
| type SignerProvider interface { | ||
| // GetMsgSigners returns the signers of the message. | ||
| GetMsgSigners(msg gogoproto.Message) ([][]byte, protoreflect.Message, error) |
There was a problem hiding this comment.
Tip
Codebase Verification
The verification process has revealed that there are still occurrences in the codebase where the old gogoproto.Message is being used in the GetMsgSigners method within test files. Specifically, the files x/accounts/utils_test.go and x/accounts/defaults/multisig/utils_test.go have not been updated to use protoreflect.Message. This needs to be addressed to ensure consistency with the changes made in the main codebase.
x/accounts/utils_test.go: TheGetMsgSignersmethod in themockSignerstruct still usesgogoproto.Message.x/accounts/defaults/multisig/utils_test.go: TheGetMsgSignersmethod in themockStateCodecstruct also still usesgogoproto.Message.
Analysis chain
The change from gogoproto.Message to protoreflect.Message in GetMsgSigners aligns with the PR's objectives. Ensure all related test files are updated to reflect this change.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all test files have been updated to use `protoreflect.Message` in `GetMsgSigners`.
# Test: Search for old usage of `gogoproto.Message` in test files. Expect: No occurrences.
rg --type go 'GetMsgSigners' | grep '_test.go' | grep 'gogoproto.Message'
Length of output: 337
|
I'll pick this up tomorrow. |
|
Vini? Sorry I noticed there's a lint error just haven't gotten a chance to fix yet |
Sorry wasn't meant for here. The lint error has been annoying. |
There was a problem hiding this comment.
Actionable comments posted: 3
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (6)
- CHANGELOG.md (1 hunks)
- baseapp/baseapp.go (5 hunks)
- types/tx_msg.go (2 hunks)
- x/accounts/defaults/multisig/utils_test.go (1 hunks)
- x/accounts/utils_test.go (2 hunks)
- x/authz/keeper/keeper.go (1 hunks)
Files not reviewed due to errors (1)
- x/authz/keeper/keeper.go (no review received)
Files skipped from review as they are similar to previous changes (1)
- baseapp/baseapp.go
Additional Context Used
GitHub Check Runs (1)
dependency-review failure (2)
types/tx_msg.go: [failure] 8-8:
"github.com/cosmos/gogoproto/proto" imported and not used
Path-based Instructions (5)
x/accounts/utils_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"types/tx_msg.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/accounts/defaults/multisig/utils_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"x/authz/keeper/keeper.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 not posted (6)
x/accounts/utils_test.go (2)
9-9: The import ofprotoreflectis appropriate for the newmockSignertype.
91-100: ThemockSignertype and itsGetMsgSignersmethod are correctly implemented.types/tx_msg.go (2)
9-9: The import ofprotoreflectis appropriate for the newGetReflectMessagesmethod.
57-63: TheGetReflectMessagesmethod is well-documented and aligns with the purpose of providing dynamic read operations on messages.x/accounts/defaults/multisig/utils_test.go (2)
Line range hint
9-9: The import ofprotoreflectis appropriate for the newGetReflectMsgSignersmethod.
49-49: TheGetReflectMsgSignersmethod is correctly implemented and aligns with the interface requirements.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (4)
- CHANGELOG.md (1 hunks)
- types/tx_msg.go (2 hunks)
- x/accounts/keeper.go (2 hunks)
- x/accounts/utils_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- types/tx_msg.go
- x/accounts/keeper.go
- x/accounts/utils_test.go
Additional Context Used
Path-based Instructions (1)
CHANGELOG.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
| * Every module has the codec already, passing it created an unneeded dependency. | ||
| * Additionally, to reflect this change, the module manager does not take a codec either. |
There was a problem hiding this comment.
Adjust the indentation of the unordered list items to maintain consistency and readability in the document.
- * Every module has the codec already, passing it created an unneeded dependency.
- * Additionally, to reflect this change, the module manager does not take a codec either.
+ * Every module has the codec already, passing it created an unneeded dependency.
+ * Additionally, to reflect this change, the module manager does not take a codec either.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.
| * Every module has the codec already, passing it created an unneeded dependency. | |
| * Additionally, to reflect this change, the module manager does not take a codec either. | |
| * Every module has the codec already, passing it created an unneeded dependency. | |
| * Additionally, to reflect this change, the module manager does not take a codec either. |
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (5)
- x/accounts/keeper.go (2 hunks)
- x/accounts/utils_test.go (1 hunks)
- x/authz/keeper/keeper.go (1 hunks)
- x/gov/keeper/proposal.go (1 hunks)
- x/group/keeper/proposal_executor.go (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- x/accounts/keeper.go
- x/accounts/utils_test.go
- x/authz/keeper/keeper.go
Additional Context Used
Path-based Instructions (2)
x/group/keeper/proposal_executor.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/gov/keeper/proposal.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (6)
x/group/keeper/proposal_executor.go (2)
Line range hint
1-1: LGTM! The functiondoExecuteMsgsis correctly implemented with proper error handling and checks.
60-60: LGTM! The functionensureMsgAuthZis correctly implemented with proper error handling and checks.x/gov/keeper/proposal.go (4)
Line range hint
1-1: LGTM! The functionSubmitProposalis correctly implemented with proper error handling and checks.
Line range hint
1-1: LGTM! The functionCancelProposalis correctly implemented with proper error handling and checks.
Line range hint
1-1: LGTM! The functionDeleteProposalis correctly implemented with proper error handling and checks.
Line range hint
1-1: LGTM! The functionActivateVotingPeriodis correctly implemented with proper error handling and checks.
* main: (95 commits) fix(x/accounts): check for overflows in multisig weights and votes (#20384) docs(x/account/auth): Improve error handling and comments in fee.go (#20426) docs: fix some markdown syntax (#20432) revert: bank change module to account change (#20427) fix: nil pointer panic when store don't exists in historical version (#20425) fix(store/v2): Remove should not error on miss (#20423) chore: upstream more changes from v2 (#20387) docs(x/auth/ante): fixed typo in TxWithTimeoutHeight interface name (#20418) fix: avoid default sendenabled for module accounts (#20419) docs(x/auth): fixed typo in command example for multisign transaction (#20417) build(deps): Bump bufbuild/buf-setup-action from 1.31.0 to 1.32.0 (#20413) build(deps): Bump github.com/hashicorp/go-plugin from 1.6.0 to 1.6.1 in /store (#20414) feat(x/accounts): Add schema caching feature and corresponding test case (#20055) refactor(runtime/v2): remove dependency on sdk (#20389) refactor!: turn MsgsV2 into ReflectMessages to make it less confusing (#19839) docs: Enhanced the ParsePagination method documentation (#20385) refactor(runtime,core): split router service (#20401) chore: fix spelling errors (#20400) docs: Documented error handling in OfferSnapshot method (#20380) build(deps): Bump google.golang.org/grpc from 1.63.2 to 1.64.0 (#20390) ...
Description
This is an alternative to #19817 that rather than just removing stuff, names things more clearly removing any mention of V2 and instead distinguishing a "reflected" version of the message from the actual version.
For instance, it's probably confusing when there is
GetMsgsandGetMsgsV2- which one am I supposed to use? Naively, you'd think v2 is better, but that's not really right.Now,
GetMsgsV2isGetReflectMessageswhich returnsprotoreflect.Messagenotprotov2.Messageand the documentation clearly states thatGetMsgsreturns the actual messages andGetReflectMessagesis just for performing some dynamic read operations (like get signers) on those messages. Hopefully this is clearer and obviates the need to do some of the less efficient stuff in #19817.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.
I have...
Summary by CodeRabbit
New Features
GetReflectMessagesmethod for enhanced message handling.Refactor
GetMsgV1SignersandGetMsgV2SignerswithGetMsgSignersandGetReflectMsgSignersfor improved consistency in message signer retrieval.Chores