Conversation
- Make `ParaHashAtSource` more generic - Modify `on_chain_parachain_header` to return `HeaderId` - Shortening variable names Signed-off-by: Serban Iorga <serban@parity.io>
Change ParachainsSource::max_head_id to Arc<Mutex<NoopOption>> Signed-off-by: Serban Iorga <serban@parity.io>
Actually this mutex doesn't seem to be heavily used and changing it to |
svyatonik
left a comment
svyatonik
left a comment
There was a problem hiding this comment.
Please fix the impl of Ord/PartialOrd and then it's fine to merge! Thank you!
primitives/runtime/src/lib.rs
Outdated
| } | ||
| } | ||
|
|
||
| impl<Hash: Eq, Number: Ord> Ord for HeaderId<Hash, Number> { |
There was a problem hiding this comment.
IIUC this breaks the exactly one of a < b, a == b or a > b is true rule (https://doc.rust-lang.org/std/cmp/trait.Ord.html). Can we, please, add hash to comparison as well?
| /// Shared updatable reference to the maximal parachain header id that we want to sync from the | ||
| /// source. | ||
| pub type RequiredHeaderIdRef<C> = Arc<Mutex<Option<HeaderIdOf<C>>>>; | ||
| pub type RequiredHeaderIdRef<C> = Arc<Mutex<NoopOption<HeaderIdOf<C>>>>; |
There was a problem hiding this comment.
This actually seems like a redundant change - we'll be only using Some or None here. right? :) So we don't need this Noop at all. But I'm fine with leaving it as is :)
relays/utils/src/lib.rs
Outdated
| } | ||
| } | ||
|
|
||
| /// An extension over rust's `Option` that adds an extra possibility: `Noop`. |
There was a problem hiding this comment.
I would rather made it less generic - like something you have suggested originally (or even enum AvailableHeader<T> { Available(T), Missing, Unavailable }, where Missing means None). It will (imo) add some context to the code + we may add more useful documentation to each option.
But please don't consider my opinion as the requirement - I'm fine with either solution, so please make your own decision here :)
* Parachains source cosmetic changes - Make `ParaHashAtSource` more generic - Modify `on_chain_parachain_header` to return `HeaderId` - Shortening variable names Signed-off-by: Serban Iorga <serban@parity.io> * Change ParachainsSource::max_head_id type Change ParachainsSource::max_head_id to Arc<Mutex<NoopOption>> Signed-off-by: Serban Iorga <serban@parity.io> * code review changes
* Parachains source cosmetic changes - Make `ParaHashAtSource` more generic - Modify `on_chain_parachain_header` to return `HeaderId` - Shortening variable names Signed-off-by: Serban Iorga <serban@parity.io> * Change ParachainsSource::max_head_id type Change ParachainsSource::max_head_id to Arc<Mutex<NoopOption>> Signed-off-by: Serban Iorga <serban@parity.io> * code review changes
2 participants
Addressing the comments in #1419
I spent a lot of time thinking about naming.
NoopOptionwas the best name I could find so far to makeParaHashAtSourcemore generic although it doesn't seem ideal.Also it would be nice to define
max_head_idas aNoopOptionfrom the start, but I didn't do it since this way we would have to create anArc<Mutex<NoopOption>>and it would be less efficient (because we would have to lock the mutex even ifmax_head_id == Noop::None, which is not needed now). We could have something more efficient using atomic variables, but that would increase the complexity of the code.