fix(stf,server/v2/cometbft): fix default events + improve codec handling #22837
Conversation
julienrbrt
added
the
backport/v0.52.x
julienrbrt
requested review from
facundomedica,
sontrinh16,
a team,
testinginprod and
kocubinski
as code owners
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces enhancements to the Changes
Possibly related PRs
Suggested reviewers
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
|
github-actions
bot
added
C:server/v2
|
@julienrbrt your pull request is missing a changelog! |
![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
🧹 Outside diff range and nitpick comments (2)
server/v2/cometbft/grpc.go (1)
259-263: Consider renaming variableeventtoeventsfor clarityThe variable
eventholds multiple events converted fromtxResult.Events. Renaming it toeventsimproves readability by accurately representing its contents.Apply this diff to rename the variable:
- event, err := intoABCIEvents(txResult.Events, map[string]struct{}{}, false) + events, err := intoABCIEvents(txResult.Events, map[string]struct{}{}, false) if err != nil { return nil, status.Errorf(codes.Unknown, "failed to convert events: %v", err) } return &txtypes.SimulateResponse{ GasInfo: &sdk.GasInfo{ GasUsed: txResult.GasUsed, GasWanted: txResult.GasWanted, }, Result: &sdk.Result{ MsgResponses: msgResponses, - Events: event, + Events: events, }, }, nilAlso applies to: 271-271
server/v2/stf/stf.go (1)
391-406: Refactor to eliminate code duplication inAttributesandDatafunctionsBoth the
AttributesandDatafunctions withincreateMessageEventconstruct the same slice ofEventAttribute. To adhere to the DRY (Don't Repeat Yourself) principle, consider defining the attributes once and reusing them in both functions.Apply this diff to refactor the code:
func createMessageEvent(msg transaction.Msg, msgIndex, eventIndex int32) event.Event { // Assumes that module name is the second element of the msg type URL // e.g. "cosmos.bank.v1beta1.MsgSend" => "bank" // It returns an empty string if the input is not a valid type URL getModuleNameFromTypeURL := func(input string) string { moduleName := strings.Split(input, ".") if len(moduleName) > 1 { return moduleName[1] } return "" } constructAttributes := func() []appdata.EventAttribute { typeURL := msgTypeURL(msg) return []appdata.EventAttribute{ {Key: "message.action", Value: "/" + typeURL}, {Key: "message.module", Value: getModuleNameFromTypeURL(typeURL)}, } } return event.Event{ MsgIndex: msgIndex, EventIndex: eventIndex, Type: "message", - Attributes: func() ([]appdata.EventAttribute, error) { - typeURL := msgTypeURL(msg) - return []appdata.EventAttribute{ - {Key: "message.action", Value: "/" + typeURL}, - {Key: "message.module", Value: getModuleNameFromTypeURL(typeURL)}, - }, nil - }, + Attributes: func() ([]appdata.EventAttribute, error) { + return constructAttributes(), nil + }, Data: func() (json.RawMessage, error) { - typeURL := msgTypeURL(msg) - attrs := []appdata.EventAttribute{ - {Key: "message.action", Value: "/" + typeURL}, - {Key: "message.module", Value: getModuleNameFromTypeURL(typeURL)}, - } - return json.Marshal(attrs) + return json.Marshal(constructAttributes()) }, } }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (3)
server/v2/cometbft/grpc.go(1 hunks)server/v2/stf/stf.go(2 hunks)tests/systemtests/tx_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
server/v2/stf/stf.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/cometbft/grpc.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
tests/systemtests/tx_test.go (3)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
🔇 Additional comments (1)
tests/systemtests/tx_test.go (1)
Line range hint 125-131: Updated test expectations reflect the changes accurately
The adjustment of the expected events count from 12 to 10 in TestSimulateTx_GRPC correctly mirrors the changes in the event handling logic. The updated comments provide clarity on the events being tested.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
server/v2/cometbft/grpc.go (1)
259-262: LGTM! Consider improving the error message.The addition of event handling to the Simulate method is well-implemented with proper error handling. The events are correctly converted and included in the response.
Consider making the error message more specific by including the actual conversion error:
- return nil, status.Errorf(codes.Unknown, "failed to convert events: %v", err) + return nil, status.Errorf(codes.Unknown, "failed to convert transaction events to ABCI events: %v", err)Also applies to: 271-271
server/v2/stf/stf.go (1)
372-408: Consider optimizing event data handling.The function correctly creates message events, but there are a few potential improvements:
- The module name extraction could fail silently for invalid type URLs
- The event attributes are duplicated in both
AttributesandDatafieldsConsider these improvements:
func createMessageEvent(msg transaction.Msg, msgIndex, eventIndex int32) event.Event { - // Assumes that module name is the second element of the msg type URL - // e.g. "cosmos.bank.v1beta1.MsgSend" => "bank" - // It returns an empty string if the input is not a valid type URL getModuleNameFromTypeURL := func(input string) string { moduleName := strings.Split(input, ".") - if len(moduleName) > 1 { + if len(moduleName) >= 2 { return moduleName[1] } - - return "" + return "unknown" } + typeURL := msgTypeURL(msg) + attrs := []appdata.EventAttribute{ + {Key: "message.action", Value: "/" + typeURL}, + {Key: "message.module", Value: getModuleNameFromTypeURL(typeURL)}, + } + return event.Event{ MsgIndex: msgIndex, EventIndex: eventIndex, Type: "message", - Attributes: func() ([]appdata.EventAttribute, error) { - typeURL := msgTypeURL(msg) - return []appdata.EventAttribute{ - {Key: "message.action", Value: "/" + typeURL}, - {Key: "message.module", Value: getModuleNameFromTypeURL(typeURL)}, - }, nil - }, + Attributes: func() ([]appdata.EventAttribute, error) { + return attrs, nil + }, Data: func() (json.RawMessage, error) { - typeURL := msgTypeURL(msg) - attrs := []appdata.EventAttribute{ - {Key: "message.action", Value: "/" + typeURL}, - {Key: "message.module", Value: getModuleNameFromTypeURL(typeURL)}, - } - return json.Marshal(attrs) }, } }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (3)
server/v2/cometbft/grpc.go(1 hunks)server/v2/stf/stf.go(2 hunks)tests/systemtests/tx_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
tests/systemtests/tx_test.go (3)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
server/v2/cometbft/grpc.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/stf/stf.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (2)
server/v2/stf/stf.go (1)
363-365: LGTM! Message event tracking added correctly.
The addition of message event creation after processing each message's events is well-placed and maintains the correct event ordering.
tests/systemtests/tx_test.go (1)
125-125: LGTM! Test documentation updated correctly.
The comment accurately reflects the current event count and provides a clear breakdown of the expected events, which helps with test maintenance and debugging.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
server/v2/stf/stf.go (2)
391-406: Eliminate attribute creation duplication.The Data function duplicates the attribute creation logic from the Attributes function.
Refactor to avoid duplication:
Attributes: func() ([]appdata.EventAttribute, error) { typeURL := msgTypeURL(msg) - return []appdata.EventAttribute{ + attrs := []appdata.EventAttribute{ {Key: "action", Value: "/" + typeURL}, {Key: "module", Value: getModuleNameFromTypeURL(typeURL)}, - }, nil + } + return attrs, nil }, Data: func() (json.RawMessage, error) { typeURL := msgTypeURL(msg) - attrs := []appdata.EventAttribute{ - {Key: "action", Value: "/" + typeURL}, - {Key: "module", Value: getModuleNameFromTypeURL(typeURL)}, - } + attrs, _ := event.Attributes() return json.Marshal(attrs) },
390-390: Define message event type as a constant.The string "message" is hardcoded and used multiple times.
Define it as a constant:
+const messageEventType = "message" + return event.Event{ MsgIndex: msgIndex, EventIndex: eventIndex, - Type: "message", + Type: messageEventType,
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
server/v2/stf/stf.go(2 hunks)server/v2/stf/stf_test.go(4 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
server/v2/stf/stf_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"
server/v2/stf/stf.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🪛 golangci-lint (1.62.2)
server/v2/stf/stf_test.go
239-239: string message has 3 occurrences, make it a constant
(goconst)
🔇 Additional comments (3)
server/v2/stf/stf_test.go (2)
228-229: LGTM: Test updates correctly reflect new message event handling.
The test has been properly updated to account for the new message event type, including the correct event count and index validation.
Also applies to: 238-239
Line range hint 251-255: LGTM: Proper validation for message event attributes.
The test correctly excludes message events from the standard index validation pattern, which aligns with their unique attribute structure.
server/v2/stf/stf.go (1)
363-365: LGTM: Message event creation properly integrated.
The message event is correctly added after processing each message's events, maintaining proper event ordering.
| typeURL := msgTypeURL(msg) | ||
| return []appdata.EventAttribute{ | ||
| {Key: "action", Value: "/" + typeURL}, | ||
| {Key: "module", Value: getModuleNameFromTypeURL(typeURL)}, | ||
| }, nil | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for msgTypeURL.
The function should handle potential errors from msgTypeURL.
Add error handling:
Attributes: func() ([]appdata.EventAttribute, error) {
typeURL := msgTypeURL(msg)
+ if typeURL == "" {
+ return nil, fmt.Errorf("failed to get type URL for message")
+ }
return []appdata.EventAttribute{
{Key: "action", Value: "/" + typeURL},
{Key: "module", Value: getModuleNameFromTypeURL(typeURL)},
}, nil
},Committable suggestion skipped: line range outside the PR's diff.
julienrbrt
changed the title
julienrbrt
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
server/v2/cometbft/server.go (1)
70-80: Well-structured codec encapsulation!The introduction of
AppCodecs[T]struct is a good architectural decision that:
- Encapsulates all codec-related components in one place
- Makes future codec additions non-breaking
- Maintains type safety through generics
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
server/v2/cometbft/abci.go(6 hunks)server/v2/cometbft/abci_test.go(1 hunks)server/v2/cometbft/grpc.go(6 hunks)server/v2/cometbft/query.go(1 hunks)server/v2/cometbft/server.go(4 hunks)simapp/v2/simdv2/cmd/commands.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
server/v2/cometbft/abci_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"
server/v2/cometbft/query.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/cometbft/server.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
simapp/v2/simdv2/cmd/commands.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/cometbft/abci.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/cometbft/grpc.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (13)
server/v2/cometbft/server.go (2)
Line range hint 82-100: Clean function signature update!
The consolidation of codec parameters into AppCodecs simplifies the function signature while maintaining all necessary functionality.
186-186: Consistent update to consensus initialization!
The change correctly integrates the new AppCodecs structure into the consensus initialization.
simapp/v2/simdv2/cmd/commands.go (1)
118-123: Clean codec initialization!
The initialization of AppCodecs is well-structured and includes all necessary codec components.
server/v2/cometbft/query.go (1)
54-54: Consistent codec usage update!
The change correctly uses the new appCodecs.TxCodec for transaction decoding while maintaining proper error handling.
server/v2/cometbft/grpc.go (5)
259-262: LGTM: Event handling in Simulate method
The event conversion and error handling is implemented correctly. The error message is descriptive and follows the gRPC status error pattern.
271-271: LGTM: Events field added to SimulateResponse
The events field is correctly added to the response, aligning with the PR objective to include events in simulation results.
282-292: LGTM: Updated transaction decoding
The transaction decoding is properly implemented with:
- Direct usage of txCodec.Decode
- Proper type assertion for sdk.Tx
- Appropriate error handling
361-361: LGTM: Improved error message
The error message is now more descriptive, providing better context about invalid requests.
529-532: LGTM: Client context initialization
The client context is properly initialized with all necessary components:
- Legacy amino codec
- App codec
- Node URI
- RPC client
- Transaction codec
Also applies to: 538-541
server/v2/cometbft/abci.go (3)
57-57: LGTM: Added appCodecs field
The appCodecs field is correctly added to consolidate all codec-related functionality.
83-85: LGTM: Improved struct organization
The fields are logically grouped, improving code organization and readability.
91-91: LGTM: Consistent use of appCodecs.TxCodec
Transaction decoding is consistently updated across all methods to use appCodecs.TxCodec:
- CheckTx
- InitChain
- PrepareProposal
- ProcessProposal
- internalFinalizeBlock
Also applies to: 323-323, 390-390, 436-436, 565-565
server/v2/cometbft/abci_test.go (1)
889-897: LGTM: Updated consensus initialization
The consensus struct is correctly initialized with all required fields, including the new appCodecs field with TxCodec.
Description
ref: #22753
It looks like stf was missing an event that we always create in baseapp.
This was making som system tests failing, and I think for feature parity it is good to add.
However, we cannot add the
message.senderaction otherwise we need to make stf aware of the address encoding.Event index is part of the event struct itself, so not sure if we need to add that as a kv?
https://github.com/cosmos/cosmos-sdk//blob/80402e7dce72c05aab53d01461495833ee3aa922/baseapp/baseapp.go#L1144-L1175
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
New Features
Bug Fixes
Tests