Conversation
PoulavBhowmick03
force-pushed
the
refactor/MultiIndexMap
branch
from
f62c47b to
a9a9661
Compare
|
|
|
|
PoulavBhowmick03
force-pushed
the
refactor/MultiIndexMap
branch
from
a9a9661 to
8b70a1a
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the MultiIndexMap implementation and its usages to improve type safety and unify index access patterns.
- Swapped out the old
UniqueIndex/NonUniqueIndextraits for the newUniqueIndexAccess/NonUniqueIndexAccessvariants. - Changed
MultiIndexMapto useIndexDescriptortypes (UniqueIndex<K>/NonUniqueIndex<K>) withArc-wrapped primary keys. - Updated imports and call sites across modules (validator_store, subnet_tracker, message_receiver, eth, database) to align with the new API.
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| anchor/validator_store/src/lib.rs | Updated database trait imports to *Access variants |
| anchor/subnet_tracker/src/lib.rs | Same import adjustments |
| anchor/message_receiver/src/manager.rs | Same import adjustments |
| anchor/eth/src/index_sync.rs | Updated UniqueIndexAccess import |
| anchor/eth/src/event_processor.rs | Same import adjustments |
| anchor/database/src/validator_operations.rs | Swapped to UniqueIndexAccess/NonUniqueIndexAccess |
| anchor/database/src/tests/utils.rs | Added missing UniqueIndexAccess import in tests |
| anchor/database/src/tests/state_tests.rs | Added missing UniqueIndexAccess import in tests |
| anchor/database/src/tests/mod.rs | Removed UniqueIndex re-export from test prelude |
| anchor/database/src/state.rs | Adjusted NetworkState insertion calls to owned values |
| anchor/database/src/multi_index.rs | Overhauled MultiIndexMap internals and API |
| anchor/database/src/lib.rs | Updated pub use to export new index types |
| anchor/database/src/cluster_operations.rs | Updated calls to MultiIndexMap::insert with new API |
Comments suppressed due to low confidence (1)
anchor/database/src/tests/mod.rs:13
- The test prelude no longer re-exports
multi_index::UniqueIndex, causing tests that referenceUniqueIndexto fail to compile. Consider re-addingmulti_index::UniqueIndexto the prelude or importing it explicitly in the tests.
pub use crate::NetworkDatabase;
|
|
||
| /// Marker trait for uniquely identifying indices | ||
| #[allow(dead_code)] | ||
| pub trait Unique {} |
There was a problem hiding this comment.
[nitpick] The marker traits Unique and NotUnique are no longer used after this refactoring. You can remove them to reduce dead code and simplify the module.
| .primary | ||
| .keys() | ||
| .find(|key| key.as_ref() == k1) | ||
| .cloned()?; |
There was a problem hiding this comment.
This linear search over self.primary.keys() makes update O(n). Consider maintaining a direct map from K1 to its Arc<K1> to allow O(1) lookups.
| if std::any::TypeId::of::<I2::Uniqueness>() == std::any::TypeId::of::<UniqueTag>() { | ||
| self.secondary_unique.insert(k2, primary_key.clone()); | ||
| } else { | ||
| self.maps | ||
| .secondary_multi | ||
| .entry(k2.clone()) | ||
| .and_modify(|vec| vec.push(k1.clone())) | ||
| .or_insert_with(|| vec![k1.clone()]); | ||
| self.secondary_multi | ||
| .entry(k2) | ||
| .or_insert(Vec::new()) | ||
| .push(primary_key.clone()); | ||
| } | ||
|
|
||
| // Handle tertiary index based on uniqueness | ||
| if std::any::TypeId::of::<U2>() == std::any::TypeId::of::<UniqueTag>() { | ||
| self.maps.tertiary_unique.insert(k3.clone(), k1.clone()); | ||
| if std::any::TypeId::of::<I3::Uniqueness>() == std::any::TypeId::of::<UniqueTag>() { | ||
| self.tertiary_unique.insert(k3.clone(), primary_key.clone()); | ||
| } else { | ||
| self.maps | ||
| .tertiary_multi | ||
| self.tertiary_multi | ||
| .entry(k3.clone()) | ||
| .and_modify(|vec| vec.push(k1.clone())) | ||
| .or_insert_with(|| vec![k1.clone()]); | ||
| .and_modify(|vec| vec.push(primary_key.clone())) | ||
| .or_insert_with(|| vec![primary_key.clone()]); | ||
| } | ||
|
|
||
| // Handle quaternary index based on uniqueness | ||
| if std::any::TypeId::of::<U3>() == std::any::TypeId::of::<UniqueTag>() { | ||
| self.maps.quaternary_unique.insert(k4.clone(), k1.clone()); | ||
| if std::any::TypeId::of::<I4::Uniqueness>() == std::any::TypeId::of::<UniqueTag>() { | ||
| self.quaternary_unique | ||
| .insert(k4.clone(), primary_key.clone()); | ||
| } else { | ||
| self.maps | ||
| .quaternary_multi | ||
| self.quaternary_multi | ||
| .entry(k4.clone()) | ||
| .and_modify(|vec| vec.push(k1.clone())) | ||
| .or_insert_with(|| vec![k1.clone()]); | ||
| .and_modify(|vec| vec.push(primary_key.clone())) | ||
| .or_insert_with(|| vec![primary_key.clone()]); | ||
| } |
There was a problem hiding this comment.
[nitpick] Using TypeId comparisons for compile-time dispatch can be brittle and may not optimize away. Consider using trait specialization or const generics to branch on index uniqueness at compile time.
anchor/database/src/multi_index.rs
Outdated
| .or_insert_with(|| vec![k1.clone()]); | ||
| self.secondary_multi | ||
| .entry(k2) | ||
| .or_insert(Vec::new()) |
There was a problem hiding this comment.
[nitpick] You can simplify or_insert(Vec::new()) to or_default() for a more concise and idiomatic entry API usage.
| .or_insert(Vec::new()) | |
| .or_default() |
PoulavBhowmick03
force-pushed
the
refactor/MultiIndexMap
branch
from
8b70a1a to
2c555b3
Compare
|
@diegomrsantos i have fixed the tests, can you check again |
PoulavBhowmick03
force-pushed
the
refactor/MultiIndexMap
branch
from
2c555b3 to
d195b92
Compare
|
Hi @PoulavBhowmick03! Thank you for your PR, this is a good start. Please take a look at the Copilot suggestions above. Also, while I like the reduction of generic parameters, it would be also great if we find a way to avoid the redundant |
|
With #463, we decided to go another way, but thank you for the PR anyway! |
2 participants
Issue Addressed
Fixes #253