refactor(katana): move predeployedAccounts under DevApi and remove KatanaApi #2310
Conversation
WalkthroughOhayo, sensei! The recent changes involve the removal of the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant DevAPI
Client->>Server: Request data
Server->>DevAPI: Call predeployed_accounts
DevAPI-->>Server: Return accounts data
Server-->>Client: Send accounts data
This diagram illustrates the interaction between the client and server when retrieving predeployed accounts through the Tip We have updated our review workflow to use the Anthropic's Claude family of models. Please share any feedback in the discussion post on our Discord. Recent review detailsConfiguration used: .coderabbit.yaml Files selected for processing (6)
Additional comments not posted (6)
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, codebase verification and nitpick comments (2)
crates/katana/runner/src/lib.rs (1)
50-51: Document the purpose of thedevfield clearly.The
devfield is a crucial addition for configuring development RPC endpoints. Ensure that its purpose and usage are well-documented for clarity./// Whether to run the katana runner with the `dev` rpc endpoints. + /// Set this to `true` to enable development-specific features. pub dev: bool,crates/katana/node/src/lib.rs (1)
Ohayo, sensei! It looks like the
KatanaApiis still lingering in the codebase, particularly within error handling components. Here are the spots where it's referenced:
crates/katana/rpc/rpc/src/dev.rs: Commented out reference toKatanaApiError.crates/katana/rpc/rpc-types/src/error/katana.rs: Definition ofKatanaApiErrorand its conversion implementation.These references indicate that
KatanaApihasn't been fully removed or refactored. You might want to review these parts to ensure they align with the current objectives of streamlining the API. If these are no longer needed, they should be removed or updated accordingly.Analysis chain
Line range hint
121-146:
Confirm removal ofKatanaApi.The removal of the
ApiKind::Katanavariant from thespawnfunction aligns with the PR's objective to streamline the API. Ensure that any client interactions relying onKatanaApiare updated or documented.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if `KatanaApi` is still referenced in the codebase. # Test: Search for references to `KatanaApi`. Expect: No occurrences. rg --type rust 'KatanaApi'Length of output: 390
glihm
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2310 +/- ##
==========================================
- Coverage 67.86% 67.85% -0.01%
==========================================
Files 359 357 -2
Lines 46959 46949 -10
==========================================
- Hits 31869 31859 -10
Misses 15090 15090 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
looks alright. removing the katana api seems fine since there's only predeployAccounts under it and it also feels somewhat redundant to have too many namespaces, but maybe we're possibly breaking user code a lil bit by outright changing the namespace of the method? because now they have to do two things; use --dev when starting katana, and change katana_ -> dev_. simply putting katana api under --dev seems less intrusive.
just a lil thought but not married to the idea. what you did is most probably fine.
Yeah I was thinking the same at first. But then, realized that may be better to have this clearly identified as a This change will impact:
We may actually make this refactor here? WDYT? Your comment on doing this namespace selection makes the devX very nice for future namespaces addition. |
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
crates/katana/rpc/rpc/src/dev.rs (1)
94-98: Ohayo, sensei! The new method looks great, but let's add a comment to improve the code quality.The new method
predeployed_accountsis implemented correctly and enhances the functionality of theDevApiServerby allowing it to expose predeployed account information.The
#[allow(deprecated)]attribute is a good practice to indicate that the method may be deprecated in the future.To improve the code quality, please add a comment to explain the purpose of the method and the reason for the
#[allow(deprecated)]attribute:+/// Returns the predeployed accounts from the backend's genesis configuration. +/// This method is marked as deprecated because it may be removed in the future. #[allow(deprecated)] async fn predeployed_accounts(&self) -> Result<Vec<Account>, Error> { Ok(self.backend.config.genesis.accounts().map(|e| Account::new(*e.0, e.1)).collect()) }
…tanaApi (dojoengine#2310) fix: move predeployedAccounts under DevApi and remove KatanaApi
* feat: add sierra to cairo debug information * Add walnut flag to sozo execute command * Pass rpc url to handle_transaction_result * Add walnut flag to sozo migrate apply command * Move walnut_debug_transaction to walnut crate * Cargo fmt * Keep only one global walnut flag * Add comments * Add walnut flag to sozo execute command * Pass rpc url to handle_transaction_result * Add walnut flag to sozo migrate apply command * Move walnut_debug_transaction to walnut crate * Cargo fmt * Keep only one global walnut flag * Add comments * Resolve conflicts * Fix lint errors * Put the walnut crate under the /sozo dir * Add constants with API and app URLs * Warn where we fail silently * Remove unnecessary comments * Check Walnut API key before migration * Add doc comments * Disable walnut flag in auto_authorize * chore; use debug for pending tx log (#2383) Update engine.rs * refactor(katana-rpc): `getEvents` include pending block (#2375) * refactor(katana): move predeployedAccounts under DevApi and remove KatanaApi (#2310) fix: move predeployedAccounts under DevApi and remove KatanaApi * remove world and indexers table in favour of contracts commit-id:98359f5f * opt(torii): batch query execution in sync_range commit-id:72f22f88 * refactor(torii): make select block cancel safe commit-id:8fbc8e6d * opt(torii): use hashmap instead of vector of event processors commit-id:7303cc72 * opt(torii): fetch receipts along with blocks instead of fetching them individually commit-id:b6db4cb5 * opt(torii): avoid re-processing of transactions in certain case fix: #2355 commit-id:a510b985 * wip * chore(dojo-world): enable manifest feature on `migration` feature * fmt * refactor: move walnut config into WalnutDebugger * fix: ensure only WalnutDebugger is exposed * fix: restore default dojo_dev.toml * dont print in library code * use concrete error types in walnut/verification * use concrete types again * remove unecessary util function * use json method instead * fix: fix test --------- Co-authored-by: glihm <dev@glihm.net> Co-authored-by: Larko <59736843+Larkooo@users.noreply.github.com> Co-authored-by: Ammar Arif <evergreenkary@gmail.com> Co-authored-by: lambda-0x <0xlambda@protonmail.com>
Closes #2292.
@kariy as discussed it makes sense to have
predeployedAccountsendpoint only available under--devoption.Also, I had the feeling that
KatanaApiwould certainly ends up being testing mostly, so I deleted it.But we may actually keep it even if empty, and be completed later if some endpoints make sense to be present even when not in
--devmode. 👍Summary by CodeRabbit
Summary by CodeRabbit
New Features
KatanaRunnerConfigto support a development mode, improving configurability for developers.Bug Fixes
KatanaAPI from default configurations, streamlining available API options and preventing potential errors in production environments.Documentation
devfield inKatanaRunnerConfigto guide users on its purpose.Chores
KatanaAPI.