feat(baseapp): integrate the appdata.Listener in baseapp
#21965
Conversation
cool-develope
requested review from
facundomedica,
aaronc,
testinginprod,
kocubinski and
a team
as code owners
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request enhance the functionality of the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
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
|
|
@cool-develope 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: 3
🧹 Outside diff range and nitpick comments (1)
baseapp/streaming.go (1)
175-176: Set appropriateBlockStagefor events.Currently,
BlockStageis set toappdata.UnknownBlockStage. If you know the specific block stage during which these events occur, consider setting it to the appropriate constant to improve event classification.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (1)
- baseapp/streaming.go (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
baseapp/streaming.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
baseapp/streaming.go
Outdated
| if err := p.listener.StartBlock(appdata.StartBlockData{ | ||
| Height: uint64(req.Height), | ||
| HeaderBytes: nil, // TODO: need to define a header struct including enc/decoding | ||
| HeaderJSON: nil, // TODO: need to define a header json struct | ||
| }); err != nil { |
There was a problem hiding this comment.
Address TODOs for HeaderBytes and HeaderJSON in StartBlockData.
The HeaderBytes and HeaderJSON fields are currently set to nil, with TODO comments indicating the need to define a header struct including encoding and decoding. Implementing these fields is important for proper handling of header information in the listener.
Would you like assistance in defining the header struct and implementing the necessary encoding/decoding functions?
baseapp/streaming.go
Outdated
| if p.listener.OnTx != nil { | ||
| for i, tx := range req.Txs { | ||
| if err := p.listener.OnTx(appdata.TxData{ | ||
| TxIndex: int32(i), | ||
| Bytes: func() ([]byte, error) { return tx, nil }, | ||
| JSON: nil, // TODO: need to define a tx json struct | ||
| }); err != nil { | ||
| return err | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Implement JSON representation for transactions in TxData.
In the OnTx listener invocation, the JSON field of TxData is set to nil, with a TODO comment indicating the need to define a JSON struct for transactions. Providing a JSON representation of transactions may be important for downstream processing or indexing.
Do you need help in defining the transaction JSON struct and implementing its encoding?
baseapp/streaming.go
Outdated
| if p.listener.OnEvent != nil { | ||
| events := make([]appdata.Event, len(res.Events)) | ||
| for i, event := range res.Events { | ||
| events[i] = appdata.Event{ | ||
| BlockStage: appdata.UnknownBlockStage, | ||
| Type: event.Type, | ||
| Data: nil, | ||
| Attributes: func() ([]appdata.EventAttribute, error) { | ||
| attrs := make([]appdata.EventAttribute, len(event.Attributes)) | ||
| for j, attr := range event.Attributes { | ||
| attrs[j] = appdata.EventAttribute{ | ||
| Key: attr.Key, | ||
| Value: attr.Value, | ||
| } | ||
| } | ||
| return attrs, nil | ||
| }, | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Events are constructed but not processed; missing call to OnEvent method.
After constructing the events slice from res.Events, the code does not invoke any method to process these events. This may result in events not being properly handled by the listener.
You may need to call p.listener.OnEvent with the constructed events slice to ensure events are processed correctly. For example:
if err := p.listener.OnEvent(events); err != nil {
return err
}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (10)
baseapp/streaming.go (2)
147-197: LGTM with a suggestion for simplificationThe
eventToAppDataEventfunction correctly convertsabci.Eventtoappdata.Event, handling various attributes and block stages. However, the attribute conversion could be simplified.Consider simplifying the attribute conversion by removing the closure:
appdataEvent := appdata.Event{ Type: event.Type, - Attributes: func() ([]appdata.EventAttribute, error) { - attrs := make([]appdata.EventAttribute, len(event.Attributes)) - for j, attr := range event.Attributes { - attrs[j] = appdata.EventAttribute{ - Key: attr.Key, - Value: attr.Value, - } - } - return attrs, nil - }, + Attributes: make([]appdata.EventAttribute, len(event.Attributes)), } +for j, attr := range event.Attributes { + appdataEvent.Attributes[j] = appdata.EventAttribute{ + Key: attr.Key, + Value: attr.Value, + } +}This change simplifies the code without altering its functionality.
210-248: LGTM: Comprehensive implementation of listener callsThe
ListenFinalizeBlockmethod now correctly implementsStartBlock,OnTx, andOnEventlistener calls with proper error handling. The event handling is comprehensive, including both block and transaction events.Consider adding a comment explaining the
+1adjustment forTxIndex,MsgIndex, andEventIndex. For example:// Add 1 to convert from 0-based to 1-based indexing appdataEvent.TxIndex = int32(txIndex + 1)This will help clarify the reasoning behind the adjustment.
baseapp/abci.go (1)
Line range hint
835-852: Improvement: Added transaction index to response eventsThe changes in this section enhance the
internalFinalizeBlockfunction by adding a transaction index to the response events. This is a valuable addition as it allows for easier tracking and referencing of transactions within a block.However, there are a couple of points to consider:
The
txIndexvariable is declared implicitly in theforloop. While this is valid Go syntax, it might be more explicit to declare it before the loop for clarity.The conversion of
txIndexto a string is done for each event attribute. This could be optimized by converting once before the inner loop.Consider the following optimizations:
for txIndex, rawTx := range req.Txs { response := app.deliverTx(rawTx) // check after every tx if we should abort select { case <-ctx.Done(): return nil, ctx.Err() default: // continue } // Convert txIndex to string once txIndexStr := strconv.Itoa(txIndex) // append the tx index to the response.Events for i, event := range response.Events { response.Events[i].Attributes = append(event.Attributes, abci.EventAttribute{Key: "tx_index", Value: txIndexStr}) } txResults = append(txResults, response) }This refactoring improves readability and potentially performance by converting the index to a string only once per transaction.
baseapp/abci_test.go (7)
Line range hint
683-750: Consider adding edge case tests forTestABCI_VoteExtensionsWhile the test covers standard scenarios, adding tests for edge cases—such as missing vote extensions, invalid signatures, or unexpected data formats—would improve the robustness of the test suite and ensure the application handles all possible scenarios correctly.
Line range hint
751-770: Correct error message capitalizationPer the Uber Go Style Guide, error messages should not be capitalized. Update the error messages to begin with a lowercase letter for consistency.
Apply this diff to correct the error messages:
- require.NoError(t, err, "Some error") + require.NoError(t, err, "some error")
Line range hint
783-800: Add a comment to explainTestABCI_PrepareProposal_FailReCheckTxIncluding a brief comment above the test function improves readability and helps other developers understand the purpose of the test.
Apply this diff to add the comment:
+// TestABCI_PrepareProposal_FailReCheckTx verifies that transactions failing ReCheckTx are not included in PrepareProposal. func TestABCI_PrepareProposal_FailReCheckTx(t *testing.T) { // Test implementation... }
Line range hint
920-940: Uset.Runfor subtests in table-driven testsTo enhance test reporting and isolation, wrap each iteration in
t.Runwith the test case name. This practice allows for better identification of failing cases.Apply this diff to modify the test:
for _, tc := range testCases { - // Existing code... + t.Run(tc.name, func(t *testing.T) { + // Existing code... + }) }
Line range hint
980-1050: Avoid non-deterministic random values in testsUsing non-deterministic random values can lead to flaky tests. Seed the random number generator or use fixed values to ensure consistent test results.
Apply this diff to seed the random number generator:
func TestBaseApp_VoteExtensions(t *testing.T) { + rand.Seed(1) // Seed the RNG for deterministic results ctrl := gomock.NewController(t) valStore := mock.NewMockValidatorStore(ctrl) // Rest of the test code...
Line range hint
1150-1170: Handle potential errors inmarshalDelimitedFnfunctionWhile using
protoio.NewDelimitedWriter, consider checking for errors that may arise during message writing to enhance error handling robustness.Apply this diff to handle possible errors:
func marshalDelimitedFn(msg proto.Message) ([]byte, error) { var buf bytes.Buffer - protoio.NewDelimitedWriter(&buf).WriteMsg(msg) + if err := protoio.NewDelimitedWriter(&buf).WriteMsg(msg); err != nil { + return nil, err + } return buf.Bytes(), nil }
Line range hint
1250-1270: Improve variable naming for clarityConsider renaming variables like
veandvresto more descriptive names, such asvoteExtensionandverifyResponse, to enhance code readability.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (5)
- baseapp/abci.go (3 hunks)
- baseapp/abci_test.go (1 hunks)
- baseapp/baseapp.go (4 hunks)
- baseapp/streaming.go (2 hunks)
- baseapp/streaming_test.go (2 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
baseapp/abci.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.baseapp/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"baseapp/baseapp.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.baseapp/streaming.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.baseapp/streaming_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"
🔇 Additional comments (10)
baseapp/streaming.go (3)
203-207: LGTM: Well-documented test helper functionThe
NewListenerWrapperfunction is a simple and clear implementation for creating alistenerWrapperinstance. It's properly documented as being used only for testing purposes.
213-214: Address TODOs forHeaderBytesandHeaderJSONinStartBlockDataThe
HeaderBytesandHeaderJSONfields are currently set tonil, with TODO comments indicating the need to define a header struct including encoding and decoding. Implementing these fields is important for proper handling of header information in the listener.
224-224: Implement JSON representation for transactions inTxDataIn the
OnTxlistener invocation, theJSONfield ofTxDatais set tonil, with a TODO comment indicating the need to define a JSON struct for transactions. Providing a JSON representation of transactions may be important for downstream processing or indexing.baseapp/baseapp.go (5)
721-729: Improved event traceability with indexingThe addition of "event_index" and "mode" attributes to all events in the
preBlockfunction enhances event traceability. This change allows for better tracking and ordering of events during the PreBlock phase.
751-751: Consistent event indexing in BeginBlockThe addition of the "event_index" attribute to BeginBlock events maintains consistency with the
preBlockfunction changes. This ensures uniform event tracking across different phases of block processing.
814-814: Consistent event indexing across all block phasesThe addition of the "event_index" attribute to EndBlock events completes the implementation of consistent event indexing across all block processing phases (PreBlock, BeginBlock, and EndBlock). This uniformity enhances the overall traceability and analysis of events throughout the block lifecycle.
1165-1170: Comprehensive event indexing including message eventsThe
createEventsfunction now includes "event_index" attributes for all events, including the message event. The indexing scheme (0 for the message event, 1 and onwards for other events) ensures clear ordering and easy identification of the primary message event. This change complements the event indexing implemented in the block processing phases, providing a comprehensive event tracking system.
Line range hint
721-1170: Summary: Comprehensive event indexing system implementedThe changes in this file introduce a robust event indexing system across various stages of block processing and message handling. Key improvements include:
- Consistent addition of "event_index" attributes in PreBlock, BeginBlock, and EndBlock functions.
- Implementation of event indexing in the
createEventsfunction, ensuring all events, including message events, are properly indexed.- Maintenance of a consistent indexing scheme across different parts of the code.
These enhancements significantly improve event traceability and provide better tools for analyzing the sequence and origin of events throughout the block lifecycle. The implementation is consistent and well-integrated with the existing codebase.
baseapp/abci.go (1)
Line range hint
1-1252: Overall assessment: Positive improvement with minor optimization opportunitiesThe changes made to the
baseapp/abci.gofile, specifically in theinternalFinalizeBlockfunction, represent a positive improvement to the transaction processing pipeline. By adding transaction indices to the response events, the code enhances the traceability and referencing capabilities of transactions within a block.While the implementation is functionally correct, there are minor optimization opportunities as mentioned in the previous comment. These optimizations could potentially improve performance and code readability slightly.
No other significant changes were observed in the provided diff for this file. The modifications are focused and appear to achieve their intended purpose without introducing any apparent issues or breaking existing functionality.
baseapp/abci_test.go (1)
Line range hint
1051-1070: Check for errors when seeding the random number generatorEnsure that the seed operation does not introduce any errors or unintended side effects in the tests.
No code changes are required, but please confirm that the seeding does not affect other tests or parts of the codebase.
| func newMockAppDataListener() *mockAppDataListener { | ||
| listener := &mockAppDataListener{} | ||
|
|
||
| // Initialize the Listener with custom behavior to store data | ||
| listener.Listener = appdata.Listener{ | ||
| StartBlock: func(data appdata.StartBlockData) error { | ||
| listener.startBlockData = append(listener.startBlockData, data) // Store StartBlockData | ||
| return nil | ||
| }, | ||
| OnTx: func(data appdata.TxData) error { | ||
| listener.txData = append(listener.txData, data) // Store TxData | ||
| return nil | ||
| }, | ||
| OnEvent: func(data appdata.EventData) error { | ||
| listener.eventData = append(listener.eventData, data) // Store EventData | ||
| return nil | ||
| }, | ||
| OnKVPair: func(data appdata.KVPairData) error { | ||
| listener.kvPairData = append(listener.kvPairData, data) // Store KVPairData | ||
| return nil | ||
| }, | ||
| Commit: func(data appdata.CommitData) (func() error, error) { | ||
| listener.commitData = append(listener.commitData, data) // Store CommitData | ||
| return nil, nil | ||
| }, | ||
| } | ||
|
|
||
| return listener | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify the newMockAppDataListener function and closures
In newMockAppDataListener, the closures capture the listener variable. To avoid potential confusion and ensure correct behavior, you can assign the function fields after initializing the struct.
Modify the function to initialize the struct and then assign the methods:
func newMockAppDataListener() *mockAppDataListener {
listener := &mockAppDataListener{}
listener.StartBlock = func(data appdata.StartBlockData) error {
listener.startBlockData = append(listener.startBlockData, data)
return nil
}
// Similarly assign other methods: OnTx, OnEvent, OnKVPair, Commit
return listener
}This avoids embedding the interface and makes the code clearer.
| "cosmossdk.io/schema/appdata" | ||
| storetypes "cosmossdk.io/store/types" | ||
|
|
||
| "github.com/cosmos/cosmos-sdk/baseapp" | ||
| baseapptestutil "github.com/cosmos/cosmos-sdk/baseapp/testutil" | ||
| sdk "github.com/cosmos/cosmos-sdk/types" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Organize import statements according to Go conventions
The import statements should be grouped into standard library packages, third-party packages, and local packages, each separated by a blank line.
Consider reorganizing the imports:
import (
"context"
"fmt"
"testing"
abci "github.com/cometbft/cometbft/api/cometbft/abci/v1"
tmproto "github.com/cometbft/cometbft/api/cometbft/types/v1"
"github.com/stretchr/testify/require"
+ "cosmossdk.io/schema/appdata"
+ storetypes "cosmossdk.io/store/types"
"github.com/cosmos/cosmos-sdk/baseapp"
baseapptestutil "github.com/cosmos/cosmos-sdk/baseapp/testutil"
sdk "github.com/cosmos/cosmos-sdk/types"
)
- "cosmossdk.io/schema/appdata"
- storetypes "cosmossdk.io/store/types"This improves readability and conforms to the Go import grouping conventions.
Committable suggestion was skipped due to low confidence.
| type mockAppDataListener struct { | ||
| appdata.Listener | ||
|
|
||
| startBlockData []appdata.StartBlockData | ||
| txData []appdata.TxData | ||
| eventData []appdata.EventData | ||
| kvPairData []appdata.KVPairData | ||
| commitData []appdata.CommitData | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid embedding interfaces; implement methods explicitly
Embedding an interface (appdata.Listener) in mockAppDataListener can lead to confusion. It's clearer to implement the interface methods explicitly.
Refactor mockAppDataListener to implement the appdata.Listener interface without embedding:
type mockAppDataListener struct {
- appdata.Listener
startBlockData []appdata.StartBlockData
txData []appdata.TxData
eventData []appdata.EventData
kvPairData []appdata.KVPairData
commitData []appdata.CommitData
}Then define the methods required by appdata.Listener on mockAppDataListener.
Committable suggestion was skipped due to low confidence.
| txCount := 5 | ||
| txs := make([][]byte, txCount) | ||
|
|
||
| for i := 0; i < txCount; i++ { | ||
| tx := newTxCounter(t, suite.txConfig, suite.ac, int64(i), int64(i)) | ||
|
|
||
| txBytes, err := suite.txConfig.TxEncoder()(tx) | ||
| require.NoError(t, err) | ||
|
|
||
| sKey := []byte(fmt.Sprintf("distKey%d", i)) | ||
| sVal := []byte(fmt.Sprintf("distVal%d", i)) | ||
| store := getFinalizeBlockStateCtx(suite.baseApp).KVStore(distKey1) | ||
| store.Set(sKey, sVal) | ||
|
|
||
| txs[i] = txBytes | ||
| } | ||
|
|
||
| _, err = suite.baseApp.FinalizeBlock(&abci.FinalizeBlockRequest{Height: 1, Txs: txs}) | ||
| require.NoError(t, err) | ||
| _, err = suite.baseApp.Commit() | ||
| require.NoError(t, err) | ||
|
|
||
| // StartBlockData | ||
| require.Len(t, mockListener.startBlockData, 1) | ||
| require.Equal(t, uint64(1), mockListener.startBlockData[0].Height) | ||
| // TxData | ||
| txData := mockListener.txData | ||
| require.Len(t, txData, len(txs)) | ||
| for i := 0; i < txCount; i++ { | ||
| require.Equal(t, int32(i), txData[i].TxIndex) | ||
| txBytes, err := txData[i].Bytes() | ||
| require.NoError(t, err) | ||
| require.Equal(t, txs[i], txBytes) | ||
| } | ||
| // KVPairData | ||
| require.Len(t, mockListener.kvPairData, 1) | ||
| updates := mockListener.kvPairData[0].Updates | ||
| for i := 0; i < txCount; i++ { | ||
| require.Equal(t, []byte(distKey1.Name()), updates[i].Actor) | ||
| require.Len(t, updates[i].StateChanges, 1) | ||
| sKey := []byte(fmt.Sprintf("distKey%d", i)) | ||
| sVal := []byte(fmt.Sprintf("distVal%d", i)) | ||
| require.Equal(t, sKey, updates[i].StateChanges[0].Key) | ||
| require.Equal(t, sVal, updates[i].StateChanges[0].Value) | ||
| } | ||
| // CommitData | ||
| require.Len(t, mockListener.commitData, 1) | ||
| // EventData | ||
| require.Len(t, mockListener.eventData, 1) | ||
| events := mockListener.eventData[0].Events | ||
| require.Len(t, events, 3+txCount*3) | ||
|
|
||
| for i := 0; i < 3; i++ { | ||
| require.Equal(t, int32(0), events[i].TxIndex) | ||
| require.Equal(t, int32(0), events[i].MsgIndex) | ||
| require.Equal(t, int32(1), events[i].EventIndex) | ||
| attrs, err := events[i].Attributes() | ||
| require.NoError(t, err) | ||
| require.Len(t, attrs, 2) | ||
| switch i { | ||
| case 0: | ||
| require.Equal(t, appdata.PreBlockStage, events[i].BlockStage) | ||
| require.Equal(t, "pre-block", events[i].Type) | ||
| case 1: | ||
| require.Equal(t, appdata.BeginBlockStage, events[i].BlockStage) | ||
| require.Equal(t, "begin-block", events[i].Type) | ||
| case 2: | ||
| require.Equal(t, appdata.EndBlockStage, events[i].BlockStage) | ||
| require.Equal(t, "end-block", events[i].Type) | ||
| } | ||
| } | ||
|
|
||
| for i := 3; i < 3+txCount*3; i++ { | ||
| require.Equal(t, appdata.TxProcessingStage, events[i].BlockStage) | ||
| require.Equal(t, int32(i/3), events[i].TxIndex) | ||
| switch i % 3 { | ||
| case 0: | ||
| require.Equal(t, "ante_handler", events[i].Type) | ||
| require.Equal(t, int32(0), events[i].MsgIndex) | ||
| require.Equal(t, int32(0), events[i].EventIndex) | ||
| attrs, err := events[i].Attributes() | ||
| require.NoError(t, err) | ||
| require.Len(t, attrs, 2) | ||
| case 1: | ||
| require.Equal(t, "message", events[i].Type) | ||
| require.Equal(t, int32(1), events[i].MsgIndex) | ||
| require.Equal(t, int32(1), events[i].EventIndex) | ||
| attrs, err := events[i].Attributes() | ||
| require.NoError(t, err) | ||
| require.Len(t, attrs, 5) | ||
| case 2: | ||
| require.Equal(t, "message", events[i].Type) | ||
| require.Equal(t, int32(1), events[i].MsgIndex) | ||
| require.Equal(t, int32(2), events[i].EventIndex) | ||
| attrs, err := events[i].Attributes() | ||
| require.NoError(t, err) | ||
| require.Len(t, attrs, 4) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage and assertion clarity
In TestAppDataListener, consider the following improvements:
- Test Edge Cases: Include tests for edge cases such as zero transactions, error handling in listener methods, and unusual event sequences.
- Use Descriptive Variable Names: Replace generic names like
iwith more descriptive names where appropriate for better readability. - Assertion Messages: Add custom messages to assertions to clarify the purpose of each test and make debugging easier.
Review and update the test function to enhance coverage and maintainability:
for txIndex := 0; txIndex < txCount; txIndex++ {
require.Equal(t, int32(txIndex), txData[txIndex].TxIndex, "TxIndex should match the transaction index")
txBytes, err := txData[txIndex].Bytes()
require.NoError(t, err)
require.Equal(t, txs[txIndex], txBytes, "Transaction bytes should match the original txBytes")
}Adding descriptive messages and variable names improves the clarity of the tests.
Committable suggestion was skipped due to low confidence.
julienrbrt
added
the
backport/v0.52.x
(cherry picked from commit 80726f7)
Description
Closes: #21526
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