Use a more typesafe approach for managing indexed data #5503
Conversation
| impl From<usize> for ValidatorIndex { | ||
| fn from(n: usize) -> Self { | ||
| // can't panic, so need to truncate | ||
| let n = n.try_into().unwrap_or(u32::MAX); |
There was a problem hiding this comment.
How would I implement this?
|
Thanks so much for the review! @eskimor |
|
I'd vote for creating our own type-wrapper for As a side note, please revert unrelated changes. |
Which changes? |
|
Not the best at git. Apologies for the numerous pushes |
| } | ||
| } | ||
|
|
||
| impl From<usize> for ValidatorIndex { |
There was a problem hiding this comment.
Why do we need an impl for From<usize> if it's not guaranteed to be ok? Wouldn't a From<u32> not make more sense and leave it to the user to decide what to do on errors instead of generating a garbage ValidatorIndex? TryFrom<usize> and From<u32> seem more reasonable to me.
There was a problem hiding this comment.
While working on this, the error:
the trait bound `K: From<usize>` is not satisfied
--> primitives/src/v2/mod.rs:1616:17
|
1616 | #[derive(Clone, RuntimeDebug)]
| ^^^^^^^^^^^^ the trait `From<usize>` is not implemented for `K`
came up. This is why I likely implemented the From<usize> trait.
| @@ -631,10 +632,10 @@ mod tests { | |||
| Sr25519Keyring::Bob, | |||
| Sr25519Keyring::Charlie, | |||
| ]), | |||
| validator_groups: vec![ | |||
| validator_groups: TypeVec::from(vec![ | |||
There was a problem hiding this comment.
we should consider adding a custom macro that resembles the ergonomy of vec![..]
There was a problem hiding this comment.
I can't write macros.
| } | ||
| } | ||
|
|
||
| impl<K: From<usize>, V: Clone> From<Vec<V>> for TypeVec<K, V> { |
There was a problem hiding this comment.
| impl<K: From<usize>, V: Clone> From<Vec<V>> for TypeVec<K, V> { | |
| impl<K: From<usize>, V: Clone, I: IntoIterator<Item=V>> From<I> for TypeVec<K, V> { |
| type Identity = (); | ||
|
|
||
| fn type_info() -> Type { | ||
| todo!() |
There was a problem hiding this comment.
nit: Must be fixed before merge
There was a problem hiding this comment.
I'm not quite sure how to implement this trait.
There was a problem hiding this comment.
Thank you for your contribution and the time invested! The direction of the impl is definitely something we want rather sooner than later 👍
Though, I am not sure the chosen crate is the best choice. Having a fallback of u32::MAX is not necessarily a good thing when values are out of bounds when the only reason to have those bounds is the chosen crate to require From<usize> to be implemented. I am aware that practically speaking this should never happen, I am just very reluctanct to introduce a future footgun.
| fn from(vec: Vec<V>) -> Self { | ||
| let mut type_vec: TypeVec<K, V> = TypeVec::new(); | ||
| for value in vec.into_iter() { | ||
| type_vec.0.push(value) |
There was a problem hiding this comment.
this is a very inefficient impl considering that TiVec impls From<Vec<T>>
primitives/src/v2/mod.rs
Outdated
| #[cfg(feature = "std")] | ||
| impl<K: From<usize>, V: Clone> MallocSizeOf for TypeVec<K, V> { | ||
| fn size_of(&self, _ops: &mut MallocSizeOfOps) -> usize { | ||
| 0 |
There was a problem hiding this comment.
incorrect, should delegate to Vec impl
There was a problem hiding this comment.
Attempting this is causing issues.
There was a problem hiding this comment.
#[cfg(feature = "std")]
has to be implemented in numerous places to trait bound generic type V
There was a problem hiding this comment.
Attempted implementation:
#[cfg(feature = "std")]
impl<K: From<usize>, V: Clone> MallocSizeOf for TypeVec<K, V> {
fn size_of(&self, ops: &mut MallocSizeOfOps) -> usize {
self.to_vec().size_of(ops)
}
}
primitives/src/v2/mod.rs
Outdated
| 0 | ||
| } | ||
| fn constant_size() -> Option<usize> { | ||
| Some(0) |
There was a problem hiding this comment.
incorrect, shouldn't be implemented
| frame_election_provider_support::ElectionDataProvider | ||
| <Self as pallet_election_provider_multi_phase::Config>::DataProvider | ||
| as | ||
| frame_election_provider_support::ElectionDataProvider |
There was a problem hiding this comment.
unrelated change (and probably doesn't pass rustfmt)
primitives/src/v2/mod.rs
Outdated
| @@ -1394,9 +1422,11 @@ pub type CheckedMultiDisputeStatementSet = Vec<CheckedDisputeStatementSet>; | |||
| #[derive(Encode, Decode, Clone, RuntimeDebug, PartialEq, TypeInfo)] | |||
| pub struct DisputeState<N = BlockNumber> { | |||
| /// A bitfield indicating all validators for the candidate. | |||
| pub validators_for: BitVec<u8, bitvec::order::Lsb0>, // one bit per validator. | |||
| pub validators_for: BitVec<u8, bitvec::order::Lsb0>, | |||
| // one bit per validator. | |||
There was a problem hiding this comment.
please revert unrelated formatting changes, we're using a specific nightly version for fmt
| @@ -258,7 +258,7 @@ where | |||
| // construct trie mapping each chunk's index to its hash. | |||
| { | |||
| let mut trie = TrieDBMut::new(&mut trie_storage, &mut root); | |||
| for (i, chunk) in chunks.as_ref().iter().enumerate() { | |||
| for (i, chunk) in chunks.iter().enumerate() { | |||
There was a problem hiding this comment.
I was getting this error:
error[E0282]: type annotations needed
--> erasure-coding/src/lib.rs:261:28
|
261 | for (i, chunk) in chunks.as_ref().iter().enumerate() {
| -------^^^^^^--
| | |
| | cannot infer type for type parameter `T` declared on the trait `AsRef`
| this method call resolves to `&T`
|
= note: type must be known at this point
And on debugging saw the as_ref() made no difference to the object.
Co-authored-by: Bernhard Schuster <[email protected]>
|
Going to reattempt this from scratch |
My attempt to resolve issue: paritytech/polkadot-sdk#978
Currently incomplete, having an error at polkadot-erasure-coding preventing me from testing, was hoping to get some advice and confirmation that I'm going about this the right way!