Sort Flow EVM events received from Event Streaming API by their TransactionIndex & EventIndex fields in ascending order
#622
Conversation
m-Peter
requested review from
janezpodhostnik,
peterargue,
zhangchiqing and
ramtinms
as code owners
WalkthroughThe changes in this pull request introduce a sorting mechanism for events in the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
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: 0
🧹 Outside diff range and nitpick comments (2)
models/events.go (1)
43-47: LGTM: Sorting mechanism implemented correctly.The sorting of events by their
EventIndexis implemented efficiently usingsort.Slice. This change aligns well with the PR objective and ensures that events are processed in the correct order.Consider adding a brief comment explaining why sorting is necessary:
// Sort events by EventIndex to ensure correct processing order sort.Slice(events.Events, func(i, j int) bool { return events.Events[i].EventIndex < events.Events[j].EventIndex })models/events_test.go (1)
166-176: Clarify variable naming for better readabilityConsider renaming the variable
txIndextoeventIndexto better reflect its role as the reversed event index. This enhances code readability and reduces potential confusion withtxIndexused elsewhere.Apply this diff to implement the change:
for i := 0; i < txCount; i++ { tx, _, txEvent, err := newTransaction(uint64(i), uint16(i)) require.NoError(t, err) // make the Flow events come in reversed order - txIndex := (txCount - 1) - i - txEvent.EventIndex = txIndex + eventIndex := (txCount - 1) - i + txEvent.EventIndex = eventIndex - hashes[txIndex] = tx.Hash() + hashes[eventIndex] = tx.Hash() blockEvents.Events = append(blockEvents.Events, txEvent) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- models/events.go (2 hunks)
- models/events_test.go (6 hunks)
🧰 Additional context used
🔇 Additional comments (6)
models/events.go (2)
5-5: LGTM: Import statement added correctly.The
sortpackage import is appropriately added and aligns with the new sorting functionality introduced in the file.
43-48: Overall changes look good and align with PR objectives.The introduction of the sorting mechanism at the beginning of
NewCadenceEventsensures that events are processed in the correct order, as required by the PR objectives and linked issue #621. This change:
- Maintains the existing structure and logic of the file.
- Improves the reliability of event processing by ensuring correct order.
- Sets a solid foundation for building a local state index on the EVM Gateway.
The implementation is efficient and does not introduce any apparent issues or side effects.
To ensure the changes don't have unintended consequences, please run the following verification script:
✅ Verification successful
Verification successful: No issues detected.
The introduced sorting in
NewCadenceEventsdoes not adversely affect existing functionality or tests. No potential race conditions were identified.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the sorting doesn't break existing functionality # Test: Check if there are any test files that might need updates fd -e go -e test.go # Test: Look for any usage of NewCadenceEvents that might be affected rg -A 5 "NewCadenceEvents" # Test: Check for any potential race conditions introduced by the sorting rg -A 5 "go func|goroutine|chan|select"Length of output: 53345
models/events_test.go (4)
72-75: Function call updated to match new signatureThe call to
newTransactionhas been correctly updated to include the newtxIndexparameter.
134-135: Function call updated to match new signatureThe call to
newTransactionincludes the newtxIndexparameter, aligning with the updated function signature.
220-223: Function call updated to match new signatureThe call to
newTransactionhas been correctly updated to include thetxIndexparameter.
Line range hint
273-301: Function updated to handle transaction indexThe
newTransactionfunction now acceptstxIndexand correctly assigns it to theres.Indexfield, ensuring that transaction indices are accurately represented in the test data.
|
I have a code like this not sure about your use case but maybe also need to order for transactionIndex: sort.Slice(cadenceEvents, func(i, j int) bool {
if cadenceEvents[i].TransactionIndex != cadenceEvents[j].TransactionIndex {
return cadenceEvents[i].TransactionIndex < cadenceEvents[j].TransactionIndex
}
return cadenceEvents[i].EventIndex < cadenceEvents[j].EventIndex
})there were some problematic blocks. |
@bluesign I assume this applies only to Update: I added such a fallback sorting in 8cbfa77, just to be on the safe side. |
|
evm block: https://evm.flowscan.io/block/4358719 |
m-Peter
force-pushed
the
mpeter/sort-flow-events-by-event-index
branch
from
8cbfa77 to
55811d2
Compare
|
m-Peter
changed the title
EventIndex field in ascending orderTransactionIndex & EventIndex fields in ascending order
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
models/events.go (2)
25-25: LGTM: Improved event type checking.The use of exact qualified identifier matching with the new constants in
isBlockExecutedEventandisTransactionExecutedEventfunctions enhances reliability and potentially improves performance.Consider caching the result of
event.EventType.QualifiedIdentifierin a variable before comparison to slightly improve readability:eventTypeID := event.EventType.QualifiedIdentifier return eventTypeID == BlockExecutedQualifiedIdentifier // or TransactionExecutedQualifiedIdentifierAlso applies to: 33-33
47-54: LGTM: Implemented event sorting as per PR objective.The new sorting mechanism ensures that events are processed in the correct order, addressing the main objective of this PR. The implementation correctly sorts first by
TransactionIndexand then byEventIndex, aligning with the suggestion made in the PR comments.Consider extracting the comparison logic into a separate function for improved readability:
sort.Slice(events.Events, func(i, j int) bool { return compareEvents(events.Events[i], events.Events[j]) }) func compareEvents(e1, e2 flow.Event) bool { if e1.TransactionIndex != e2.TransactionIndex { return e1.TransactionIndex < e2.TransactionIndex } return e1.EventIndex < e2.EventIndex }models/events_test.go (3)
157-227: LGTM: Comprehensive test case for event ordering.This new test case effectively verifies the sorting mechanism for EVM events based on both TransactionIndex and EventIndex. It covers various scenarios and includes assertions for both Flow events and EVM transactions/receipts ordering.
Consider adding a comment explaining the significance of the specific TransactionIndex and EventIndex values chosen for each transaction. This would enhance the test's readability and make it easier for other developers to understand the test scenarios.
Line range hint
299-327: LGTM: Updated newTransaction function with txIndex parameter.The
newTransactionfunction has been correctly updated to include thetxIndexparameter, and theIndexfield of thetypes.Resultstruct is now properly set. This change supports the new event sorting mechanism based on transaction indices.Consider adding a comment explaining the purpose of the
txIndexparameter and its role in event sorting. This would improve the function's documentation and make it clearer for other developers.
Line range hint
1-391: Overall assessment: Changes align with PR objectives and improve test coverage.The modifications in this file successfully implement and test the new event sorting mechanism based on TransactionIndex and EventIndex. The changes are consistent throughout the file, and the new test cases provide comprehensive coverage for the updated event processing logic. These updates effectively address the PR's objective of sorting Flow events received from the Event Streaming API.
To further enhance the robustness of the tests, consider adding edge cases such as:
- Events with identical TransactionIndex but different EventIndex values.
- A large number of events to test the sorting algorithm's performance.
- Events with very high TransactionIndex or EventIndex values to ensure proper handling of integer overflow.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- models/events.go (3 hunks)
- models/events_test.go (6 hunks)
🧰 Additional context used
🔇 Additional comments (6)
models/events.go (3)
5-5: LGTM: Import addition for sorting functionality.The addition of the
sortpackage import is appropriate for the new sorting functionality implemented in theNewCadenceEventsfunction.
15-18: LGTM: Addition of typed constants for event identifiers.The introduction of
BlockExecutedQualifiedIdentifierandTransactionExecutedQualifiedIdentifierconstants enhances code clarity and type safety. These constants are well-defined and consistent with theeventspackage.
Line range hint
1-254: Overall implementation looks good, consider adding tests.The changes implemented in this file successfully address the main objectives of the PR:
- Sorting of Flow events by
TransactionIndexandEventIndex.- Improved event type checking using qualified identifiers.
The overall structure of the code and error handling remain robust.
To ensure the new sorting logic is working as expected, please verify that appropriate test cases have been added or updated in the corresponding test file. Run the following command to check for changes in the test file:
If no changes are found in the test file, consider adding test cases to verify the new sorting behavior.
models/events_test.go (3)
72-72: LGTM: Updated function call with transaction index.The
newTransactionfunction call has been correctly updated to include the transaction index as a parameter. This change aligns with the new sorting mechanism based on transaction indices.
Line range hint
134-156: LGTM: Test case updated correctly.The "block with more transaction hashes" test case has been properly updated to use the new
newTransactionfunction signature. The test still effectively verifies the scenario where a block references more transactions than it should.
246-246: LGTM: Updated function call in event decoding test.The
newTransactionfunction call in theTest_EventDecodingfunction has been correctly updated to include the transaction index. This change ensures consistency with the new function signature and supports the updated event processing logic.
| @@ -39,6 +44,15 @@ type CadenceEvents struct { | |||
|
|
|||
| // NewCadenceEvents decodes the events into evm types. | |||
| func NewCadenceEvents(events flow.BlockEvents) (*CadenceEvents, error) { | |||
There was a problem hiding this comment.
Why Event Streaming API returns a different order?
Can we mention what was the default order?
And have we thought about changing the order in the EventStreaming API?
There was a problem hiding this comment.
That's a good question. So the default order seems to be ascending, for the most part. I am not sure if the Event Streaming API does offer such a guarantee, on the received order of the events.
However, regardless of whatever fixes we chose to do on the Event Streaming API, there are blocks that have EVM-related events out-of-order, see: #622 (comment).
And some other cases:
FlowTxIndex: 0, FlowEventIndex: 15
[EVM]: A.e467b9dd11fa00df.EVM.TransactionExecuted(hash: [...], index: 0, ...)
FlowTxIndex: 0, FlowEventIndex: 16
[EVM]: A.e467b9dd11fa00df.EVM.TransactionExecuted(hash: [...], index: 1, ...)
FlowTxIndex: 1, FlowEventIndex: 15
[EVM]: A.e467b9dd11fa00df.EVM.TransactionExecuted(hash: [...], index: 0, ...)This case though seems to be a race, as the index field of EVM.TransactionExecuted is not strictly increasing in the scope of an EVM block.
In order to incorporate the offchain package from flow-go, and replay EVM blocks & transactions to build a local state index, it is a prerequisite that we supply the blocks & transactions in the correct order, otherwise there will be state mismatches.
So the change in this PR, is more of a safety check, to ensure that we can mitigate such cases, no matter how the 3rd party tools, such as Event Streaming API, behave.
… from streaming API response
m-Peter
force-pushed
the
mpeter/sort-flow-events-by-event-index
branch
from
55811d2 to
3e05ef5
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
models/events_test.go (1)
157-227: LGTM: Comprehensive event ordering test with room for enhancementThe test case effectively validates the event ordering requirements by:
- Testing mixed TransactionIndex and EventIndex values
- Verifying both transaction and receipt ordering
- Including block events in the verification
Consider enhancing the test coverage with:
- Edge cases (e.g., maximum uint16 values for indices)
- Additional assertions for event payload consistency
Would you like me to provide examples of additional test cases?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- models/events.go (3 hunks)
- models/events_test.go (6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- models/events.go
🔇 Additional comments (2)
models/events_test.go (2)
Line range hint
299-327: LGTM: Well-structured transaction creation with index trackingThe updated
newTransactionfunction correctly incorporates the transaction index parameter, which is essential for proper event ordering. The implementation maintains good error handling and properly sets the index in the Result structure.
72-72: LGTM: Test cases properly updated for new transaction indexingThe existing test cases have been correctly updated to accommodate the new txIndex parameter while maintaining their original validation purposes.
Also applies to: 246-246
Closes: #621
Description
This is a prerequisite for incorporating the offchain package of flow-go, and build a local state index on EVM Gateway.
For contributor use:
masterbranchFiles changedin the Github PR explorerSummary by CodeRabbit
New Features
Bug Fixes
Tests