Aggregate and expose EVM-related event payloads #628
Conversation
m-Peter
requested review from
janezpodhostnik,
peterargue,
zhangchiqing and
ramtinms
as code owners
WalkthroughThe changes in this pull request primarily involve modifications to the event decoding functions and their associated tests within the codebase. The Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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 (
|
![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
🧹 Outside diff range and nitpick comments (10)
models/receipt_test.go (1)
Line range hint
10-21: Improve test documentation and structure.The test function would benefit from:
- A clear description of what aspects are being tested
- Documentation of test data setup via
createTestEvent- Table-driven tests for better coverage of different scenarios
Here's a suggested improvement:
+// Test_DecodeReceipts verifies that transaction events are correctly decoded into receipts +// and their associated event payloads. It ensures: +// - Receipt logs are correctly decoded +// - Log topics match expected values +// - Transaction event payloads are properly extracted func Test_DecodeReceipts(t *testing.T) { + // Test cases with different event scenarios + tests := []struct { + name string + eventBin []byte + wantErr bool + }{ + { + name: "successful decode", + eventBin: evmTxBinary, + wantErr: false, + }, + // Add more test cases + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Existing test logic here + }) + } }models/block.go (2)
74-74: Update function documentation to reflect the new return value.The function comment should be updated to document the additional
*events.BlockEventPayloadreturn value and explain its purpose.Apply this diff to improve the documentation:
-// decodeBlockEvent takes a cadence event that contains executed block payload and -// decodes it into the Block type. +// decodeBlockEvent takes a cadence event that contains executed block payload and +// decodes it into both Block and BlockEventPayload types. The BlockEventPayload +// contains the raw event data needed for the Block Replayer component.
Line range hint
74-105: Consider adding payload validation.The function correctly decodes and returns the payload, but it might be beneficial to add validation for critical payload fields before returning, especially since this data will be used by the Block Replayer component.
Consider adding validation before the return statement:
+ // Validate critical payload fields + if payload.Height == 0 { + return nil, nil, fmt.Errorf("invalid block height: %d", payload.Height) + } + if payload.Timestamp == 0 { + return nil, nil, fmt.Errorf("invalid block timestamp: %d", payload.Timestamp) + } + return &Block{ Block: &types.Block{ ParentBlockHash: payload.ParentBlockHash,models/block_test.go (2)
115-115: Consider testing the new event payload.While ignoring the payload with
_is valid for maintaining existing test behavior, consider adding test cases to verify the contents of the newBlockEventPayloadbeing returned bydecodeBlockEvent. This would ensure the payload aggregation functionality works as expected.
115-115: Ensure comprehensive test coverage for event payload aggregation.Both test functions have been updated to accommodate the new payload return value from
decodeBlockEvent, but neither verifies the payload contents. Given that this PR's objective is to "aggregate and expose EVM-related event payloads", we should have dedicated test coverage for:
- Payload contents for current block format
- Payload handling for legacy block format
- Proper sorting of aggregated payloads
- Edge cases (empty payloads, malformed data)
This will ensure the Block Replayer component receives correctly formatted and ordered data.
Would you like me to help create these additional test cases?
Also applies to: 153-153
models/events.go (1)
39-44: Consider memory optimization for large payloads.The
CadenceEventsstruct now stores both raw events and their decoded payloads. For blocks with many transactions, this could lead to increased memory usage. Consider implementing lazy loading for payloads or adding a method to release the raw events after decoding if they're no longer needed.models/events_test.go (2)
Line range hint
197-224: LGTM! Consider enhancing test coverage.The block event payload verification is well implemented and aligns with the PR objectives. The test correctly validates:
- Block event payload collection
- Block hash consistency
- Event ordering based on TransactionIndex and EventIndex
Consider adding edge cases to test:
- Block events with duplicate TransactionIndex values
- Events with very large EventIndex values
- Events with gaps in their EventIndex sequence
232-237: LGTM! Consider testing error scenarios.The transaction event payloads verification is well implemented. The test correctly validates:
- Transaction event payloads ordering
- Hash consistency between transactions and their payloads
- Block height consistency across payloads
Consider adding test cases for error scenarios:
- Missing transaction event payloads
- Mismatched block heights between transactions
- Invalid transaction hashes
models/transaction_test.go (2)
134-135: Add test assertions for DirectCall transaction event payload.Similar to the previous test, this test case should validate the new
TransactionEventPayloadreturn value, particularly for DirectCall transactions which might have specific payload characteristics.decTx, _, _, err := decodeTransactionEvent(cdcEv) require.NoError(t, err) + +// Add assertions for the transaction event payload +_, _, payload, _ := decodeTransactionEvent(cdcEv) +require.NotNil(t, payload) +// Add specific assertions for DirectCall transaction payload
182-183: Add test assertions for transaction event payloads in UnmarshalTransaction tests.Both test cases in
Test_UnmarshalTransactionshould validate the newTransactionEventPayloadreturn value to ensure proper handling of event payloads across different transaction types.tx, _, _, err := decodeTransactionEvent(cdcEv) require.NoError(t, err) + +// Add assertions for the transaction event payload +_, _, payload, _ := decodeTransactionEvent(cdcEv) +require.NotNil(t, payload) +// Add specific assertions based on the transaction typeAlso applies to: 236-237
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- models/block.go (2 hunks)
- models/block_test.go (2 hunks)
- models/events.go (3 hunks)
- models/events_test.go (2 hunks)
- models/receipt_test.go (1 hunks)
- models/transaction.go (3 hunks)
- models/transaction_test.go (4 hunks)
🔇 Additional comments (10)
models/block_test.go (1)
153-153: Verify legacy event payload handling.The legacy event decoding also ignores the new payload. Consider adding test cases to verify how legacy events handle the payload - either it should be nil or contain default values to maintain backward compatibility.
models/events.go (3)
39-44: LGTM! Well-structured field additions.The new fields
blockEventPayloadandtxEventPayloadsare properly typed and documented, clearly indicating their cardinality and purpose.
169-173: LGTM! Well-documented accessor methods.The new accessor methods
BlockEventPayloadandTxEventPayloadsare properly documented and follow Go conventions. The comments clearly indicate when nil values might be returned.Also applies to: 181-186
116-133: Verify event payload ordering.The PR objectives emphasize the importance of correct payload ordering for the Block Replayer. While the events are sorted at the beginning of
NewCadenceEvents, let's verify that the order is maintained in the stored payloads.✅ Verification successful
Let me gather more context about the event handling implementation to verify the payload ordering.
Let me try a different approach to examine the event handling implementation.
Let me check the implementation of decodeCadenceEvents and how it maintains the ordering.
Event payload ordering is correctly maintained
The implementation ensures correct ordering through multiple mechanisms:
- Events are explicitly sorted at the beginning of
NewCadenceEventsby both TransactionIndex and EventIndex- The
CadenceEventsstruct maintains ordered slices for transactions and their payloads- Events are processed sequentially in
decodeCadenceEvents, preserving the sorted order when populating the payload slices🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the event payload ordering implementation # Test: Search for sorting logic related to event payloads rg -A 5 "sort.*Events" .Length of output: 422
Script:
#!/bin/bash # Check the full implementation of NewCadenceEvents and related event handling ast-grep --pattern 'func NewCadenceEvents($_) { $$$ }'Length of output: 58
Script:
#!/bin/bash # Search for the NewCadenceEvents function and its context rg -A 20 "func NewCadenceEvents" . # Also check for event payload related structs and types rg "type.*Event" .Length of output: 1875
Script:
#!/bin/bash # Check the decodeCadenceEvents implementation and how events are processed rg -B 2 -A 20 "func decodeCadenceEvents" . # Also check the CadenceEvents struct implementation rg -B 2 -A 10 "type CadenceEvents struct" .Length of output: 2420
models/transaction.go (4)
170-175: LGTM: Function signature change aligns with PR objectives.The addition of
*events.TransactionEventPayloadto the return values appropriately exposes the event payload for the Block Replayer component.
178-182: LGTM: Error handling properly updated.All error paths have been consistently updated to handle the new return value while maintaining proper error wrapping and context.
Also applies to: 198-198, 223-227, 233-237
243-243: LGTM: Return statement properly includes event payload.The successful path correctly returns the decoded event payload, enabling the Block Replayer to access the necessary event data.
Line range hint
170-243: Verify handling of the new return value by consumers.Since this function now returns an additional payload value, we should ensure all callers are updated appropriately.
models/transaction_test.go (2)
Line range hint
4-4: Add test coverage for event payload sorting.Given the PR's objective to ensure proper sorting of event payloads for the Block Replayer, consider adding test cases that specifically validate the sorting functionality.
#!/bin/bash # Search for existing sorting-related code rg -l "sort|order" --type go # Search for Block Replayer related code rg -l "BlockReplayer|Replayer" --type go
88-89: Add test assertions for the new TransactionEventPayload.The test ignores the new return value (
*events.TransactionEventPayload) fromdecodeTransactionEvent. Consider adding assertions to validate the payload's content and structure to ensure proper event payload handling.decTx, _, _, err := decodeTransactionEvent(cdcEv) require.NoError(t, err) + +// Add assertions for the transaction event payload +_, _, payload, _ := decodeTransactionEvent(cdcEv) +require.NotNil(t, payload) +// Add specific assertions based on the TransactionEventPayload structure
| @@ -10,7 +10,7 @@ import ( | |||
| func Test_DecodeReceipts(t *testing.T) { | |||
| cdcEv, rec := createTestEvent(t, evmTxBinary) | |||
|
|
|||
| _, receipt, err := decodeTransactionEvent(cdcEv) | |||
| _, receipt, _, err := decodeTransactionEvent(cdcEv) | |||
There was a problem hiding this comment.
Consider testing the new event payload.
The test ignores the newly added event payload return value, which is a key part of the PR's objective to aggregate EVM event payloads. Consider adding assertions to verify the correctness of this payload.
Here's a suggested improvement:
- _, receipt, _, err := decodeTransactionEvent(cdcEv)
+ _, receipt, payload, err := decodeTransactionEvent(cdcEv)
require.NoError(t, err)
+ // Verify the event payload
+ assert.NotNil(t, payload, "event payload should not be nil")
+ // Add specific assertions for payload fields based on test event dataCommittable suggestion was skipped due to low confidence.
| tx, receipt, txEventPayload, err := decodeTransactionEvent(val) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| e.transactions = append(e.transactions, tx) | ||
| e.txEventPayloads = append(e.txEventPayloads, *txEventPayload) |
There was a problem hiding this comment.
Add nil check for txEventPayload.
Consider adding a nil check before dereferencing txEventPayload to prevent potential panic.
- e.txEventPayloads = append(e.txEventPayloads, *txEventPayload)
+ if txEventPayload != nil {
+ e.txEventPayloads = append(e.txEventPayloads, *txEventPayload)
+ }📝 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.
| tx, receipt, txEventPayload, err := decodeTransactionEvent(val) | |
| if err != nil { | |
| return nil, err | |
| } | |
| e.transactions = append(e.transactions, tx) | |
| e.txEventPayloads = append(e.txEventPayloads, *txEventPayload) | |
| tx, receipt, txEventPayload, err := decodeTransactionEvent(val) | |
| if err != nil { | |
| return nil, err | |
| } | |
| e.transactions = append(e.transactions, tx) | |
| if txEventPayload != nil { | |
| e.txEventPayloads = append(e.txEventPayloads, *txEventPayload) | |
| } |
m-Peter
force-pushed
the
mpeter/aggregate-evm-event-payloads
branch
from
ecdb6ac
to
d3e3847
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
models/transaction.go (1)
Line range hint
170-243: Consider implementing a validation layer for event payload consistency.The current implementation successfully exposes the event payload for the Block Replayer. For future improvements, consider adding a validation layer that ensures consistency between the transaction data and event payload. This could help catch potential issues early in the process.
Some validation points to consider:
- Transaction hash matches between tx and event
- Gas consumption alignment
- Contract address consistency for contract creation transactions
- Transaction type consistency
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- models/block.go (2 hunks)
- models/block_test.go (2 hunks)
- models/events.go (3 hunks)
- models/events_test.go (2 hunks)
- models/receipt_test.go (1 hunks)
- models/transaction.go (3 hunks)
- models/transaction_test.go (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- models/block_test.go
- models/events_test.go
- models/receipt_test.go
- models/transaction_test.go
🔇 Additional comments (8)
models/block.go (2)
Line range hint
74-81: LGTM! Function signature change and error handling look good.The updated signature properly exposes the block event payload while maintaining good error handling practices. The error message is descriptive and includes the event string for debugging purposes.
Line range hint
82-105: Consider adding validation for payload fields.While the block construction and payload handling look correct, consider adding validation for critical payload fields before constructing the block. This would help catch any potential issues early in the decoding process.
Let's check if there are any existing validation patterns in the codebase:
Consider adding validation before block construction:
func decodeBlockEvent(event cadence.Event) (*Block, *events.BlockEventPayload, error) { payload, err := events.DecodeBlockEventPayload(event) if err != nil { return nil, nil, fmt.Errorf( "failed to Cadence-decode EVM block event [%s]: %w", event.String(), err, ) } + + // Validate critical fields + if payload.Height == 0 { + return nil, nil, fmt.Errorf("invalid block height: %d", payload.Height) + } + if payload.Timestamp == 0 { + return nil, nil, fmt.Errorf("invalid block timestamp: %d", payload.Timestamp) + }models/events.go (4)
39-44: LGTM! Well-structured field additions.The new fields
blockEventPayloadandtxEventPayloadsare properly documented and align well with the PR's objective of aggregating EVM event payloads.
127-133: Add nil check for txEventPayload (still applicable).The previous review comment about adding a nil check for
txEventPayloadis still valid. Dereferencing without a nil check could cause a panic.
169-186: LGTM! Well-documented getter methods.The new methods
BlockEventPayloadandTxEventPayloadsare well-documented, follow consistent patterns with other getters, and properly describe nil cases in their documentation.
Line range hint
48-55: Verify event sorting maintains execution order.The existing sorting logic appears to maintain proper execution order by sorting first by TransactionIndex and then by EventIndex. This is crucial for the Block Replayer component.
Let's verify the event ordering implementation:
✅ Verification successful
Let me gather more context about the event handling implementation to ensure there are no other ordering mechanisms that might affect the execution order.
Let me check the event-related test files to verify the sorting behavior and any potential edge cases being tested.
Event sorting implementation maintains correct execution order
The sorting implementation has been thoroughly verified through test cases. Specifically, the test "EVM events are ordered by Flow TransactionIndex & EventIndex" confirms that:
- Events are properly sorted first by TransactionIndex and then by EventIndex
- The ordering is maintained across different transaction types (EVM and Flow)
- The implementation correctly handles multiple events from the same transaction
- The block event ordering is preserved in relation to transaction events
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any other event sorting implementations that might affect order # Test: Search for sorting logic related to events rg -A 5 'sort\.'Length of output: 402
Script:
#!/bin/bash # Search for event-related structs and interfaces to understand the complete event handling flow ast-grep --pattern 'type $_ struct { $$$ Events $$$ $$$ }' # Also check for any event-related methods that might affect ordering rg -l "func.*Event"Length of output: 676
Script:
#!/bin/bash # Check test files for event sorting verification rg -A 10 "Test.*Event.*Sort|test.*event.*order" models/events_test.go # Also check the actual event usage in tests cat models/events_test.goLength of output: 10546
models/transaction.go (2)
170-175: LGTM! Function signature and error handling updates are well-implemented.The addition of
*events.TransactionEventPayloadto the return signature aligns with the PR objectives of exposing EVM event payloads. Error handling has been consistently updated throughout the function to accommodate the new return value.Also applies to: 178-182
243-243: Verify consistency between transaction and event payload data.While exposing the event payload is correct, it's important to ensure that the transaction data matches the corresponding event payload data for consistency.
Let's verify the consistency between transaction and event payload data:
Closes: #627
Description
This is going to be used as the input to the Block Replayer component. The event payloads are sorted in advance, to make sure they will be executed in their correct order.
For contributor use:
masterbranchFiles changedin the Github PR explorerSummary by CodeRabbit
New Features
Bug Fixes
Tests