feat(sozo): add support for blake2s casm class hashing #3377
Conversation
|
Ohayo, sensei! WalkthroughAdds optional Blake2s CASM class hash support (threaded as a boolean flag), consolidates slot dependencies to a single Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SozoCLI as Sozo CLI
participant Metadata as Scarb/Metadata ext
participant WorldLocal
participant RPC as RPC Provider
User->>SozoCLI: invoke build / commands
SozoCLI->>SozoCLI: parse --use-blake2s-casm-class-hash (or autodetect)
SozoCLI->>Metadata: load_dojo_world_local(use_blake2s_flag)
Metadata->>WorldLocal: WorldLocal::from_directory(..., use_blake2s_flag)
WorldLocal->>WorldLocal: compute casm hash (V2 if use_blake2s else V1)
SozoCLI->>RPC: query class hash at BlockTag::Latest
RPC-->>SozoCLI: class hash / deployment info
SozoCLI->>User: report results
sequenceDiagram
participant OldFlow as Previous
participant NewFlow as Updated
OldFlow->>OldFlow: imports slot_session / slot_core
NewFlow->>NewFlow: imports unified slot crate
OldFlow->>RPC: queries at PreConfirmed
NewFlow->>RPC: queries at Latest
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas needing extra attention:
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bin/sozo/src/commands/options/transaction.rs (1)
193-215: Finality-status test doesn’t assert the positive casesThe
matches!(...)calls intest_parse_finality_statuscompute a boolean that’s immediately discarded, so only the"INVALID"case is actually asserted. This weakens coverage and can let regressions slip through.Consider asserting the matches explicitly, e.g.:
#[test] fn test_parse_finality_status() -> Result<()> { - matches!( - parse_finality_status(Some("PRE_CONFIRMED".to_string())), - Ok(TransactionFinalityStatus::PreConfirmed) - ); + assert!(matches!( + parse_finality_status(Some("PRE_CONFIRMED".to_string())), + Ok(TransactionFinalityStatus::PreConfirmed) + )); - matches!( - parse_finality_status(Some("ACCEPTED_ON_L2".to_string())), - Ok(TransactionFinalityStatus::AcceptedOnL2) - ); + assert!(matches!( + parse_finality_status(Some("ACCEPTED_ON_L2".to_string())), + Ok(TransactionFinalityStatus::AcceptedOnL2) + )); - matches!( - parse_finality_status(Some("ACCEPTED_ON_L1".to_string())), - Ok(TransactionFinalityStatus::AcceptedOnL1) - ); + assert!(matches!( + parse_finality_status(Some("ACCEPTED_ON_L1".to_string())), + Ok(TransactionFinalityStatus::AcceptedOnL1) + )); - matches!(parse_finality_status(None), Ok(TransactionFinalityStatus::AcceptedOnL2)); + assert!(matches!( + parse_finality_status(None), + Ok(TransactionFinalityStatus::AcceptedOnL2) + ));
🧹 Nitpick comments (7)
bin/sozo/src/commands/options/starknet.rs (1)
22-28: Newuse_blake2s_casm_class_hashflag is wired cleanlyOhayo sensei, the new boolean flag is correctly exposed via clap (global, long name, default
false), and the help text clearly explains when it should be used. As long as the downstream world-loading code actually performs the advertised auto-detection from the RPC URL, this addition looks solid.As an optional follow-up, consider adding a small clap-based test to assert the flag’s default and that
--use-blake2s-casm-class-hashon the CLI flips it as expected.Please ensure the code that consumes this flag (e.g., world loading/build paths) implements the described RPC-URL-based auto-detection so the CLI help remains accurate.
crates/dojo/macros/Cargo.toml (1)
32-33: Macros crate now tracksstarknetgit branch for blake2s supportOhayo sensei, pointing
starknet-cryptoandstarknetat thefeat/types-rs-100-blake2sbranch keeps this proc-macro crate in sync with the workspace’s StarkNet stack and the new blake2s functionality.For long-term reproducibility, consider pinning these to a specific
revonce the branch stabilizes and you’re ready to cut a release.Please verify that this branch is the one you want to track for all consumers of
dojo_cairo_macros, especially if they’re built outside this workspace.Cargo.toml (4)
56-57: Cainome crates moved totypes-rs-100-blake2sbranchOhayo sensei, updating
cainomeandcainome-cairo-serdeto thetypes-rs-100-blake2sbranch matches the rest of the stack’s move to the new StarkNet/types ecosystem and should ensure ABI/gen code stays compatible.Because this tracks a moving branch, consider pinning to a specific
revbefore shipping a release build to keep dependency resolution reproducible across time.Please confirm that this branch of
cainomeis aligned with thestarknet-rsandstarknet-types-coreversions you’re using elsewhere in the workspace.
85-85: Relaxedanyhowdependency to the 1.x lineOhayo sensei, changing
anyhowto"1"removes the strict pin to a specific patch while staying on the same major version, which is usually fine and reduces churn when patch versions update.If fully reproducible builds are critical, you might still prefer an exact version pin here, but it’s not strictly necessary.
Please confirm that this relaxation doesn’t conflict with any consumers that require a more specific
anyhowversion (e.g., via other crates’ MSRV policies).
141-141: Simplifiedserde_jsonto the 1.x line witharbitrary_precisionOhayo sensei, moving to
serde_json"1"while enablingarbitrary_precisionis a reasonable choice for handling large numeric types often seen in StarkNet contexts. This keeps things flexible while retaining the needed feature.Again, if you need strict reproducibility, you might prefer pinning to an exact patch version later.
Please confirm there are no consumers depending on a specific
serde_jsonpatch version (for example, due to known bugs/fixes in earlier releases).
147-147: Adopting pre-releasestarknet_api = 0.16.0-rc.1Ohayo sensei, pulling in
starknet_api0.16.0-rc.1is consistent with enabling blake2s CASM class hashing, but being an RC it may still change in backward-incompatible ways.This seems fine for now, especially if you’re actively tracking that ecosystem, but once a stable 0.16.x is released, you may want to move off the RC to reduce churn.
Please verify that all crates using
starknet_apiare updated to its 0.16 API surface and that there aren’t any lingering calls relying on older versions’ types or hashing behavior.crates/dojo/world/src/local/artifact_to_local.rs (1)
701-708: Test updated correctly, but consider additional coverage, sensei.The test correctly passes the new parameter. However, it only tests the
falsecase (legacy hashing). Consider adding a test case withuse_blake2s_casm_class_hash = trueto verify the blake2s code path works as expected.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.lockcrates/dojo/macros/Cargo.lockis excluded by!**/*.lockspawn-and-move-db.tar.gzis excluded by!**/*.gz
📒 Files selected for processing (30)
Cargo.toml(5 hunks)bin/sozo/Cargo.toml(2 hunks)bin/sozo/src/commands/build.rs(1 hunks)bin/sozo/src/commands/events.rs(1 hunks)bin/sozo/src/commands/options/account/controller.rs(4 hunks)bin/sozo/src/commands/options/account/mod.rs(2 hunks)bin/sozo/src/commands/options/account/type.rs(2 hunks)bin/sozo/src/commands/options/starknet.rs(1 hunks)bin/sozo/src/commands/options/transaction.rs(1 hunks)bin/sozo/src/utils.rs(2 hunks)crates/dojo/bindgen/src/lib.rs(1 hunks)crates/dojo/macros/Cargo.toml(1 hunks)crates/dojo/utils/src/tx/declarer.rs(1 hunks)crates/dojo/utils/src/tx/deployer.rs(1 hunks)crates/dojo/world/Cargo.toml(1 hunks)crates/dojo/world/src/diff/mod.rs(1 hunks)crates/dojo/world/src/local/artifact_to_local.rs(12 hunks)crates/dojo/world/src/local/mod.rs(2 hunks)crates/dojo/world/src/local/resource.rs(2 hunks)crates/dojo/world/src/remote/events_to_remote.rs(1 hunks)crates/sozo/mcp/src/resources/mod.rs(5 hunks)crates/sozo/ops/src/migrate/mod.rs(2 hunks)crates/sozo/ops/src/model.rs(2 hunks)crates/sozo/ops/src/tests/migration.rs(1 hunks)crates/sozo/scarb_metadata_ext/src/metadata.rs(2 hunks)examples/simple/dojo_sepolia.toml(1 hunks)examples/simple/manifest_mainnet.json(4 hunks)examples/simple/manifest_sepolia.json(4 hunks)examples/simple/src/lib.cairo(2 hunks)xtask/generate-test-db/src/main.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2024-11-07T13:57:57.616Z
Learnt from: glihm
Repo: dojoengine/dojo PR: 2650
File: bin/sozo/src/commands/migrate.rs:64-65
Timestamp: 2024-11-07T13:57:57.616Z
Learning: In the codebase, the use of `into()` and `try_into()` for converting `TransactionOptions` is intentional. Conversions that may fail use `try_into()`, while those that cannot fail use `into()`. This approach is appropriate and should be preserved.
Examples:
- `try_into()` is used in `bin/sozo/src/commands/migrate.rs` and `bin/sozo/src/commands/execute.rs` where conversions can fail.
- `into()` is used in `bin/sozo/src/commands/register.rs` and `bin/sozo/src/commands/auth.rs` where conversions are infallible.
Applied to files:
bin/sozo/src/commands/options/transaction.rs
📚 Learning: 2024-11-05T04:30:12.852Z
Learnt from: glihm
Repo: dojoengine/dojo PR: 2633
File: crates/dojo/world/src/contracts/abigen/world.rs:3809-0
Timestamp: 2024-11-05T04:30:12.852Z
Learning: Code in `crates/dojo/world/src/contracts/abigen/` is auto-generated and should be excluded from code reviews.
Applied to files:
crates/dojo/world/src/remote/events_to_remote.rscrates/dojo/world/src/local/artifact_to_local.rs
📚 Learning: 2024-11-05T04:29:12.288Z
Learnt from: glihm
Repo: dojoengine/dojo PR: 2633
File: crates/dojo/core/src/world/storage.cairo:484-0
Timestamp: 2024-11-05T04:29:12.288Z
Learning: In the Cairo codebase for the Dojo project, within `crates/dojo/core/src/world/storage.cairo`, length checks between `entity_ids` and `values` are not required in test API functions like `write_values_from_ids_test`.
Applied to files:
examples/simple/src/lib.cairo
🧬 Code graph analysis (9)
crates/dojo/world/src/diff/mod.rs (1)
crates/dojo/utils/src/tx/deployer.rs (1)
is_deployed(108-123)
crates/sozo/scarb_metadata_ext/src/metadata.rs (1)
crates/dojo/world/src/local/artifact_to_local.rs (1)
from_directory(40-471)
crates/sozo/mcp/src/resources/mod.rs (1)
crates/sozo/mcp/src/resources/parser.rs (3)
parse_contract_uri(55-57)parse_model_uri(61-63)parse_event_uri(67-69)
crates/sozo/ops/src/migrate/mod.rs (1)
bin/sozo/src/commands/options/account/provider.rs (1)
block_number(268-273)
crates/dojo/world/src/remote/events_to_remote.rs (4)
bin/sozo/src/commands/options/starknet.rs (1)
provider(40-58)crates/dojo/world/src/contracts/abigen/model.rs (2)
provider(27-29)provider(56-58)crates/dojo/world/src/contracts/abigen/world.rs (2)
provider(27-29)provider(56-58)crates/dojo/world/src/config/environment.rs (1)
world_address(26-28)
bin/sozo/src/commands/options/account/controller.rs (1)
bin/sozo/src/commands/options/account/mod.rs (1)
account(70-95)
crates/dojo/world/src/local/artifact_to_local.rs (2)
crates/sozo/ui/src/lib.rs (1)
trace(173-175)crates/dojo/world/src/local/resource.rs (2)
abi(176-188)class_hash(161-173)
bin/sozo/src/commands/build.rs (2)
crates/dojo/world/src/local/mod.rs (1)
default(51-78)crates/dojo/world/src/local/artifact_to_local.rs (1)
from_directory(40-471)
crates/dojo/bindgen/src/lib.rs (1)
crates/dojo/world/src/local/artifact_to_local.rs (1)
from_directory(40-471)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: fmt
🔇 Additional comments (38)
examples/simple/dojo_sepolia.toml (1)
8-8: Ohayo sensei! Clarify the intent of this configuration comment.This change adds a commented alternative RPC URL from Lava, but it's unclear how it relates to the PR objectives around blake2s CASM class hashing support and dependency updates. Is this an intentional documentation addition, or should it be scoped to a separate PR focused on configuration examples?
examples/simple/manifest_sepolia.json (1)
3-4: Ohayo sensei! Verify updated hash and address values reflect Blake2s changes.The manifest has been updated with new class hashes and contract addresses. These changes are expected with the Blake2s CASM class hash migration, but I want to confirm:
- Are these values auto-generated from the compilation/deployment process?
- Have all dependent references to these hashes and addresses been updated elsewhere in the codebase?
- Should there be a note in the documentation explaining when/how to regenerate these manifests?
If this is an auto-generated artifact, consider documenting the regeneration process to help maintainers and contributors understand when these files need to be updated.
Also applies to: 36-37, 54-55, 64-65
examples/simple/manifest_mainnet.json (1)
3-4: Ohayo sensei! Manifest hashes updated for blake2s support ✨The world and contract entries have been updated with new class_hash and address values, aligning with the PR's blake2s CASM class hashing implementation. All hex values follow the correct format with
0xprefix.Quick observation: The contracts at lines 36-37 and 64-65 both reference the same class_hash (
0x6640063225e26aa901371598204cc4895922fd30083f334aa4283fb659d4053). If intentional, this makes sense; just wanted to flag it for your awareness.Please verify that these hash values have been correctly computed using blake2s and match the compiled CASM output. You can cross-reference against the actual contract compilation results to ensure consistency across the ecosystem.
Also applies to: 36-37, 54-55, 64-65
examples/simple/src/lib.cairo (2)
124-139: Literal change inmv.v—just confirm dependent expectations, senseiohayo sensei, the change to
mv.v = 15;looks fine given this is an example/value‑mutation flow. Just double‑check any tests or fixtures that might still assert on the previous value so they stay in sync.
144-154: Updatedsystem_freereturn constant—ensure callers/tests match, senseiReturning
Some(48)instead of the previous literal is harmless by itself, but please confirm any example code, documentation, or tests that rely on this value have been updated accordingly so nothing breaks unexpectedly.bin/sozo/src/commands/events.rs (1)
178-182: Ohayo sensei! I found a critical inconsistency that needs addressing.The change to
BlockTag::Latestinevents.rsis inconsistent with the rest of the codebase. Other similar commands in the same module still useBlockTag::PreConfirmed:
bin/sozo/src/commands/model.rslines 168, 185, 203 (Layout, Schema, Get commands) — all still usePreConfirmedbin/sozo/src/commands/call.rsline 113 — still usesPreConfirmedThis is particularly concerning because
events.rscallsmodel_getto fetch model data (line 305), which should behave consistently with themodel.rsGet command. The mismatch means:
- Event processing now queries the Latest block state
- While direct model queries still query PreConfirmed state
- This creates potential data divergence
Either update model.rs, call.rs, and other commands to Latest for consistency across the codebase, or revert events.rs to PreConfirmed to maintain consistency. The git history search returned no results explaining the reasoning behind this isolated change.
bin/sozo/src/commands/options/transaction.rs (1)
68-71: Align finality-status help text with actual enum valuesOhayo sensei, updating the help text to use
PRE_CONFIRMED(instead of a hyphenated form) now matchesparse_finality_statusand the tests, reducing CLI confusion. No further changes needed here.crates/sozo/ops/src/model.rs (1)
19-21: UseLatestblock context for model hash/address lookupsOhayo sensei, switching these helpers to
BlockTag::Latestmakes them consistently query the freshest world state, which matches the broader PR changes (e.g., diff and deploy checks usingLatest). For read-only lookups like class hash and contract address, this behavior is reasonable and I don’t see issues here.Please double-check that your target networks all support
BlockTag::Latestwith the expected semantics and that you’re comfortable with the reduced confirmation level compared to a more finalized tag.Also applies to: 37-39
crates/dojo/utils/src/tx/deployer.rs (1)
112-122:is_deployednow checks deployment atLatestOhayo sensei, updating
get_class_hash_atto useBlockId::Tag(BlockTag::Latest)makes the “already deployed” check reflect the current head of the chain, which matches the rest of the PR’s move away fromPreConfirmed. The control flow and logging remain sound.Please verify on your target RPCs that
Latestis supported and that this confirmation level matches your intended safety guarantees for skipping deployments.crates/sozo/ops/src/tests/migration.rs (1)
32-35: Explicitly disabling blake2s in Katana tests clarifies intentOhayo sensei, threading
use_blake2s_casm_class_hash = falseintoload_dojo_world_localkeeps this test in sync with the new API and documents that Katana currently doesn’t use the blake2s class hash. This makes future flips totruean intentional change rather than an implicit default. Looks good.Cargo.toml (4)
83-83:katana-runnernow follows the same types-rs-100-blake2s branchOhayo sensei, pointing
katana-runnerat thetypes-rs-100-blake2sbranch keeps your local node/testing story in sync with the updated StarkNet types and blake2s behavior, which is important for end-to-end tests.Just be aware you’re again following a branch rather than a fixed revision; you may want to freeze it to a
revonce this flow stabilizes.Please verify your current CI matrix (and developers’ toolchains) are all compatible with this branch of
katana.
94-94: Exactly pinningcairo-lang-starknet-classesto 2.13.1Ohayo sensei, using
=2.13.1forcairo-lang-starknet-classeslocks this crate to a known-good version, which is sensible given how sensitive StarkNet class representations are to compiler changes.This looks appropriate as long as all your Cairo-related tooling is aligned on 2.13.1.
Please ensure your compiler/toolchain setup (e.g., Scarb/Cairo compiler) is also targeting 2.13.1 to avoid mismatches.
171-171: Workspace-levelslotnow unified on the types-rs-100-blake2s branchOhayo sensei, introducing the workspace
slotdependency from thetypes-rs-100-blake2sbranch aligns with the changes inbin/sozo/Cargo.tomland consolidates all slot usage to a single crate and branch, which should reduce version skew.Just as with the other git-based deps, consider pinning to a
revonce you’re satisfied with the integration.Please confirm that all crates previously using
slot-core/slot-sessionnow depend only on this unifiedslotcrate.
173-176: StarkNet stack updated to git branch withstarknet-types-core1.0.0Ohayo sensei, pointing
starknetandstarknet-cryptoat thefeat/types-rs-100-blake2sbranch and addingstarknet-types-core = 1.0.0(witharbitrary+hashfeatures) is exactly in line with the PR’s goal of supporting blake2s CASM class hashing and the newer types stack.This is a significant but coherent dependency shift; the main concerns are branch tracking and ensuring all crates (including those outside this workspace) that depend on these types are updated accordingly.
Please:
- Run
cargo check/ your full test suite across the workspace to validate API compatibility with this new StarkNet stack.- Confirm that any downstream consumers expecting the old hash behavior are updated or gated appropriately.
bin/sozo/Cargo.toml (1)
9-9: Unifiedslotdependency migration verifiedOhayo sensei! The verification confirms there are no remaining references to
slot_coreorslot_sessionthroughout the codebase. The consolidation to the single optionalslotdependency with thecontrollerfeature properly wired to["dep:slot"]is clean and complete.crates/dojo/world/src/local/resource.rs (1)
1-1: LGTM! Type refactor looks correct.Ohayo sensei! The migration from
CompiledClasstoCasmContractClasslooks clean and consistent with the blake2s support changes.Also applies to: 29-29
bin/sozo/src/commands/build.rs (1)
132-142: LGTM! Forward-looking default is reasonable.Ohayo sensei! Setting
use_blake2s_casm_class_hash = trueas the default for build stats makes sense given the comment about 0.14.1 becoming standard. The rationale is clear and the flag is properly propagated toWorldLocal::from_directory.crates/dojo/bindgen/src/lib.rs (1)
150-158: LGTM! Conservative default with clear TODO.Ohayo sensei! Setting
use_blake2s_casm_class_hash = falsefor bindgen is appropriate since class hashes aren't currently used here. The TODO comment properly flags future work needed.bin/sozo/src/commands/options/account/mod.rs (1)
11-11: LGTM! Import path consolidation.Ohayo sensei! The import path change from
slot_session::account_sdktoslot::account_sdklooks like a clean dependency consolidation.crates/dojo/world/src/remote/events_to_remote.rs (1)
49-49: LGTM! Consistent BlockTag usage.Ohayo sensei! The change to
BlockTag::Latestwhen checking if the world contract exists is consistent with the broader PR changes and makes sense for verifying deployed state.bin/sozo/src/utils.rs (2)
35-35: LGTM! RPC version upgraded to stable.Ohayo sensei! Bumping
RPC_SPEC_VERSIONfrom"0.9.0-rc.2"to"0.9.0"properly reflects the stable release.
99-99: LGTM! Flag propagation consistent.Ohayo sensei! The addition of
starknet.use_blake2s_casm_class_hashparameter toload_dojo_world_localis consistent with the broader pattern of threading this flag through the world loading paths.crates/dojo/world/src/local/mod.rs (1)
13-37: Ohayo! LGTM - Clean type refactor for Blake2s support.The migration from
CompiledClasstoCasmContractClassis clean and aligns perfectly with the PR's Blake2s hashing objective. The import fromcairo_lang_starknet_classes(v2.13.1) and the type update on thecasm_classfield are consistent. Tests remain compatible since they useNonefor the optional field.bin/sozo/src/commands/options/account/controller.rs (2)
5-9: Ohayo sensei! Module path consolidation looks solid.The migration from
slot_sessionto the unifiedslotcrate namespace is consistent across all imports. Nice cleanup!
41-86: Credential and session path updates are consistent.All references updated from
slot_core::credentials→slot::credentialandslot_session::session→slot::session. The changes maintain existing functionality while aligning with the unified slot crate structure.xtask/generate-test-db/src/main.rs (1)
37-39: Ohayo sensei! Clear parameter addition with good documentation.The
use_blake2s_casm_class_hash = falsedefault is well-documented with the comment explaining Katana's behavior. The parameter is cleanly integrated into the world loading flow.bin/sozo/src/commands/options/account/type.rs (2)
5-5: Ohayo! Import path updated consistently.The
CartridgeJsonRpcProviderimport path change fromslot_sessiontoslotaligns with the crate consolidation.
31-31: Error type path updated to match slot consolidation.The
SignErrortype path is now correctly referencingslot::account_sdk::signers::SignErrorinstead of the oldslot_sessionpath.crates/sozo/scarb_metadata_ext/src/metadata.rs (1)
168-174: Clean parameter propagation to WorldLocal.The implementation correctly forwards the
use_blake2s_casm_class_hashparameter through toWorldLocal::from_directory. Logic is straightforward.crates/sozo/mcp/src/resources/mod.rs (2)
45-68: Ohayo! Parameter propagation with clear TODOs.The
use_blake2s_casm_class_hashparameter is cleanly added toload_world_localand forwarded appropriately. Good work on the function signature update.
113-116: Default wiring with appropriate TODO markers.The local variable initialization to
falsewith TODO comment noting future MCP exposure is a pragmatic approach. The same pattern is consistently applied across contract, model, and event ABI handlers (lines 127-128, 141-142).crates/sozo/ops/src/migrate/mod.rs (2)
822-827: Ohayo sensei! Block number fallback is pragmatic but warrants caution.The search confirms your concern has merit: the
block_numberfield is actively used in indexing contexts acrossExternalContractRemoteand stored resources, and block_number comparisons are used to detect sync status between local and remote state.The fallback to
0is already established pattern in the codebase—ReceiptBlock::PreConfirmed { block_number: 0 }is used elsewhere for pre-confirmed transactions—so consistency is maintained. However, using genesis block0as a fallback risks indexers re-processing events from the start instead of the actual deployment block, silently skipping indexed records.The current implementation is acceptable as a temporary workaround (as the comment states), but consider adding a logging marker or handling pre-confirmed deployments explicitly in the indexer to prevent silent misalignment.
931-935: Ohayo sensei! I've traced the complete flow and found that the review comment mischaracterizes the control flow.Review comment is incorrect — control flow properly distinguishes errors from already-deployed state.
The code at lines 931–935 is not changing error handling to silent skip. The
.await?on line 927 ensures actual errors are propagated. TheNonefromdeploy_via_udc_getcallrepresents an intentional design choice: the contract is already deployed and needs no re-deployment. The caller at line 553 explicitly checksif deploy_call.is_some()and only processes the call when present, properly handling theNonecase. This is a correct pattern whereNonehas semantic meaning (already deployed) rather than masking deployment failures.Likely an incorrect or invalid review comment.
crates/dojo/world/src/diff/mod.rs (1)
182-199: Ohayo sensei! I need to gather some context about the Starknet BlockTag semantics to verify whether this change is correct and safe.Ohayo sensei! The BlockTag::Latest change is correct—no issues found.
After verification:
The current Starknet Rust library's BlockTag enum only supports
LatestandPending, and Starknet v0.14.0 renamed transaction statuses fromPENDINGtoPRE_CONFIRMED. The code usingBlockTag::PreConfirmedin the codebase appears to be legacy or generated code—the production API no longer exposes this tag.For
get_class_hash_at, thelatestblock tag returns the most recent block in the blockchain and current state. UsingLatestfor world deployment detection is semantically correct—it verifies the contract exists in the finalized state, preventing false positives from ephemeral pre-confirmed transactions. The concern about missing recently deployed worlds is moot; deployment verification should use the finalized state, not provisional pre-confirmations.The migration from
PreConfirmedtoLatestis a correctness improvement aligning with current Starknet RPC semantics.crates/dojo/world/src/local/artifact_to_local.rs (4)
34-34: Ohayo! Type change looks good, sensei.The field type update from
CompiledClasstoCasmContractClassaligns with the PR objectives to use the implementation fromstarknet_apicrate.
103-108: LGTM, sensei!The deserialization update to
CasmContractClassis consistent with the type changes throughout the file.
137-141: Nice debugging improvements, sensei!The enhanced trace logging with formatted class hashes will be helpful for troubleshooting the blake2s implementation.
Also applies to: 155-157, 191-194, 236-238, 266-268, 297-299
475-492: Implementation is correct and requires no changes, sensei.After thorough verification, the hash version mapping at line 487 is sound. The implementation correctly maps
use_blake2s_class_hash=truetoHashVersion::V2andfalsetoHashVersion::V1. This pattern is consistently applied throughout the codebase (24+ occurrences), and the parameter name directly indicates the intent. The TODO comment at line 114 questioning "the casm must also use blake2s?" is already answered by the implementation—the casm class hash function receives and properly handles theuse_blake2s_class_hashparameter.
| // Currently, we have no way to know the network chain-id from here. | ||
| // We try to read the rpc_url of the config to infer the chain-id. | ||
| let autodetect_blake2s_class_hash = profile_config | ||
| .env | ||
| .as_ref() | ||
| .map(|env| { | ||
| env.rpc_url.as_ref().unwrap().contains("sepolia") | ||
| || env.rpc_url.as_ref().unwrap().contains("testnet") | ||
| }) | ||
| .unwrap_or(false); | ||
|
|
||
| let use_blake2s_class_hash = use_blake2s_casm_class_hash || autodetect_blake2s_class_hash; | ||
|
|
||
| trace!(use_blake2s_class_hash, "Using blake2s class hash for local world."); |
There was a problem hiding this comment.
Potential panic in autodetection logic, sensei!
Lines 59-60 perform .unwrap() on env.rpc_url.as_ref() which will panic if rpc_url is None. The outer map only ensures env is Some, not that rpc_url is Some.
Apply this diff to handle the case safely:
let autodetect_blake2s_class_hash = profile_config
.env
.as_ref()
- .map(|env| {
- env.rpc_url.as_ref().unwrap().contains("sepolia")
- || env.rpc_url.as_ref().unwrap().contains("testnet")
- })
+ .and_then(|env| {
+ env.rpc_url.as_ref().map(|url| {
+ url.contains("sepolia") || url.contains("testnet")
+ })
+ })
.unwrap_or(false);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Currently, we have no way to know the network chain-id from here. | |
| // We try to read the rpc_url of the config to infer the chain-id. | |
| let autodetect_blake2s_class_hash = profile_config | |
| .env | |
| .as_ref() | |
| .map(|env| { | |
| env.rpc_url.as_ref().unwrap().contains("sepolia") | |
| || env.rpc_url.as_ref().unwrap().contains("testnet") | |
| }) | |
| .unwrap_or(false); | |
| let use_blake2s_class_hash = use_blake2s_casm_class_hash || autodetect_blake2s_class_hash; | |
| trace!(use_blake2s_class_hash, "Using blake2s class hash for local world."); | |
| // Currently, we have no way to know the network chain-id from here. | |
| // We try to read the rpc_url of the config to infer the chain-id. | |
| let autodetect_blake2s_class_hash = profile_config | |
| .env | |
| .as_ref() | |
| .and_then(|env| { | |
| env.rpc_url.as_ref().map(|url| { | |
| url.contains("sepolia") || url.contains("testnet") | |
| }) | |
| }) | |
| .unwrap_or(false); | |
| let use_blake2s_class_hash = use_blake2s_casm_class_hash || autodetect_blake2s_class_hash; | |
| trace!(use_blake2s_class_hash, "Using blake2s class hash for local world."); |
🤖 Prompt for AI Agents
In crates/dojo/world/src/local/artifact_to_local.rs around lines 53 to 66, the
autodetection logic calls unwrap() on env.rpc_url.as_ref() which can panic when
rpc_url is None; change the closure to safely handle an absent rpc_url (e.g.,
use env.rpc_url.as_deref().map(|u| u.contains("sepolia") ||
u.contains("testnet")).unwrap_or(false) or an if let / match to check Some(url)
before calling contains) so autodetect_blake2s_class_hash never unwraps a None,
then keep use_blake2s_class_hash = use_blake2s_casm_class_hash ||
autodetect_blake2s_class_hash and retain the trace call.
| // TODO: the casm must also use blake2s? | ||
| let casm_class_hash = | ||
| casm_class_hash_from_sierra_file(&path, use_blake2s_class_hash)?; |
There was a problem hiding this comment.
🧩 Analysis chain
TODO needs clarification, sensei.
The TODO comment questions whether CASM should use blake2s hashing. This is a critical implementation detail that affects hash compatibility across the system.
Do you want me to help verify the correct hashing approach by searching for documentation on Starknet's CASM class hash requirements for Sepolia/testnet networks?
🌐 Web query:
Does Starknet Sepolia and testnet require blake2s hashing for CASM class hashes?
💡 Result:
Short answer: Yes. Starknet moved CASM (compiled_class_hash) to a BLAKE2s-based hash (HashVersion V2); Declare/compiled_class_hash on testnet (Sepolia) expects the BLAKE2s computation and will reject non‑BLAKE hashes (migration of existing classes is gradual). (community.starknet.io)
Citations:
Ohayo, sensei!
Remove the outdated TODO comment—the code is already correct.
The web search confirms that Starknet moved CASM to BLAKE2s-based hash (HashVersion V2) and testnet (Sepolia) expects the BLAKE2s computation. Your code already implements this correctly by passing use_blake2s_class_hash to the function. The TODO comment asking "must also use blake2s?" can be safely removed since the question has been definitively answered and the implementation is aligned with Starknet requirements.
🤖 Prompt for AI Agents
In crates/dojo/world/src/local/artifact_to_local.rs around lines 114 to 116,
remove the outdated TODO comment "// TODO: the casm must also use blake2s?"
because the code already passes use_blake2s_class_hash into
casm_class_hash_from_sierra_file and implements the correct BLAKE2s-based hash;
delete that comment so the code reflects the implemented behaviour and no longer
asks an answered question.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
bin/sozo/src/commands/options/account/mod.rs(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-19T22:37:38.236Z
Learnt from: glihm
Repo: dojoengine/dojo PR: 3377
File: crates/dojo/utils/src/tx/declarer.rs:159-159
Timestamp: 2025-11-19T22:37:38.236Z
Learning: In Dojo migration code (crates/dojo/utils/src/tx/declarer.rs), using BlockTag::Latest in is_declared() is safe because migrations are sequential. The account's BlockTag::PreConfirmed configuration is specifically for nonce management, not for class existence checks.
Applied to files:
bin/sozo/src/commands/options/account/mod.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: fmt
🔇 Additional comments (1)
bin/sozo/src/commands/options/account/mod.rs (1)
11-11: LGTM! Import path updated for slot consolidation.Ohayo sensei! The import path change from
slot_sessiontoslotaligns with the consolidation to a single slot crate. This refactoring looks good.
| // Since now the block frequency is higher than before, using latest is | ||
| // totally fine. We keep it explicitely set here to easy toggle if necessary. | ||
| account.set_block_id(BlockId::Tag(BlockTag::Latest)); |
There was a problem hiding this comment.
Fix typo in comment.
Ohayo sensei! The updated comment is much clearer and addresses the previous confusion about BlockTag behavior. Good fix! However, there's a small typo: "explicitely" should be "explicitly".
Apply this diff to fix the typo:
- // Since now the block frequency is higher than before, using latest is
- // totally fine. We keep it explicitely set here to easy toggle if necessary.
+ // Since now the block frequency is higher than before, using latest is
+ // totally fine. We keep it explicitly set here to easy toggle if necessary.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Since now the block frequency is higher than before, using latest is | |
| // totally fine. We keep it explicitely set here to easy toggle if necessary. | |
| account.set_block_id(BlockId::Tag(BlockTag::Latest)); | |
| // Since now the block frequency is higher than before, using latest is | |
| // totally fine. We keep it explicitly set here to easy toggle if necessary. | |
| account.set_block_id(BlockId::Tag(BlockTag::Latest)); |
🤖 Prompt for AI Agents
In bin/sozo/src/commands/options/account/mod.rs around lines 133 to 135, there's
a typo in the comment: change "explicitely" to "explicitly" so the comment reads
correctly; update the comment text accordingly on that line.
This PR adds the support for blake2s using the implementation from
starknet_api.Since
starknet-types-corewas required to be0.2.3or higher, it has been bumped to the stable1.0.0that will be used by the ecosystem soon.Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.