Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

DigestItem trait #636

Closed
wants to merge 1 commit into from
Closed

DigestItem trait #636

wants to merge 1 commit into from

Conversation

svyatonik
Copy link
Contributor

Required for #269 and #628 (comment)

This PR doesn't change anything - just adds some traits + example implementation + usage.
Traits are: DigestItem + AuthoritiesChangeDigest. More (like ChangesTrieRootDigest) are to follow in next PRs.

@svyatonik svyatonik added the A0-please_review Pull request needs code review. label Aug 31, 2018
@gavofyork
Copy link
Member

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.
Copy link
Member

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.
Copy link
Member

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
Copy link
Member

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>;
Copy link
Member

Choose a reason for hiding this comment

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

Consider rename to OriginalAuthorities

Copy link
Member

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() {
Copy link
Member

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 {
Copy link
Member

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;
Copy link
Member

Choose a reason for hiding this comment

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

Use () now.

Copy link
Member

@gavofyork gavofyork left a comment

Choose a reason for hiding this comment

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

Some suggestions there

@svyatonik svyatonik added A5-grumble and removed A0-please_review Pull request needs code review. labels Aug 31, 2018
@svyatonik svyatonik closed this Sep 3, 2018
@svyatonik svyatonik mentioned this pull request Sep 3, 2018
lamafab pushed a commit to lamafab/substrate that referenced this pull request Jun 16, 2020
* Merge

* Bump Substrate
liuchengxu pushed a commit to chainx-org/substrate that referenced this pull request Aug 23, 2021
helin6 pushed a commit to boolnetwork/substrate that referenced this pull request Jul 25, 2023
* Add conversion and default functions for `NumberOrHex`

* Update subxt/src/rpc.rs

Co-authored-by: Andrew Jones <[email protected]>

* Derive `Debug` with `thiserror::Error`

* Remove unnecessary `#[cfg(test)]`

* Add `#[error(…)]`

* Remove `()`

Co-authored-by: Andrew Jones <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants