refactor(sozo): fetch writes from events and calculate diff for that + some refactor
#2203
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2203 +/- ##
==========================================
+ Coverage 70.15% 70.20% +0.05%
==========================================
Files 343 343
Lines 44981 45185 +204
==========================================
+ Hits 31556 31722 +166
- Misses 13425 13463 +38 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Nice work mate and good addition of the selector on the auth part. Some changes have been merged on main related to auth, you may have conflict there.
About the writers, it looks good on the logic, and make the flow easy for the user to simply use migrate apply and everything is computed.
The only downside of the current implementation is that when revoking, only the selectors are displayed. And fetching each one of the selector info to display the name might be worthy for easier debugging / understanding of the operations, wdyt?
lambda-0x
force-pushed
the
refactor-things
branch
from
571aca1 to
25358dc
Compare
lambda-0x
changed the title
metadata.toml to track state during migration failure + some refactorwrites from events and calculate diff for that + some refactor
WalkthroughOhayo, sensei! The recent changes enhance the codebase's performance and clarity by optimizing parameter handling, improving resource management, and refining the migration process. Key modifications include passing parameters by reference for better efficiency, simplifying function signatures, and adding new functionalities such as tracking authorization changes during migrations. Overall, these adjustments contribute to a more robust and efficient system. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MigrationService
participant AuthService
User->>MigrationService: Start Migration
MigrationService->>AuthService: Request Authorization
AuthService->>MigrationService: Return Authorization Result
MigrationService-->>User: Migrate Complete
Assessment against linked issues
Recent review detailsConfiguration used: .coderabbit.yaml Files selected for processing (1)
Additional comments not posted (8)
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 as PR comments)
Additionally, you can add 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, codebase verification and nitpick comments (2)
crates/sozo/ops/src/migration/mod.rs (1)
127-129: Dry run enhancement.The TODO comment suggests that the dry run can also show the diffs for things apart from the world state. This is a good enhancement for future implementation.
crates/sozo/ops/src/migration/migrate.rs (1)
877-878: Ohayo, sensei! Add a comment for future handling of owners.Adding a comment to note that handling of owners might be added in the future will help avoid forgetting this enhancement.
+ // For now we just handle writers, handling of owners might be added in the future
good point, updated it to show more appropriate resource type instead of selector where possible. |
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range, codebase verification and nitpick comments (3)
crates/sozo/ops/src/migration/migrate.rs (3)
Line range hint
78-113:
Ohayo, sensei! Simplify the check formigration_output.full.The function is well-structured, but the check for
migration_output.fullcan be simplified by combining the if-else blocks.if let Some(block_number) = migration_output.world_block_number { ui.print(format!( "\n🎉 Successfully migrated World on block #{} at address {}\n", block_number, bold_message(format!("{:#x}", strategy.world_address)) )); } else { ui.print(format!( "\n🎉 Successfully migrated World at address {}\n", bold_message(format!("{:#x}", strategy.world_address)) )); }
Line range hint
115-204:
Ohayo, sensei! Simplify the match statements for readability.The function is well-structured, but the match statements can be simplified to improve readability.
match &strategy.base { Some(base) => { ui.print_header("# Base Contract"); match base.declare(&migrator, &txn_config).await { Ok(res) => ui.print_sub(format!("Class Hash: {:#x}", res.class_hash)), Err(MigrationError::ClassAlreadyDeclared) => ui.print_sub(format!("Already declared: {:#x}", base.diff.local_class_hash)), Err(MigrationError::ArtifactError(e)) => return Err(handle_artifact_error(&ui, base.artifact_path(), e)), Err(e) => { ui.verbose(format!("{e:?}")); return Err(e.into()); } }; } None => {} }; match &strategy.world { Some(world) => { ui.print_header("# World"); if let Some(_) = world.diff.remote_class_hash { let _deploy_result = upgrade_contract( world, "world", world.diff.original_class_hash, strategy.base.as_ref().unwrap().diff.original_class_hash, &migrator, &ui, &txn_config, ).await.map_err(|e| { ui.verbose(format!("{e:?}")); anyhow!("Failed to upgrade world: {e}") })?; ui.print_sub(format!("Upgraded Contract at address: {:#x}", world.contract_address)); } else { let calldata = vec![strategy.base.as_ref().unwrap().diff.local_class_hash]; let deploy_result = deploy_contract(world, "world", calldata.clone(), &migrator, &ui, &txn_config).await.map_err(|e| { ui.verbose(format!("{e:?}")); anyhow!("Failed to deploy world: {e}") })?; (world_tx_hash, world_block_number) = if let ContractDeploymentOutput::Output(deploy_result) = deploy_result { (Some(deploy_result.transaction_hash), deploy_result.block_number) } else { (None, None) }; ui.print_sub(format!("Contract address: {:#x}", world.contract_address)); } } None => {} };
Line range hint
277-350:
Ohayo, sensei! Improve error handling for IPFS uploads.The function is well-structured, but the error handling for the IPFS uploads can be improved by consolidating error messages.
// world if migration_output.world_tx_hash.is_some() { match dojo_metadata.world.upload().await { Ok(hash) => { let resource = create_resource_metadata(Felt::ZERO, hash.clone())?; ui.print_sub(format!("world: ipfs://{}", hash)); resources.push(resource); } Err(err) => ui.print_sub(format!("Failed to upload World metadata:\n{err}")), } } // models if !migration_output.models.is_empty() { for model_tag in migration_output.models { if let Some(m) = dojo_metadata.resources_artifacts.get(&model_tag) { ipfs.push(upload_on_ipfs_and_create_resource( &ui, compute_selector_from_tag(&model_tag), m.clone(), )); } } } // contracts let migrated_contracts = migration_output.contracts.into_iter().flatten().collect::<Vec<_>>(); if !migrated_contracts.is_empty() { for contract in migrated_contracts { if let Some(m) = dojo_metadata.resources_artifacts.get(&contract.tag) { ipfs.push(upload_on_ipfs_and_create_resource( &ui, naming::compute_selector_from_tag(&contract.tag), m.clone(), )); } } } // upload IPFS resources.extend( future::try_join_all(ipfs) .await .map_err(|_| anyhow!("Unable to upload IPFS artifacts."))?, ); ui.print("> All IPFS artifacts have been successfully uploaded.".to_string());
fix: #2079
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Documentation
Style