refactor(katana): include protocol version in chainspec #2543
Conversation
WalkthroughOhayo, sensei! This pull request introduces significant updates to the handling of versioning across various components of the Katana project. It replaces hardcoded version constants with dynamic retrieval from the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
🪧 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: 4
🧹 Outside diff range and nitpick comments (6)
crates/katana/core/src/backend/mod.rs (1)
68-68: Ohayo, sensei! LGTM with a small suggestion.The change to use
self.chain_spec.version.clone()instead of a hardcoded constant is a great improvement. It aligns well with the PR objectives and makes the system more flexible.Consider caching the cloned version at the struct level to avoid repeated cloning in every
do_mine_blockcall. This could be done in theBackendconstructor:pub struct Backend<EF: ExecutorFactory> { // ... other fields ... cached_version: StarknetVersion, } impl<EF: ExecutorFactory> Backend<EF> { pub fn new(/* ... */) -> Self { let cached_version = chain_spec.version.clone(); Self { // ... other fields ... cached_version, } } }Then in
do_mine_block, you could use:version: self.cached_version.clone(),This would reduce the number of clones performed during block mining.
crates/katana/executor/tests/fixtures/mod.rs (1)
91-91: Excellent move, sensei! Embracing the dynamic version!Your kung fu is strong! Replacing the hardcoded version with
CURRENT_STARKNET_VERSIONis a masterful technique. It aligns perfectly with our goal of using the chainspec-specified version.One small suggestion to level up your code:
Consider adding a comment explaining the significance of using
CURRENT_STARKNET_VERSIONhere. It could help future ninjas understand the importance of this change. Something like:// Use the current Starknet version to ensure test fixtures are always up-to-date let version = CURRENT_STARKNET_VERSION;What do you think, sensei? Shall we add this little nugget of wisdom?
crates/katana/node/src/lib.rs (1)
208-209: Ohayo, sensei! Nice update to use the forked network's version.The change to set
config.chain.versionusing the parsedstarknet_versionfrom the forked block is a great improvement. It ensures that the node operates with the correct version context when working with a forked network.Consider adding a debug log statement after parsing the version. This could help with troubleshooting if there are any issues with version compatibility in the future. Here's a suggestion:
config.chain.version = Version::parse(&block.starknet_version)?; tracing::debug!(parsed_version = ?config.chain.version, "Parsed Starknet version from forked block");This will provide valuable information during development and debugging without affecting production performance.
crates/katana/rpc/rpc/src/starknet/read.rs (2)
89-89: Ohayo, sensei! Nice dynamic versioning!The change to use
this.inner.backend.chain_spec.version.clone()for theversionfield is a good improvement. It makes the code more flexible by using the version from the chain specification instead of a hardcoded value.Consider caching the chain spec version if it's accessed frequently to avoid repeated cloning. For example:
let version = Arc::new(this.inner.backend.chain_spec.version.clone()); // Then use version.clone() which will be cheaper than cloning the original each timeThis optimization might be beneficial if this method is called often.
233-233: Ohayo! Consistency hat-trick achieved, sensei!Great job on maintaining consistency across all three methods. The use of
this.inner.backend.chain_spec.version.clone()here completes the trifecta of consistent version handling.To reduce code duplication, consider extracting the
PartialHeadercreation into a separate method:impl<EF: ExecutorFactory> StarknetApi<EF> { fn create_partial_header(&self, block_env: &BlockEnv, parent_hash: Felt) -> PartialHeader { PartialHeader { number: block_env.number, gas_prices: block_env.l1_gas_prices.clone(), parent_hash, timestamp: block_env.timestamp, version: self.inner.backend.chain_spec.version.clone(), sequencer_address: block_env.sequencer_address, } } }Then you can use this method in all three places:
let header = this.create_partial_header(&block_env, latest_hash);This refactoring will make the code more DRY and easier to maintain.
crates/katana/primitives/src/version.rs (1)
85-87: Ohayo sensei! Add a test case for version segments exceedingu8maximum valueConsider adding a test to verify that parsing fails when version segments exceed 255, ensuring the error handling is correct for overflow cases.
Here's an example test:
#[test] fn parse_version_segment_overflow_should_fail() { let version = "1.256.0.0"; assert!(Version::parse(version).is_err()); }
📜 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 (8)
- crates/katana/core/src/backend/mod.rs (1 hunks)
- crates/katana/core/src/service/block_producer.rs (1 hunks)
- crates/katana/executor/tests/fixtures/mod.rs (4 hunks)
- crates/katana/node/src/lib.rs (2 hunks)
- crates/katana/primitives/Cargo.toml (1 hunks)
- crates/katana/primitives/src/chain_spec.rs (5 hunks)
- crates/katana/primitives/src/version.rs (1 hunks)
- crates/katana/rpc/rpc/src/starknet/read.rs (3 hunks)
🧰 Additional context used
🔇 Additional comments (11)
crates/katana/executor/tests/fixtures/mod.rs (3)
24-24: Ohayo, sensei! Nice update to the versioning import!This change aligns perfectly with our mission to refactor the version struct for the Starknet protocol. Using
CURRENT_STARKNET_VERSIONis a wise move, making it easier to manage and update versions across the project. Dojo approves!
105-105: Ohayo, sensei! A subtle yet powerful change!Your attention to detail is impressive! By using
version.clone()instead of justversion, you've ensured that each block in our test dojo has its own copy of the version. This is a wise move, young grasshopper!This change:
- Prevents potential issues with shared mutable state across blocks.
- Ensures thread safety if these tests are run in parallel.
- Follows best practices for dealing with shared data in complex scenarios.
Your code fu is strong, and this change adds an extra layer of robustness to our tests. Dojo approves!
Line range hint
1-280: Ohayo, sensei! Your code-fu has impressed the entire dojo!You've successfully implemented the versioning updates in our test fixtures, aligning perfectly with our PR objectives. Here's a summary of your achievements:
- Updated the import to use
CURRENT_STARKNET_VERSION.- Replaced hardcoded version with the dynamic
CURRENT_STARKNET_VERSION.- Ensured each block has its own copy of the version.
These changes enhance the maintainability and flexibility of our test fixtures, making them more robust and future-proof. The dojo elders nod in approval!
One small suggestion to consider: adding a comment explaining the use of
CURRENT_STARKNET_VERSIONin thevalid_blocksfunction. This would help future ninjas understand the significance of this change.Overall, excellent work, sensei! Your contributions make our dojo stronger. 🥋✨
crates/katana/rpc/rpc/src/starknet/read.rs (1)
178-178: Consistency is key, sensei!Excellent work on maintaining consistency with the previous change. Using
this.inner.backend.chain_spec.version.clone()here as well ensures that theversionfield is set consistently across different methods.This consistent approach will make the code easier to maintain and understand in the future.
crates/katana/primitives/src/chain_spec.rs (5)
36-37: Ohayo, sensei! LGTM: Version field added to ChainSpecThe addition of the
versionfield to theChainSpecstruct aligns perfectly with the PR objectives. This change enhances the flexibility of version handling within the project, allowing for better management of protocol versions.
54-54: Ohayo, sensei! LGTM: Dynamic version in block creationThe update to use
self.versioninstead ofCURRENT_STARKNET_VERSIONin theblockmethod is a great improvement. This change ensures that the version specified in the chainspec is used when creating blocks, aligning with the PR objectives and enhancing flexibility.
160-160: Ohayo, sensei! LGTM: Version included in DEV_UNALLOCATEDThe update to include the
versionfield in theDEV_UNALLOCATEDstatic reference is spot on. This change ensures that the default chain specification includes the version information, maintaining consistency with theChainSpecstruct modifications.
319-319: Ohayo, sensei! LGTM: Test updated with version fieldThe update to include the
versionfield in thegenesis_block_and_state_updatestest function is crucial. This change ensures that the test case covers the newversionfield in theChainSpec, maintaining test coverage and accuracy. Well done on keeping the tests up-to-date!
36-37: Ohayo, sensei! Great job on version integrationThe changes made to include the protocol version in the chainspec are well-implemented and consistent throughout the file. The modifications to the
ChainSpecstruct,blockmethod,DEV_UNALLOCATEDstatic reference, and test function all work together seamlessly to improve version handling and flexibility in the project. These changes align perfectly with the PR objectives and enhance the overall architecture of the system. Keep up the excellent work!Also applies to: 54-54, 160-160, 319-319
crates/katana/primitives/src/version.rs (1)
2-2: Ohayo sensei! Good use ofconstforCURRENT_STARKNET_VERSIONChanging from
statictoconstis appropriate here asconstis more efficient for immutable data known at compile time.crates/katana/core/src/service/block_producer.rs (1)
583-583: Ohayo, sensei! Successfully updated block version sourcingGreat job updating the block header's
versionfield to usebackend.chain_spec.version.clone(). This ensures that the block version dynamically aligns with the chain specification, enhancing flexibility and consistency with the network configuration as per the PR objectives.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
examples/rpc/starknet/starknet_getBlockWithTxs.hurl (1)
19-19: Ohayo again, sensei! Consider adding an additional test case.While this change correctly validates the new version format, it might be beneficial to add another test case that verifies the version returned matches the one specified in the chainspec. This would ensure that the version from the chainspec is indeed being used when creating blocks, as mentioned in the PR objectives.
Would you like assistance in crafting this additional test case, sensei?
crates/katana/primitives/src/version.rs (1)
88-144: Ohayo, sensei! The test suite is looking comprehensive!The updated tests cover the new four-segment version format and the added serialization/deserialization functionality well. They include both valid and invalid cases, ensuring robust error handling.
To make the test suite even more robust:
Consider adding a test for the case where the last segment is zero and should be omitted in the string representation:
#[test] fn display_omits_last_zero() { let version = Version::new([1, 2, 3, 0]); assert_eq!(version.to_string(), "1.2.3"); }This additional test will ensure that the
Displayimplementation correctly handles the case of omitting the last zero segment.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- crates/katana/primitives/src/version.rs (1 hunks)
- examples/rpc/starknet/starknet_getBlockWithReceipts.hurl (1 hunks)
- examples/rpc/starknet/starknet_getBlockWithTxHashes.hurl (1 hunks)
- examples/rpc/starknet/starknet_getBlockWithTxs.hurl (1 hunks)
🧰 Additional context used
🔇 Additional comments (8)
examples/rpc/starknet/starknet_getBlockWithTxs.hurl (1)
19-19: Ohayo, sensei! This change looks great!The updated regex pattern now supports both
major.minor.patchandmajor.minor.patch.buildversion formats. This aligns perfectly with the PR objectives of accommodating the Starknet protocol version format in the chainspec.examples/rpc/starknet/starknet_getBlockWithTxHashes.hurl (1)
21-21: Ohayo, sensei! This change looks great!The updated regex pattern for
starknet_versionvalidation is a smart move. It now accommodates bothX.Y.ZandX.Y.Z.Wversion formats, aligning perfectly with the Starknet protocol version requirements. This flexibility ensures we can handle current and future version formats without breaking our tests.examples/rpc/starknet/starknet_getBlockWithReceipts.hurl (1)
19-19: Ohayo, sensei! This change looks great!The updated regex pattern for
starknet_versionnow accommodates both three-segment and four-segment version numbers. This aligns perfectly with our PR objectives, allowing for more flexible version formats, including the Starknet protocol version format.The new pattern
/^[0-9]+\.[0-9]+\.[0-9]+(\.[0-9]+)?$/ensures:
- At least three version segments (major.minor.patch) are required.
- An optional fourth segment is allowed.
- Dots in the regex are properly escaped.
This change enhances our version validation, making it more robust and future-proof.
crates/katana/primitives/src/version.rs (5)
2-2: Ohayo, sensei! The version update looks great!The
CURRENT_STARKNET_VERSIONconstant has been correctly updated to use the new four-segment version format, aligning with the Starknet protocol version 0.13.1.1. This change addresses the versioning issue mentioned in the linked issue #2539.
11-13: Ohayo, sensei! The new constructor looks sharp!The
Version::newmethod has been updated to accept an array of fouru8values, which is consistent with the new struct definition. This change provides a more compact and precise representation of version numbers.
29-33: Ohayo, sensei! The default version looks good!The
Defaultimplementation forVersionhas been correctly updated to use the new four-segment format[0, 1, 0, 0]. This change is consistent with the new version structure and provides a sensible default value.
61-84: Ohayo, sensei! The serde implementation is looking sharp!The new serialization and deserialization implementations for
Versionare well-designed and provide excellent flexibility:
- They handle both human-readable and non-human-readable formats.
- For human-readable format, they use the
Displayimplementation, ensuring consistency with the string representation.- For non-human-readable format, they directly serialize/deserialize the
segmentsarray, which is efficient.This implementation will make it easy to use the
Versionstruct in various serialization contexts.
1-144: Ohayo, sensei! Overall, this is a solid implementation of the new versioning system!The changes in this file successfully implement the new four-segment version format and address the versioning issues mentioned in the PR objectives. Here's a summary of the key points:
- The
Versionstruct and related methods have been updated to handle the new format.- Error handling has been improved in the
parsemethod.- The
Displayimplementation cleverly handles the omission of trailing zeros.- Serialization and deserialization support has been added with flexibility for different formats.
- The test suite has been updated to cover the new functionality.
There are a few suggestions for further improvement:
- Consider using
u16instead ofu8for future-proofing.- The
parsemethod could be made more robust to handle potential overflow.- The
Displayimplementation could be simplified for better readability.- An additional test case for omitting trailing zeros in the display could be added.
Overall, great work on implementing this new versioning system!
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
crates/katana/primitives/src/version.rs (2)
20-38: Ohayo, sensei! TheProtocolVersionimplementation looks solid!The
newandparsemethods have been well-adapted to the new four-segment format. The error handling in theparsemethod is particularly good. However, we can make a small optimization to avoid an unnecessary allocation:Consider replacing the
segmentsparsing with:let segments: [u8; 4] = version .split('.') .map(|s| s.parse::<u8>()) .collect::<Result<Vec<_>, _>>()? .try_into() .map_err(|_| ParseVersionError::InvalidFormat)?; Ok(Self { segments })This approach avoids creating an intermediate
Vecand directly creates the fixed-size array.
47-70: Ohayo, sensei! TheDisplayimplementation is working well, but we can make it more elegant!The current implementation correctly formats the version and handles the requirement to omit the last zero segment. However, we can simplify it for better readability:
Consider this simplified implementation:
impl core::fmt::Display for ProtocolVersion { fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { let mut segments = self.segments.iter().map(u8::to_string); if let Some(first) = segments.next() { write!(f, "{}", first)?; for segment in segments.take(2) { write!(f, ".{}", segment)?; } if self.segments[3] != 0 { write!(f, ".{}", self.segments[3])?; } } Ok(()) } }This version is more concise and easier to understand at a glance.
crates/katana/primitives/src/block.rs (1)
Line range hint
89-99: Ohayo, sensei! Consider updating compute_hash method.The
compute_hashmethod still usesFelt::ZEROfor the protocol version. Consider updating this to use the newversionfield for more accurate hash computation.Here's a suggested change:
pub fn compute_hash(&self) -> Felt { compute_hash_on_elements(&vec![ self.number.into(), // block number self.state_root, // state root self.sequencer_address.into(), // sequencer address self.timestamp.into(), // block timestamp Felt::ZERO, // transaction commitment Felt::ZERO, // event commitment - Felt::ZERO, // protocol version + self.version.into(), // protocol version Felt::ZERO, // extra data self.parent_hash, // parent hash ]) }This change assumes that
ProtocolVersioncan be converted intoFelt. If this is not the case, you may need to implement the necessary conversion.crates/saya/provider/src/rpc/mod.rs (1)
103-103: Ohayo again, sensei! The version parsing looks good, but let's level up our error handling!The change to
ProtocolVersionis spot on and aligns perfectly with our refactoring goals. However, the use ofunwrap()might lead to unexpected panics if the parsing fails. Consider using a more graceful error handling approach, like this:version: ProtocolVersion::parse(&block.starknet_version) .map_err(|e| anyhow!("Failed to parse protocol version: {}", e))?,This way, we'll get a more informative error message and avoid potential runtime panics. What do you think, sensei?
crates/katana/primitives/src/chain_spec.rs (1)
319-319: Ohayo, sensei! Great test update, but let's make it even betterThe inclusion of the
versionfield in thegenesis_block_and_state_updatestest function is excellent. It ensures that the test case remains consistent with the updatedChainSpecstruct.However, to make this test even more robust, consider adding an explicit assertion to verify that the
versionin the generated block matches the expected version. This will help catch any potential issues with version handling in the future.Here's a suggested addition to the test:
assert_eq!(actual_block.header.gas_prices, expected_block.header.gas_prices); assert_eq!(actual_block.header.version, expected_block.header.version); +assert_eq!(actual_block.header.version, CURRENT_STARKNET_VERSION, "Block version should match the expected version"); assert_eq!(actual_block.body, expected_block.body);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
- crates/katana/node/src/lib.rs (2 hunks)
- crates/katana/primitives/src/block.rs (3 hunks)
- crates/katana/primitives/src/chain_spec.rs (5 hunks)
- crates/katana/primitives/src/version.rs (1 hunks)
- crates/saya/provider/src/rpc/mod.rs (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/katana/node/src/lib.rs
🧰 Additional context used
🔇 Additional comments (14)
crates/katana/primitives/src/version.rs (6)
2-2: Ohayo, sensei! The version update looks good!The
CURRENT_STARKNET_VERSIONconstant has been successfully updated to use the newProtocolVersionstruct with a four-segment format. The version number has been changed to 0.13.1.1, which might be a significant update for the project.
6-10: Ohayo, sensei! Great improvement on theProtocolVersionstruct!The new
ProtocolVersionstruct using[u8; 4]for segments is a solid improvement. It addresses the previous concerns about supporting larger version numbers while maintaining a compact representation. The derived traits (Debug, Clone, PartialEq, Eq) are appropriate for this type of struct.
12-18: Ohayo, sensei! Excellent error handling implementation!The new
ParseVersionErrorenum provides specific and descriptive error types for version parsing. Using thethiserrorcrate for deriving the Error trait is a great practice that simplifies error handling and improves the overall robustness of the code.
41-45: Ohayo, sensei! TheDefaultimplementation looks good!The default version of [0, 1, 0, 0] is a reasonable starting point for the
ProtocolVersion. The implementation is simple and correct.
73-78: Ohayo, sensei! TheTryFrom<String>implementation is spot on!This implementation provides a convenient way to convert strings to
ProtocolVersionusing thetry_frommethod. It's correct and straightforward, leveraging the existingparsemethod.
80-157: Ohayo, sensei! Excellent work on serialization and testing!The added serde implementation for
ProtocolVersionis well-thought-out:
- It uses a human-readable string format for serialization, which is a good choice for maintaining compatibility if the internal representation changes.
- The deserialization leverages the existing
parsemethod, ensuring consistency.The new tests are particularly valuable:
- They cover both human-readable (JSON) and non-human-readable (postcard) formats.
- The round-trip tests ensure that serialization and deserialization work correctly together.
These additions significantly improve the robustness and usability of the
ProtocolVersionstruct.crates/katana/primitives/src/block.rs (3)
5-5: Ohayo, sensei! LGTM: Import for ProtocolVersion added.The import for
ProtocolVersionhas been correctly added, which is necessary for the version type changes in the structs.
40-40: Ohayo, sensei! LGTM: PartialHeader version field updated.The
versionfield in thePartialHeaderstruct has been successfully updated to useProtocolVersion. This change aligns with the PR objective of refactoring the version struct to accommodate the Starknet protocol version format.
71-71: Ohayo, sensei! LGTM: Header version field updated.The
versionfield in theHeaderstruct has been successfully updated to useProtocolVersion. This change is consistent with the modification in thePartialHeaderstruct and aligns with the PR objective.crates/saya/provider/src/rpc/mod.rs (2)
15-15: Ohayo, sensei! LGTM on the import change.The replacement of
VersionwithProtocolVersionaligns well with our objective to refactor the version struct for Starknet protocol compatibility. Nice work on maintaining consistency!
Line range hint
1-170: Ohayo one last time, sensei! Let's wrap this up!Overall, the changes in this file are solid and align perfectly with our PR objectives. You've successfully refactored the version handling to accommodate the Starknet protocol version format. The consistency in using
ProtocolVersionthroughout the file is commendable.Just remember to consider the error handling suggestion for the
parsemethod to make our code even more robust. Keep up the great work, sensei!crates/katana/primitives/src/chain_spec.rs (3)
36-37: Ohayo, sensei! LGTM: Version field added to ChainSpecThe addition of the
versionfield to theChainSpecstruct is a great improvement. It aligns perfectly with the PR objectives and enhances the flexibility of version handling in the chainspec.
54-54: Excellent update to block method, sensei!The modification to use
self.versioninstead ofCURRENT_STARKNET_VERSIONin theblockmethod is spot on. It ensures that the version specified in the chainspec is used when creating blocks, aligning perfectly with the PR objectives.
160-160: Ohayo! Nice update to DEV_UNALLOCATED, senseiThe inclusion of the
versionfield in theDEV_UNALLOCATEDstatic reference is a great touch. It ensures that the default chain specification is consistent with the updatedChainSpecstruct, maintaining the integrity of the version information throughout the codebase.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
crates/katana/storage/db/src/version.rs (1)
84-84: Excellent work updating the test, sensei!The
test_current_versionfunction has been correctly updated to assert thatCURRENT_DB_VERSIONequals 2. This change maintains the consistency between the constant and its test.Consider adding an additional test to verify the behavior of the
check_db_versionfunction with both matching and mismatching versions. This would enhance our test coverage and ensure the version checking logic works as expected.crates/katana/primitives/src/version.rs (4)
6-10: Ohayo, sensei! LGTM with a suggestion: ProtocolVersion structThe new
ProtocolVersionstruct withsegments: [u8; 4]is a good improvement, allowing for more flexible version representation. The derived traits (Debug, Clone, PartialEq, Eq) are appropriate for this struct.However, consider using
u16instead ofu8for future-proofing:pub struct ProtocolVersion { segments: [u16; 4], }This would allow for larger version numbers without breaking the current functionality.
20-41: Ohayo, sensei! LGTM with a suggestion: ProtocolVersion implementationThe
newandparsemethods are well-implemented. Theparsemethod correctly handles version strings with up to 4 segments, filling missing segments with zeros.To make the
parsemethod more robust, consider this modification:pub fn parse(version: &str) -> Result<Self, ParseVersionError> { let segments: Result<Vec<u8>, _> = version.split('.') .take(4) .map(|s| s.parse::<u8>()) .collect(); match segments { Ok(mut segments) => { segments.resize(4, 0); Ok(Self::new([segments[0], segments[1], segments[2], segments[3]])) } Err(e) => Err(ParseVersionError::ParseSegment(e)), } }This version simplifies the logic and ensures that exactly 4 segments are always used.
56-73: Ohayo, sensei! LGTM with a suggestion: Display implementation for ProtocolVersionThe
Displayimplementation correctly formats the version as a string, omitting the last segment if it's zero. However, we can simplify this implementation using iterators:impl core::fmt::Display for ProtocolVersion { fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { let mut segments = self.segments.iter().map(u8::to_string); if let Some(first) = segments.next() { write!(f, "{}", first)?; for segment in segments.take(2) { write!(f, ".{}", segment)?; } if self.segments[3] != 0 { write!(f, ".{}", self.segments[3])?; } } Ok(()) } }This version is more concise and potentially more efficient.
82-159: Ohayo, sensei! LGTM with a suggestion: Serde implementation and testsThe serde implementation for
ProtocolVersionis well-done, using a string representation for serialization. This approach prevents breaking the database encoding format if the memory representation changes. The tests cover both human-readable (JSON) and non-human-readable (postcard) serialization, which is great.Consider adding a test for deserializing an invalid version string:
#[test] fn deserialize_invalid_version() { let result: Result<ProtocolVersion, _> = serde_json::from_str("\"invalid.version\""); assert!(result.is_err()); }This test would ensure that the deserialization correctly handles invalid input.
crates/katana/rpc/rpc-types/src/block.rs (1)
Line range hint
262-262: Ohayo, sensei! Heads up: Missed protocol version update.It looks like we missed updating the
starknet_versioninPendingBlockWithReceipts. To keep everything consistent and avoid any version mismatches with pending blocks, we should update this line too.Here's the change we need to make:
- starknet_version: header.version.to_string(), + starknet_version: header.protocol_version.to_string(),This will ensure that pending blocks also use the new protocol version, maintaining consistency across all block types. What do you think, sensei?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (2)
spawn-and-move-db.tar.gzis excluded by!**/*.gztypes-test-db.tar.gzis excluded by!**/*.gz
📒 Files selected for processing (6)
- crates/katana/primitives/src/block.rs (3 hunks)
- crates/katana/primitives/src/chain_spec.rs (7 hunks)
- crates/katana/primitives/src/version.rs (1 hunks)
- crates/katana/rpc/rpc-types/src/block.rs (3 hunks)
- crates/katana/storage/db/src/version.rs (2 hunks)
- crates/saya/provider/src/rpc/mod.rs (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/katana/primitives/src/block.rs
- crates/saya/provider/src/rpc/mod.rs
🧰 Additional context used
🔇 Additional comments (13)
crates/katana/storage/db/src/version.rs (2)
8-8: Ohayo, sensei! The database version update looks good!The increment of
CURRENT_DB_VERSIONfrom 1 to 2 aligns perfectly with our PR objectives. This change ensures that we're not backward compatible with older databases, which is crucial given the serialization implementation changes.
Line range hint
1-86: Ohayo, sensei! Overall, these changes look solid!The update to
CURRENT_DB_VERSIONand its corresponding test successfully implements the required database version increment. This change is crucial for maintaining consistency with the new serialization implementation mentioned in the PR objectives.Key points:
- The database version has been incremented from 1 to 2.
- The test case has been updated to reflect this change.
- These modifications ensure that the new version is not backward compatible with older databases, as intended.
The changes are minimal and focused, which reduces the risk of unintended side effects. Great job on maintaining code quality and consistency!
crates/katana/primitives/src/version.rs (4)
2-2: Ohayo, sensei! LGTM: Updated CURRENT_STARKNET_VERSIONThe constant has been correctly updated to use the new
ProtocolVersionstruct with the version [0, 13, 1, 1]. This change aligns well with the PR objectives of including the protocol version in the chainspec.
12-18: Ohayo, sensei! LGTM: ParseVersionError enumThe new
ParseVersionErrorenum is a great addition for handling parsing errors. The use ofthiserrorfor deriving theErrortrait simplifies error handling. The two variants,InvalidFormatandParseSegment, provide clear and specific error types for parsing failures.
43-47: Ohayo, sensei! LGTM: Default implementation for ProtocolVersionThe
Defaultimplementation forProtocolVersionis correct and sets an appropriate default version of [0, 1, 0, 0]. This provides a sensible starting point for new instances ofProtocolVersion.
75-79: Ohayo, sensei! LGTM: TryFrom implementation for ProtocolVersionThe
TryFrom<String>implementation forProtocolVersionis well-done. It provides a convenient way to convert strings toProtocolVersioninstances, leveraging theparsemethod. This addition enhances the usability of theProtocolVersionstruct.crates/katana/rpc/rpc-types/src/block.rs (3)
38-38: Ohayo, sensei! LGTM: Aligned with protocol version update.The change from
versiontoprotocol_versionis spot on and aligns perfectly with our PR objectives. It ensures we're using the correct versioning information from the chainspec. Nice work!
115-115: Ohayo again, sensei! LGTM: Consistent protocol version usage.Excellent job maintaining consistency! The change to
protocol_versionhere mirrors the update inBlockWithTxs, ensuring a uniform approach across different block representations. Keep up the good work!
222-222: Ohayo once more, sensei! LGTM: Protocol version consistency maintained.Great job keeping the consistency train rolling! The
protocol_versionupdate here inBlockWithReceiptsis in perfect harmony with the changes in other structs. You're nailing this versioning update!crates/katana/primitives/src/chain_spec.rs (4)
36-37: Ohayo, sensei! LGTM: Addition ofversionfield toChainSpecThe addition of the
versionfield of typeProtocolVersionto theChainSpecstruct is a great improvement. This change aligns perfectly with the PR objectives and allows for better versioning control across different protocol versions.
54-54: Ohayo, sensei! LGTM: Updatedblockmethod to useself.versionThe modification of the
blockmethod to useself.versioninstead of the hardcodedCURRENT_STARKNET_VERSIONis a great improvement. This change ensures consistency between the block's protocol version and the chainspec's version, making the implementation more flexible and maintainable.
160-160: Ohayo, sensei! LGTM: UpdatedDEV_UNALLOCATEDto includeversionThe update to the
DEV_UNALLOCATEDstatic reference to include theversionfield initialized withCURRENT_STARKNET_VERSIONis spot on. This change ensures consistency with the updatedChainSpecstruct and provides a proper default for the unallocated dev chain specification.
319-319: Ohayo, sensei! LGTM: Updated test function for protocol versionThe updates to the
genesis_block_and_state_updatestest function are on point! TheChainSpecinitialization now includes theversionfield, and the assertion for the block's protocol version has been properly updated. These changes ensure that the test case remains consistent with the modifications made to theChainSpecstruct andblockmethod.Also applies to: 362-362
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
crates/katana/primitives/src/version.rs (4)
2-2: Ohayo, sensei! Great update to the CURRENT_STARKNET_VERSION constantThe change to use the new
ProtocolVersionstruct is spot on. It's good to see the version number updated to0.13.1.1.Consider adding a comment explaining the significance of this version number or linking to relevant documentation about Starknet protocol versions. This could help future maintainers understand the context of version changes.
6-10: Ohayo, sensei! The new ProtocolVersion struct looks solidThe change to use an array of 4
u8values for version segments is a good improvement. It provides more flexibility and aligns well with the Starknet protocol version format.Consider adding a brief doc comment to the
segmentsfield explaining the meaning of each segment (e.g., major, minor, patch, build). This would enhance the struct's self-documentation.
25-44: Ohayo, sensei! The parse method is looking sharpThe
parsemethod is well-implemented, handling various input scenarios and providing robust error handling. It's great to see the method filling missing segments with zeros and validating the input format.Consider a small optimization: instead of collecting segments into a
Vec, you could usetake(4)on the iterator to limit the number of segments processed. This would avoid allocating a vector and simplify the length check. Here's a suggested implementation:pub fn parse(version: &str) -> Result<Self, ParseVersionError> { if version.is_empty() { return Err(ParseVersionError::InvalidFormat); } let mut buffer = [0u8; 4]; let mut segments = version.split('.').take(4); for (i, seg) in segments.enumerate() { buffer[i] = seg.parse()?; } if segments.next().is_some() { return Err(ParseVersionError::InvalidFormat); } Ok(Self::new(buffer)) }This approach would be more efficient for inputs with more than 4 segments.
108-175: Ohayo, sensei! The test suite is looking robustThe updated test cases provide excellent coverage for the new
ProtocolVersionstruct and its associated methods. It's great to see tests for various parsing scenarios, including edge cases and error conditions.The addition of serde tests for both human-readable and binary formats is a valuable inclusion, ensuring that serialization and deserialization work correctly.
Consider adding a test case for parsing a version string with leading zeros in some segments (e.g., "1.02.03.04"). This would verify that the parsing logic handles such inputs correctly, either by preserving or trimming the leading zeros.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (2)
spawn-and-move-db.tar.gzis excluded by!**/*.gztypes-test-db.tar.gzis excluded by!**/*.gz
📒 Files selected for processing (1)
- crates/katana/primitives/src/version.rs (1 hunks)
🧰 Additional context used
🔇 Additional comments (2)
crates/katana/primitives/src/version.rs (2)
12-18: Ohayo, sensei! Excellent error handling with ParseVersionErrorThe introduction of the
ParseVersionErrorenum with specific error types for invalid format and segment parsing issues is a great addition. Usingthiserrorfor deriving theErrortrait is a nice touch, simplifying error handling and providing clear error messages.This improvement will make debugging and error reporting much more straightforward. Good job!
86-103: Ohayo, sensei! The serde implementation is on pointThe serde implementation for
ProtocolVersionis well thought out. Serializing and deserializing the version as a string is a smart choice, as it ensures compatibility even if the internal representation changes in the future.Using the
Displayandparsemethods for conversion maintains consistency with the human-readable format used elsewhere in the code. The conditional compilation with theserdefeature is also a nice touch, keeping the serialization code separate when not needed.
| impl core::fmt::Display for ProtocolVersion { | ||
| fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { | ||
| for (idx, segment) in self.segments.iter().enumerate() { | ||
| // If it's the last segment, don't print it if it's zero. | ||
| if idx == self.segments.len() - 1 { | ||
| if *segment != 0 { | ||
| write!(f, ".{segment}")?; | ||
| } | ||
| } else if idx == 0 { | ||
| write!(f, "{segment}")?; | ||
| } else { | ||
| write!(f, ".{segment}")?; | ||
| } | ||
| } | ||
|
|
||
| Ok(Self::new(major, minor, patch)) | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo, sensei! The Display implementation works well, but we can make it more elegant
The current implementation correctly formats the version string and handles the requirement to omit the last zero segment. However, we can simplify it for better readability and maintainability.
Consider this more concise implementation:
impl core::fmt::Display for ProtocolVersion {
fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
let mut segments = self.segments.iter().map(u8::to_string);
if let Some(first) = segments.next() {
write!(f, "{}", first)?;
for segment in segments.take(2) {
write!(f, ".{}", segment)?;
}
if self.segments[3] != 0 {
write!(f, ".{}", self.segments[3])?;
}
}
Ok(())
}
}This implementation achieves the same result with less code and fewer conditional checks, making it easier to read and maintain.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2543 +/- ##
==========================================
+ Coverage 69.51% 69.53% +0.02%
==========================================
Files 388 388
Lines 49868 49907 +39
==========================================
+ Hits 34665 34705 +40
+ Misses 15203 15202 -1 ☔ View full report in Codecov by Sentry. |
the values in the new fields are mostly dumb values rn bcs we arent sampling anything from the L1. so the next todo to make these changes correct, is to create a gas oracle that samples the actual values from the l1. similar to PR #2543, the database format for `Header` is changed. considering we already bumped the db version in PR #2543, we can keep using the same version and defer bumping it again once this commit is included in a release.
partially resolves #2539
ProtocolVersionto better reflect its intended usageIMPORTANT
because its serde implementation is changed, its not backward compatible with database created before this change. thus the database version has to be incremented.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores