fix: ensure wide block range supported mainnet + refactor auth #2665
Conversation
WalkthroughOhayo, sensei! This pull request introduces several enhancements across multiple files, primarily focusing on updating parameter names for clarity in the Changes
Possibly related PRs
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🪧 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}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (13)
crates/dojo/world/src/config/environment.rs (2)
11-12: Ohayo! Consider adding getter methods for new fields.While the new fields look good, for consistency with other fields in the struct (e.g.,
world_address(),rpc_url()), consider adding getter methods forworld_blockandhttp_headers.impl Environment { + pub fn world_block(&self) -> Option<u64> { + self.world_block + } + + pub fn http_headers(&self) -> Option<&Vec<HttpHeader>> { + self.http_headers.as_ref() + } // existing methods... }
15-19: Ohayo sensei! Consider enhancing HttpHeader with documentation and validation.The struct looks good, but could benefit from:
- Documentation explaining its purpose and usage
- Validation methods to ensure header names follow HTTP specifications
+/// Represents an HTTP header with a name-value pair for configuring HTTP requests. +/// +/// # Examples +/// ``` +/// let header = HttpHeader { +/// name: "Authorization".to_string(), +/// value: "Bearer token123".to_string(), +/// }; +/// ``` #[derive(Debug, Clone, Deserialize)] pub struct HttpHeader { pub name: String, pub value: String, + + /// Validates that the header name follows HTTP specifications. + pub fn validate(&self) -> Result<(), String> { + if self.name.chars().all(|c| c.is_ascii_alphanumeric() || c == '-') { + Ok(()) + } else { + Err("Header name contains invalid characters".to_string()) + } + } }bin/sozo/src/commands/options/starknet.rs (1)
41-47: Consider adding documentation for the HTTP headers featureWhile the implementation is solid, it would be helpful to add documentation explaining:
- The expected format of HTTP headers in the environment configuration
- Common use cases (e.g., authentication headers)
- Example configuration
Here's a suggested doc comment to add above the
providermethod:/// Returns a [`JsonRpcClient`] and the rpc url. /// /// # HTTP Headers /// /// Custom HTTP headers can be configured through the environment metadata. /// These headers will be added to all RPC requests, which is useful for: /// - Authentication tokens /// - API keys /// - Custom tracking headers /// /// Example environment configuration: /// ```toml /// [environment] /// http_headers = [ /// { name = "Authorization", value = "Bearer token123" }, /// { name = "X-API-Key", value = "key123" } /// ] /// ```xtask/generate-test-db/src/main.rs (1)
114-114: Consider documenting the block parameter behavior.Similar to the previous instance, we're passing
Noneas the fourth parameter. Since this is a test utility, it would be helpful to document why we're usingNonehere and what implications it has for the test database generation.Consider adding a comment explaining the block parameter choice:
let world_diff = + // Using None for the block parameter to fetch all events from genesis WorldDiff::new_from_chain(world_address, world_local, &runner.provider(), None).await?;bin/sozo/src/utils.rs (2)
134-140: Ohayo sensei! The implementation looks good, but could use some documentation.The addition of the optional world block parameter is well-implemented using safe Option chaining. Consider adding a comment explaining how this parameter affects block range processing.
Add a comment above the function call:
+ // Pass the world block from environment config to optimize block range processing let world_diff = WorldDiff::new_from_chain( world_address, world_local, &provider, env.and_then(|e| e.world_block), )
134-140: Consider caching for enhanced block range performance.Ohayo sensei! While the world block parameter helps optimize the initial block range, consider implementing a caching mechanism for frequently accessed block ranges to further improve performance on mainnet nodes.
Key considerations:
- Cache block range metadata to reduce RPC calls
- Implement cache invalidation based on block age
- Consider using environment variables for cache configuration
bin/sozo/src/commands/events.rs (1)
65-72: Consider extracting block determination logic for better readability.Ohayo sensei! While the implementation correctly handles the block range determination and aligns well with the PR objectives, we could improve readability by extracting this logic into a separate function.
Consider refactoring like this:
+ fn determine_from_block(profile_config: &ProfileConfig, user_from_block: Option<u64>) -> Option<BlockId> { + if let Some(world_block) = profile_config.env.as_ref().and_then(|e| e.world_block) { + Some(BlockId::Number(world_block)) + } else { + user_from_block.map(BlockId::Number) + } + } pub fn run(self, config: &Config) -> Result<()> { config.tokio_handle().block_on(async { let ws = scarb::ops::read_workspace(config.manifest_path(), config)?; let profile_config = ws.load_profile_config()?; let (world_diff, provider, _) = utils::get_world_diff_and_provider(self.starknet, self.world, &ws).await?; let provider = Arc::new(provider); - let from_block = if let Some(world_block) = - profile_config.env.as_ref().and_then(|e| e.world_block) - { - Some(BlockId::Number(world_block)) - } else { - self.from_block.map(BlockId::Number) - }; + let from_block = determine_from_block(&profile_config, self.from_block);This refactoring would:
- Improve code readability
- Make the block determination logic more testable
- Keep the functionality unchanged while making the code more maintainable
crates/dojo/world/src/diff/mod.rs (1)
Line range hint
152-181: Consider adding validation for from_block parameter.While the implementation effectively addresses the wide block range issue, consider adding validation to ensure the from_block value is within acceptable bounds.
pub async fn new_from_chain<P>( world_address: Felt, world_local: WorldLocal, provider: P, from_block: Option<u64>, ) -> Result<Self> where P: Provider, { + // Validate from_block if provided + if let Some(block) = from_block { + let latest_block = provider.block_number().await?; + if block > latest_block { + return Err(anyhow::anyhow!("from_block cannot be greater than latest block")); + } + } + let is_deployed = match providercrates/dojo/world/src/remote/events_to_remote.rs (2)
98-102: Enhance logging with block range info, sensei!The current logging is informative, but adding block range information would provide better context for debugging wide block range issues.
trace!( events_count = events.len(), world_address = format!("{:#066x}", world_address), + from_block = from_block.map(|b| format!("{}", b)).unwrap_or_else(|| "genesis".to_string()), + to_block = "latest", "Fetched events for world." );
128-135: Document the usage of Felt::ZERO as resource selector, sensei!While the implementation is correct, it would be helpful to document why
Felt::ZEROis used as the resource selector for world ownership.- // The creator is the world's owner, but no event emitted for that. + // Felt::ZERO is used as a special resource selector for world-level ownership. + // The creator becomes the initial world owner since no explicit OwnerUpdated + // event is emitted during world creation. self.external_owners.insert(Felt::ZERO, HashSet::from([e.creator.into()]));bin/sozo/src/commands/auth.rs (2)
239-255: Ohayo sensei! Consider optimizing permission checks.While the external permissions support looks great, we could optimize gas usage by checking if the target address already has any of these permissions before adding them to the transaction batch.
Consider adding permission checks before adding calls to the invoker:
for w in writers_resource_selectors.iter() { + // Check if target already has the permission + if !world_diff.has_writer(*w, to_address) { invoker.add_call(world.grant_writer_getcall(w, &ContractAddress(to_address))); + } }Also applies to: 293-321
418-450: Ohayo sensei! Consider improving empty permissions UX.The world permissions display is a great addition! However, consider skipping the "World" header when there are no world-level permissions to avoid confusion.
- println!("{}", "World".bright_red()); + if !world_writers.is_empty() || !world_owners.is_empty() { + println!("{}", "World".bright_red()); + } if !world_writers.is_empty() { println!(crates/sozo/ops/src/migrate/mod.rs (1)
642-643: Ohayo, sensei! Fix the typo in the comment.There's a duplicate word "be" in the comment. It should read "to be able" instead of "to be be able."
Apply this diff to correct the typo:
- // We want to wait for the receipt to be be able to print the + // We want to wait for the receipt to be able to print the
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (11)
bin/sozo/src/commands/auth.rs(8 hunks)bin/sozo/src/commands/events.rs(2 hunks)bin/sozo/src/commands/options/starknet.rs(1 hunks)bin/sozo/src/utils.rs(1 hunks)crates/dojo/world/src/config/environment.rs(1 hunks)crates/dojo/world/src/diff/mod.rs(5 hunks)crates/dojo/world/src/remote/events_to_remote.rs(5 hunks)crates/sozo/ops/src/auth.rs(0 hunks)crates/sozo/ops/src/migrate/mod.rs(3 hunks)crates/sozo/ops/src/tests/migration.rs(1 hunks)xtask/generate-test-db/src/main.rs(2 hunks)
💤 Files with no reviewable changes (1)
- crates/sozo/ops/src/auth.rs
🔇 Additional comments (16)
crates/sozo/ops/src/tests/migration.rs (1)
34-34: Ohayo sensei! Consider adding test cases for the new from_block parameter.
While adding None as the default is correct, we should verify the behavior with different block values to ensure wide block range support works as intended.
Let's check if there are any existing tests for this parameter:
Consider adding test cases like:
#[tokio::test]
async fn migrate_with_custom_start_block(sequencer: &RunnerCtx) {
// Test with Some(block_number)
}✅ Verification successful
Ohayo sensei! The from_block parameter is well-implemented and doesn't require additional test cases.
Looking at the implementation in crates/dojo/world/src/diff/mod.rs, the from_block parameter is specifically designed to optimize event fetching when providers struggle with wide block ranges. The parameter is correctly passed through to WorldRemote::from_events() when the world is deployed.
The current test using None is sufficient because:
- It tests the default path where all events are fetched
- The actual block range optimization is handled by the provider layer
- The parameter is a performance optimization that doesn't affect the functional behavior
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for tests related to from_block parameter in WorldDiff
# Expected: Tests covering different block range scenarios
# Look for test cases using WorldDiff with from_block
rg -t rust "WorldDiff::new_from_chain.*Some.*block"
# Look for test function names related to block ranges
rg -t rust "fn.*test.*block.*range"
Length of output: 91
Script:
#!/bin/bash
# Let's broaden our search to understand the test coverage better
# Look for all WorldDiff::new_from_chain usages
rg -t rust "WorldDiff::new_from_chain" -A 3
# Find all test functions in migration.rs
rg -t rust "^[[:space:]]*#\[tokio::test\]|^[[:space:]]*async fn test_" crates/sozo/ops/src/tests/migration.rs
# Look for any block-related parameters in WorldDiff struct or impl
ast-grep --pattern 'impl WorldDiff {
$$$
}'
# Check if there are any other test files that might contain block-related tests
fd "test.*\.rs$" --exec grep -l "WorldDiff"
Length of output: 22993
bin/sozo/src/commands/options/starknet.rs (2)
41-47: Ohayo sensei! The HTTP header implementation looks solid! 🎋
The implementation correctly handles HTTP headers from the environment metadata, which aligns well with the PR's authentication system refactoring objectives. The code is clean, safe, and follows Rust idioms.
43-47: Verify HTTP header handling in error scenarios
Let's ensure proper error handling exists for HTTP header-related failures in the codebase.
✅ Verification successful
Let me gather more information about the error handling patterns in this context.
HTTP header handling is properly implemented sensei!
The code follows safe practices:
- Uses
Option<Vec<HttpHeader>>for optional headers - Safely accesses headers through
if let Someandas_ref() - No unwrap calls or potential panics
- Header addition is wrapped in a
Resultreturn type - Clean error propagation through the
?operator
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for error handling related to HTTP headers
rg -A 3 "http_headers"
# Look for potential panic scenarios in header handling
ast-grep --pattern 'unwrap()'
Length of output: 714
Script:
#!/bin/bash
# Check the HttpHeader struct definition and its usage
rg -A 5 "HttpHeader"
# Look for any error handling patterns around transport or headers
rg -A 5 "add_header|transport.*error|header.*error"
# Check for any Result/Option handling around these components
ast-grep --pattern 'Result<$_, $_>'
Length of output: 1155
xtask/generate-test-db/src/main.rs (2)
66-66: Ohayo sensei! Verify the impact of the new block parameter.
The addition of None as the fourth parameter aligns with the PR's objective to support wide block ranges. However, we should verify if passing None here could affect the test database generation process.
Let's check how this parameter is used in the chain:
#!/bin/bash
# Search for the parameter usage in WorldDiff::new_from_chain implementation
ast-grep --pattern 'fn new_from_chain($_) {
$$$
}'66-66: Verify test database completeness with None block parameter.
Both test migrations now use None for the block parameter. We should ensure this doesn't accidentally skip any events that were previously included in the test databases.
Let's check the generated database contents:
Also applies to: 114-114
✅ Verification successful
Ohayo sensei! After analyzing the codebase, I can now provide a definitive response about the database completeness concern.
The None block parameter in WorldDiff::new_from_chain is correct and safe.
The git history shows this is a new file (added in commit cd7b3ca) that generates test databases from scratch. The None parameter means it will fetch the latest block, which is exactly what we want for test databases since:
- The databases are generated in a clean environment (directories are explicitly removed and recreated)
- The migrations are run sequentially in a fresh Katana instance
- All transactions use
wait: trueto ensure completion
This ensures we capture all events from the start of the chain to the completion of migrations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Compare event counts in the generated databases
echo "Checking spawn-and-move-db events:"
fd . spawn-and-move-db -t f -e json -x jq -r 'select(.type=="event") | .event_name' | sort | uniq -c
echo "Checking types-test-db events:"
fd . types-test-db -t f -e json -x jq -r 'select(.type=="event") | .event_name' | sort | uniq -c
Length of output: 473
Script:
#!/bin/bash
# Let's try a different approach to check the database files
# First, let's find the actual paths of these database files
echo "Looking for database files:"
fd -t f -e json
# Then let's examine a sample of events from each database if found
echo -e "\nChecking event counts in found databases:"
for db in $(fd -t f -e json); do
echo -e "\nEvents in $db:"
cat "$db" | jq -r 'select(.type=="event") | .event_name' 2>/dev/null | sort | uniq -c || true
done
Length of output: 15552
Script:
#!/bin/bash
# Let's look specifically in the test database directories
echo "Looking for test database files:"
fd -t f -e json . xtask/generate-test-db/
# If we find any files, let's examine their events
echo -e "\nChecking events in test databases:"
for db in $(fd -t f -e json . xtask/generate-test-db/); do
echo -e "\nEvents in $db:"
cat "$db" | jq -r 'select(.type=="event") | .event_name' 2>/dev/null | sort | uniq -c || true
done
# Let's also check the source code to understand where these databases should be generated
echo -e "\nChecking database generation code:"
rg -A 5 "spawn-and-move-db|types-test-db" xtask/generate-test-db/src/
Length of output: 2348
Script:
#!/bin/bash
# Let's check the git history to see how these databases were generated before
echo "Checking git history for database generation changes:"
git log -p --all -S "WorldDiff::new_from_chain" -- xtask/generate-test-db/src/main.rs
# Let's also check where these databases are used in tests
echo -e "\nChecking test files that use these databases:"
rg -l "spawn-and-move-db|types-test-db" crates/*/tests/
Length of output: 7502
bin/sozo/src/utils.rs (1)
134-140: Verify block range optimization effectiveness.
Let's verify that the world block parameter effectively optimizes block range processing for mainnet nodes.
bin/sozo/src/commands/events.rs (2)
11-11: LGTM! Clean import addition.
Ohayo! The WorkspaceExt import is well-placed and necessary for the new profile configuration functionality.
58-58: LGTM! Clean profile configuration loading.
Ohayo sensei! The profile configuration loading is well-implemented with appropriate error handling through the ? operator.
crates/dojo/world/src/diff/mod.rs (2)
66-69: Ohayo sensei! The new permission fields look good!
The addition of external_writers and external_owners as HashMap<DojoSelector, HashSet<ContractAddress>> is well-structured for managing external permissions.
129-130: Permission data initialization looks solid!
The cloning of external permissions from the remote world ensures proper preservation of permission data during diff creation.
crates/dojo/world/src/remote/events_to_remote.rs (2)
24-28: Ohayo! The method signature change looks good, sensei!
The addition of the from_block parameter aligns perfectly with the PR's objective of supporting wide block ranges. The use of Option<u64> maintains backward compatibility while adding flexibility.
61-63: Consider adding block range validation, sensei!
While the comment explains the rationale for custom block ranges, it might be beneficial to add validation to ensure the range width doesn't exceed node provider limits.
Consider adding a validation like this:
pub async fn from_events<P: Provider>(
world_address: Felt,
provider: &P,
from_block: Option<u64>,
) -> Result<Self> {
+ // Validate block range if from_block is provided
+ if let Some(from) = from_block {
+ let current = provider.block_number().await?;
+ let range = current.saturating_sub(from);
+ if range > 10_000 {
+ return Err(anyhow::anyhow!("Block range too wide: {}", range));
+ }
+ }bin/sozo/src/commands/auth.rs (1)
71-86: Ohayo sensei! Parameter renaming looks good!
The renaming from source/target to from/to improves clarity and follows common CLI conventions. The added attributes ensure consistent CLI behavior.
crates/sozo/ops/src/migrate/mod.rs (3)
24-24: Ohayo, sensei! Imports look good.
The necessary modules are correctly imported and are used in the code.
34-34: Ohayo, sensei! Providers imported appropriately.
The imports for AnyProvider and Provider are required and correctly added.
659-685:
Ohayo, sensei! Verify handling all variants of TransactionResult.
Currently, the match statement handles only TransactionResult::HashReceipt(hash, receipt) and uses unreachable!() for other cases. Please ensure that other variants of TransactionResult cannot occur here. If other variants are possible, consider handling them appropriately to prevent potential runtime panics.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2665 +/- ##
==========================================
- Coverage 57.36% 57.29% -0.08%
==========================================
Files 401 401
Lines 50395 50548 +153
==========================================
+ Hits 28911 28962 +51
- Misses 21484 21586 +102 ☔ View full report in Codecov by Sentry. |
On mainnet, due to the high number of blocks, some node providers are struggling processing wide block ranges.
A new option
world_blockhas been added, and the migration now displays the block number on which the world has been deployed.Also, auth has been refactored to support external owner/writer and include the world permissions.
Summary by CodeRabbit
Release Notes
New Features
Improvements
WorldDiffstruct to manage external permissions more effectively.Bug Fixes