Skip to content

Conversation

macladson
Copy link
Member

@macladson macladson commented Sep 11, 2025

Issue Addressed

#8012

Proposed Changes

Replace all instances of VariableList::from and FixedVector::from to their try_from variants.

While I tried to use proper error handling in most cases, there were certain situations where adding an expect for situations where try_from can trivially never fail avoided adding a lot of extra complexity.

@macladson macladson requested a review from jxs as a code owner September 11, 2025 16:18
@macladson macladson added the work-in-progress PR is a work-in-progress label Sep 11, 2025
Copy link
Member

@michaelsproul michaelsproul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good!

Just a few expects here and there that I wonder if we should remove

N: Unsigned,
{
VariableList::try_from_iter(list.into_iter().map(Into::into))
.expect("conversion to JSON should preserve withdrawals length")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a little spicy, but I think it's OK for now.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is the expect-less version: 6aa826b

attesting_indices: vec![self.attester_index].into(),
attesting_indices: vec![self.attester_index]
.try_into()
.expect("Single index should not exceed MaxValidatorsPerSlot"),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This probably fails clippy, maybe we should just make this method return a Result. It probably isn't used in that many places

Copy link
Member Author

@macladson macladson Sep 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried that initially, but to_indexed gets used in attestation verification which then requires a new error variant. That error variant then needs to be covered in match statements in some networking code and it ended up being really ugly just for this one error which can never occur anyway. The expect felt like the lesser of two evils. I'll keep trying and see if I can come up with something cleaner

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

expect-less version: 12c260a

@macladson macladson added ready-for-review The code is ready for review dependencies Pull requests that update a dependency file waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed work-in-progress PR is a work-in-progress ready-for-review The code is ready for review labels Sep 15, 2025
@macladson macladson added ready-for-review The code is ready for review and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Sep 15, 2025
@michaelsproul michaelsproul added v8.0.0-rc.0 Q3 2025 release for Fusaka on Holesky v8.0.0 Q4 2025 Fusaka Mainnet Release waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed v8.0.0-rc.0 Q3 2025 release for Fusaka on Holesky ready-for-review The code is ready for review labels Sep 16, 2025
@michaelsproul
Copy link
Member

Sorry @macladson, after discussing with Jimmy we decided it would be better to remove these expects before merging. Can you have another go at propagating the errors, especially the ones for the JSON types, and anything that is reachable at runtime (anything outside a static/const).

bytes.truncate(MAX_ERROR_LEN as usize);
Self(
VariableList::try_from(bytes)
.expect("length should not exceed MaxErrorLen after truncation"),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I convert this whole impl to a TryFrom?

Comment on lines 2031 to +2035

let empty_data_columns_by_root_id = DataColumnsByRootIdentifier {
block_root: Hash256::zero(),
columns: VariableList::from(vec![0; E::number_of_columns()]),
columns: VariableList::try_from(vec![0; E::number_of_columns()])
.expect("VariableList::try_from should succeed"),
Copy link
Member Author

@macladson macladson Sep 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function could also be a Result<usize, Error>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file v8.0.0 Q4 2025 Fusaka Mainnet Release waiting-on-author The reviewer has suggested changes and awaits thier implementation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants