crates/katana/storage/db/src/abstraction/transaction.rs (2)

72-92: Ohayo sensei! Acknowledge the TODO about cursor type duplication.

Referring to lines 77-79, there's a noted TODO regarding the duplication of cursor type definitions. Consider extracting a shared associated type or trait that can be used universally, which could help avoid redundancy and make the code more maintainable.

---

94-128: Inherit documentation or unify references to avoid duplication.

Similar to DbTxRef, DbTxMutRef also duplicates cursor type definitions. Evaluate whether a single trait or unified approach could manage both read-only and read-write references to tables, aligning with the DRY principle.

crates/katana/trie/src/contracts.rs (1)

17-34: Ohayo sensei! Ensure multiproof usage aligns with concurrency needs.
Currently, multiproof takes &mut self, but the method does not appear to modify the internals. If the call truly modifies the trie’s internal state, it’s fine. Otherwise, if it’s only reading data, you could consider using &self to allow more flexible concurrency.

-pub fn multiproof(&mut self, addresses: Vec<ContractAddress>) -> MultiProof {
+pub fn multiproof(&self, addresses: Vec<ContractAddress>) -> MultiProof {

crates/katana/trie/src/classes.rs (1)

11-20: Ohayo sensei! ClassesMultiProof is straightforward and cohesive.
Exposing a verify method that delegates to crate::verify_proof keeps things modular. Just ensure that result validation (TODO mentioned) is completed or tracked.

Would you like to open an issue or add test coverage for more thorough validation checks?

crates/katana/node/src/config/rpc.rs (5)

14-15: Ohayo sensei! Clarify usage of DEFAULT_RPC_MAX_PROOF_KEYS.

Defining a default for starknet_getStorageProof keys clarifies resource limits. Be sure to document the rationale for “100” (e.g., performance or memory constraints).

---

17-35: Ohayo sensei! Check for potential future modules.

The RpcModuleKind enum is well-defined. It may be helpful to create a placeholder for future modules or mention them in doc comments, preventing repeated refactors.

---

71-73: Ohayo sensei! Good error encapsulation for invalid modules.

InvalidRpcModuleError is straightforward. Consider extending the error message to explain how to list valid module names.

---

75-99: Ohayo sensei! Keep an eye on concurrency usage.

RpcModulesList is simple and well-structured. If usage grows to multi-threaded contexts, consider a thread-safe collection or a read/write lock around mutations.

---

135-183: Ohayo sensei! Test coverage is admirable.

The tests handle standard cases and edge conditions. You might want to test concurrency or override environment variables if relevant to your workflow.

crates/katana/trie/src/lib.rs (6)

8-19: Ohayo sensei! Good pattern for re-exports and modular organization.

Exposing bitvec and reorganizing the modules clarifies the trie’s building blocks. This should make external usage simpler.

---

20-26: Ohayo sensei! Document the purpose of derived tries more thoroughly.

BonsaiTrie is clearly described as a shim. Adding an example usage or referencing which submodule (e.g., ClassesTrie) suits which scenario can help new developers.

---

27-32: Ohayo sensei! Consistent constraints on the generic DB and hash.

Ensuring DB: BonsaiDatabase and Hash: StarkHash + Send + Sync is wise. If future expansions require more constraints, you can unify them in a trait alias.

---

71-84: Ohayo sensei! Insert and commit methods are consistent.

insert with a BitVec index is straightforward. On the other hand, consider clarifying concurrency or multi-thread usage if soon relevant.

---

86-94: Ohayo sensei! Debug implementation is helpful.

Additional fields in the debug output can help trace usage but might cause clutter. Consider gating with a feature flag if you want fine-grained logging.

---

Line range hint 96-140: Ohayo sensei! compute_merkle_root is a neat utility.

Returning anyhow::Result ensures robust error capturing. Possibly separate large merges from smaller merges to keep the function’s scope narrower.

crates/katana/executor/src/abstraction/mod.rs (2)

14-16: Ohayo sensei! Extended trait imports strengthen abstraction.

StateProofProvider and StateRootProvider complement StateProvider. Ensure you manage potential naming collisions with any future trait expansions.

---

227-243: Ohayo sensei! Root methods in StateRootProvider are consistent.

These are vital for verifying on-chain data. Ensure Option<Felt> for storage roots is properly documented so users know how to handle missing contracts.

crates/katana/rpc/rpc-types/src/trie.rs (5)

10-16: Ohayo sensei! ContractStorageKeys structure clarifies request parameters.

Renaming address to contract_address and keys to storage_keys is user-friendly. Great job making the naming consistent with JSON fields.

---

27-50: Ohayo sensei! MerkleNode variants well-structured.

Both Edge and Binary carry essential path details. The inline doc comments are great. Possibly add a convenience method for node type checking.

---

66-77: Ohayo sensei! Well-structured GetStorageProofResponse.

Combining multiple proofs in a single response is efficient. Just be mindful of the response’s size with large sets.

---

79-106: Ohayo sensei! Good separation of proofs into ClassesProof, ContractsProof, and ContractStorageProofs.

Organizing data by domain (classes, contracts) makes the logic more direct. If you foresee merges of these proof objects, define a unifying type.

---

172-193: Ohayo sensei! felt_to_path and path_to_felt preserving leading zeros is crucial.

This is an easy place to introduce off-by-one errors. Your test examples handle typical edge cases.

crates/katana/storage/db/src/trie/snapshot.rs (3)

15-23: Ohayo sensei! Document read-only nature in SnapshotTrieDb.

Highlight “modifying trie snapshot is not supported” across multiple methods. This ensures devs quickly grasp it’s for historical reads only.

---

35-47: Ohayo sensei! recent_change_from_block is a neat helper.

Returning the largest block <= target from the block list is a common pattern. This is a prime candidate for a quick doc test with an example.

---

125-244: Ohayo sensei! Testing with property-based approach is thorough.

Inserting random key-values across blocks, then verifying snapshots, is a robust strategy. Good job ensuring coverage for partial commits.

crates/katana/core/src/backend/mod.rs (1)

169-169: Ohayo sensei! The unified impl block is neat.
Centralizing class and contract logic under one writer simplifies extension and reduces duplication.

bin/katana/src/cli/init/mod.rs (3)

49-92: Ohayo sensei! Great use of serialization.
Storing chain config as TOML is user-friendly and ensures easy versioning. Consider catching potential version mismatch or config corruption.

---

94-120: Ohayo sensei! The execute method is well-organized.
It orchestrates config generation and file output. Perhaps wrap file system errors in more descriptive messages.

---

121-199: Ohayo sensei! Interactive prompts are user-friendly.
Good job verifying each input is valid on the settlement chain. Consider an option for “no prompts” to allow CI integration.

crates/katana/storage/provider/src/providers/db/state.rs (2)

195-213: Ohayo sensei! Exposing class, contract, and storage roots from the latest state is clear.
We might add a fallback mechanism if the root is missing or uninitialized.

---

348-385: Ohayo sensei! Historical multiproof logic is consistent with the latest approach.
Ensure that the expect("should exist") calls don’t hamper advanced use cases with partial data.

crates/katana/storage/db/src/trie/mod.rs (4)

7-10: Ohayo sensei! Using katana_trie with CommitId is a major architectural improvement.
It creates a clearer snapshot model, but careful not to bloat the commit history with frequent snapshots.

---

290-299: Ohayo sensei! Insertion logic merges well with the write_cache.
Consider adding logging to identify potential collisions or overwrites.

---

360-367: Ohayo sensei! Merging is unimplemented.
Local merges may be useful for replaying historical changes across multiple blocks. Potential extension for reorg logic.

Do you want me to help outline an approach for implementing local merges?

---

370-371: Ohayo sensei! The fallback to a Some snapshot is reasonable.
Optionally you could check for actual existence in the DB to confirm.

crates/katana/storage/provider/src/providers/fork/state.rs (3)

105-124: Ohayo sensei! Placeholder multiproof logic.
All multiproof methods return Default::default(). Consider implementing actual multiproof logic or clarifying the plan to keep them unimplemented.

---

181-200: Ohayo sensei! LatestStateProvider multiproof placeholders.
Similar to ForkedStateDb, returning default multiproof might be temporary. Verify that no practical usage requires real proof data.

---

288-307: Ohayo sensei! Snapshot multiproof stubs.
The ForkedSnapshot returns default katana_trie::MultiProof. If future expansions are needed, highlight them in docs.

crates/katana/storage/provider/src/providers/fork/backend.rs (1)

665-688: Ohayo sensei! Stubs for multiproof in SharedStateProvider.
The unimplemented!("not supported...") calls highlight a future need or justification to remain stubs.

crates/katana/rpc/rpc/src/starknet/config.rs (1)

7-11: Ohayo sensei! Great addition for proof keys limit.
This new field max_proof_keys provides flexibility for controlling proof retrieval size. Consider adding basic tests ensuring that errors are raised when proofs exceed this limit.

crates/katana/storage/db/src/mdbx/tx.rs (1)

36-36: Ohayo sensei! Good clarity in initialization
Using an explicit [None; NUM_TABLES] array is more readable and ensures a well-defined initial state for db_handles. If further customization is needed, consider naming the struct field or constant more clearly (e.g., AVAILABLE_TABLE_HANDLES).

crates/katana/executor/src/implementation/noop.rs (1)

199-211: Ohayo sensei! Validate zero-state root usage
Returning Felt::ZERO for class and contract roots could be correct for a no-op, but confirm there’s no logic elsewhere that treats zero as invalid.

crates/katana/storage/provider/tests/block.rs (3)

14-14: Ohayo sensei! Unused import check
It looks like StateRootProvider might no longer be needed since you're now relying on StateFactoryProvider. Consider removing this import to keep the code clean.

-use katana_provider::traits::state::{StateFactoryProvider, StateRootProvider};
+use katana_provider::traits::state::StateFactoryProvider;

---

32-32: Ohayo sensei! Don't forget to re-enable
The #[ignore] attribute prevents this test from running. Once trie computation for forked mode becomes supported, please remove the ignore so we can get full coverage.

---

49-49: Ohayo sensei! Same note about disabling tests
Like the previous test, consider removing #[ignore] once forked mode trie computation is implemented.

crates/katana/cli/src/options.rs (1)

132-135: Ohayo sensei! Thoughtful defaults
Providing defaults for http_modules and max_proof_keys in ServerOptions spares users from manual overrides in most setups.

crates/katana/core/src/backend/storage.rs (2)

30-30: Ohayo sensei! Clean macros
Your usage of short_string! from starknet::macros is neat for hash computations.

---

221-240: Ohayo sensei! Manual root computation
Your approach to calculate class_trie_root and contract_trie_root, then hashing them for genesis_state_root is elegant. Just ensure that the constants like STARKNET_STATE_V0 remain consistent across version upgrades.

crates/katana/storage/provider/src/providers/fork/mod.rs (2)

603-603: Ohayo sensei! The trie_insert_declared_classes stub is helpful, but it’s still unimplemented.
If this remains a placeholder, consider documenting the expected logic or raising a TODO to guide future maintainers.

---

613-613: Ohayo sensei! The trie_insert_contract_updates stub mirrors the declared classes approach well.
Likewise, adding a note for the expected implementation or final form would be beneficial for those who pick up the code in the future.

crates/katana/executor/src/implementation/blockifier/state.rs (2)

241-267: Ohayo sensei! The StateProofProvider multiproof methods are stubs.
They nicely outline how multiproofs might be fetched for classes, contracts, and storage. If these remain unimplemented for long, consider documenting usage assumptions or trade-offs.

---

269-281: Ohayo sensei! The StateRootProvider methods, likewise, are placeholders.
They cleanly delineate responsibilities for retrieving roots of classes, contracts, and storage. As with the multiproof stubs, providing comments or TODOs for the unimplemented logic would increase clarity.

.gitmodules (1)

4-6: Ohayo sensei! Welcoming Piltover as a submodule.
Bringing in the Piltover repository paves the way for expanded functionality. Ensure you’ve pinned a stable commit or branch to avoid unexpected breakage down the line.

crates/katana/contracts/Makefile (1)

19-21: Build process needs error handling, sensei! 🛡️

The build steps could fail silently if the piltover directory doesn't exist. Let's add some safeguards.

$(BUILD_DIR): ./piltover/src/*
-	cd piltover && scarb build
-	mv target/dev/$(ORIGINAL_CLASS_NAME) $(BUILD_DIR)/$(CLASS_NAME)
+	@if [ ! -d "./piltover" ]; then \
+		echo "Error: piltover directory not found"; \
+		exit 1; \
+	fi
+	cd piltover && scarb build
+	mv target/dev/$(PILTOVER_ORIGINAL_CLASS_NAME) $(BUILD_DIR)/$(PILTOVER_CLASS_NAME)

bin/katana/Cargo.toml (1)

10-14: Consider workspace version management, sensei! 🎯

Some dependencies use explicit versions while others use workspace inheritance. For consistency and easier maintenance:

-dirs = "5.0.1"
-inquire = "0.7.5"
+dirs.workspace = true
+inquire.workspace = true

Consider adding these dependencies to the workspace configuration for unified version management.

Also applies to: 18-31

crates/katana/rpc/rpc/Cargo.toml (1)

Line range hint 20-34: Consider feature flagging some dependencies, sensei! 🎋

Some of these dependencies might not be needed for all use cases. Consider grouping them under feature flags:

• Metrics-related: dojo-metrics, metrics
• Observability: tracing
• HTTP-specific: tower-http

This could help reduce the dependency tree for users who don't need all functionality.

Example structure:

[features]
metrics = ["dojo-metrics", "metrics"]
tracing = ["tracing"]
http = ["tower-http"]
default = ["metrics", "tracing", "http"]

crates/katana/node/Cargo.toml (1)

28-30: Ohayo! The dependency updates look solid, sensei! 🎋

The addition of thiserror and enabling "full" features for tower and tower-http aligns well with enhanced RPC capabilities. However, it would be good to document why we need the full feature sets.

Consider adding a comment in the toml file explaining which specific features from the "full" sets are actually required. This will help future maintainers understand if we can optimize by selecting only needed features.

crates/katana/rpc/rpc-types-builder/src/state_update.rs (1)

33-38: Consider adding error context to expect messages

While the expect messages are descriptive, they could be more specific about which block they're referring to.

Consider enhancing the error messages:

-            .expect("should exist if block exists")
+            .expect(&format!("state should exist for block {:?}", self.block_id))

Also, consider adding a comment explaining why we're confident that these states should always exist when the block exists.

Also applies to: 45-49

crates/dojo/test-utils/src/sequencer.rs (1)

125-128: Consider test-specific RPC module selection

Using RpcModulesList::all() in test configuration might be too permissive. Also, the magic numbers for max sizes should be constants.

Consider these improvements:

-        apis: RpcModulesList::all(),
+        apis: RpcModulesList::new(vec![
+            // List only the modules needed for tests
+            RpcModuleKind::Starknet,
+            RpcModuleKind::Dev,
+        ]),
+        max_connections: DEFAULT_RPC_MAX_CONNECTIONS,
-        max_event_page_size: Some(100),
-        max_proof_keys: Some(100),
+        max_event_page_size: Some(DEFAULT_TEST_EVENT_PAGE_SIZE),
+        max_proof_keys: Some(DEFAULT_TEST_PROOF_KEYS),

Also, consider adding these constants to a test configuration module:

pub const DEFAULT_TEST_EVENT_PAGE_SIZE: u32 = 100;
pub const DEFAULT_TEST_PROOF_KEYS: u32 = 100;

crates/katana/rpc/rpc/tests/test_data/test_sierra_contract.json (1)

Line range hint 892-896: Consider adding debug information for better testing experience.

The debug arrays (type_names, libfunc_names, user_func_names) are empty. While this works, having debug information would make the test contract more maintainable and easier to debug.

   "sierra_program_debug_info": {
-    "type_names": [],
-    "libfunc_names": [],
-    "user_func_names": []
+    "type_names": [
+      "felt252",
+      "ContractAddress",
+      "Array<felt252>"
+    ],
+    "libfunc_names": [
+      "store_temp",
+      "function_call",
+      "return"
+    ],
+    "user_func_names": [
+      "__validate_deploy__",
+      "__validate_declare__",
+      "__validate__",
+      "__execute__"
+    ]
   },

crates/katana/storage/db/src/models/trie.rs (1)

85-97: ⚠️ Potential issue

Ohayo sensei! Consider returning an error instead of panicking.
Currently, if the buffer is too short, panic! is triggered. Returning a CodecError might be more resilient in production, enabling the caller to handle incomplete data gracefully.

-if bytes.len() < 2 {
-    panic!("empty buffer")
-}
...
-if bytes.len() < 2 + key_len {
-    panic!("Buffer too short for key length");
-}
+if bytes.len() < 2 {
+    return Err(CodecError::new("Empty buffer for TrieDatabaseKey"))
+}
...
+if bytes.len() < 2 + key_len {
+    return Err(CodecError::new("Buffer too short for key length"))
+}

Committable suggestion skipped: line range outside the PR's diff.

crates/katana/trie/src/lib.rs (1)

56-64: 🛠️ Refactor suggestion

Ohayo sensei! Keep an eye on error handling for root retrieval.

Panics on root_hash failure might lead to partial commits being unrecoverable. Consider returning a Result or having a fallback strategy.

crates/katana/core/src/backend/mod.rs (1)

255-263: 🛠️ Refactor suggestion

Ohayo sensei! Consider more robust error handling and logs here.
The current .expect("failed to update") calls could be replaced by a custom error or a graceful fallback to avoid panics in production.

crates/katana/storage/db/src/trie/mod.rs (1)

34-47: 🛠️ Refactor suggestion

Ohayo sensei! Exposing latest() and historical() unifies trie creation while reducing boilerplate.
Maybe handle None returns if the block does not exist, for safer error handling.

crates/katana/contracts/Makefile (1)

16-17: 🛠️ Refactor suggestion

Ohayo! Variable naming could use improvement, sensei! 🍜

The reuse of ORIGINAL_CLASS_NAME and CLASS_NAME variables might cause issues since Make variables are global. Consider using unique names for Piltover-specific variables.

-ORIGINAL_CLASS_NAME := piltover_appchain$(CONTRACT_CLASS_SUFFIX)
-CLASS_NAME := appchain_core_contract.json
+PILTOVER_ORIGINAL_CLASS_NAME := piltover_appchain$(CONTRACT_CLASS_SUFFIX)
+PILTOVER_CLASS_NAME := appchain_core_contract.json

📝 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.

PILTOVER_ORIGINAL_CLASS_NAME := piltover_appchain$(CONTRACT_CLASS_SUFFIX)
PILTOVER_CLASS_NAME := appchain_core_contract.json