feat(katana): implement more feeder gateway types #2744
Conversation
WalkthroughOhayo, sensei! This pull request introduces significant modifications across several files in the Katana project, primarily focusing on enhancing transaction handling and data structures. Key changes include the addition of new transaction variants, such as Changes
Possibly related PRs
Suggested reviewers
Feel free to reach out if you need further details or clarifications, sensei! 🪧 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: 3
🧹 Outside diff range and nitpick comments (9)
crates/katana/feeder-gateway/src/types/transaction.rs (2)
30-38: Ohayo, sensei! Consider refactoring to avoid redundant definition ofDataAvailabilityMode.Defining
DataAvailabilityModeagain can lead to maintenance challenges. Perhaps explore ways to unify the serde implementations or use feature flags to prevent code duplication.
244-245: Ohayo, sensei! Enhance readability by defining constants for transaction versions.Using
Felt::ZERO,Felt::ONE, andFelt::THREEdirectly can be unclear. Defining constants likeconst VERSION_0: Felt = Felt::ZERO;improves code clarity and reduces potential errors.Also applies to: 254-255, 263-264
crates/katana/executor/src/implementation/blockifier/utils.rs (1)
Line range hint
98-98: Ohayo, sensei! Safeguard against potential overflow when castinginitial_gas.Casting
initial_gasfromu128tou64might overflow if the value exceedsu64::MAX. Consider validatinginitial_gasor handling values that are too large.crates/katana/feeder-gateway/src/types/receipt.rs (1)
8-22: Ohayo! Add documentation for the ConfirmedReceipt struct and its fields, sensei!While the implementation looks solid, adding documentation would help other developers understand:
- The purpose of ConfirmedReceipt
- When each optional field is expected to be present/absent
- The relationship between execution_status and revert_error
Example documentation:
/// Represents a confirmed transaction receipt containing execution details and messages pub struct ConfirmedReceipt { /// The unique identifier of the transaction pub transaction_hash: Felt, /// The position of the transaction within its block pub transaction_index: u64, // ... (continue for other fields) }crates/katana/feeder-gateway/src/types/mod.rs (1)
75-98: Ohayo! Consider adding timestamp validation and documentation, sensei!The Block struct looks well-designed, but consider these improvements:
- Add validation for the timestamp field to ensure it's not in the future
- Add documentation explaining the relationship between optional and required fields
Example implementation:
impl Block { /// Validates block timestamp is not in the future pub fn validate_timestamp(&self) -> Result<(), &'static str> { let current_time = std::time::SystemTime::now() .duration_since(std::time::UNIX_EPOCH) .unwrap() .as_secs(); if self.timestamp > current_time { return Err("Block timestamp cannot be in the future"); } Ok(()) } }crates/katana/pool/src/tx.rs (1)
156-158: LGTM! Consider adding documentation about fee behaviorThe implementation correctly handles the absence of max_fee in V0 and V3 by returning 0.
Consider adding a doc comment explaining why these versions return 0 and any implications for transaction processing.
crates/katana/primitives/src/block.rs (2)
66-72: LGTM! Consider adding format documentationThe serde aliases and custom deserialization improve compatibility.
Consider adding a doc comment specifying the expected string formats (e.g., "0x"-prefixed hex or decimal).
94-104: Ohayo! Consider using generics to reduce code duplicationThe
deserialize_u64function is nearly identical todeserialize_u128.Consider using a generic implementation:
-pub fn deserialize_u128<'de, D>(deserializer: D) -> Result<u128, D::Error> -where - D: serde::Deserializer<'de>, -{ - let s = String::deserialize(deserializer)?; - if let Some(hex) = s.strip_prefix("0x") { - u128::from_str_radix(hex, 16).map_err(serde::de::Error::custom) - } else { - s.parse::<u128>().map_err(serde::de::Error::custom) - } -} - -pub fn deserialize_u64<'de, D>(deserializer: D) -> Result<u64, D::Error> +pub fn deserialize_number<'de, D, T>(deserializer: D) -> Result<T, D::Error> where D: serde::Deserializer<'de>, + T: std::str::FromStr + From<u8>, + <T as std::str::FromStr>::Err: std::fmt::Display, { let s = String::deserialize(deserializer)?; if let Some(hex) = s.strip_prefix("0x") { - u64::from_str_radix(hex, 16).map_err(serde::de::Error::custom) + T::from_str_radix(hex, 16).map_err(serde::de::Error::custom) } else { - s.parse::<u64>().map_err(serde::de::Error::custom) + s.parse::<T>().map_err(serde::de::Error::custom) } }crates/katana/rpc/rpc-types/src/transaction.rs (1)
Line range hint
579-582: Consider addressing the architectural TODO, senseiThe TODO comment suggests moving away from starknet-rs RPC types for more flexibility. This architectural change could reduce the need for type conversions and improve maintainability.
Consider:
- Defining custom RPC types within Katana
- Implementing direct conversions to/from these types
- Using these types as the primary interface
📜 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 (12)
crates/katana/executor/src/implementation/blockifier/utils.rs(2 hunks)crates/katana/feeder-gateway/Cargo.toml(1 hunks)crates/katana/feeder-gateway/src/client.rs(6 hunks)crates/katana/feeder-gateway/src/types/mod.rs(2 hunks)crates/katana/feeder-gateway/src/types/receipt.rs(1 hunks)crates/katana/feeder-gateway/src/types/transaction.rs(1 hunks)crates/katana/pool/src/tx.rs(4 hunks)crates/katana/primitives/src/block.rs(3 hunks)crates/katana/primitives/src/da/mod.rs(1 hunks)crates/katana/primitives/src/fee.rs(1 hunks)crates/katana/primitives/src/transaction.rs(16 hunks)crates/katana/rpc/rpc-types/src/transaction.rs(2 hunks)
🔇 Additional comments (13)
crates/katana/feeder-gateway/src/types/transaction.rs (1)
1-10: Ohayo, sensei! The initial imports and definitions look solid.
The necessary modules and structs are correctly imported, establishing a good foundation for the transaction types.
crates/katana/primitives/src/transaction.rs (1)
56-56: Ohayo, sensei! Remember to handle the new Deploy variant in all match statements.
With Deploy(DeployTx) added to the Tx enum, ensure all pattern matches are updated to include this variant to prevent runtime issues.
crates/katana/feeder-gateway/Cargo.toml (1)
15-15: Ohayo, sensei! The addition of serde_json dependency is well-placed.
Including serde_json will aid in JSON serialization tasks, enhancing the feeder-gateway's capabilities.
crates/katana/primitives/src/da/mod.rs (1)
22-23:
Ohayo, sensei! Be cautious with changes to DataAvailabilityMode serialization.
Assigning integer values and removing serde renames might alter the serialized form, possibly affecting backward compatibility. Please verify that this modification doesn't disrupt existing data deserialization.
crates/katana/primitives/src/fee.rs (1)
8-12: Ohayo! Verify numeric bounds handling, sensei!
The implementation looks solid with proper type constraints. Let's verify the deserialization handles numeric bounds correctly.
✅ Verification successful
Let me check the actual deserializer implementations to verify the numeric bounds handling.
Ohayo! The numeric bounds are properly handled, sensei!
The deserialization implementation is secure and handles bounds correctly through Rust's native parsing:
- For u128: Uses
u128::from_str_radixfor hex andparse::<u128>()for decimal - For u64: Uses
u64::from_str_radixfor hex andparse::<u64>()for decimal
These native parsing methods automatically enforce type bounds and return errors for any out-of-range values. The implementation supports both hexadecimal (with "0x" prefix) and decimal formats while maintaining type safety.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for numeric bound checks in deserializer implementations
rg -A 5 "deserialize_u(64|128)"
Length of output: 5910
Script:
#!/bin/bash
# Let's examine the deserializer implementations
cat crates/katana/primitives/src/block.rs | grep -A 20 "pub fn deserialize_u"
Length of output: 1164
crates/katana/feeder-gateway/src/types/mod.rs (1)
94-95: Verify transaction and receipt count consistency, sensei!
Ensure that transactions and transaction_receipts vectors maintain consistent lengths and ordering.
crates/katana/pool/src/tx.rs (2)
176-179: LGTM! Consistent handling of tip across versions
The implementation correctly handles the absence of tip in V0 and V1 versions.
139-142: Verify the semantic difference between contract_address and sender_address
The V0 implementation uses contract_address while V1 and V3 use sender_address. Please confirm this architectural difference is intentional.
✅ Verification successful
Ohayo sensei! Based on the search results we've gathered, I can now provide a conclusive response about the architectural difference between V0 and later versions of InvokeTx.
The difference between contract_address and sender_address is intentional and architecturally correct
The implementation correctly reflects the evolution of Starknet's transaction versions:
- V0 uses
contract_addressas it represents the contract being invoked - V1 and V3 use
sender_addressas they represent the account initiating the transaction
This change aligns with Starknet's transition from contract-based transactions (V0) to account abstraction (V1+), where transactions are initiated by account contracts rather than being direct contract calls.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other usages of these fields to verify consistency
rg -A 2 "contract_address.*InvokeTx::V0"
rg -A 2 "sender_address.*InvokeTx::(V1|V3)"
Length of output: 88
Script:
#!/bin/bash
# Let's search for the struct definitions to understand the field differences
ast-grep --pattern 'struct $name {
$$$
}'
# Also search for any usage patterns of these fields
rg -B 2 -A 2 "contract_address|sender_address"
Length of output: 68207
Script:
#!/bin/bash
# Let's search for the InvokeTx struct definitions to understand the field differences
ast-grep --pattern 'enum InvokeTx {
$$$
}'
# Also search for any usage patterns of these specific fields in the context of InvokeTx
rg -B 2 -A 2 "InvokeTx::(V0|V1|V3).*(contract_address|sender_address)"
Length of output: 653
Script:
#!/bin/bash
# Let's look for the struct definitions of V0, V1, and V3 to understand their fields
ast-grep --pattern 'struct $name {
$$$
contract_address: $_,
$$$
}'
ast-grep --pattern 'struct $name {
$$$
sender_address: $_,
$$$
}'
# Also search for any documentation or comments about these versions
rg -B 3 -A 3 "(V0|V1|V3).*struct.*Invoke"
Length of output: 193
crates/katana/feeder-gateway/src/client.rs (2)
60-60: LGTM! Clear and consistent method naming
The rename from with_query_param to add_query_param better reflects the method's purpose.
Also applies to: 72-72, 84-84
121-124: LGTM! Clean builder pattern implementation
The implementation is concise and follows Rust's builder pattern conventions.
crates/katana/primitives/src/block.rs (1)
82-92: LGTM! Robust number parsing implementation
The function properly handles both hexadecimal and decimal formats with appropriate error handling.
crates/katana/rpc/rpc-types/src/transaction.rs (2)
397-406: Deploy transaction conversion implementation looks good, sensei!
The conversion from internal deploy transaction to RPC deploy transaction is properly implemented with all required fields correctly mapped.
267-269:
Ohayo! Critical: Unimplemented InvokeTx::V0 variant will cause runtime panics
The todo!() macro will panic if an InvokeTx::V0 transaction is processed. This needs to be implemented before merging to prevent runtime failures.
| V3(InvokeTxV3), | ||
| } | ||
|
|
There was a problem hiding this comment.
Ohayo, sensei! Add Deploy variant to TxRef enum for completeness.
Including Deploy(&'a DeployTx) in TxRef ensures that references to DeployTx are properly handled across the codebase.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
crates/katana/rpc/rpc-types/src/transaction.rs (3)
Line range hint
44-61: Ohayo sensei! Refactor duplicated code invalidate_compiled_class_hashThe match arms for
BroadcastedDeclareTransaction::V2andBroadcastedDeclareTransaction::V3contain duplicated logic. Refactoring to combine these cases can enhance maintainability and reduce code duplication.Consider refactoring as follows:
BroadcastedDeclareTransaction::V2(tx) | BroadcastedDeclareTransaction::V3(tx) => { let hash = compiled_class_hash_from_flattened_sierra_class(&tx.contract_class)?; hash == tx.compiled_class_hash }
Line range hint
74-77: Ohayo sensei! Address the TODO comment regarding contract class typeThere's a TODO comment suggesting changing the contract class type for the broadcasted transaction to
katana-rpc-typesfor easier conversion. Implementing this change could improve code clarity and ease future maintenance.Would you like assistance in implementing this change or opening a GitHub issue to track this task?
Line range hint
351-354: Ohayo sensei! Handle potential overflow when convertingnonceUsing
expect("nonce should fit in u64")might cause a panic ifnonceexceedsu64limits. To enhance robustness, consider handling this conversion safely.Apply this diff to prevent potential panics:
- nonce: tx.nonce.to_u64().expect("nonce should fit in u64"), + nonce: match tx.nonce.to_u64() { + Some(nonce_u64) => nonce_u64, + None => { + // Handle the error appropriately, perhaps return an error or set a maximum value + }, + },
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
crates/katana/rpc/rpc-types/src/transaction.rs(2 hunks)
🔇 Additional comments (1)
crates/katana/rpc/rpc-types/src/transaction.rs (1)
267-278: Ohayo sensei! Verify if the version field should be included in InvokeTransactionV0
In the conversion to InvokeTransactionV0, the version field is not set. Since other transactions like DeployTransaction include the version, please verify whether it's necessary to include it here for consistency and correctness.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
crates/katana/primitives/src/fee.rs (1)
8-12: Excellent solution for feeder gateway compatibility, sensei!The custom deserialization approach elegantly solves the feeder gateway compatibility issue while maintaining the existing database format. The conditional compilation ensures the serde features are only included when needed.
Consider documenting these custom deserialization choices in a comment block above the struct to help future maintainers understand the rationale behind these specific implementations.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
crates/katana/primitives/src/fee.rs(1 hunks)
🔇 Additional comments (2)
crates/katana/primitives/src/fee.rs (2)
1-1: Ohayo! Clean import for deserialization functions.
The import statement correctly brings in the required deserialization functions from the block module.
15-24: Clean implementation of ResourceBoundsMapping with API compatibility.
The struct correctly implements the feeder gateway API mapping with appropriate aliases.
Let's verify the naming consistency across the codebase:
✅ Verification successful
Field naming conventions are consistent across the codebase
The verification shows that the l1_gas and l2_gas field names in ResourceBoundsMapping follow the established naming patterns in the codebase. The search results confirm:
- The
l1_gasprefix is consistently used across the codebase for L1-related gas fields (e.g., inDataAvailabilityResources,BlockEnv) - The snake_case naming convention is uniformly applied for all gas-related fields
- The struct maintains API compatibility through serde aliases while adhering to Rust naming conventions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check naming conventions for gas-related fields
# Expected: Consistent use of l1_gas/l2_gas vs other variations
# Search for other gas-related field names
rg -g '*.rs' 'gas.*:' ./crates/
Length of output: 16561
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
crates/katana/feeder-gateway/src/types/mod.rs (1)
76-103: Ohayo, sensei! The Block struct implementation looks solid.The struct is well-structured with appropriate use of optional fields and good field organization. Consider adding documentation for each field to improve maintainability.
Add documentation comments for each field, for example:
pub struct Block { + /// The unique identifier of the block #[serde(default)] pub block_hash: Option<BlockHash>, + /// The sequential number of the block in the chain #[serde(default)] pub block_number: Option<BlockNumber>,
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
crates/katana/feeder-gateway/src/types/mod.rs(2 hunks)crates/katana/primitives/src/block.rs(0 hunks)crates/katana/primitives/src/fee.rs(1 hunks)crates/katana/primitives/src/transaction.rs(11 hunks)
💤 Files with no reviewable changes (1)
- crates/katana/primitives/src/block.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/katana/primitives/src/fee.rs
🔇 Additional comments (3)
crates/katana/primitives/src/transaction.rs (3)
243-246: Ohayo, sensei! The hash calculation for InvokeTx::V0 is still missing.
The todo!() implementation will cause a panic if invoked. Please implement the hash calculation for V0 transactions.
567-582: Ohayo, sensei! Add Deploy variant to TxRef enum for consistency.
The DeployTx struct is well-implemented, but it needs to be added to the TxRef enum to maintain consistency across the codebase.
187-187: Ohayo, sensei! Chain ID implementation looks great!
The consistent addition of chain_id with #[serde(default)] across all transaction types provides good replay attack protection while maintaining backward compatibility.
Also applies to: 209-209, 301-301, 324-324, 349-349, 426-426, 480-480, 506-506
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2744 +/- ##
==========================================
- Coverage 56.35% 56.10% -0.25%
==========================================
Files 425 427 +2
Lines 54269 54516 +247
==========================================
+ Hits 30583 30586 +3
- Misses 23686 23930 +244 ☔ View full report in Codecov by Sentry. |
ref #639
initially, i wanted to deserialize the transaction data from the fgw client directly into the primitive transaction types. but because on v3 transaction, the type
DataAvailabilityMode(ienonce_data_availability_modeandfee_data_availability_modefields) has different serde implementation than what is expected from the fgw client. changing the serde impl to match the client would require breaking the database format which should never be taken too lightly.im pretty sure its possible to change the serde impl of
DataAvailabilityModeto both (1) maintain the impl expected by the current db format, and (2) deserialize from the fgw impl. but havent gone too deeply into this yet and might require a relatively complexDeserializeimpl. so i chose the easiest path for now.Summary by CodeRabbit
Release Notes
New Features
InvokeTx::V0, enhancing transaction handling capabilities.ConfirmedReceiptandConfirmedTransaction.Blockstruct, providing detailed block information.EntryPointSelectorto improve transaction handling flexibility.Bug Fixes
Documentation
with_query_paramtoadd_query_param.Refactor