Fix issue with identity update storer #744
Conversation
WalkthroughThe changes update the construction of the topic parameter used in querying gateway envelopes within the Changes
Sequence Diagram(s)sequenceDiagram
participant Test as TestStoreSequential
participant Storer as IdentityUpdateStorer
participant Validation as MockedMLSValidationService
participant DB as GatewayEnvelopeStore
Test->>Storer: StoreLog(identityUpdateLog1)
Storer->>Validation: GetAssociationStateFromEnvelopes([])
Validation-->>Storer: StateDiff with new member
Storer->>DB: Store envelope for log1
Test->>DB: Query envelopes for inboxId
DB-->>Test: Return 1 envelope
Test->>Storer: StoreLog(identityUpdateLog2)
Storer->>Validation: GetAssociationStateFromEnvelopes([envelope1])
Validation-->>Storer: Empty StateDiff
Storer->>DB: Store envelope for log2 (if valid)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (1.64.8)Error: you are using a configuration file for golangci-lint v2 with golangci-lint v1: please use golangci-lint v2 ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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
CodeRabbit Configuration File (
|
Replace string-based inbox topic formatting with structured topic creation in identity update storer to fix topic identifier construction
📍Where to StartStart with the Macroscope summarized 3c4bf78. |
neekolas
force-pushed
the
04-24-fix_issue_with_identity_update_storer
branch
from
7030e20 to
3c4bf78
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/indexer/storer/identityUpdate_test.go (2)
124-125: Consider adding more specific assertions.While checking the length of previous envelopes is good, consider adding more specific assertions to verify the exact content and sequence ID of the previous envelope.
if numCalls > 1 { require.Len(t, prevEnvs, 1) + require.Equal(t, int64(sequenceID), prevEnvs[0].OriginatorSequenceID) + // Add additional checks for the envelope content if needed
172-173: Missing envelope content verification.While you're checking the count of envelopes, consider adding verification of the envelope content after the first store operation, similar to what you do in
TestStoreIdentityUpdate(lines 104-106).require.NoError(t, queryErr) require.Equal(t, len(envelopes), 1) + +// Verify the content of the stored envelope +firstEnvelope := envelopes[0] +deserializedEnvelope := envelopesProto.OriginatorEnvelope{} +require.NoError(t, proto.Unmarshal(firstEnvelope.OriginatorEnvelope, &deserializedEnvelope)) +require.Greater(t, len(deserializedEnvelope.UnsignedOriginatorEnvelope), 0)
🛑 Comments failed to post (1)
pkg/indexer/storer/identityUpdate_test.go (1)
184-185: 🛠️ Refactor suggestion
Add verification after second update.
The test stores a second update but doesn't verify that there are now two envelopes or check the sequence ID of the second envelope. Consider adding this verification.
require.NoError(t, storer.StoreLog( ctx, logMessage, )) + + // Verify that both envelopes are now stored + envelopes, queryErr = querier.SelectGatewayEnvelopes( + ctx, + queries.SelectGatewayEnvelopesParams{ + OriginatorNodeIds: []int32{IDENTITY_UPDATE_ORIGINATOR_ID}, + RowLimit: 10, + }, + ) + require.NoError(t, queryErr) + require.Equal(t, 2, len(envelopes)) + + // Check that the sequence IDs are correct + var sequenceIDs []int64 + for _, env := range envelopes { + sequenceIDs = append(sequenceIDs, env.OriginatorSequenceID) + } + require.Contains(t, sequenceIDs, int64(sequenceID)) + require.Contains(t, sequenceIDs, int64(sequenceID+1))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.require.NoError(t, storer.StoreLog( ctx, logMessage, )) // Verify that both envelopes are now stored envelopes, queryErr = querier.SelectGatewayEnvelopes( ctx, queries.SelectGatewayEnvelopesParams{ OriginatorNodeIds: []int32{IDENTITY_UPDATE_ORIGINATOR_ID}, RowLimit: 10, }, ) require.NoError(t, queryErr) require.Equal(t, 2, len(envelopes)) // Check that the sequence IDs are correct var sequenceIDs []int64 for _, env := range envelopes { sequenceIDs = append(sequenceIDs, env.OriginatorSequenceID) } require.Contains(t, sequenceIDs, int64(sequenceID)) require.Contains(t, sequenceIDs, int64(sequenceID+1)) }
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/indexer/storer/identityUpdate_test.go (1)
110-170: Good addition of sequential update test case.This test effectively validates that sequential identity updates for the same inbox work correctly with the new topic implementation. It properly tests the validation service interaction by ensuring previous envelopes are correctly passed to subsequent validation calls.
Consider enhancing this test by verifying the database state after both store operations, similar to how TestStoreIdentityUpdate checks for stored envelopes and address logs. This would provide a more complete verification that both updates were properly persisted.
require.NoError(t, storer.StoreLog( ctx, logMessage, )) + + // Verify database state after storing both updates + querier := queries.New(storer.db) + + envelopes, queryErr := querier.SelectGatewayEnvelopes( + ctx, + queries.SelectGatewayEnvelopesParams{ + OriginatorNodeIds: []int32{IDENTITY_UPDATE_ORIGINATOR_ID}, + RowLimit: 10, + }, + ) + require.NoError(t, queryErr) + require.Equal(t, len(envelopes), 2) // Should have two envelopes now + + getInboxIdResult, logsErr := querier.GetAddressLogs(ctx, []string{newAddress}) + require.NoError(t, logsErr) + require.Equal(t, getInboxIdResult[0].InboxID, utils.HexEncode(inboxId[:]))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/indexer/storer/identityUpdate.go(1 hunks)pkg/indexer/storer/identityUpdate_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/indexer/storer/identityUpdate.go
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test (Node)
- GitHub Check: Upgrade Tests
🔇 Additional comments (1)
pkg/indexer/storer/identityUpdate_test.go (1)
83-86: Good refactoring of error handling.The simplified error handling improves code readability by directly asserting no error from
storer.StoreLoginstead of using an intermediate error variable. This reduces unnecessary code while maintaining the same functionality.
TL;DR
What changed?
BuildInboxTopicfunction with the standardizedtopic.NewTopicimplementationBuildInboxTopicfunctionTestStoreSequentialto verify sequential identity updates work correctly with the new topic formatHow to test?
Why make this change?
This change standardizes how we handle topics for identity updates by using the common topic package instead of a custom string format. This improves consistency across the codebase and ensures that all components use the same topic format when working with identity updates.
Summary by CodeRabbit