feat(katana): modify DevApi trait with new endpoints #2490
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2490 +/- ##
==========================================
- Coverage 55.82% 55.76% -0.06%
==========================================
Files 446 448 +2
Lines 57728 57854 +126
==========================================
+ Hits 32225 32264 +39
- Misses 25503 25590 +87 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
we can just proxy them like we did for the /health endpoint here.
so based on the predefined paths we can just proxy them to the appropriate json rpc methods. eg /fee_token , we can add a new ProxyGetRequestLayer::new("/fee_token", "dev_feeToken")
and so on.
based on the ProxyGetRequest implementation, seems like it can only proxy GET request only. and will treat all POST as is.
what we could do is probably implement our own ProxyLayer that will proxy specific paths but for both GET and POST. Or perhaps for this PR we can just do for the GET requests only (ie /predeployed_accounts, /account_balance, /fee_token)
glihm
changed the title
- Path is not necesary now when calling the DevnetProxyLayer - Using a match to return back the params and the method to call through rpc call
|
Thank you @fabrobles92 for the rebase. You could check the |
WalkthroughThe pull request introduces enhancements to the Katana devnet RPC functionality, focusing on adding new HTTP endpoints for account-related operations. The changes span multiple files across the primitives and RPC modules, implementing string parsing for price units, adding methods for account balance retrieval, and creating a proxy layer to handle HTTP GET requests for RPC methods. Changes
Assessment against linked issues
Sequence DiagramsequenceDiagram
participant Client
participant DevnetProxyLayer
participant RpcServer
participant DevApi
Client->>DevnetProxyLayer: GET /account_balance
DevnetProxyLayer->>RpcServer: Transform to RPC call
RpcServer->>DevApi: account_balance method
DevApi-->>RpcServer: Return account balance
RpcServer-->>DevnetProxyLayer: RPC response
DevnetProxyLayer-->>Client: HTTP response with balance
Ohayo, sensei! The PR looks like it's making solid progress towards exposing those devnet endpoints. While not all requested features are fully implemented, it's laying down a robust foundation for future expansions. Keep coding! 🚀 Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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: 2
🧹 Nitpick comments (8)
crates/katana/rpc/rpc/src/proxy_get_request.rs (3)
25-34: Ohayo sensei! Gentle reminder about error handling in the constructor.Currently,
DevnetProxyLayer::new()unconditionally returnsOk(Self {}). Consider if there's any scenario where invalid config or environment might warrant returning an error. If none is expected, this is fine!
70-86: Ohayo sensei! Potential improvements for concurrency checks.The
Serviceimplementation blocks on transformations and error handling within a single async flow. If future concurrency expansions are needed, ensure there's no shared mutable data that requires synchronization here.
96-116: Ohayo sensei! Mindful handling of fallback scenario.When
method.is_empty(), the code short-circuits to return the original response. Double-check that this behavior is consistent with other devnet endpoints. If you plan to support more GET endpoints, consider grouping them or returning a descriptive error.crates/katana/rpc/rpc-api/src/dev.rs (1)
31-32: Ohayo sensei! Clarifymintmethod's purpose.The
mintmethod is empty. If it's intentionally left unimplemented, please add a doc comment explaining. Otherwise, you could remove it until it's ready or return an explicit unimplemented error.crates/katana/rpc/rpc/src/transport/http.rs (1)
33-36: Ohayo sensei! Confirm success response body format.
ok_responsereturns the raw body as a string. If some calls need to wrap it in a JSON object (e.g.,{"data": ...}), consider implementing a pattern to avoid confusion in the client.crates/katana/rpc/rpc/src/lib.rs (1)
118-118: Ohayo sensei! Validate usage scope before layering.
DevnetProxyLayeris layered for all incoming requests. If certain requests need bypassing, consider conditionally applying the layer or scoping it specifically.crates/katana/rpc/rpc/src/dev.rs (1)
118-120: Ohayo sensei! ‘mint’ method awaiting details.This method is an empty stub. Confirm its usage in
starknet-devnetand possibly clarify if it should produce minted tokens or remain unimplemented.crates/katana/primitives/src/genesis/constant.rs (1)
122-129: Ohayo sensei! A small suggestion to simplify the contract address conversion.Currently, you store
erc20_contract_addressas aContractAddress, then you callContractAddress::new(erc20_contract_address.into()). SinceDEFAULT_ETH_FEE_TOKEN_ADDRESSandDEFAULT_STRK_FEE_TOKEN_ADDRESSare alreadyContractAddress, you can directly return them or return a cloned value without chaining the constructor.For instance, you could do:
pub fn get_erc20_address(unit: &PriceUnit) -> Result<ContractAddress, Error> { let erc20_contract_address = match unit { PriceUnit::Wei => DEFAULT_ETH_FEE_TOKEN_ADDRESS, PriceUnit::Fri => DEFAULT_STRK_FEE_TOKEN_ADDRESS, }; - Ok(ContractAddress::new(erc20_contract_address.into())) + Ok(erc20_contract_address) }This avoids allocating a new
ContractAddressand keeps the code straightforward.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
crates/katana/primitives/src/fee.rs(2 hunks)crates/katana/primitives/src/genesis/constant.rs(2 hunks)crates/katana/rpc/rpc-api/src/dev.rs(1 hunks)crates/katana/rpc/rpc/Cargo.toml(2 hunks)crates/katana/rpc/rpc/src/dev.rs(2 hunks)crates/katana/rpc/rpc/src/lib.rs(2 hunks)crates/katana/rpc/rpc/src/proxy_get_request.rs(1 hunks)crates/katana/rpc/rpc/src/transport/http.rs(1 hunks)crates/katana/rpc/rpc/src/transport/mod.rs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- crates/katana/rpc/rpc/src/transport/mod.rs
🔇 Additional comments (7)
crates/katana/rpc/rpc/src/proxy_get_request.rs (1)
132-142: Ohayo sensei! Watch out for JSON parse failures.In
RpcPayload, onlyresultis deserialized. If the JSON schema changes orresultis missing, we reply with an internal error. You may consider logging partial parse results for debugging.crates/katana/rpc/rpc-api/src/dev.rs (1)
28-29: Ohayo sensei! Validate input onaccount_balance.While
addressis a string, consider if it can be validated directly in this trait and return an explicit error if invalid. Relying on the server impl to unwrap might lead to inconsistent error handling.crates/katana/rpc/rpc/src/transport/http.rs (1)
1-16: Ohayo sensei! Kudos on the modular response approach.
internal_error()looks neat. If you foresee various error types, consider abstracting them. Right now, everything isInternalError.crates/katana/rpc/rpc/src/lib.rs (1)
6-6: Ohayo sensei! Good use of layering.Importing
DevnetProxyLayerneatly organizes the proxied GET requests. Keep ensuring minimal coupling by exposing only essential public items from the module.crates/katana/primitives/src/fee.rs (1)
34-43: Ohayo sensei! Looks great for straightforward string parsing.If your usage scenario might involve different cases (e.g., "wei" or "Wei"), you could match them in a case-insensitive way. Otherwise, this is perfectly fine. Proceed as is if a strict match on "WEI" and "FRI" is desired.
crates/katana/rpc/rpc/Cargo.toml (2)
37-46: Ohayo sensei! These asynchronous and serde additions are good for the new devnet endpoints.Just make sure no library remains unused and consider any potential overlap with existing features in the workspace. Everything else looks consistent. Nicely done!
10-12: Ohayo sensei! Verify thejsonrpseeandhyperversions for compatibility.Double-check that these versions align with the rest of the workspace and that there are no known security advisories. This ensures a stable and secure dependency set.
✅ Verification successful
Ohayo sensei! The dependencies are compatible and secure.
Looking at the workspace dependencies in the root Cargo.toml:
hyperis set to workspace version "0.14.27" which is compatible with jsonrpsee's requirementsjsonrpseedependencies in the RPC crate are pinned to version "0.16.3" which is a stable releaseThe versions are consistent across the workspace and there are no conflicting version requirements.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Checking cargo tree for potential conflicts cargo tree -p jsonrpsee-core cargo tree -p hyperLength of output: 244
Script:
#!/bin/bash # Let's check for jsonrpsee and hyper dependencies across the workspace fd Cargo.toml --exec grep -l "jsonrpsee\|hyper" # Then examine the content of these files fd Cargo.toml --exec sh -c 'echo "=== {} ==="; cat {}' # Also check workspace dependencies in the root Cargo.toml if [ -f "Cargo.toml" ]; then echo "=== Root Cargo.toml ===" cat Cargo.toml fiLength of output: 71528
| fn get_account_balance(query: Option<&str>) -> (Box<JsonRawValue>, std::string::String) { | ||
| let default = String::new(); | ||
|
|
||
| let query = query.unwrap_or(&default); | ||
| let params: HashMap<_, _> = form_urlencoded::parse(query.as_bytes()).into_owned().collect(); | ||
|
|
||
| let address = params.get("contract_address").unwrap_or(&default); | ||
| let unit = params.get("unit").unwrap_or(&default); | ||
|
|
||
| let json_string = format!(r#"{{"address":"{}", "unit":"{}"}}"#, address, unit); | ||
| (JsonRawValue::from_string(json_string).unwrap(), "dev_accountBalance".to_string()) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo sensei! Suggest graceful error handling for query extraction.
get_account_balance uses unwrap() for from_string. A malformed query string would panic. Consider returning a user-friendly error or defaulting gracefully.
fn get_account_balance(query: Option<&str>) -> (Box<JsonRawValue>, std::string::String) {
...
- (JsonRawValue::from_string(json_string).unwrap(), "dev_accountBalance".to_string())
+ let raw_value = match JsonRawValue::from_string(json_string) {
+ Ok(val) => val,
+ Err(_) => {
+ // fallback or error handling
+ JsonRawValue::from_string(r#"{}"#.to_string()).unwrap()
+ }
+ };
+ (raw_value, "dev_accountBalance".to_string())
}Committable suggestion skipped: line range outside the PR's diff.
| async fn account_balance(&self, address: String, unit: String) -> Result<u128, Error> { | ||
| let account_address: ContractAddress = Felt::from_str(address.as_str()).unwrap().into(); | ||
| let unit = PriceUnit::from_str(unit.to_uppercase().as_str()).unwrap_or(PriceUnit::Wei); | ||
|
|
||
| let erc20_address = | ||
| get_erc20_address(&unit).map_err(|_| http::response::internal_error()).unwrap(); | ||
|
|
||
| let provider = self.backend.blockchain.provider(); | ||
| let state = provider.latest().unwrap(); | ||
| let storage_slot = get_fee_token_balance_base_storage_address(account_address); | ||
| let balance_felt = state.storage(erc20_address, storage_slot).unwrap().unwrap(); | ||
| let balance: u128 = balance_felt.to_string().parse().unwrap(); | ||
| Ok(balance) | ||
| } |
There was a problem hiding this comment.
Ohayo sensei! Avoid panics on invalid address or unit.
Felt::from_str(address.as_str()).unwrap() and PriceUnit::from_str(...) can cause runtime panics if the user sends malformed data. Returning a structured error might be safer.
- let account_address: ContractAddress = Felt::from_str(address.as_str()).unwrap().into();
- let unit = PriceUnit::from_str(unit.to_uppercase().as_str()).unwrap_or(PriceUnit::Wei);
+ let account_address: ContractAddress = Felt::from_str(address.as_str())
+ .map_err(|_| DevApiError::InvalidAddress(address.clone()))?
+ .into();
+ let unit = PriceUnit::from_str(unit.to_uppercase().as_str())
+ .map_err(|_| DevApiError::InvalidPriceUnit(unit.clone()))?;📝 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.
| async fn account_balance(&self, address: String, unit: String) -> Result<u128, Error> { | |
| let account_address: ContractAddress = Felt::from_str(address.as_str()).unwrap().into(); | |
| let unit = PriceUnit::from_str(unit.to_uppercase().as_str()).unwrap_or(PriceUnit::Wei); | |
| let erc20_address = | |
| get_erc20_address(&unit).map_err(|_| http::response::internal_error()).unwrap(); | |
| let provider = self.backend.blockchain.provider(); | |
| let state = provider.latest().unwrap(); | |
| let storage_slot = get_fee_token_balance_base_storage_address(account_address); | |
| let balance_felt = state.storage(erc20_address, storage_slot).unwrap().unwrap(); | |
| let balance: u128 = balance_felt.to_string().parse().unwrap(); | |
| Ok(balance) | |
| } | |
| async fn account_balance(&self, address: String, unit: String) -> Result<u128, Error> { | |
| let account_address: ContractAddress = Felt::from_str(address.as_str()) | |
| .map_err(|_| DevApiError::InvalidAddress(address.clone()))? | |
| .into(); | |
| let unit = PriceUnit::from_str(unit.to_uppercase().as_str()) | |
| .map_err(|_| DevApiError::InvalidPriceUnit(unit.clone()))?; | |
| let erc20_address = | |
| get_erc20_address(&unit).map_err(|_| http::response::internal_error()).unwrap(); | |
| let provider = self.backend.blockchain.provider(); | |
| let state = provider.latest().unwrap(); | |
| let storage_slot = get_fee_token_balance_base_storage_address(account_address); | |
| let balance_felt = state.storage(erc20_address, storage_slot).unwrap().unwrap(); | |
| let balance: u128 = balance_felt.to_string().parse().unwrap(); | |
| Ok(balance) | |
| } |
Description
Add
starknet-devnet's endpoint proxy.Methods
The full list of the methods are as follows, taken from
starknet-devnet-rs:POST /create_blockPOST /abort_blocksPOST /restartPOST /set_timePOST /increase_timeGET /predeployed_accountsGET /account_balanceGET /fee_tokenPOST /mintGET /fork_statusbut for now, this PR will only addressing the account related endpoints:
GET /predeployed_accountsGET /account_balanceGET /fee_tokenRelated issue
Please link related issues: Fixes #1586
Tests
Added to documentation?
Checklist
scripts/prettier.sh,scripts/rust_fmt.sh,scripts/cairo_fmt.sh)scripts/clippy.sh,scripts/docs.sh)Summary by CodeRabbit
New Features
PriceUnitenumDependency Updates
Refactor