-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(server/v2/stf): delayed marshalling of typed event #22684
base: main
Are you sure you want to change the base?
Changes from all commits
2c942e7
816183f
32c8f63
9331c67
b086719
d5302e2
5a873c5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -122,9 +122,9 @@ func New[T transaction.Tx]( | |
} | ||
} | ||
|
||
indexEvents := make(map[string]struct{}, len(srv.config.AppTomlConfig.IndexEvents)) | ||
for _, e := range srv.config.AppTomlConfig.IndexEvents { | ||
indexEvents[e] = struct{}{} | ||
indexedABCIEvents := make(map[string]struct{}, len(srv.config.AppTomlConfig.IndexABCIEvents)) | ||
for _, e := range srv.config.AppTomlConfig.IndexABCIEvents { | ||
indexedABCIEvents[e] = struct{}{} | ||
Comment on lines
+125
to
+127
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Codebase verification Inconsistent event indexing variable names across the codebase The codebase shows two different naming conventions being used:
Both variables serve the same purpose of indexing ABCI events, but the inconsistent naming could lead to confusion. Consider:
🔗 Analysis chainLGTM: Efficient map initialization with pre-allocated capacity The map initialization with pre-allocated capacity is a good performance optimization. The renamed variable better reflects its ABCI-specific purpose. Let's verify the consistency of this change across the codebase: 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Verify the consistent usage of indexedABCIEvents across the codebase
# and ensure no old references to indexEvents remain
# Check for any remaining references to the old name
rg "indexEvents" --type go
# Check the usage pattern of the new name
rg "indexedABCIEvents" --type go -A 2 -B 2
Length of output: 2687 |
||
} | ||
|
||
ss := store.GetStateStorage().(snapshots.StorageSnapshotter) | ||
|
@@ -185,7 +185,7 @@ func New[T transaction.Tx]( | |
checkTxHandler: srv.serverOptions.CheckTxHandler, | ||
extendVote: srv.serverOptions.ExtendVoteHandler, | ||
chainID: chainID, | ||
indexedEvents: indexEvents, | ||
indexedABCIEvents: indexedABCIEvents, | ||
initialHeight: 0, | ||
queryHandlersMap: queryHandlers, | ||
getProtoRegistry: sync.OnceValues(gogoproto.MergedRegistry), | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i dont get why the first one is needed? in both cases when its true it doesnt emit events for comet but one still includes it in the finalize block response? why is that needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, well I didn't know if it was needed so I added it if someone was relying on that use case (block streaming, and doing manual indexing, but still while not willing comet to do it)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah okay, i see what you mean. lets make this one value. The second one. if events dont want to be indexed then the node operator should set
tx_index:
tonull
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, thought about that too, but learned that toml doesn't support nil/null/none value, so that was our only option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
its used here https://github.com/cometbft/cometbft/blob/main/config/config.toml.tpl#L548.