Add RuntimeVariableList and RuntimeFixedVector#67
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #67 +/- ##
==========================================
- Coverage 57.10% 50.00% -7.11%
==========================================
Files 10 13 +3
Lines 345 462 +117
==========================================
+ Hits 197 231 +34
- Misses 148 231 +83 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
macladson
force-pushed
the
add-runtime-types
branch
from
d825a64 to
83b3516
Compare
macladson
left a comment
macladson
left a comment
There was a problem hiding this comment.
This is ready now. Although we may want to wait for the context_deserialize addition so we can combine the implementation locations
macladson
force-pushed
the
add-runtime-types
branch
from
c0fe3f1 to
a8db9c1
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR moves RuntimeVariableList and RuntimeFixedVector types from the Lighthouse codebase into ssz_types, providing runtime-determined length variants of the existing compile-time length types. These types are gated behind a new runtime_types feature flag with an additional dependency on educe.
- Introduces
RuntimeVariableList<T>as a runtime equivalent toVariableList<T, N> - Introduces
RuntimeFixedVector<T>as a runtime equivalent toFixedVector<T, N> - Adds support for SSZ encoding/decoding and tree hashing for
RuntimeVariableList
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/runtime_types/runtime_variable_list.rs | Core implementation of runtime-sized variable list with SSZ and tree hash support |
| src/runtime_types/runtime_fixed_vector.rs | Core implementation of runtime-sized fixed vector |
| src/runtime_types/context_deserialize.rs | Context deserialization support for RuntimeVariableList |
| src/runtime_types/mod.rs | Module exports for the new runtime types |
| src/lib.rs | Library integration and public API exposure behind feature flag |
| Cargo.toml | Feature flag and dependency configuration for runtime types |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| impl<T> RuntimeVariableList<T> { | ||
| /// Returns `Ok` if the given `vec` equals the fixed length of `Self`. Otherwise returns |
There was a problem hiding this comment.
Documentation is incorrect. The function accepts vectors with length <= max_len, not just those that equal a fixed length. The comment should say 'Returns Ok if the given vec length does not exceed max_len. Otherwise returns Err(OutOfBounds { .. }).'
| /// Returns `Ok` if the given `vec` equals the fixed length of `Self`. Otherwise returns | |
| /// Returns `Ok` if the given `vec` length does not exceed `max_len`. Otherwise returns |
|
|
||
| /// Returns the type-level maximum length. | ||
| /// | ||
| /// Returns `None` if self is uninitialized with a max_len. |
There was a problem hiding this comment.
Documentation is incorrect. The function returns usize, not Option<usize>, so it cannot return None. Remove the line about returning None or change the return type if None is a valid state.
| /// Returns `None` if self is uninitialized with a max_len. |
| /// let mut long: RuntimeVariableList<_> = RuntimeVariableList::new(base, 5).unwrap(); | ||
| /// assert_eq!(&long[..], &[1, 2, 3, 4]); | ||
| /// | ||
| /// // Push a value to if it does not exceed the maximum |
There was a problem hiding this comment.
Grammar error: remove 'to' from 'Push a value to if'. Should read 'Push a value if it does not exceed the maximum'.
| /// // Push a value to if it does not exceed the maximum | |
| /// // Push a value if it does not exceed the maximum |
| where | ||
| D: Deserializer<'de>, | ||
| { | ||
| // First parse out a Vec<C> using the Vec<C> impl you already have. |
There was a problem hiding this comment.
Comment incorrectly states 'Vec' but the actual type is 'Vec'. Update comment to say 'Vec' for accuracy.
| // First parse out a Vec<C> using the Vec<C> impl you already have. | |
| // First parse out a Vec<T> using the Vec<T> impl you already have. |
There was a problem hiding this comment.
Weird way to phrase this, but it's right
|
@macladson Maybe we should address Copilot's feedback in a separate PR and merge this as-is |
Yep agreed, this is just a straight rip of the Lighthouse code so it makes sense for that to be 1-to-1 |
| } | ||
| } | ||
|
|
||
| // We can delete this once the upstream `vec_tree_hash_root` is modified to use a runtime max len. |
There was a problem hiding this comment.
I think we recently made this change in etheruem_ssz so this is also a good candidate for a post-merge cleanup
Moves the
RuntimeVariableListandRuntimeFixedVectortypes from Lighthouse tossz_typeswhich is a more natural place for them to exist.They have some additional dependencies so I have gated them behind the
runtime-typesfeature.