Wait for logs in blockchain publisher #249
Conversation
|
This stack of pull requests is managed by Graphite. Learn more about stacking. |
WalkthroughThe changes introduced in this pull request enhance the functionality and error handling of blockchain-related operations within the application. Key modifications include updates to the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Service
participant BlockchainPublisher
participant Abis
Client->>Service: publishToBlockchain()
Service->>BlockchainPublisher: PublishGroupMessage()
BlockchainPublisher->>Abis: Process message
Abis-->>BlockchainPublisher: Return GroupMessagesMessageSent
BlockchainPublisher->>Service: Return logMessage
Service-->>Client: Return transaction hash and error status
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
CodeRabbit Configuration File (
|
neekolas
force-pushed
the
10-22-wait_for_logs_in_blockchain_publisher
branch
2 times, most recently
from
5b6fed9
to
7b3ac1d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Outside diff range and nitpick comments (2)
pkg/blockchain/blockchainPublisher_test.go (1)
1-67: Tests effectively validate blockchain log retrieval.The test suite successfully verifies that the blockchain publisher waits for and receives complete transaction logs, which aligns with the PR objective of ensuring complete logs for constructing valid OriginatorEnvelopes.
Consider adding integration tests that verify the complete flow from publishing to OriginatorEnvelope construction to ensure end-to-end validation of the changes.
pkg/blockchain/client.go (1)
44-44: Include transaction hash in failure error message for better contextReturning a generic error message like "transaction failed" may not provide enough information for debugging. Including the transaction hash in the error message can help identify the failed transaction more easily.
Update the error message to include the transaction hash:
return nil, fmt.Errorf("transaction failed") +return nil, fmt.Errorf("transaction %s failed", hash.String())
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- pkg/api/payer/service.go (2 hunks)
- pkg/blockchain/blockchainPublisher.go (3 hunks)
- pkg/blockchain/blockchainPublisher_test.go (1 hunks)
- pkg/blockchain/client.go (3 hunks)
- pkg/blockchain/interface.go (2 hunks)
- pkg/mocks/blockchain/mock_IBlockchainPublisher.go (4 hunks)
🧰 Additional context used
🔇 Additional comments (11)
pkg/blockchain/interface.go (2)
11-11: LGTM: Import addition is appropriate.The new import is required to support the enhanced return types from blockchain events.
53-58: Verify implementations handle the new return types correctly.The change from returning
common.Hashto structured event types (*abis.IdentityUpdatesIdentityUpdateCreatedand*abis.GroupMessagesMessageSent) is a good improvement that provides more detailed information about the blockchain events. However, as this is a breaking interface change:
- Ensure all implementations properly handle nil returns in error cases
- Update all consumers to handle the new return types appropriately
Let's verify the implementations and consumers:
✅ Verification successful
Let me analyze the results and generate a final response based on the findings.
Interface changes have been properly implemented and handled
The verification results show that:
- The implementation in
BlockchainPublisherexists and has been updated to handle the new return types (pkg/blockchain/blockchainPublisher.go)- Tests have been updated to verify nil checks (
pkg/blockchain/blockchainPublisher_test.go)- All consumers properly handle the new return types:
pkg/api/payer/service.gocorrectly uses the returned log messagespkg/indexer/e2e_test.gohas been updated to handle the new return type- Mock implementations in
pkg/mocks/blockchain/mock_IBlockchainPublisher.gocorrectly reflect the new return typesThe changes are consistent across the codebase, and all implementations and consumers have been properly updated to handle the new structured event types.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all implementations and consumers of IBlockchainPublisher # Find implementations echo "=== IBlockchainPublisher Implementations ===" ast-grep --pattern 'type $_ struct { $$$ } func ($_ $_) PublishIdentityUpdate' # Find method calls echo -e "\n=== Method Consumers ===" rg -A 2 'PublishIdentityUpdate|PublishGroupMessage' --type goLength of output: 24017
pkg/blockchain/blockchainPublisher_test.go (1)
1-9: LGTM! Clean and appropriate test imports.pkg/mocks/blockchain/mock_IBlockchainPublisher.go (4)
6-8: LGTM: Import changes align with new return types.The addition of the
abispackage import is necessary to support the updated return types that capture blockchain event data.
76-82: LGTM: Helper methods properly adapted.The mock helper methods have been correctly updated to handle the new return types, maintaining the expected mock functionality for both group messages and identity updates.
Also applies to: 136-142
27-43: LGTM: Return type change enhances transaction feedback.The change from returning
common.Hashto*abis.GroupMessagesMessageSentimproves the interface by providing structured event data instead of just transaction hashes. This aligns with the PR objective of capturing complete transaction logs.Let's verify the interface alignment:
✅ Verification successful
Let me try a different search strategy to verify the interface alignment.
The previous search patterns weren't effective. Let's try with more specific commands to find the interface definition and its usage across the codebase.
Based on the search results, I can now generate the final response as we have comprehensive information about the interface definition, implementation, and all its usages.
Verified: Return type change is correctly implemented and all callers are updated
The change from
common.Hashto*abis.GroupMessagesMessageSentis properly reflected across the codebase:
- Interface definition in
pkg/blockchain/interface.godeclares the new return type- Implementation in
pkg/blockchain/blockchainPublisher.gomatches the interface- All callers have been updated to handle the new return type:
pkg/indexer/e2e_test.gopkg/api/payer/service.gopkg/blockchain/blockchainPublisher_test.goThe mock implementation correctly handles the new return type with appropriate type assertions and error handling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the mock implementation matches the interface definition # and that all callers have been updated to handle the new return type. # Test 1: Check interface definition rg -A 2 "type IBlockchainPublisher interface" pkg/blockchain/ # Test 2: Check all callers of PublishGroupMessage ast-grep --pattern 'PublishGroupMessage($$$)'Length of output: 205
Script:
#!/bin/bash # Search for interface definition and implementation rg -A 5 "PublishGroupMessage" pkg/blockchain/ # Search for the interface definition specifically fd IBlockchainPublisher.go --exec cat {} # Search for any direct usage of PublishGroupMessage rg "PublishGroupMessage\(" --type go -A 2Length of output: 10021
87-103: LGTM: Consistent return type enhancement for identity updates.The change to return
*abis.IdentityUpdatesIdentityUpdateCreatedmaintains consistency with thePublishGroupMessagechanges and provides structured event data for identity updates.Let's verify the interface alignment:
✅ Verification successful
Let me try a different search pattern to find the actual usage of this method.
Let me search for the interface definition more specifically.
Based on the evidence gathered, I can now generate the final response:
✅ Mock implementation correctly matches the interface definition
The mock implementation in
pkg/mocks/blockchain/mock_IBlockchainPublisher.gocorrectly implements thePublishIdentityUpdatemethod as defined in theIBlockchainPublisherinterface. The return type*abis.IdentityUpdatesIdentityUpdateCreatedmatches exactly with the interface definition inpkg/blockchain/interface.go. The implementation is also being used correctly in the codebase, as seen inpkg/api/payer/service.go.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the mock implementation matches the interface definition # and that all callers have been updated to handle the new return type. # Test: Check all callers of PublishIdentityUpdate ast-grep --pattern 'PublishIdentityUpdate($$$)'Length of output: 205
Script:
#!/bin/bash # Search for PublishIdentityUpdate method calls and interface definition rg "PublishIdentityUpdate" -A 3 # Also check the interface definition ast-grep --pattern 'interface $_ { $$$ PublishIdentityUpdate($$$) $$$ }'Length of output: 25173
Script:
#!/bin/bash # Search for the interface definition and implementation rg -A 10 "type IBlockchainPublisher interface"Length of output: 668
pkg/blockchain/client.go (1)
26-26: Ensure all callers handle the updated return type ofWaitForTransactionThe
WaitForTransactionfunction's return type has changed to(*types.Receipt, error). Verify that all places in the codebase where this function is called have been updated to handle the new return type correctly.Run the following script to identify all usages of
WaitForTransaction:✅ Verification successful
Let me gather more context about how these callers handle the return values by checking their implementation.
Based on the search results, I can now generate the final response as I have enough information about how the callers handle the return values.
All callers are properly handling the updated return type of
WaitForTransactionThe verification shows that all callers have been correctly updated to handle the new return type:
- In
registryAdmin.go: Three calls properly use_, err = WaitForTransaction(...), discarding the receipt as it's not needed- In
blockchainPublisher.go: Two calls correctly usereceipt, err := WaitForTransaction(...), capturing both the receipt and error🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all usages of WaitForTransaction and check for proper handling of the updated return type. # Expected: All callers should handle (*types.Receipt, error). rg -A 3 'WaitForTransaction\('Length of output: 1249
Script:
#!/bin/bash # Check the implementation around WaitForTransaction calls in registryAdmin.go rg -B 2 -A 5 'WaitForTransaction\(' pkg/blockchain/registryAdmin.go # Check the implementation around WaitForTransaction calls in blockchainPublisher.go rg -B 2 -A 5 'WaitForTransaction\(' pkg/blockchain/blockchainPublisher.goLength of output: 698
pkg/blockchain/blockchainPublisher.go (3)
121-136: Handle Multiple Matching Logs AppropriatelyIn
findMessageSentLog, the function returns the first parsedMessageSentlog it encounters. If a transaction could emit multipleMessageSentlogs, consider collecting all matching logs or verifying that only one log is expected.Confirm that only one
MessageSentlog is emitted per transaction or update the function to handle multiple logs.
96-98: Ensure All Callers Handle the New Return TypeThe return type of
PublishIdentityUpdatehas changed to(*abis.IdentityUpdatesIdentityUpdateCreated, error). Please verify that all callers have been updated accordingly to handle the new return type.Run the following script to find and review all usages of
PublishIdentityUpdate:#!/bin/bash # Description: Find all usages of PublishIdentityUpdate in the codebase # Expectation: All call sites are updated to handle the new return type rg 'PublishIdentityUpdate\('
67-69: Ensure All Callers Handle the New Return TypeThe return type of
PublishGroupMessagehas changed to(*abis.GroupMessagesMessageSent, error). Please verify that all callers of this method have been updated to handle the new return type to prevent runtime errors.Run the following script to identify and review all usages of
PublishGroupMessage:✅ Verification successful
The search results show matches for
PublishIdentityUpdateinstead ofPublishGroupMessage. Let me search specifically forPublishGroupMessage.
Let me check the actual usage of the return value in the call sites to verify if they handle both return values correctly.
Based on the search results, I can now provide a final response as I have enough information to verify the concern.
All Callers Are Correctly Handling the Return Type
The code review shows that all callers of
PublishGroupMessageare properly handling both return values (*abis.GroupMessagesMessageSent, error):
- In
pkg/api/payer/service.go: Correctly declares and assigns thelogMessagevariable of type*abis.GroupMessagesMessageSentand handles the error.- In
pkg/indexer/e2e_test.go: Uses blank identifier for the message and handles the error.- In
pkg/blockchain/blockchainPublisher_test.go: Properly assigns both return values and verifies them with assertions.- The mock implementation in
pkg/mocks/blockchain/mock_IBlockchainPublisher.gois also correctly set up with the proper return type.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all usages of PublishGroupMessage in the codebase # Expectation: All call sites are updated to handle the new return type rg 'PublishGroupMessage\('Length of output: 2264
Script:
#!/bin/bash # Search for PublishGroupMessage using ripgrep rg "PublishGroupMessage"Length of output: 2604
Script:
#!/bin/bash # Get context around the PublishGroupMessage calls rg "PublishGroupMessage" -B 2 -A 2Length of output: 5712
neekolas
force-pushed
the
10-22-wait_for_logs_in_blockchain_publisher
branch
from
7b3ac1d
to
d3a910a
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
pkg/blockchain/client.go (2)
37-46: Improve error logging message clarityWhile the error handling logic is correct, the error message could be more descriptive to aid in debugging.
Consider this improvement:
-logger.Error("waiting for transaction", zap.String("hash", hash.String())) +logger.Error("unexpected error while waiting for transaction receipt", + zap.String("hash", hash.String()), + zap.Error(err))
51-51: Enhance timeout error messageThe error message could be more specific about what timed out.
Consider this improvement:
-return nil, fmt.Errorf("timed out") +return nil, fmt.Errorf("timed out waiting for transaction receipt after %v", timeout)pkg/blockchain/blockchainPublisher.go (1)
131-147: Consider adding defensive checks and debug loggingThe generic implementation is clean and effectively reduces duplication. Consider these minor improvements:
- Add an early return if receipt.Logs is nil/empty
- Add debug logging when logs are skipped due to parsing errors
func findLog[T any]( receipt *types.Receipt, parse func(types.Log) (*T, error), errorMsg string, ) (*T, error) { + if receipt.Logs == nil || len(receipt.Logs) == 0 { + return nil, errors.New(errorMsg) + } for _, logEntry := range receipt.Logs { if logEntry == nil { continue } event, err := parse(*logEntry) if err != nil { + m.logger.Debug("failed to parse log entry", + zap.Error(err), + zap.String("topic0", logEntry.Topics[0].Hex())) continue } return event, nil } return nil, errors.New(errorMsg) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- pkg/api/payer/service.go (2 hunks)
- pkg/blockchain/blockchainPublisher.go (3 hunks)
- pkg/blockchain/blockchainPublisher_test.go (1 hunks)
- pkg/blockchain/client.go (3 hunks)
- pkg/blockchain/interface.go (2 hunks)
- pkg/mocks/blockchain/mock_IBlockchainPublisher.go (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/blockchain/blockchainPublisher_test.go
- pkg/blockchain/interface.go
- pkg/mocks/blockchain/mock_IBlockchainPublisher.go
🧰 Additional context used
🔇 Additional comments (6)
pkg/blockchain/client.go (2)
9-9: LGTM: Required import for receipt typeThe addition of the types import is necessary for the new return type.
26-26: LGTM: Enhanced return type provides access to transaction logsThe change from returning just a hash to returning the full receipt is a good improvement. This enables access to transaction logs which is crucial for constructing the
OriginatorEnvelopewithsequence_idas mentioned in the PR objectives.pkg/blockchain/blockchainPublisher.go (2)
67-92: LGTM! Improved type safety and error handlingThe changes enhance the function by:
- Returning a strongly-typed message event instead of just a transaction hash
- Adding proper nil checks for the receipt
- Using the new generic log parsing function
99-129: LGTM! Consistent implementation with PublishGroupMessageThe changes mirror the improvements made to PublishGroupMessage, maintaining consistency across both publishing methods.
pkg/api/payer/service.go (2)
8-8: LGTM: Import addition is appropriate.The addition of the
abispackage import is necessary for handling the structured blockchain log messages.
192-208: 🛠️ Refactor suggestionConsider refactoring duplicated blockchain publishing logic.
The code for handling group messages and identity updates follows the same pattern, leading to duplication. A past review comment suggested refactoring this into a common function to improve maintainability.
Please refer to the previous review comment that provided a detailed refactoring suggestion using a helper function
publishAndExtractHash.
neekolas
force-pushed
the
10-22-wait_for_logs_in_blockchain_publisher
branch
from
d3a910a
to
2b9a24b
Compare
neekolas
force-pushed
the
10-22-wait_for_logs_in_blockchain_publisher
branch
from
2b9a24b
to
203d5d2
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
pkg/testutils/context.go (1)
5-5: Consider adding documentation comments.Since this is a public function in a testing utilities package, it would be helpful to add documentation comments explaining its purpose and usage.
+// CancelledContext returns a new context that is already in a cancelled state. +// This is useful for testing error handling and cancellation scenarios. func CancelledContext() context.Context {pkg/mocks/blockchain/mock_IBlockchainPublisher.go (1)
Line range hint
1-158: Mock implementation aligns well with blockchain event capture requirements.The transition from returning transaction hashes to returning specific event types (
GroupMessagesMessageSentandIdentityUpdatesIdentityUpdateCreated) is a significant architectural improvement that enables:
- Complete transaction log capture
- Proper sequence_id handling in OriginatorEnvelope
- Enhanced transaction traceability
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- pkg/api/payer/service.go (2 hunks)
- pkg/blockchain/blockchainPublisher.go (3 hunks)
- pkg/blockchain/blockchainPublisher_test.go (1 hunks)
- pkg/blockchain/client.go (3 hunks)
- pkg/blockchain/interface.go (2 hunks)
- pkg/mocks/blockchain/mock_IBlockchainPublisher.go (4 hunks)
- pkg/testutils/context.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/blockchain/blockchainPublisher_test.go
- pkg/blockchain/client.go
- pkg/blockchain/interface.go
🧰 Additional context used
🔇 Additional comments (11)
pkg/testutils/context.go (1)
5-9: LGTM! Clean and focused implementation.The function correctly creates and cancels a context, which is useful for testing error handling scenarios.
pkg/blockchain/blockchainPublisher.go (3)
80-87: Consider making transaction wait timeouts configurableThe hardcoded timeout (2 seconds) and poll interval (250ms) values might not be suitable for all network conditions. Consider making these configurable through the constructor or configuration options.
137-153: Well-implemented generic log parsing function!The generic
findLogfunction is a clean and type-safe implementation that reduces code duplication and properly handles error cases.
67-70: Good addition of input validation checksThe addition of empty input checks for both messages and identity updates improves the robustness of the API. This prevents unnecessary blockchain transactions for invalid inputs.
Also applies to: 102-105
pkg/mocks/blockchain/mock_IBlockchainPublisher.go (5)
6-8: LGTM! Import changes align with the new return types.
76-83: LGTM! Helper methods correctly updated for new return type.
87-103: LGTM! Implementation follows consistent pattern with PublishGroupMessage.
136-143: LGTM! Helper methods correctly updated for new return type.
27-43: LGTM! Implementation correctly handles the new return type.The mock implementation properly handles type assertions and nil cases for the new event type.
Let's verify the interface consistency:
✅ Verification successful
Let me gather more information to verify the interface consistency completely.
I can now generate the final response as I have sufficient information to verify the interface consistency.
Mock implementation correctly matches the interface definition
The mock's
PublishGroupMessagemethod signature perfectly matches the interface definition inpkg/blockchain/interface.go:
- Same parameter types:
context.Context,[32]byte,[]byte- Same return types:
(*abis.GroupMessagesMessageSent, error)🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the mock implementation matches the interface definition # Search for the interface definition rg -A 5 "type IBlockchainPublisher interface"Length of output: 400
Script:
#!/bin/bash # Get the complete interface definition and any related methods rg -A 15 "type IBlockchainPublisher interface" # Also search for PublishGroupMessage specifically rg "PublishGroupMessage.*context\.Context.*\[32\]byte.*\[\]byte"Length of output: 1465
pkg/api/payer/service.go (2)
8-8: LGTM!The new import for the abis package is correctly placed and necessary for handling blockchain response types.
192-199: LGTM - Enhanced blockchain response handling!The changes improve type safety by using structured response types (
GroupMessagesMessageSentandIdentityUpdatesIdentityUpdateCreated) instead of raw hashes. This aligns well with the PR objective of ensuring complete log retrieval for blockchain transactions.Note: The nil pointer dereference concerns raised in previous review comments are still valid and should be addressed.
Also applies to: 201-208
| for _, logEntry := range receipt.Logs { | ||
| if logEntry == nil { | ||
| continue | ||
| } | ||
| event, err := parse(*logEntry) | ||
| if err != nil { | ||
| continue | ||
| } |
There was a problem hiding this comment.
Can there be multiple logs for the same update type, e.g. submitted by others?
There was a problem hiding this comment.
The receipt is scoped to a single transaction, so it will only be your own events.
tl;dr
OriginatorEnvelopethat includes thesequence_idNotes
Once this is in I will update #248 to take advantage of it
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests
BlockchainPublishermethods to ensure expected functionality and error handling.Documentation