fix(torii-core): correct parallelized task hash #2860
Conversation
WalkthroughOhayo, sensei! The changes in the Changes
The changes are surgical and precise, focusing on improving the accuracy of event processing by deriving task identifiers from more contextually relevant event data. The method's overall structure remains intact, with only the internal hashing logic being refined. No public API changes or method signature modifications were introduced in this update. 🪧 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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| // model selector | ||
| event.keys[1].hash(&mut hasher); | ||
| // entity id | ||
| event.keys[2].hash(&mut hasher); |
There was a problem hiding this comment.
💡 Codebase verification
Add boundary check for event keys to prevent panic
Ohayo, sensei! After analyzing the codebase, I can confirm that there's no boundary check for the event keys in the hashing logic. While some event processors do validate the length of event keys (like ERC20 and ERC721 processors), the specific code in question that uses event.keys[1] and event.keys[2] for hashing doesn't have such validation.
The suggested fix in the review comment is appropriate:
if event.keys.len() < 3 {
warn!(target: LOG_TARGET, "Event does not have enough keys (>=3) for hashing required for parallelized tasks. Skipping...");
return Ok(());
}Key findings from the codebase:
- ERC20 transfer validates:
event.keys.len() == 3 - ERC721 transfer validates:
event.keys.len() == 5 - Messaging service validates:
event.keys.len() != 4 - But no validation in the hashing logic that uses indices 1 and 2
🔗 Analysis chain
Ensure event keys have sufficient length to avoid out-of-bounds errors.
Ohayo, sensei! The new hashing logic references event.keys[1] and event.keys[2], but there's no check on the number of keys within event.keys. If the event does not contain at least three keys, it will panic.
As a possible fix, you might add a boundary check before hashing:
let mut hasher = DefaultHasher::new();
+ // Ensure we have enough keys for hashing
+ if event.keys.len() < 3 {
+ warn!(target: LOG_TARGET, "Event does not have enough keys (>=3) for hashing required for parallelized tasks. Skipping...");
+ return Ok(());
+ }
// model selector
event.keys[1].hash(&mut hasher);
// entity id
event.keys[2].hash(&mut hasher);
hasher.finish()To verify whether there are any existing checks to ensure a sufficient number of keys for these event types, consider running:
Let me check for any validation of event keys before they are accessed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# ohayo sensei! Let's check if there are boundary checks for events with event.keys indexing
rg -A 5 "event.keys\["
Length of output: 25064
Script:
#!/bin/bash
# Let's check for any validation of event keys length
rg -A 5 "event\.keys\.len"
Length of output: 3706
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2860 +/- ##
==========================================
+ Coverage 55.82% 55.83% +0.01%
==========================================
Files 446 446
Lines 57728 57730 +2
==========================================
+ Hits 32225 32234 +9
+ Misses 25503 25496 -7 ☔ View full report in Codecov by Sentry. |
Summary by CodeRabbit