[KLC-2309] Dispatch websocket account events without indexer#38
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
<review_stack_artifact> </review_stack_artifact> 🚥 Pre-merge checks | ✅ 3 | ❌ 5❌ Failed checks (5 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR ensures WebSocket account subscriptions work even when the Elasticsearch indexer is disabled by dispatching account events directly from node state during block processing.
Changes:
- Added an
accountsDBdependency toeventsProcessor(wired throughArgEventsProcessorand node startup). - Introduced
processAccountEvents()+buildAccountInfo()to build and enqueue ACCOUNTS events without the indexer. - Added tests covering the new non-indexer dispatch path and skip conditions.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
indexer/eventsProcessor.go |
Adds AccountsDB-backed account-event dispatch in the non-indexer WebSocket path and refactors account-info building into a helper. |
indexer/eventsProcessor_test.go |
Adds tests for dispatching/skipping account events depending on AccountsDB presence and indexer activity. |
indexer/dataIndexerArgs.go |
Extends ArgEventsProcessor with an AccountsDB dependency. |
cmd/node/startup.go |
Injects stateComponents.AccountsAdapter into eventsProcessor. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@indexer/eventsProcessor_test.go`:
- Around line 741-785: Modify the AccountsStub used in the test so
LoadAccountCalled inspects the incoming []byte address and returns distinct
state.AccountHandler (or data.AccountInfo entries) for the sender and the
receiver instead of always returning the same stub; then after calling
ep.SaveBlock(ArgsSaveBlockData{...}) drain events as before but assert that the
ACCOUNTS event's Message map contains both the sender and receiver keys (use the
same byte slices used when creating the transaction via
createTransactionHandlerMock and the TransferContract.ToAddress) to verify
fan-out to both accounts rather than merely non-empty presence.
In `@indexer/eventsProcessor.go`:
- Around line 175-180: The accountsMap is being keyed by the raw `address`
string while you construct a canonical address in `info` via `buildAccountInfo`,
which can produce duplicate/mismatched entries; change the insertion to use the
canonical `info.Address` as the map key (i.e., replace `accountsMap[address] =
info` with `accountsMap[info.Address] = info`) and, if applicable, update the
surrounding log (`processAccountEvents` warning) to reference `info.Address` so
all downstream code (including `SaveAccounts`) receives the normalized key.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: eb92e32f-71e4-44aa-8474-6a977ab8e4c6
📒 Files selected for processing (4)
cmd/node/startup.goindexer/dataIndexerArgs.goindexer/eventsProcessor.goindexer/eventsProcessor_test.go
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Agent
- GitHub Check: setup-and-lint / setup-and-lint
- GitHub Check: Analyze (go)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (Custom checks)
**/*.go: Verify that any new or modified concurrent code (goroutines, channels, mutexes, sync primitives) is free of race conditions. Check for: proper lock/unlock pairing, no goroutine leaks, correct channel lifecycle management, and proper context cancellation propagation.
Verify that errors are not silently discarded. Check for: unchecked error returns, error wrapping with context, proper error propagation up the call chain, and no bare panic() calls outside of init() functions.
Files:
cmd/node/startup.goindexer/eventsProcessor.goindexer/dataIndexerArgs.goindexer/eventsProcessor_test.go
**/*_test.go
⚙️ CodeRabbit configuration file
**/*_test.go: Test files. Review for: - Adequate coverage of edge cases and error paths - Proper use of test helpers and assertions - Race condition coverage (tests should use -race flag patterns) - No hardcoded sleep for synchronization (use channels or sync primitives) - Test isolation (no shared mutable state between tests)
Files:
indexer/eventsProcessor_test.go
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
indexer/eventsProcessor_test.go (1)
741-785:⚠️ Potential issue | 🟡 MinorThese account-event tests still allow false positives.
Both tests return the same
UserAccountHandlerfor every lookup and then only assertNotEmptyor raw call count. A regression that emits one canonical account, or callsLoadAccountthree times for the same address, would still pass. Make the stub distinguish sender/receipt addresses and assert those exact keys are present in the emittedACCOUNTSmap.As per coding guidelines,
**/*_test.go: Test files. Review for: - Adequate coverage of edge cases and error paths - Proper use of test helpers and assertions.Also applies to: 791-837
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@indexer/eventsProcessor_test.go` around lines 741 - 785, The tests currently return the same UserAccountHandler for every LoadAccount lookup which permits false positives; update the mock AccountsStub.LoadAccountCalled used by createTestEventsProcessorWithAccountsDB to inspect the input address bytes and return distinct account handlers (e.g., senderAccount vs receiverAccount) for the sender and receiver addresses used in createTransactionHandlerMock, then after calling ep.SaveBlock assert that the ACCOUNTS Event (Event.EvType == ACCOUNTS) contains exactly the expected keys (the sender and receiver address strings) and that their values match the corresponding account handlers instead of using require.NotEmpty or only call counts; apply the same change to the duplicate test region that starts around the other failing block so both tests verify distinct addresses are present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@indexer/eventsProcessor_test.go`:
- Line 753: The test currently discards the error return from
createTransactionHandlerMock; update each call site (e.g., where tx, _ :=
createTransactionHandlerMock(...) is used—also at the other occurrences
mentioned) to capture the error and immediately assert it with
require.NoError(t, err) (or t.Fatalf on failure) so fixture-construction
failures fail fast; locate createTransactionHandlerMock usages in
eventsProcessor_test.go and replace the ignored error with a checked error
variable and a require.NoError(t, err) after the call.
In `@indexer/eventsProcessor.go`:
- Around line 166-169: The call in processAccountEvents uses
accountsDB.LoadAccount which may materialize or create accounts; verify by
running the suggested grep scripts whether LoadAccount implementations persist
or allocate new accounts—if they do, change this call to the non-creating lookup
GetExistingAccount (i.e., replace accountsDB.LoadAccount(addrBytes) with
accountsDB.GetExistingAccount(addrBytes)) or, if LoadAccount is confirmed
read-only, explicitly handle a "not found" result (check the returned value/err
for a not-found case and skip rather than allow creation); reference
processAccountEvents, accountsDB.LoadAccount, and GetExistingAccount when making
the change.
---
Duplicate comments:
In `@indexer/eventsProcessor_test.go`:
- Around line 741-785: The tests currently return the same UserAccountHandler
for every LoadAccount lookup which permits false positives; update the mock
AccountsStub.LoadAccountCalled used by createTestEventsProcessorWithAccountsDB
to inspect the input address bytes and return distinct account handlers (e.g.,
senderAccount vs receiverAccount) for the sender and receiver addresses used in
createTransactionHandlerMock, then after calling ep.SaveBlock assert that the
ACCOUNTS Event (Event.EvType == ACCOUNTS) contains exactly the expected keys
(the sender and receiver address strings) and that their values match the
corresponding account handlers instead of using require.NotEmpty or only call
counts; apply the same change to the duplicate test region that starts around
the other failing block so both tests verify distinct addresses are present.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4a4577d3-a2b3-4699-a68d-12781b8c9e86
📒 Files selected for processing (2)
indexer/eventsProcessor.goindexer/eventsProcessor_test.go
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: setup-and-lint / setup-and-lint
- GitHub Check: Analyze (go)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (Custom checks)
**/*.go: Verify that any new or modified concurrent code (goroutines, channels, mutexes, sync primitives) is free of race conditions. Check for: proper lock/unlock pairing, no goroutine leaks, correct channel lifecycle management, and proper context cancellation propagation.
Verify that errors are not silently discarded. Check for: unchecked error returns, error wrapping with context, proper error propagation up the call chain, and no bare panic() calls outside of init() functions.
Files:
indexer/eventsProcessor.goindexer/eventsProcessor_test.go
**/*_test.go
⚙️ CodeRabbit configuration file
**/*_test.go: Test files. Review for: - Adequate coverage of edge cases and error paths - Proper use of test helpers and assertions - Race condition coverage (tests should use -race flag patterns) - No hardcoded sleep for synchronization (use channels or sync primitives) - Test isolation (no shared mutable state between tests)
Files:
indexer/eventsProcessor_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
indexer/eventsProcessor_test.go (1)
739-788:⚠️ Potential issue | 🟡 MinorTest does not verify both sender and receiver are dispatched.
The stub returns the same account for all lookups, and the assertion only checks
require.NotEmpty(accountsMap). This test passes even if only the sender or only the recipient is processed.Track which addresses are loaded and assert the map contains entries for both expected addresses.
💡 Suggested improvement
func TestEventsProcessor_SaveBlock_DispatchesAccountEvents(t *testing.T) { testQueue := saveAndRestoreEventQueue(t, true) + var loadedAddresses sync.Map acc := createTestAccountStub() accountsDB := &mock.AccountsStub{ - GetExistingAccountCalled: func(_ []byte) (state.AccountHandler, error) { + GetExistingAccountCalled: func(addr []byte) (state.AccountHandler, error) { + loadedAddresses.Store(hex.EncodeToString(addr), true) + // Return stub with address set to the requested address + stub := createTestAccountStub() + stub.AddressBytesCalled = func() []byte { return addr } return acc, nil }, } // ... rest of test ... - require.NotEmpty(t, accountsMap) + senderKey := hex.EncodeToString([]byte("sender")) + receiverKey := hex.EncodeToString([]byte("receiver")) + require.Contains(t, accountsMap, senderKey, "sender should be in accounts map") + require.Contains(t, accountsMap, receiverKey, "receiver should be in accounts map") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@indexer/eventsProcessor_test.go` around lines 739 - 788, The test TestEventsProcessor_SaveBlock_DispatchesAccountEvents currently only checks that accountsMap is non-empty and uses a stub that returns the same account for any lookup; update the test to track which addresses are requested by replacing or augmenting mock.AccountsStub.GetExistingAccountCalled to record the address bytes passed (e.g., append to a slice or map of string keys), then after ep.SaveBlock assert that the recorded lookups (and the keys of accountsMap from the dispatched ACCOUNTS event) contain both the sender ("sender") and receiver ("receiver") addresses so you verify both accounts were processed and included in accountsMap.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@indexer/eventsProcessor.go`:
- Around line 188-211: The AccountInfo returned by
eventsProcessor.buildAccountInfo omits the Foundation field causing
inconsistency with the elastic processor; update the struct literal returned by
buildAccountInfo (in eventsProcessor) to explicitly set Foundation: false
(matching the elasticProcessor's handling) so the field is present in code even
if omitted in JSON via omitempty.
---
Duplicate comments:
In `@indexer/eventsProcessor_test.go`:
- Around line 739-788: The test
TestEventsProcessor_SaveBlock_DispatchesAccountEvents currently only checks that
accountsMap is non-empty and uses a stub that returns the same account for any
lookup; update the test to track which addresses are requested by replacing or
augmenting mock.AccountsStub.GetExistingAccountCalled to record the address
bytes passed (e.g., append to a slice or map of string keys), then after
ep.SaveBlock assert that the recorded lookups (and the keys of accountsMap from
the dispatched ACCOUNTS event) contain both the sender ("sender") and receiver
("receiver") addresses so you verify both accounts were processed and included
in accountsMap.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b784361d-b22c-4506-905a-118b7cc3e5c9
📒 Files selected for processing (2)
indexer/eventsProcessor.goindexer/eventsProcessor_test.go
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: setup-and-lint / setup-and-lint
- GitHub Check: Analyze (go)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (Custom checks)
**/*.go: Verify that any new or modified concurrent code (goroutines, channels, mutexes, sync primitives) is free of race conditions. Check for: proper lock/unlock pairing, no goroutine leaks, correct channel lifecycle management, and proper context cancellation propagation.
Verify that errors are not silently discarded. Check for: unchecked error returns, error wrapping with context, proper error propagation up the call chain, and no bare panic() calls outside of init() functions.
Files:
indexer/eventsProcessor.goindexer/eventsProcessor_test.go
**/*_test.go
⚙️ CodeRabbit configuration file
**/*_test.go: Test files. Review for: - Adequate coverage of edge cases and error paths - Proper use of test helpers and assertions - Race condition coverage (tests should use -race flag patterns) - No hardcoded sleep for synchronization (use channels or sync primitives) - Test isolation (no shared mutable state between tests)
Files:
indexer/eventsProcessor_test.go
🔇 Additional comments (10)
indexer/eventsProcessor.go (5)
17-23: LGTM — OptionalaccountsDBdependency added correctly.The
accountsDBfield is optional (no nil check incheckArgEventsProcessor), and the code handles a nil adapter gracefully inprocessAccountEventsvia thecheck.IfNilguard.Also applies to: 45-45
76-89: LGTM — Clean conditional dispatch when indexer is inactive.The guard at line 81 ensures websocket events (including accounts) are only dispatched when no active indexer is processing. Transaction preparation is done once and reused for both event types.
121-133: LGTM — Simple dispatch helper.Early return on empty slice prevents unnecessary event dispatch.
135-186: Verified: Past review concerns addressed.
- Uses
GetExistingAccount(line 166) instead ofLoadAccount, avoiding unintended account creation.- Map keyed by canonical
info.Address(line 182), consistent withSaveAccounts.- Receipt field extraction (lines 146-150) defensively handles non-string values via type assertion with
okcheck.
264-282: LGTM — Refactored to sharebuildAccountInfologic.Consistent with
processAccountEvents: both paths now use the same builder and key by canonicalinfo.Address.indexer/eventsProcessor_test.go (5)
264-265: Good fix — error handling added.Fixture errors will now fail fast instead of producing confusing downstream failures.
726-737: LGTM — Consistent helper pattern.
790-839: Good coverage of receipt address extraction.The load count assertion at line 822 validates that all three distinct addresses (sender, receipt
from, receiptto) trigger account lookups. This complements the previous test by verifying the fan-out count.
841-875: LGTM — Validates nilaccountsDBguard.Confirms that the
check.IfNil(ep.accountsDB)guard inprocessAccountEventsprevents account event dispatch.
877-922: LGTM — Validates indexer-active bypass.Confirms that when an active indexer is configured, the websocket dispatch path (including account events) is skipped entirely, delegating to the indexer's own event handling.
phcarneirobc
requested review from
Beroni,
RomuloSiebra,
fbsobreira and
nickgs1337
fbsobreira
left a comment
fbsobreira
left a comment
There was a problem hiding this comment.
The intent is correct, but the current implementation does not match the account-change detection already used by the indexer path.
The main issue is that the new fallback derives affected accounts only from:
tx.Sender- receipt
"from" - receipt
"to"
That is narrower than the existing altered-account logic in the indexer code, so the websocket fallback will still miss valid account updates for several transaction and receipt types.
Findings
1. Account detection is inconsistent with the existing indexer logic
processAccountEvents() currently collects addresses from:
tx.Sender- receipt
"from" - receipt
"to"
This misses account mutations that are already handled by the indexer through AlteredData.Accounts, including receipt fields such as:
"id"forUpdateValidator"address"forUpdateAccountPermission"owner"forUpdateMetadata"contract"forSCTrigger"address"forSetAccountName
It also misses contract-driven account changes already captured via DecodeContract() and the altered-account flow.
Impact:
- The PR fixes simple transfer-style scenarios.
- It does not restore feature parity with the indexer-backed account event path.
- Future changes will be easy to miss because the logic is now duplicated in two places.
2. The fallback can produce expected lookup failures and warning noise
The new code loads accounts for every collected address and warns on load failure. Some receipt flows use the zero address or otherwise refer to values that should not be treated as regular user accounts.
The existing indexer logic already handles zero-address flows specially during mint/burn-style operations. The new websocket fallback does not.
Impact:
- unnecessary trie lookups
- noisy warnings for expected cases
- more work on the block save path than needed
The main concern is correctness and operational behavior:
- synchronous
AccountsDB.GetExistingAccount()calls are now added to theSaveBlock()path - blocks touching many accounts can add extra trie reads before the method returns
- warning logs may increase significantly for normal edge cases if zero/missing accounts are not filtered
Recommended Approach
The better implementation is to reuse the existing altered-account extraction logic instead of creating a second account-detection heuristic inside eventsProcessor.
Proposed design
- Extract a shared helper from the current transaction/indexing path that analyzes the block transactions and returns:
- prepared transactions
- altered accounts
- optionally the full
AlteredData
- Use that same helper in both paths:
- indexer path for persistence/indexing
- websocket fallback path for account event dispatch
- In the websocket fallback:
- iterate the altered account keys
- skip
ZeroAddressDecoded - load current account state from
AccountsDB - ignore
ErrAccNotFoundwithout warning - build
AccountInfo - dispatch one
ACCOUNTSevent with the resulting map
Why this is better
- one source of truth for "which accounts changed"
- avoids duplication and future drift
- automatically covers all currently supported receipt and contract cases
- keeps websocket behavior aligned with the indexer behavior
Practical Refactor Suggestion
- Extract a pure analysis helper, for example
analyzeBlockTransactions(...) - Make it return:
[]*data.Transaction*data.AlteredData
- Have the indexer/database flow call that helper and continue with persistence
- Have
eventsProcessorcall that helper and dispatch account websocket events fromAlteredData.Accounts.GetAll()
Verification Checklist
To validate the better implementation, I would expect tests covering:
- transfer updates dispatch sender and recipient account events
UpdateValidatordispatches account events for receipt"id"UpdateAccountPermissiondispatches account events for receipt"address"UpdateMetadatadispatches account events for receipt"owner"SCTriggerdispatches account events for receipt"contract"SetAccountNamedispatches account events for the updated address- zero-address flows are skipped without warning noise
- missing accounts (
ErrAccNotFound) are ignored quietly - no websocket account events are emitted when the indexer is active
- websocket fallback and indexer-altered-account detection produce the same address set for the same block
phcarneirobc
dismissed stale reviews from Beroni and coderabbitai[bot]
via
814a15c
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@indexer/eventsProcessor.go`:
- Around line 138-170: The function dispatchAccountEventsFromAlteredAccounts
currently builds accountsMap and calls dispatchAccountEvents even when it is
empty; change it to check the populated accountsMap (built in
dispatchAccountEventsFromAlteredAccounts) and only call dispatchAccountEvents
when len(accountsMap) > 0 (otherwise return), so avoid emitting empty ACCOUNTS
events; update logic around accountsMap and the final dispatchAccountEvents call
accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2c1e58db-ab4b-47c5-b3f9-4e941ffe0b66
📒 Files selected for processing (2)
indexer/eventsProcessor.goindexer/eventsProcessor_test.go
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: notify
- GitHub Check: Analyze (go)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (Custom checks)
**/*.go: Verify that any new or modified concurrent code (goroutines, channels, mutexes, sync primitives) is free of race conditions. Check for: proper lock/unlock pairing, no goroutine leaks, correct channel lifecycle management, and proper context cancellation propagation.
Verify that errors are not silently discarded. Check for: unchecked error returns, error wrapping with context, proper error propagation up the call chain, and no bare panic() calls outside of init() functions.
Files:
indexer/eventsProcessor_test.goindexer/eventsProcessor.go
**/*_test.go
⚙️ CodeRabbit configuration file
**/*_test.go: Test files. Review for: - Adequate coverage of edge cases and error paths - Proper use of test helpers and assertions - Race condition coverage (tests should use -race flag patterns) - No hardcoded sleep for synchronization (use channels or sync primitives) - Test isolation (no shared mutable state between tests)
Files:
indexer/eventsProcessor_test.go
🧠 Learnings (1)
📚 Learning: 2026-04-21T20:12:22.959Z
Learnt from: phcarneirobc
Repo: klever-io/klever-go PR: 38
File: indexer/eventsProcessor.go:188-211
Timestamp: 2026-04-21T20:12:22.959Z
Learning: In Go structs that are JSON-marshaled, if a field is a `bool` and has the `json:"...,omitempty"` tag, then leaving that field at its zero value (`false`) is functionally equivalent (in the resulting JSON) to explicitly setting `Foundation: false`. Reviewers should not flag struct literals that omit such `bool` fields as an inconsistency; they will serialize identically because `omitempty` suppresses `false` values.
Applied to files:
indexer/eventsProcessor_test.goindexer/eventsProcessor.go
🔇 Additional comments (1)
indexer/eventsProcessor_test.go (1)
742-788: Still not proving sender/recipient fan-out in the new account-event tests.Both tests still use an
AccountsStubthat returns the same account for every lookup and then only assert event presence or lookup count. They will pass even if one affected address is dropped or the payload is keyed incorrectly. Make the stub return distinct accounts per requested address and assert that theACCOUNTSpayload contains each expected key.As per coding guidelines,
**/*_test.go: Test files. Review for: - Adequate coverage of edge cases and error paths - Proper use of test helpers and assertions.Also applies to: 793-830
There was a problem hiding this comment.
♻️ Duplicate comments (1)
indexer/eventsProcessor_test.go (1)
967-1008:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAssert emitted account keys, not just lookup count.
This test still passes if the websocket payload collapses all three altered accounts into one entry, because
GetExistingAccountCalledalways returns the samecreateTestAccountStub()and that stub always reportsAddressBytes() == "testaddr". Make the stub echo the requestedaddrBytesand assert theACCOUNTSevent containssenderAddr,fromAddr, andtoAddr.Suggested direction
- acc := createTestAccountStub() accountsDB := &mock.AccountsStub{ - GetExistingAccountCalled: func(_ []byte) (state.AccountHandler, error) { + GetExistingAccountCalled: func(addrBytes []byte) (state.AccountHandler, error) { atomic.AddInt32(&loadCount, 1) + acc := createTestAccountStub() + acc.AddressBytesCalled = func() []byte { return append([]byte(nil), addrBytes...) } return acc, nil }, } @@ - _, ok := accountEvent.Message.(map[string]*data.AccountInfo) - require.True(t, ok) + accountsMap, ok := accountEvent.Message.(map[string]*data.AccountInfo) + require.True(t, ok) + require.Contains(t, accountsMap, senderAddr) + require.Contains(t, accountsMap, fromAddr) + require.Contains(t, accountsMap, toAddr)As per coding guidelines,
**/*_test.go: Test files. Review for: - Adequate coverage of edge cases and error paths - Proper use of test helpers and assertions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@indexer/eventsProcessor_test.go` around lines 967 - 1008, The test currently only asserts load count but not that the ACCOUNTS event contains the three distinct keys; change the mock in TestEventsProcessor_DispatchAccountEventsFromAlteredAccounts_IncludesAllAddresses so GetExistingAccountCalled returns an account stub that echoes the requested addrBytes (instead of always using createTestAccountStub()), e.g. construct a state.AccountHandler whose AddressBytes() returns the incoming []byte parameter, then call ep.dispatchAccountEventsFromAlteredAccounts and inspect the dequeued ACCOUNTS event (from testQueue) to assert the event.Message map contains keys senderAddr, fromAddr and toAddr (and fail if any are missing) while keeping the existing load count assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@indexer/eventsProcessor_test.go`:
- Around line 967-1008: The test currently only asserts load count but not that
the ACCOUNTS event contains the three distinct keys; change the mock in
TestEventsProcessor_DispatchAccountEventsFromAlteredAccounts_IncludesAllAddresses
so GetExistingAccountCalled returns an account stub that echoes the requested
addrBytes (instead of always using createTestAccountStub()), e.g. construct a
state.AccountHandler whose AddressBytes() returns the incoming []byte parameter,
then call ep.dispatchAccountEventsFromAlteredAccounts and inspect the dequeued
ACCOUNTS event (from testQueue) to assert the event.Message map contains keys
senderAddr, fromAddr and toAddr (and fail if any are missing) while keeping the
existing load count assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 06fb34f1-f8b1-4319-8ca3-05d24726c11b
📒 Files selected for processing (2)
indexer/eventsProcessor.goindexer/eventsProcessor_test.go
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (Custom checks)
**/*.go: Verify that any new or modified concurrent code (goroutines, channels, mutexes, sync primitives) is free of race conditions. Check for: proper lock/unlock pairing, no goroutine leaks, correct channel lifecycle management, and proper context cancellation propagation.
Verify that errors are not silently discarded. Check for: unchecked error returns, error wrapping with context, proper error propagation up the call chain, and no bare panic() calls outside of init() functions.
Files:
indexer/eventsProcessor.goindexer/eventsProcessor_test.go
**/*_test.go
⚙️ CodeRabbit configuration file
**/*_test.go: Test files. Review for: - Adequate coverage of edge cases and error paths - Proper use of test helpers and assertions - Race condition coverage (tests should use -race flag patterns) - No hardcoded sleep for synchronization (use channels or sync primitives) - Test isolation (no shared mutable state between tests)
Files:
indexer/eventsProcessor_test.go
🧠 Learnings (1)
📚 Learning: 2026-04-21T20:12:22.959Z
Learnt from: phcarneirobc
Repo: klever-io/klever-go PR: 38
File: indexer/eventsProcessor.go:188-211
Timestamp: 2026-04-21T20:12:22.959Z
Learning: In Go structs that are JSON-marshaled, if a field is a `bool` and has the `json:"...,omitempty"` tag, then leaving that field at its zero value (`false`) is functionally equivalent (in the resulting JSON) to explicitly setting `Foundation: false`. Reviewers should not flag struct literals that omit such `bool` fields as an inconsistency; they will serialize identically because `omitempty` suppresses `false` values.
Applied to files:
indexer/eventsProcessor.goindexer/eventsProcessor_test.go
🔇 Additional comments (1)
indexer/eventsProcessor.go (1)
81-90: This concern does not apply—account state is committed beforeSaveBlockis invoked.In
CommitBlock(core/process/block/block.go line 589),mp.commitAll()persists all account changes to the database beforemp.indexBlock()is called at line 618, which triggerseventsProcessor.SaveBlock(). The websocket account events indispatchAccountEventsFromAlteredAccountsread viaep.accountsDB.GetExistingAccount(), retrieving post-commit state. No stale data risk exists here.> Likely an incorrect or invalid review comment.
GoalMake
Constraints
Current (broken) flowTwo real bugs revealed by this trace:
Target flowConcrete change set1. Carry prepared data through the indexer interfaceAdd an opaque payload to // data/indexer/args.go (or wherever ArgsSaveBlockData lives)
type ArgsSaveBlockData struct {
// … existing fields …
Prepared *PreparedBlockData // optional; if non-nil, indexer skips its own prep
}
type PreparedBlockData struct {
Txs []*data.Transaction
TxsMap map[string]*data.Transaction
Altered *data.AlteredData
}If 2. Refactor
|
| Aspect | Indexer path | This PR's websocket path | Action |
|---|---|---|---|
| Zero address skip | none | skips | Remove the skip at eventsProcessor.go:146 |
| Account loader | LoadAccount (creates on miss) |
GetExistingAccount (errors) |
Switch to LoadAccount |
| Type-assert miss | log.Warn |
silent continue |
Add log.Warn |
| Kapp accounts | uses kappsDB.LoadAccount |
(not loaded) | Add kappsDB field, mirror elasticProcessor.go:1414-1421 |
This also closes findings #4, #5, #6 from the multi-agent review.
5. Mirror the same pattern in SaveAccounts
SaveAccounts at eventsProcessor.go:237 has the same asymmetric guard (UseEventQueue && … && (indexer == nil)). Apply the same fix: dispatch websocket ACCOUNTS unconditionally when UseEventQueue, then forward to indexer when configured.
6. Pool mutation containment
Now that there's a single caller of prepareTransactionsForDatabase per block, the delete(txPool.Txs, hash) inside it has only one source. Document it (one-line comment on the function) or — preferred — move the delete out and have the orchestrator decide. With prepared-data plumbing, the indexer never needs the original pool entries anyway (txsMap carries everything).
Indexed data invariant — explicitly preserved
The following stays identical:
BuildTransactionis unchanged (samedata.Transactionshape, sameSearchOrderassignment viasetTransactionSearchOrder).DecodeContract,indexReceipt,indexAlteredAccounts,indexTransactions,indexKDAPools,indexAssets,indexProposals,indexITOs,indexMarketplaces,indexOrders,prepareAndIndexLogs,indexScDeploys— all called with the same arguments.- The
ad(altered data) object is the same instance the indexer already used; just constructed one level higher. - Elastic JSON documents — byte-identical.
What does change behaviorally for elastic-enabled deployments:
USER_TRANSACTIONS/TRANSACTIONSwebsocket events are now enqueued on the commit goroutine (matched in order withBLOCKS), instead of being enqueued later from the elastic worker goroutine. Subscribers see them in consistent order with no extra latency hop. This is a fix, not a regression.BLOCKSandACCOUNTSwebsocket events now also fire when indexer is enabled (previously didn't).
Worth a one-line note in the PR description so operators can update consumers if any rely on the old async timing.
Test plan
Replace the no-indexer-only tests with a 2×2 matrix:
UseEventQueue |
indexer enabled | Expected outcome |
|---|---|---|
| false | false | prepareTransactionsForDatabase not called; no events |
| true | false | All websocket events fire; no indexer call |
| false | true | No websocket events; indexer receives Prepared != nil |
| true | true | All websocket events fire and indexer receives Prepared != nil (single prep) |
Plus a parity test that asserts the same account set hits the websocket queue as ad.Accounts keys (no zero-address filter, no ErrAccNotFound skip). The existing TestEventsProcessor_WebsocketAndIndexerProduceSameAddressSet already exists — just remove the if addr != ZeroAddressDecoded filter on line 1168.
Performance note (from review F13)
accountsDB.GetExistingAccount / LoadAccount take mutOp.Lock() (write lock). Calling them synchronously from the block-commit goroutine for every altered account is a new cost on indexer-disabled nodes. Out of scope for this replay, but worth a follow-up: dispatch the trie reads to a buffered worker so commit isn't blocked by trie I/O. Track separately.
Suggested PR title rewrite
KLC-2309 Centralize block prep in events processor; dispatch websocket and indexer independently
Order of work
- Extend
ArgsSaveBlockDatawithPrepared *PreparedBlockData(pure additive). - Update
Indexer/saveBlockIndexerinterfaces +workItemBlockto thread it through. - Rewrite
eventsProcessor.SaveBlockper §2; delete now-dead helpers. - Update
elasticProcessor.SaveTransactionsper §3; remove itstrySendEventblock. - Align
dispatchAccountEventsFromAlteredAccountsper §4; addkappsDB. - Mirror in
SaveAccountsper §5. - Update tests for the 2×2 matrix and the parity test.
- Run the existing indexer integration test (if any) to confirm elastic JSON is unchanged.
fbsobreira
force-pushed
the
KLC-2309-dispatch-accounts-events-without-indexer
branch
2 times, most recently
from
1ba18b1 to
db5a529
Compare
fbsobreira
force-pushed
the
KLC-2309-dispatch-accounts-events-without-indexer
branch
from
db5a529 to
dc512ca
Compare
…ndexer Promote eventsProcessor to a per-block orchestrator so websocket subscribers receive BLOCKS / USER_TRANSACTIONS / TRANSACTIONS / ACCOUNTS events whether or not the elasticsearch indexer is configured. The orchestrator runs prepareTransactionsForDatabase once on the commit goroutine and forwards the prepared payload to the elastic worker via ArgsSaveBlockData.Prepared, guaranteeing websocket and elastic see the same data with no duplicate prep. Key changes: - eventsProcessor.SaveBlock is now the single per-block dispatch site for websocket events. dispatchWebsocketEvents / dispatchToIndexer / the lighter prepareTransactionsForEvents shadow path are removed. - ACCOUNTS events are derived from AlteredData.Accounts (the same set the indexer would index), each address resolved via AccountsDB.GetExistingAccount so ghost-account payloads (e.g. ZeroAddress for mint/burn) are not emitted. AccountsDB is wired through ArgEventsProcessor / startup.go. - A single buildAccountInfo (plus convertPermissions / getAllowanceWithPendingRewards / calculateUnfrozenBalance) lives in indexer/accountInfo.go and is the source of truth for AccountInfo on both the elastic and websocket paths — no more parallel implementations. - prepareTransactionsForDatabase no longer mutates pool.Txs. With the orchestrator running prep before the work item, the stale delete drained pool.Txs and caused workItems.ComputeSizeOfTxs and dispatchBlockEvent to see an empty pool, producing SizeTxs=0 in both the indexed block and the websocket BLOCKS payload. - dispatchBlockEvent now uses workItems.ComputeSizeOfTxs (bytes) for SizeTxs, matching the elastic-write path. The count is already carried by the separate TxCount field, so no information is lost. - elasticProcessor.SaveTransactions accepts a *data.PreparedBlockData and reuses it when present; the fallback re-prep is kept for direct test calls and as a safety net. - elasticProcessor no longer emits BLOCKS / TRANSACTIONS / USER_TRANSACTIONS / ACCOUNTS events; those are owned by the orchestrator. Centralization invariants are locked in by tests. - Per-block prep failure is logged at Error (not Warn) with the block nonce. Tests: - Behavioral matrix (ws on/off × indexer on/off) for SaveBlock. - Per-receipt-type fan-out tests for sender + recipient extraction (transfer, validator, account-permission, metadata, SC-trigger, set-account-name). - Regression test asserting SaveBlock does not drain TransactionsPool.Txs. - Fallback-path test for SaveTransactions(prepared=nil, non-empty pool). - Parity test confirming websocket and indexer query the same address set. - BenchmarkEventsProcessor_SaveBlock (0 / 50 / 500 tx, ws-only / ws+indexer) to anchor commit-thread cost.
…hread prep for indexer-only nodes - gate prepareTransactionsForDatabase on wsEnabled in SaveBlock; indexer-only nodes let the elastic worker re-prep on its own goroutine via the fallback in elasticProcessor.SaveTransactions, restoring pre-PR commit-thread cost - extract prepare() helper and indexerEnabled() method; unify 6 inconsistent nil-checks on ep.indexer - collapse 4 log.Warn calls in dispatchAccountEventsFromAlteredAccounts into buildAlteredAccountInfo helper; drop unreachable UserAccountHandler cast - delete narrative/change-history comments across production + tests - add BenchmarkEventsProcessor_SaveBlock_*_IndexerOnly variants to anchor the F1 gate (~30x cheaper at 500 tx/block vs WSOnly path)
…e complexity Extract resolvePreparedBlockData and indexBlockArtifacts helpers so the top-level SaveTransactions stays under the cognitive-complexity ceiling. Behavior is unchanged: same prep fallback, same sequential index steps, same first-error short-circuit.
fbsobreira
force-pushed
the
KLC-2309-dispatch-accounts-events-without-indexer
branch
from
f115ccc to
ee93104
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@indexer/eventsProcessor_test.go`:
- Around line 406-407: Replace the blocking receive "ev := <-testQueue" used
before asserting ev.EvType == BLOCKS with a non-blocking select that waits for
the event or fails fast on timeout; for example, use select { case ev :=
<-testQueue: require.Equal(t, BLOCKS, ev.EvType) case
<-time.After(1*time.Second): t.Fatalf("timed out waiting for %v event on
testQueue", BLOCKS) } (or use context.WithTimeout and require.FailNow) to avoid
deadlocks — apply this change where testQueue is read and BLOCKS is asserted
(functions/tests referencing testQueue, ev, and require.Equal).
In `@indexer/eventsProcessor.go`:
- Around line 75-99: In eventsProcessor.SaveBlock, ensure args.Prepared is
cleared each call to avoid reusing stale prepared payloads: at the start of
SaveBlock (before checking UseEventQueue/indexerEnabled) set args.Prepared =
nil, or explicitly set args.Prepared = nil in the non-websocket branch so that
prepare()’s result is only used when freshly produced; update references to
eventsProcessor.prepare, UseEventQueue, indexerEnabled and args.Prepared
accordingly.
In `@indexer/workItems/workItemBlock_test.go`:
- Around line 55-56: The tests set SaveTransactionsCalled mocks but never verify
the forwarded prepared parameter — update each mock (the SaveTransactionsCalled
closures) to capture/compare the incoming prepared value against the expected
prepared object: define an expectedPrepared variable in the test, have the mock
assert require.Equal(t, expectedPrepared, prepared) (or use reflect.DeepEqual +
t.Fatal) and return the same error paths as before; do this for both occurrences
of SaveTransactionsCalled so the forwarding contract for prepared is explicitly
asserted.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3b64d699-9d33-4eee-9a41-331c252822b1
📒 Files selected for processing (15)
cmd/node/startup.godata/indexer/dtos.goindexer/accountInfo.goindexer/data/prepared.goindexer/dataIndexerArgs.goindexer/elasticProcessor.goindexer/elasticProcessor_test.goindexer/eventsProcessor.goindexer/eventsProcessor_test.goindexer/interface.goindexer/mock/elasticProcessorStub.goindexer/processTransactions.goindexer/workItems/interface.goindexer/workItems/workItemBlock.goindexer/workItems/workItemBlock_test.go
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
**/*.go
📄 CodeRabbit inference engine (Custom checks)
**/*.go: Verify that any new or modified concurrent code (goroutines, channels, mutexes, sync primitives) is free of race conditions. Check for: proper lock/unlock pairing, no goroutine leaks, correct channel lifecycle management, and proper context cancellation propagation.
Verify that errors are not silently discarded. Check for: unchecked error returns, error wrapping with context, proper error propagation up the call chain, and no bare panic() calls outside of init() functions.
Files:
indexer/workItems/workItemBlock.godata/indexer/dtos.goindexer/interface.goindexer/dataIndexerArgs.goindexer/workItems/workItemBlock_test.gocmd/node/startup.goindexer/data/prepared.goindexer/workItems/interface.goindexer/mock/elasticProcessorStub.goindexer/accountInfo.goindexer/processTransactions.goindexer/elasticProcessor_test.goindexer/elasticProcessor.goindexer/eventsProcessor_test.goindexer/eventsProcessor.go
data/**
⚙️ CodeRabbit configuration file
data/**: Core data structures and state management. - Verify protobuf serialization/deserialization is correct and backwards-compatible - Check for proper nil/empty checks on decoded data - Ensure trie operations maintain data integrity - Validate state transitions are deterministic
Files:
data/indexer/dtos.go
**/*_test.go
⚙️ CodeRabbit configuration file
**/*_test.go: Test files. Review for: - Adequate coverage of edge cases and error paths - Proper use of test helpers and assertions - Race condition coverage (tests should use -race flag patterns) - No hardcoded sleep for synchronization (use channels or sync primitives) - Test isolation (no shared mutable state between tests)
Files:
indexer/workItems/workItemBlock_test.goindexer/elasticProcessor_test.goindexer/eventsProcessor_test.go
🧠 Learnings (1)
📚 Learning: 2026-04-21T20:12:22.959Z
Learnt from: phcarneirobc
Repo: klever-io/klever-go PR: 38
File: indexer/eventsProcessor.go:188-211
Timestamp: 2026-04-21T20:12:22.959Z
Learning: In Go structs that are JSON-marshaled, if a field is a `bool` and has the `json:"...,omitempty"` tag, then leaving that field at its zero value (`false`) is functionally equivalent (in the resulting JSON) to explicitly setting `Foundation: false`. Reviewers should not flag struct literals that omit such `bool` fields as an inconsistency; they will serialize identically because `omitempty` suppresses `false` values.
Applied to files:
indexer/workItems/workItemBlock.godata/indexer/dtos.goindexer/interface.goindexer/dataIndexerArgs.goindexer/workItems/workItemBlock_test.gocmd/node/startup.goindexer/data/prepared.goindexer/workItems/interface.goindexer/mock/elasticProcessorStub.goindexer/accountInfo.goindexer/processTransactions.goindexer/elasticProcessor_test.goindexer/elasticProcessor.goindexer/eventsProcessor_test.goindexer/eventsProcessor.go
🔇 Additional comments (12)
indexer/workItems/workItemBlock.go (1)
61-61: LGTM!data/indexer/dtos.go (1)
14-17: LGTM!indexer/interface.go (1)
34-34: LGTM!indexer/dataIndexerArgs.go (1)
57-57: LGTM!cmd/node/startup.go (1)
732-732: LGTM!indexer/data/prepared.go (1)
1-9: LGTM!indexer/workItems/interface.go (1)
19-19: LGTM!indexer/mock/elasticProcessorStub.go (1)
21-22: LGTM!Also applies to: 88-90
indexer/accountInfo.go (1)
17-44: LGTM!Also applies to: 46-103
indexer/processTransactions.go (1)
52-54: LGTM!indexer/elasticProcessor_test.go (1)
170-201: LGTM!Also applies to: 238-239, 306-307, 320-321, 360-361, 1365-1393, 1395-1437, 1490-1592
indexer/elasticProcessor.go (1)
527-600: LGTM!Also applies to: 1591-1592
Problem
WebSocket account subscriptions only worked when the Elasticsearch indexer was active. Without it, account state changes were never dispatched
to subscribers — the entire account event path ran exclusively through the indexer pipeline.
Solution
Added
processAccountEventsto thedispatchWebsocketEventspath insideeventsProcessor. When no indexer is active, it extracts theaffected addresses (senders + receipt
"to"fields) from the block's transactions, loads each account's current state fromAccountsDB, anddispatches the account events directly to the WebSocket event queue.
Key Changes
processAccountEvents()inindexer/eventsProcessor.go— extracts affected addresses from transactions and dispatches ACCOUNTSevents
buildAccountInfo()inindexer/eventsProcessor.go— builds*data.AccountInfodirectly fromUserAccountHandlerArgEventsProcessorinindexer/dataIndexerArgs.go— addedAccountsDB state.AccountsAdapterfieldeventsProcessorstruct — addedaccountsDBdependency injected at constructioncmd/node/startup.go— passesstateComponents.AccountsAdapterwhen creatingeventsProcessorindexer/eventsProcessor_test.go— 3 new test cases covering the new dispatch pathTesting
make tests)TestEventsProcessor_SaveBlock_DispatchesAccountEvents— verifies ACCOUNTS event is dispatched when AccountsDB is presentTestEventsProcessor_SaveBlock_SkipsAccountEventsWhenNoAccountsDB— verifies no dispatch when AccountsDB is nilTestEventsProcessor_SaveBlock_SkipsAccountEventsWhenIndexerActive— verifies no websocket events when indexer is handling dispatchConfiguration Changes
None
Breaking Changes
None
This pull request ensures WebSocket subscribers receive account-state websocket ACCOUNTS events even when the Elasticsearch indexer is not running by dispatching account events directly during the SaveBlock/commit path.
Blockchain-Critical Components Affected
State management / account state access
Transaction processing / event dispatch
Node Stability, Data Integrity & Error Handling
Cross-Cutting / Dependency Changes
Testing & Verification
Risk Summary
Reviewer Notes (design critique)