feat(katana): sequencer feeder gateway client #2732
Conversation
kariy
force-pushed
the
katana/fgw-client
branch
from
dc20773 to
1cbf24e
Compare
WalkthroughOhayo! This pull request introduces significant updates to the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
🪧 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
🧹 Outside diff range and nitpick comments (11)
crates/katana/feeder-gateway/src/types.rs (2)
3-6: ConsolidateCasmContractClassImport and AliasOhayo, sensei! I noticed that
CasmContractClassis re-exported on line 3 fromkatana_primitives::class::CasmContractClass, and a type alias is also defined on lines 22-23 fromkatana_cairo::lang::starknet_classes::casm_contract_class::CasmContractClass. This might cause confusion due to multiple sources. Consider consolidating these to a single import to improve code clarity.
82-120: Optimize Iteration inFrom<StateDiff>ImplementationOhayo, sensei! The
From<StateDiff>implementation involves multiple iterations and collections. To improve performance and readability, consider optimizing these iterations, possibly by using combinators to reduce the number of transient collections.crates/katana/primitives/src/class.rs (3)
21-24: Evaluate Placement ofCasmContractClassType AliasOhayo, sensei! The type alias for
CasmContractClassis introduced on lines 22-23. Since this type is essential and used across multiple modules, consider re-exporting it from a central location to enhance maintainability and reduce redundancy.
Line range hint
26-55: Ensure Consistency in Error HandlingOhayo, sensei! In the definitions around
ContractClassCompilationErrorandComputeClassHashError, make sure that error handling is consistent and provides meaningful messages. This will aid in debugging and improve user experience.
Line range hint
79-97: Consistency in Hash Computation FunctionsOhayo, sensei! The
compute_sierra_class_hashandcompute_legacy_class_hashfunctions are implemented differently. Consider aligning their structures or providing comments to clarify their differences, enhancing code readability.Cargo.toml (2)
23-23: Maintain Alphabetical Order in Workspace MembersOhayo, sensei! For better readability, consider adding
"crates/katana/feeder-gateway"to the workspace members list in alphabetical order.
101-101: Consistency in Dependency Naming ConventionsOhayo, sensei! In the
[workspace.dependencies]section, the new dependency is added askatana-feeder-gateway. To maintain consistency with the rest of the dependencies, consider using underscores (_) instead of hyphens (-), likekatana_feeder_gateway.crates/katana/feeder-gateway/src/client.rs (4)
35-35: Ohayo, sensei! Consider handling URL parsing errors without usingunwrap().Using
unwrap()onUrl::parsecan lead to panics if the URL is invalid. Even though the URLs are hardcoded and unlikely to fail, handling the potential error gracefully improves robustness.Apply this diff:
-pub fn sn_mainnet() -> Self { - Self::new(Url::parse("https://alpha-mainnet.starknet.io/").unwrap()) +pub fn sn_mainnet() -> Result<Self, Error> { + let url = Url::parse("https://alpha-mainnet.starknet.io/")?; + Ok(Self::new(url)) }And update the method's return type and usage accordingly.
42-42: Ohayo, sensei! Handle URL parsing errors insn_sepoliamethod as well.Similarly, avoid using
unwrap()to prevent potential panics.Apply this diff:
-pub fn sn_sepolia() -> Self { - Self::new(Url::parse("https://alpha-sepolia.starknet.io/").unwrap()) +pub fn sn_sepolia() -> Result<Self, Error> { + let url = Url::parse("https://alpha-sepolia.starknet.io/")?; + Ok(Self::new(url)) }
90-93: Ohayo, sensei! Handle potential errors in thefeeder_gatewaymethod to prevent panics.Using
expect("invalid base url")can cause a panic if the base URL cannot have path segments appended. Consider handling this error to enhance stability.Apply this diff:
fn feeder_gateway(&self, method: &str) -> RequestBuilder<'_> { let mut url = self.base_url.clone(); - url.path_segments_mut().expect("invalid base url").extend(["feeder_gateway", method]); + if let Ok(mut segments) = url.path_segments_mut() { + segments.extend(["feeder_gateway", method]); + } else { + // Handle the error appropriately, perhaps by returning an error + // or logging an error message + } RequestBuilder { client: &self.client, url } }
229-251: Ohayo, sensei! Reassess the ignored testrequest_block_id_overwrite.The test
request_block_id_overwriteis marked with#[ignore]. Ignored tests might hide potential issues. Consider enabling the test or documenting the reason for ignoring it.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
Cargo.toml(2 hunks)crates/katana/feeder-gateway/Cargo.toml(1 hunks)crates/katana/feeder-gateway/src/client.rs(1 hunks)crates/katana/feeder-gateway/src/lib.rs(1 hunks)crates/katana/feeder-gateway/src/types.rs(1 hunks)crates/katana/primitives/src/class.rs(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- crates/katana/feeder-gateway/src/lib.rs
- crates/katana/feeder-gateway/Cargo.toml
🔇 Additional comments (2)
crates/katana/feeder-gateway/src/types.rs (1)
15-20: Verify All Variants in ContractClass Enum
Ohayo, sensei! The ContractClass enum uses #[serde(untagged)] for deserialization. Please ensure that all possible contract class variants returned by the feeder gateway API are accounted for to prevent potential deserialization errors.
Cargo.toml (1)
Line range hint 171-176: Update Version Constraints for Dependencies
Ohayo, sensei! Verify that the version constraints for the tonic and wasm-tonic dependencies are compatible, especially since they are used in different contexts (standard vs. WASM). This will help prevent potential build or runtime issues.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2732 +/- ##
==========================================
+ Coverage 56.24% 56.34% +0.09%
==========================================
Files 415 423 +8
Lines 53241 54166 +925
==========================================
+ Hits 29948 30521 +573
- Misses 23293 23645 +352 ☔ View full report in Codecov by Sentry. |
why need to implement our own client instead of using an existing one (ie from starknet-rs) ? to ease type conversions.
ref #2724
why need to implement our own client instead of using an existing one (ie from
starknet-rs) ? to ease type conversions. right now the only methods implemented are the ones that will be used for the syncing through the feeder gateway. there are still some types/conversions that haven't been defined yet but can be done progressively.