From bc9dac480ca136d69f77d2421db53b5e8f3a2c65 Mon Sep 17 00:00:00 2001 From: CodeSandwich Date: Sun, 14 Jun 2020 21:49:21 +0200 Subject: [PATCH 1/4] Add versioning to users state --- CHANGELOG.md | 2 ++ client/src/interface.rs | 29 +++++++++++++++++++ client/src/lib.rs | 8 +++--- core/src/lib.rs | 29 ------------------- core/src/message.rs | 4 +-- core/src/state.rs | 63 ++++++++++++++++++++++++++++++++--------- runtime/src/registry.rs | 58 ++++++++++++++++++------------------- 7 files changed, 115 insertions(+), 78 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index afbf1bf3..4dc54013 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,8 @@ Upcoming ### Breaking changes + +* runtime: abandon `Users` storage in favor of `Users1` * runtime: abandon `Orgs` storage in favor of `Orgs1` * runtime: We introduce the `CheckVersion` transaction validation that requires authors to include the runtime version in the transaction. diff --git a/client/src/interface.rs b/client/src/interface.rs index f43e8233..916f5034 100644 --- a/client/src/interface.rs +++ b/client/src/interface.rs @@ -76,6 +76,35 @@ impl Org { } } +/// User +/// +/// Different from [state::UserV1] in which this type gathers +/// both the [`Id`] and the other [`User`] fields, respectively stored +/// as an User's storage key and data, into one complete type. +#[derive(Clone, Debug, Eq, PartialEq)] +pub struct User { + // Unique id of a User. + pub id: Id, + + /// See [state::UserV1::account_id] + pub account_id: AccountId, + + /// See [state::UserV1::projects] + pub projects: Vec, +} + +impl User { + pub fn new(id: Id, user: state::Users1Data) -> Self { + match user { + state::Users1Data::V1(user) => Self { + id, + account_id: user.account_id, + projects: user.projects, + }, + } + } +} + /// Result of a transaction being included in a block. /// /// Returned after submitting an transaction to the blockchain. diff --git a/client/src/lib.rs b/client/src/lib.rs index 1ade2315..ee36b7c8 100644 --- a/client/src/lib.rs +++ b/client/src/lib.rs @@ -265,17 +265,17 @@ impl ClientT for Client { } async fn get_user(&self, id: Id) -> Result, Error> { - self.fetch_map_value::(id.clone()) + self.fetch_map_value::(id.clone()) .await - .map(|maybe_user: Option| maybe_user.map(|user| User::new(id, user))) + .map(|maybe_user| maybe_user.map(|user| User::new(id, user))) } async fn list_users(&self) -> Result, Error> { - let users_prefix = registry::store::Users::final_prefix(); + let users_prefix = registry::store::Users1::final_prefix(); let keys = self.backend.fetch_keys(&users_prefix, None).await?; let mut user_ids: Vec = Vec::with_capacity(keys.len()); for key in keys { - let user_id = registry::store::Users::decode_key(&key) + let user_id = registry::store::Users1::decode_key(&key) .expect("Invalid runtime state key. Cannot extract user ID"); user_ids.push(user_id); } diff --git a/core/src/lib.rs b/core/src/lib.rs index 782b90b0..ffd39e10 100644 --- a/core/src/lib.rs +++ b/core/src/lib.rs @@ -20,8 +20,6 @@ extern crate alloc; -use alloc::vec::Vec; - use parity_scale_codec::{Decode, Encode}; use sp_core::{ed25519, H256}; use sp_runtime::traits::BlakeTwo256; @@ -102,30 +100,3 @@ impl Project { } pub type CheckpointId = H256; - -/// User -/// -/// Different from [state::User] in which this type gathers -/// both the [`Id`] and the other [`User`] fields, respectively stored -/// as an User's storage key and data, into one complete type. -#[derive(Clone, Debug, Eq, PartialEq)] -pub struct User { - // Unique id of a User. - pub id: Id, - - /// See [state::User::account_id] - pub account_id: AccountId, - - /// See [state::User::projects] - pub projects: Vec, -} - -impl User { - pub fn new(id: Id, user: state::User) -> User { - User { - id, - account_id: user.account_id, - projects: user.projects, - } - } -} diff --git a/core/src/message.rs b/core/src/message.rs index 041d7b4b..9210b6f2 100644 --- a/core/src/message.rs +++ b/core/src/message.rs @@ -64,9 +64,9 @@ pub struct UnregisterOrg { /// /// # State changes /// -/// If successful, a new [crate::state::User] with the given properties is added to the state. +/// If successful, a new [crate::state::Users1Data] with the given properties is added to the state. /// -/// [crate::state::User::account_id] is generated randomly. +/// [crate::state::Users1Data::account_id] is generated randomly. /// /// # State-dependent validations /// diff --git a/core/src/state.rs b/core/src/state.rs index 10fb5568..df091dba 100644 --- a/core/src/state.rs +++ b/core/src/state.rs @@ -226,25 +226,57 @@ impl OrgV1 { } } -/// # Storage -/// -/// This type is only used for storage. See [crate::User] for the -/// complete User type to be used everywhere else. -/// -/// Users are stored as a map with the key derived from [crate::User::id]. +/// Users are stored as a map with the key derived from [crate::Id]. /// The user ID can be extracted from the storage key. /// -/// # Invariants -/// -/// * `account_id` is immutable -/// * `projects` is a set of all the projects owned by the User. -/// /// # Relevant messages /// /// * [crate::message::RegisterUser] /// * [crate::message::UnregisterUser] #[derive(Clone, Debug, Decode, Encode, Eq, PartialEq)] -pub struct User { +pub enum Users1Data { + V1(UserV1), +} + +impl Users1Data { + /// Creates new instance in the most up to date version + pub fn new(account_id: AccountId, projects: Vec) -> Self { + Self::V1(UserV1 { + account_id, + projects, + }) + } + + /// Account ID that holds the user funds. + pub fn account_id(&self) -> AccountId { + match self { + Self::V1(user) => user.account_id, + } + } + + /// Set of all projects owned by the user. + pub fn projects(&self) -> &Vec { + match self { + Self::V1(user) => &user.projects, + } + } + + /// Add the given project to the list of [Users1Data::projects]. + /// Return a new User with the new project included or the + /// same user if the user already owns that project. + pub fn add_project(self, project_name: ProjectName) -> Self { + match self { + Self::V1(user) => Self::V1(user.add_project(project_name)), + } + } +} + +/// # Invariants +/// +/// * `account_id` is immutable +/// * `projects` is a set of all the projects owned by the User. +#[derive(Clone, Debug, Decode, Encode, Eq, PartialEq)] +pub struct UserV1 { /// Account ID that holds the user funds. pub account_id: AccountId, @@ -252,8 +284,11 @@ pub struct User { pub projects: Vec, } -impl User { - pub fn add_project(mut self, project_name: ProjectName) -> User { +impl UserV1 { + /// Add the given project to the list of [UserV1::projects]. + /// Return a new User with the new project included or the + /// same user if the user already owns that project. + pub fn add_project(mut self, project_name: ProjectName) -> Self { if !self.projects.contains(&project_name) { self.projects.push(project_name); } diff --git a/runtime/src/registry.rs b/runtime/src/registry.rs index c6956018..4e844d2b 100644 --- a/runtime/src/registry.rs +++ b/runtime/src/registry.rs @@ -98,7 +98,7 @@ pub mod store { // The storage for Users, indexed by Id. // We use the blake2_128_concat hasher so that the Id can be extraced from the key. - pub Users: map hasher(blake2_128_concat) Id => Option; + pub Users1: map hasher(blake2_128_concat) Id => Option; // We use the blake2_128_concat hasher so that the ProjectId can be extracted from the // key. @@ -181,11 +181,11 @@ decl_module! { store::Orgs1::insert(org_id, org.add_project(message.project_name.clone())); }, ProjectDomain::User(user_id) => { - let user = store::Users::get(user_id).ok_or(RegistryError::InexistentUser)?; - if user.account_id != sender { + let user = store::Users1::get(user_id).ok_or(RegistryError::InexistentUser)?; + if user.account_id() != sender { return Err(RegistryError::InsufficientSenderPermissions.into()); } - store::Users::insert(user_id, user.add_project(message.project_name.clone())); + store::Users1::insert(user_id, user.add_project(message.project_name.clone())); }, }; @@ -209,7 +209,7 @@ decl_module! { return Err(RegistryError::InsufficientSenderPermissions.into()); } - if store::Users::get(message.user_id.clone()).is_none() { + if store::Users1::get(message.user_id.clone()).is_none() { return Err(RegistryError::InexistentUser.into()); } @@ -231,7 +231,7 @@ decl_module! { return Err(RegistryError::DuplicateOrgId.into()); } - let user = get_user_with_account(sender).ok_or(RegistryError::AuthorHasNoAssociatedUser)?; + let user_id = get_user_id_with_account(sender).ok_or(RegistryError::AuthorHasNoAssociatedUser)?; let random_account_id = AccountId::unchecked_from( pallet_randomness_collective_flip::Module::::random( @@ -239,7 +239,7 @@ decl_module! { ) ); - let new_org = state::Orgs1Data::new(random_account_id, vec![user.id], Vec::new(),); + let new_org = state::Orgs1Data::new(random_account_id, vec![user_id], Vec::new()); store::Orgs1::insert(message.org_id.clone(), new_org); Self::deposit_event(Event::OrgRegistered(message.org_id)); Ok(()) @@ -248,8 +248,8 @@ decl_module! { #[weight = SimpleDispatchInfo::InsecureFreeNormal] pub fn unregister_org(origin, message: message::UnregisterOrg) -> DispatchResult { fn can_be_unregistered(org: state::Orgs1Data, sender: AccountId) -> bool { - org.projects().is_empty() && get_user_with_account(sender) - .map(|user| org.members() == &[user.id]).unwrap_or(false) + org.projects().is_empty() && get_user_id_with_account(sender) + .map(|user_id| org.members() == &[user_id]).unwrap_or(false) } let sender = ensure_signed(origin)?; @@ -273,19 +273,19 @@ decl_module! { pub fn register_user(origin, message: message::RegisterUser) -> DispatchResult { let sender = ensure_signed(origin)?; - if store::Users::get(message.user_id.clone()).is_some() { + if store::Users1::get(message.user_id.clone()).is_some() { return Err(RegistryError::DuplicateUserId.into()) } - if get_user_with_account(sender).is_some() { + if get_user_id_with_account(sender).is_some() { return Err(RegistryError::UserAccountAssociated.into()) } - let new_user = state::User { - account_id: sender, - projects: Vec::new(), - }; - store::Users::insert(message.user_id.clone(), new_user); + let new_user = state::Users1Data::new( + sender, + Vec::new(), + ); + store::Users1::insert(message.user_id.clone(), new_user); Self::deposit_event(Event::UserRegistered(message.user_id)); Ok(()) } @@ -293,16 +293,16 @@ decl_module! { #[weight = SimpleDispatchInfo::InsecureFreeNormal] pub fn unregister_user(origin, message: message::UnregisterUser) -> DispatchResult { let sender = ensure_signed(origin)?; - let sender_user = get_user_with_account(sender).ok_or(RegistryError::InexistentUser)?; + let sender_user_id = get_user_id_with_account(sender).ok_or(RegistryError::InexistentUser)?; - if sender_user.id != message.user_id { + if sender_user_id != message.user_id { return Err(RegistryError::InsufficientSenderPermissions.into()); } - if find_org(|org| org.members().contains(&sender_user.id)).is_some() { + if find_org(|org| org.members().contains(&sender_user_id)).is_some() { return Err(RegistryError::UnregisterableUser.into()); } - store::Users::remove(message.user_id.clone()); + store::Users1::remove(message.user_id.clone()); Self::deposit_event(Event::UserUnregistered(message.user_id)); Ok(()) } @@ -435,10 +435,10 @@ decl_module! { // TODO(xla): This is a naive first version of the check to see if an account is // already associated to a user. While fine for small dataset this needs to be reworked // in the future. -pub fn get_user_with_account(account_id: AccountId) -> Option { - store::Users::iter() - .find(|(_, user)| user.account_id == account_id) - .map(|(id, user)| User::new(id, user)) +pub fn get_user_id_with_account(account_id: AccountId) -> Option { + store::Users1::iter() + .find(|(_, user)| user.account_id() == account_id) + .map(|(id, _)| id) } pub fn find_org(predicate: impl Fn(&state::Orgs1Data) -> bool) -> Option { @@ -451,8 +451,8 @@ pub fn find_org(predicate: impl Fn(&state::Orgs1Data) -> bool) -> Option bool { - match get_user_with_account(account_id) { - Some(user) => org.members().contains(&user.id), + match get_user_id_with_account(account_id) { + Some(user_id) => org.members().contains(&user_id), None => false, } } @@ -499,7 +499,7 @@ impl DecodeKey for store::Projects { } } -impl DecodeKey for store::Users { +impl DecodeKey for store::Users1 { type Key = Id; fn decode_key(key: &[u8]) -> Result { @@ -555,8 +555,8 @@ mod test { #[test] fn users_decode_key_identity() { let user_id = Id::try_from("cloudhead").unwrap(); - let hashed_key = store::Users::storage_map_final_key(user_id.clone()); - let decoded_key = store::Users::decode_key(&hashed_key).unwrap(); + let hashed_key = store::Users1::storage_map_final_key(user_id.clone()); + let decoded_key = store::Users1::decode_key(&hashed_key).unwrap(); assert_eq!(decoded_key, user_id); } } From b096638fd647ccc29dfc5ca3d64972f04d1537b3 Mon Sep 17 00:00:00 2001 From: CodeSandwich Date: Sun, 14 Jun 2020 23:02:07 +0200 Subject: [PATCH 2/4] Add versioning to projects state --- CHANGELOG.md | 1 + client/src/interface.rs | 33 +++++++++++++++++++++ client/src/lib.rs | 6 ++-- core/src/lib.rs | 29 +----------------- core/src/message.rs | 4 +-- core/src/state.rs | 66 +++++++++++++++++++++++++++++++++-------- runtime/src/registry.rs | 29 ++++++++---------- 7 files changed, 106 insertions(+), 62 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4dc54013..19c47d19 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ Upcoming ### Breaking changes +* runtime: abandon `Projects` storage in favor of `Projects1` * runtime: abandon `Users` storage in favor of `Users1` * runtime: abandon `Orgs` storage in favor of `Orgs1` * runtime: We introduce the `CheckVersion` transaction validation that requires diff --git a/client/src/interface.rs b/client/src/interface.rs index 916f5034..cabd9664 100644 --- a/client/src/interface.rs +++ b/client/src/interface.rs @@ -105,6 +105,39 @@ impl User { } } +/// Project +/// +/// Different from [state::ProjectV1] in which this type gathers +/// [`ProjectName`], [`ProjectDomain`] and the other [`Project`] fields into one complete type. +#[derive(Clone, Debug, Eq, PartialEq)] +pub struct Project { + /// The name of the project, unique within its org. + pub name: ProjectName, + + /// The domain to which the project belongs. + pub domain: ProjectDomain, + + /// See [state::ProjectV1::current_cp] + pub current_cp: CheckpointId, + + /// See [state::ProjectV1::metadata] + pub metadata: Bytes128, +} + +impl Project { + /// Build a [crate::Project] given all its properties obtained from storage. + pub fn new(name: ProjectName, domain: ProjectDomain, project: state::Projects1Data) -> Self { + match project { + state::Projects1Data::V1(project) => Project { + name, + domain, + current_cp: project.current_cp, + metadata: project.metadata, + }, + } + } +} + /// Result of a transaction being included in a block. /// /// Returned after submitting an transaction to the blockchain. diff --git a/client/src/lib.rs b/client/src/lib.rs index ee36b7c8..165014dc 100644 --- a/client/src/lib.rs +++ b/client/src/lib.rs @@ -289,7 +289,7 @@ impl ClientT for Client { project_domain: ProjectDomain, ) -> Result, Error> { let project_id = (project_name.clone(), project_domain.clone()); - self.fetch_map_value::(project_id.clone()) + self.fetch_map_value::(project_id.clone()) .await .map(|maybe_project| { maybe_project.map(|project| Project::new(project_name, project_domain, project)) @@ -297,11 +297,11 @@ impl ClientT for Client { } async fn list_projects(&self) -> Result, Error> { - let project_prefix = registry::store::Projects::final_prefix(); + let project_prefix = registry::store::Projects1::final_prefix(); let keys = self.backend.fetch_keys(&project_prefix, None).await?; let mut project_ids = Vec::with_capacity(keys.len()); for key in keys { - let project_id = registry::store::Projects::decode_key(&key) + let project_id = registry::store::Projects1::decode_key(&key) .expect("Invalid runtime state key. Cannot extract project ID"); project_ids.push(project_id); } diff --git a/core/src/lib.rs b/core/src/lib.rs index ffd39e10..8b9c6b6e 100644 --- a/core/src/lib.rs +++ b/core/src/lib.rs @@ -58,7 +58,7 @@ pub type Balance = u128; /// The id of a project. Used as storage key. pub type ProjectId = (ProjectName, ProjectDomain); -/// The domain of a [Project] +/// The domain of a [crate::state::Projects1Data] #[derive(Decode, Encode, Clone, Debug, Eq, PartialEq)] pub enum ProjectDomain { Org(Id), @@ -72,31 +72,4 @@ impl std::fmt::Display for ProjectDomain { } } -#[derive(Clone, Debug, Eq, PartialEq)] -pub struct Project { - /// The name of the project, unique within its org. - pub name: ProjectName, - - /// The domain to which the project belongs. - pub domain: ProjectDomain, - - /// See [state::Project::current_cp] - pub current_cp: CheckpointId, - - /// See [state::Project::metadata] - pub metadata: Bytes128, -} - -impl Project { - /// Build a [crate::Project] given all its properties obtained from storage. - pub fn new(name: ProjectName, domain: ProjectDomain, project: state::Project) -> Self { - Project { - name, - domain, - current_cp: project.current_cp, - metadata: project.metadata, - } - } -} - pub type CheckpointId = H256; diff --git a/core/src/message.rs b/core/src/message.rs index 9210b6f2..e8d23b09 100644 --- a/core/src/message.rs +++ b/core/src/message.rs @@ -122,7 +122,7 @@ pub struct RegisterMember { /// /// # State changes /// -/// If successful, a new [crate::state::Project] with the given +/// If successful, a new [crate::state::Projects1Data] with the given /// properties is added to the state. /// /// @@ -169,7 +169,7 @@ pub struct CreateCheckpoint { pub previous_checkpoint_id: Option, } -/// Updates [crate::state::Project::current_cp]. +/// Updates [crate::state::ProjectV1::current_cp]. /// /// # State changes /// diff --git a/core/src/state.rs b/core/src/state.rs index df091dba..05aa5ad9 100644 --- a/core/src/state.rs +++ b/core/src/state.rs @@ -23,7 +23,7 @@ use crate::{AccountId, Balance, Bytes128, CheckpointId, Hashing, Id, ProjectName /// A checkpoint defines an immutable state of a project’s off-chain data via a hash. /// -/// Checkpoints are used by [Project::current_cp] +/// Checkpoints are used by [ProjectV1::current_cp] /// /// Checkpoints are identified by their content hash. See [Checkpoint::id]. /// @@ -53,26 +53,57 @@ impl Checkpoint { } } -/// # Storage -/// -/// This type is only used for storage. See [crate::Project] for the -/// complete Project type to be used everywhere else. -/// /// Projects are stored as a map with the key derived from a given [crate::ProjectId]. /// The project ID can be extracted from the storage key. /// -/// # Invariants -/// -/// * `current_cp` is guaranteed to point to an existing [Checkpoint] -/// * `metadata` is immutable -/// /// # Relevant messages /// /// * [crate::message::SetCheckpoint] /// * [crate::message::RegisterProject] #[derive(Decode, Encode, Clone, Debug, Eq, PartialEq)] -pub struct Project { - /// Links to the checkpoint of project state. +pub enum Projects1Data { + V1(ProjectV1), +} + +impl Projects1Data { + /// Creates new instance in the most up to date version + pub fn new(current_cp: CheckpointId, metadata: Bytes128) -> Self { + Self::V1(ProjectV1 { + current_cp, + metadata, + }) + } + + /// Links to the current checkpoint of project state. + pub fn current_cp(&self) -> CheckpointId { + match self { + Self::V1(project) => project.current_cp, + } + } + + /// Opaque metadata that is controlled by the DApp. + pub fn metadata(&self) -> &Bytes128 { + match self { + Self::V1(project) => &project.metadata, + } + } + + /// Sets the given checkpoint as a [Projects1Data::current_cp]. + /// Return a new Project with the new checkpoint. + pub fn with_current_cp(self, current_cp: CheckpointId) -> Self { + match self { + Self::V1(project) => Self::V1(project.set_current_cp(current_cp)), + } + } +} + +/// # Invariants +/// +/// * `current_cp` is guaranteed to point to an existing [Checkpoint] +/// * `metadata` is immutable +#[derive(Decode, Encode, Clone, Debug, Eq, PartialEq)] +pub struct ProjectV1 { + /// Links to the current checkpoint of project state. /// /// Updated with the [crate::message::SetCheckpoint] transaction. pub current_cp: CheckpointId, @@ -81,6 +112,15 @@ pub struct Project { pub metadata: Bytes128, } +impl ProjectV1 { + /// Sets the given checkpoint as a [ProjectV1::current_cp]. + /// Return a new Project with the new checkpoint. + pub fn set_current_cp(mut self, current_cp: CheckpointId) -> Self { + self.current_cp = current_cp; + self + } +} + /// Balance associated with an [crate::AccountId]. /// /// See the [Balances Pallet](https://substrate.dev/rustdocs/master/pallet_balances/index.html) for diff --git a/runtime/src/registry.rs b/runtime/src/registry.rs index 4e844d2b..3e54c338 100644 --- a/runtime/src/registry.rs +++ b/runtime/src/registry.rs @@ -102,7 +102,7 @@ pub mod store { // We use the blake2_128_concat hasher so that the ProjectId can be extracted from the // key. - pub Projects: map hasher(blake2_128_concat) ProjectId => Option; + pub Projects1: map hasher(blake2_128_concat) ProjectId => Option; // The below map indexes each existing project's id to the // checkpoint id that it was registered with. @@ -168,7 +168,7 @@ decl_module! { } let project_id = (message.project_name.clone(), message.project_domain.clone()); - if store::Projects::get(project_id.clone()).is_some() { + if store::Projects1::get(project_id.clone()).is_some() { return Err(RegistryError::DuplicateProjectId.into()); }; @@ -189,11 +189,11 @@ decl_module! { }, }; - let new_project = state::Project { - current_cp: message.checkpoint_id, - metadata: message.metadata - }; - store::Projects::insert(project_id.clone(), new_project); + let new_project = state::Projects1Data::new( + message.checkpoint_id, + message.metadata + ); + store::Projects1::insert(project_id.clone(), new_project); store::InitialCheckpoints::insert(project_id, message.checkpoint_id); Self::deposit_event(Event::ProjectRegistered(message.project_name, message.project_domain)); @@ -364,7 +364,7 @@ decl_module! { return Err(RegistryError::InexistentCheckpointId.into()) } let project_id = (message.project_name.clone(), message.project_domain.clone()); - let opt_project = store::Projects::get(project_id.clone()); + let opt_project = store::Projects1::get(project_id.clone()); let org_id = match &message.project_domain { ProjectDomain::Org(org_id) => org_id, @@ -376,10 +376,7 @@ decl_module! { if !org_has_member_with_account(&org, sender) { return Err(RegistryError::InsufficientSenderPermissions.into()) } - state::Project { - current_cp: message.new_checkpoint_id, - ..prj - } + prj.with_current_cp(message.new_checkpoint_id) } _ => return Err(RegistryError::InexistentProjectId.into()), @@ -393,7 +390,7 @@ decl_module! { return Err(RegistryError::InvalidCheckpointAncestry.into()) } - store::Projects::insert(project_id, new_project); + store::Projects1::insert(project_id, new_project); Self::deposit_event(Event::CheckpointSet( message.project_name.clone(), @@ -491,7 +488,7 @@ impl DecodeKey for store::Orgs1 { } } -impl DecodeKey for store::Projects { +impl DecodeKey for store::Projects1 { type Key = ProjectId; fn decode_key(key: &[u8]) -> Result { @@ -545,8 +542,8 @@ mod test { let org_id = Id::try_from("monadic").unwrap(); let project_name = ProjectName::try_from("radicle".to_string()).unwrap(); let project_id: ProjectId = (project_name, ProjectDomain::Org(org_id)); - let hashed_key = store::Projects::storage_map_final_key(project_id.clone()); - let decoded_key = store::Projects::decode_key(&hashed_key).unwrap(); + let hashed_key = store::Projects1::storage_map_final_key(project_id.clone()); + let decoded_key = store::Projects1::decode_key(&hashed_key).unwrap(); assert_eq!(decoded_key, project_id); } From daa1d9512abc7c57d58afd8da067191211221ec6 Mon Sep 17 00:00:00 2001 From: CodeSandwich Date: Sun, 14 Jun 2020 23:29:53 +0200 Subject: [PATCH 3/4] Add versioning to initial checkpoints state --- CHANGELOG.md | 1 + core/src/state.rs | 24 ++++++++++++++++++++++++ runtime/src/registry.rs | 9 +++++---- 3 files changed, 30 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 19c47d19..99823ec7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ Upcoming ### Breaking changes +* runtime: abandon `InitialCheckpoints` storage in favor of `InitialCheckpoints1` * runtime: abandon `Projects` storage in favor of `Projects1` * runtime: abandon `Users` storage in favor of `Users1` * runtime: abandon `Orgs` storage in favor of `Orgs1` diff --git a/core/src/state.rs b/core/src/state.rs index 05aa5ad9..bb295513 100644 --- a/core/src/state.rs +++ b/core/src/state.rs @@ -21,6 +21,30 @@ use sp_runtime::traits::Hash; use crate::{AccountId, Balance, Bytes128, CheckpointId, Hashing, Id, ProjectName, H256}; +/// The checkpoint id that an existing project was registered with. +/// +/// # Relevant messages +/// +/// * [crate::message::RegisterProject] +#[derive(Decode, Encode, Clone, Debug, Eq, PartialEq)] +pub enum InitialCheckpoints1Data { + V1(CheckpointId), +} + +impl InitialCheckpoints1Data { + /// Creates new instance in the most up to date version + pub fn new(initial_cp: CheckpointId) -> Self { + Self::V1(initial_cp) + } + + /// An id of the checkpoint + pub fn initial_cp(&self) -> CheckpointId { + match self { + Self::V1(initial_cp) => *initial_cp, + } + } +} + /// A checkpoint defines an immutable state of a project’s off-chain data via a hash. /// /// Checkpoints are used by [ProjectV1::current_cp] diff --git a/runtime/src/registry.rs b/runtime/src/registry.rs index 3e54c338..39291349 100644 --- a/runtime/src/registry.rs +++ b/runtime/src/registry.rs @@ -106,7 +106,7 @@ pub mod store { // The below map indexes each existing project's id to the // checkpoint id that it was registered with. - pub InitialCheckpoints: map hasher(opaque_blake2_256) ProjectId => Option; + pub InitialCheckpoints1: map hasher(opaque_blake2_256) ProjectId => Option; // The below map indexes each checkpoint's id to the checkpoint // it points to, should it exist. @@ -194,7 +194,8 @@ decl_module! { message.metadata ); store::Projects1::insert(project_id.clone(), new_project); - store::InitialCheckpoints::insert(project_id, message.checkpoint_id); + let checkpoint_data = state::InitialCheckpoints1Data::new(message.checkpoint_id); + store::InitialCheckpoints1::insert(project_id, checkpoint_data); Self::deposit_event(Event::ProjectRegistered(message.project_name, message.project_domain)); Ok(()) @@ -382,9 +383,9 @@ decl_module! { }; - let initial_cp = match store::InitialCheckpoints::get(project_id.clone()) { + let initial_cp = match store::InitialCheckpoints1::get(project_id.clone()) { None => return Err(RegistryError::InexistentInitialProjectCheckpoint.into()), - Some(cp) => cp, + Some(cp) => cp.initial_cp(), }; if !descends_from_initial_checkpoint(message.new_checkpoint_id, initial_cp) { return Err(RegistryError::InvalidCheckpointAncestry.into()) From a8b7e083c177cae62e8e0688d3dfb9debc47071c Mon Sep 17 00:00:00 2001 From: CodeSandwich Date: Mon, 15 Jun 2020 00:46:53 +0200 Subject: [PATCH 4/4] Add versioning to checkpoints state --- CHANGELOG.md | 3 +- client/src/interface.rs | 29 ++++++++++- client/src/lib.rs | 5 +- client/tests/end_to_end.rs | 5 +- core/src/message.rs | 6 ++- core/src/state.rs | 53 +++++++++++++++++---- runtime-tests/tests/checkpoing_setting.rs | 8 ++-- runtime-tests/tests/checkpoint_creation.rs | 13 ++--- runtime-tests/tests/project_registration.rs | 5 +- runtime/src/registry.rs | 22 ++++----- 10 files changed, 102 insertions(+), 47 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 99823ec7..ff503d3f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,7 +6,8 @@ Upcoming ### Breaking changes - +* client: `ClientT::get_checkpoint` returns a new `Checkpoint` structure +* runtime: abandon `Checkpoints` storage in favor of `Checkpoints1` * runtime: abandon `InitialCheckpoints` storage in favor of `InitialCheckpoints1` * runtime: abandon `Projects` storage in favor of `Projects1` * runtime: abandon `Users` storage in favor of `Users1` diff --git a/client/src/interface.rs b/client/src/interface.rs index cabd9664..4bae4dcb 100644 --- a/client/src/interface.rs +++ b/client/src/interface.rs @@ -138,6 +138,33 @@ impl Project { } } +/// Checkpoint +/// +/// Different from [state::CheckpointV1] in which this type gathers +/// both the [`CheckpointId`] and the other [`Checkpoint`] fields, respectively stored +/// as an Checkpoint's storage key and data, into one complete type. +#[derive(Clone, Debug, Eq, PartialEq)] +pub struct Checkpoint { + /// Checkpoint ID. + pub id: CheckpointId, + /// Previous checkpoint in the project history. + pub parent: Option, + /// Hash that identifies a project’s off-chain data. + pub hash: H256, +} + +impl Checkpoint { + pub fn new(cp: state::Checkpoints1Data) -> Self { + match cp { + state::Checkpoints1Data::V1(cp) => Self { + id: cp.id(), + parent: cp.parent, + hash: cp.hash, + }, + } + } +} + /// Result of a transaction being included in a block. /// /// Returned after submitting an transaction to the blockchain. @@ -237,7 +264,7 @@ pub trait ClientT { async fn list_projects(&self) -> Result, Error>; - async fn get_checkpoint(&self, id: CheckpointId) -> Result, Error>; + async fn get_checkpoint(&self, id: CheckpointId) -> Result, Error>; async fn onchain_runtime_version(&self) -> Result; } diff --git a/client/src/lib.rs b/client/src/lib.rs index 165014dc..693d4e6e 100644 --- a/client/src/lib.rs +++ b/client/src/lib.rs @@ -308,9 +308,10 @@ impl ClientT for Client { Ok(project_ids) } - async fn get_checkpoint(&self, id: CheckpointId) -> Result, Error> { - self.fetch_map_value::(id) + async fn get_checkpoint(&self, id: CheckpointId) -> Result, Error> { + self.fetch_map_value::(id) .await + .map(|cp_opt| cp_opt.map(Checkpoint::new)) } async fn onchain_runtime_version(&self) -> Result { diff --git a/client/tests/end_to_end.rs b/client/tests/end_to_end.rs index fa37203b..5a35e2e1 100644 --- a/client/tests/end_to_end.rs +++ b/client/tests/end_to_end.rs @@ -87,10 +87,7 @@ async fn register_project() { .any(|id| *id == (message.project_name.clone(), message.project_domain.clone())); assert!(has_project, "Registered project not found in project list"); - let checkpoint_ = state::Checkpoint { - parent: None, - hash: project_hash, - }; + let checkpoint_ = Checkpoint::new(state::Checkpoints1Data::new(None, project_hash)); let checkpoint = client.get_checkpoint(checkpoint_id).await.unwrap().unwrap(); assert_eq!(checkpoint, checkpoint_); diff --git a/core/src/message.rs b/core/src/message.rs index e8d23b09..d42d1e47 100644 --- a/core/src/message.rs +++ b/core/src/message.rs @@ -158,7 +158,8 @@ pub struct RegisterProject { /// /// # State changes /// -/// If successful, adds a new [crate::state::Checkpoint] with the given parameters to the state. +/// If successful, adds a new [crate::state::Checkpoints1Data] with the given parameters +/// to the state. /// /// # State-dependent validations /// @@ -173,7 +174,8 @@ pub struct CreateCheckpoint { /// /// # State changes /// -/// If successful, adds a new [crate::state::Checkpoint] with the given parameters to the state. +/// If successful, adds a new [crate::state::Checkpoints1Data] with the given parameters +/// to the state. /// /// # State-dependent validations /// diff --git a/core/src/state.rs b/core/src/state.rs index bb295513..705edce8 100644 --- a/core/src/state.rs +++ b/core/src/state.rs @@ -49,29 +49,62 @@ impl InitialCheckpoints1Data { /// /// Checkpoints are used by [ProjectV1::current_cp] /// -/// Checkpoints are identified by their content hash. See [Checkpoint::id]. +/// Checkpoints are identified by their content hash. See [Checkpoints1Data::id]. /// /// # Storage /// -/// Checkpoints are stored as a map using [Checkpoint::id] to derive the key. -/// -/// # Invariants -/// -/// * If `parent` is [Some] then the referenced checkpoint exists in the state. +/// Checkpoints are stored as a map using [Checkpoints1Data::id] to derive the key. /// /// # Relevant messages /// /// * [crate::message::CreateCheckpoint] /// * [crate::message::SetCheckpoint] #[derive(Decode, Encode, Clone, Debug, Eq, PartialEq)] -pub struct Checkpoint { - /// Previous checkpoint in the project histor. +pub enum Checkpoints1Data { + V1(CheckpointV1), +} + +impl Checkpoints1Data { + /// Creates new instance in the most up to date version + pub fn new(parent: Option, hash: H256) -> Self { + Self::V1(CheckpointV1 { parent, hash }) + } + + /// Previous checkpoint in the project history. + pub fn parent(&self) -> Option { + match self { + Self::V1(cp) => cp.parent, + } + } + + /// Hash that identifies a project’s off-chain data. + pub fn hash(&self) -> H256 { + match self { + Self::V1(cp) => cp.hash, + } + } + + /// Checkpoint ID + pub fn id(&self) -> CheckpointId { + match self { + Self::V1(cp) => cp.id(), + } + } +} + +/// # Invariants +/// +/// * If `parent` is [Some] then the referenced checkpoint exists in the state. +#[derive(Decode, Encode, Clone, Debug, Eq, PartialEq)] +pub struct CheckpointV1 { + /// Previous checkpoint in the project history. pub parent: Option, /// Hash that identifies a project’s off-chain data. pub hash: H256, } -impl Checkpoint { +impl CheckpointV1 { + /// Checkpoint ID pub fn id(&self) -> CheckpointId { Hashing::hash_of(&self) } @@ -123,7 +156,7 @@ impl Projects1Data { /// # Invariants /// -/// * `current_cp` is guaranteed to point to an existing [Checkpoint] +/// * `current_cp` is guaranteed to point to an existing [Checkpoints1Data] /// * `metadata` is immutable #[derive(Decode, Encode, Clone, Debug, Eq, PartialEq)] pub struct ProjectV1 { diff --git a/runtime-tests/tests/checkpoing_setting.rs b/runtime-tests/tests/checkpoing_setting.rs index c2f14af5..7933c8ab 100644 --- a/runtime-tests/tests/checkpoing_setting.rs +++ b/runtime-tests/tests/checkpoing_setting.rs @@ -54,10 +54,10 @@ async fn create_checkpoint() { .unwrap(); assert_eq!( checkpoint, - state::Checkpoint { - parent: Some(project.current_cp), - hash: project_hash - }, + Checkpoint::new(state::Checkpoints1Data::new( + Some(project.current_cp), + project_hash + )), ); assert_eq!( diff --git a/runtime-tests/tests/checkpoint_creation.rs b/runtime-tests/tests/checkpoint_creation.rs index 65b3f18c..8594a5da 100644 --- a/runtime-tests/tests/checkpoint_creation.rs +++ b/runtime-tests/tests/checkpoint_creation.rs @@ -52,10 +52,7 @@ async fn create_checkpoint() { .result .unwrap(); - let checkpoint1_ = state::Checkpoint { - parent: None, - hash: project_hash1, - }; + let checkpoint1_ = Checkpoint::new(state::Checkpoints1Data::new(None, project_hash1)); let checkpoint1 = client .get_checkpoint(checkpoint_id1) .await @@ -63,10 +60,10 @@ async fn create_checkpoint() { .unwrap(); assert_eq!(checkpoint1, checkpoint1_); - let checkpoint2_ = state::Checkpoint { - parent: Some(checkpoint_id1), - hash: project_hash2, - }; + let checkpoint2_ = Checkpoint::new(state::Checkpoints1Data::new( + Some(checkpoint_id1), + project_hash2, + )); let checkpoint2 = client .get_checkpoint(checkpoint_id2) .await diff --git a/runtime-tests/tests/project_registration.rs b/runtime-tests/tests/project_registration.rs index 448b75ea..c5936bf1 100644 --- a/runtime-tests/tests/project_registration.rs +++ b/runtime-tests/tests/project_registration.rs @@ -85,10 +85,7 @@ async fn register_project() { .any(|id| *id == (message.project_name.clone(), message.project_domain.clone())); assert!(has_project, "Registered project not found in project list"); - let checkpoint_ = state::Checkpoint { - parent: None, - hash: project_hash, - }; + let checkpoint_ = Checkpoint::new(state::Checkpoints1Data::new(None, project_hash)); let checkpoint = client.get_checkpoint(checkpoint_id).await.unwrap().unwrap(); assert_eq!(checkpoint, checkpoint_); diff --git a/runtime/src/registry.rs b/runtime/src/registry.rs index 39291349..ed8cb26a 100644 --- a/runtime/src/registry.rs +++ b/runtime/src/registry.rs @@ -110,7 +110,7 @@ pub mod store { // The below map indexes each checkpoint's id to the checkpoint // it points to, should it exist. - pub Checkpoints: map hasher(opaque_blake2_256) CheckpointId => Option; + pub Checkpoints1: map hasher(opaque_blake2_256) CheckpointId => Option; } } } @@ -133,8 +133,8 @@ fn descends_from_initial_checkpoint( // // The loop's total runtime will also depend on the performance of // each `store::StorageMap::get` request. - while let Some(cp) = store::Checkpoints::get(ancestor_id) { - match cp.parent { + while let Some(cp) = store::Checkpoints1::get(ancestor_id) { + match cp.parent() { None => return false, Some(cp_id) => { if cp_id == initial_cp_id { @@ -163,7 +163,7 @@ decl_module! { pub fn register_project(origin, message: message::RegisterProject) -> DispatchResult { let sender = ensure_signed(origin)?; - if store::Checkpoints::get(message.checkpoint_id).is_none() { + if store::Checkpoints1::get(message.checkpoint_id).is_none() { return Err(RegistryError::InexistentCheckpointId.into()) } @@ -336,19 +336,19 @@ decl_module! { match message.previous_checkpoint_id { None => {} Some(cp_id) => { - match store::Checkpoints::get(cp_id) { + match store::Checkpoints1::get(cp_id) { None => return Err(RegistryError::InexistentCheckpointId.into()), Some(_) => {} } } }; - let checkpoint = state::Checkpoint { - parent: message.previous_checkpoint_id, - hash: message.project_hash, - }; + let checkpoint = state::Checkpoints1Data::new( + message.previous_checkpoint_id, + message.project_hash, + ); let checkpoint_id = Hashing::hash_of(&checkpoint); - store::Checkpoints::insert(checkpoint_id, checkpoint); + store::Checkpoints1::insert(checkpoint_id, checkpoint); Self::deposit_event(Event::CheckpointCreated(checkpoint_id)); Ok(()) @@ -361,7 +361,7 @@ decl_module! { ) -> DispatchResult { let sender = ensure_signed(origin)?; - if store::Checkpoints::get(message.new_checkpoint_id).is_none() { + if store::Checkpoints1::get(message.new_checkpoint_id).is_none() { return Err(RegistryError::InexistentCheckpointId.into()) } let project_id = (message.project_name.clone(), message.project_domain.clone());