-
Notifications
You must be signed in to change notification settings - Fork 179
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(torii): only update curor when required #2438
Conversation
commit-id:6ff455c9
WalkthroughOhayo, sensei! The changes in this pull request focus on refactoring the Changes
Possibly related PRs
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
Early access features: enabledWe are currently testing the following features in early access:
Note:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- crates/torii/core/src/engine.rs (4 hunks)
- crates/torii/core/src/sql.rs (1 hunks)
Additional comments not posted (3)
crates/torii/core/src/engine.rs (2)
429-438
: Maintain consistency in awaiting database operationsPlease ensure that you
await
all asynchronous database method calls here as well to prevent any potential issues.
478-481
: Efficiently reset pending transaction pointersGreat work resetting the pending transaction pointers after processing the range data. This keeps the system state clean and prevents stale data from causing issues. Keep it up, sensei!
crates/torii/core/src/sql.rs (1)
113-121
: Ohayo, sensei! Simplifiedset_head
method looks goodThe refactoring of the
set_head
method to update only thehead
field enhances code clarity and adheres to the single-responsibility principle.Please ensure that all calls to
set_head
in the codebase have been updated to match the new signature.Run the following script to find any outdated usage:
Verification successful
Ohayo, sensei! It seems our previous attempt didn't yield the results we were looking for. Let's dig a bit deeper to make sure we're not missing anything. I'll run a few more scripts to gather more information about the
set_head
usage in our codebase.
Ohayo, sensei! I've got some good news for you about our
set_head
method!All
set_head
usages are up-to-date and consistent with the new signatureAfter a thorough investigation of the codebase, I can confirm that all calls to
set_head
are using the new signature with a single argument. Here's a quick summary of what I found:
- The
set_head
method is defined incrates/torii/core/src/sql.rs
with the signaturepub fn set_head(&mut self, head: u64)
.- There are four usages of
set_head
incrates/torii/core/src/engine.rs
, all of which use a single argument.- No outdated usages or comments suggesting recent changes were found.
It looks like our codebase is in perfect harmony with the new
set_head
implementation. Great job on maintaining consistency!Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all function calls to `set_head` with the old signature (more than one argument). # Search for calls to `set_head` with multiple arguments. ast-grep --lang rust --pattern 'set_head($_, $_)'Length of output: 51
Script:
#!/bin/bash # Search for any usage of set_head echo "Searching for set_head usage:" rg "set_head" -n # Look for the definition of set_head echo -e "\nSearching for set_head definition:" ast-grep --lang rust --pattern 'fn set_head($$$)' # Search for comments mentioning set_head echo -e "\nSearching for comments mentioning set_head:" rg "set_head" -n --type-add 'rust:*.rs' --type rust -g '!target' --multiline --context 2Length of output: 3439
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2438 +/- ##
=======================================
Coverage 68.20% 68.21%
=======================================
Files 365 365
Lines 48002 48022 +20
=======================================
+ Hits 32742 32757 +15
- Misses 15260 15265 +5 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
Summary by CodeRabbit
set_head
method to improve clarity and focus on updating the blockchain head.