-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Conversation
CC #637 |
@@ -170,6 +171,42 @@ pub type Council = council::Module<Concrete>; | |||
/// Council voting module for this concrete runtime. | |||
pub type CouncilVoting = council::voting::Module<Concrete>; | |||
|
|||
/// Concrete log type. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could do with more information. consider what it is in relation to other types and why it exists as a type at all.
#[derive(Clone, PartialEq, Eq, Encode, Decode)] | ||
#[cfg_attr(feature = "std", derive(Debug, Serialize, Deserialize))] | ||
pub enum ConcreteLog { | ||
/// Authority changes log. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
more information. describe specifically under what circumstances this item is used as opposed to any other.
@@ -96,6 +100,14 @@ decl_module! { | |||
} | |||
} | |||
|
|||
decl_storage! { | |||
trait Store for Module<T: Trait> as Consensus { | |||
// Authorities set actual at the block execution start. IsSome only if |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IsSome
or !empty()
? (with default
you don't get access to whether it's Some
or None
)
trait Store for Module<T: Trait> as Consensus { | ||
// Authorities set actual at the block execution start. IsSome only if | ||
// the set has been changed. | ||
SavedAuthorities get(saved_authorities): default Vec<T::SessionKey>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider rename to OriginalAuthorities
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also consider removing default
, to ensure it's Some
in the case that the set changed and None
otherwise.
let previous_authorities = AuthorityStorageVec::<T::SessionKey>::items(); | ||
if previous_authorities != authorities { | ||
let saved_authorities = Self::saved_authorities(); | ||
if saved_authorities.is_empty() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this would then become original_authorities.is_none()
} | ||
|
||
/// Single digest item. | ||
pub trait DigestItem: Member { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider copying the Event
infrastructure for this - it should be able to be used almost wholesale.
@@ -305,6 +305,7 @@ mod tests { | |||
const NOTE_OFFLINE_POSITION: u32 = 1; | |||
type SessionKey = u64; | |||
type OnOfflineValidator = (); | |||
type ConvertSessionKeyToAuthorityId = Empty; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use ()
now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some suggestions there
* Merge * Bump Substrate
* Add conversion and default functions for `NumberOrHex` * Update subxt/src/rpc.rs Co-authored-by: Andrew Jones <ascjones@gmail.com> * Derive `Debug` with `thiserror::Error` * Remove unnecessary `#[cfg(test)]` * Add `#[error(…)]` * Remove `()` Co-authored-by: Andrew Jones <ascjones@gmail.com>
Required for #269 and #628 (comment)
This PR doesn't change anything - just adds some traits + example implementation + usage.
Traits are:
DigestItem
+AuthoritiesChangeDigest
. More (likeChangesTrieRootDigest
) are to follow in next PRs.