From 5c239251ee3c8b8b94b3496fce2caa52b503a6ec Mon Sep 17 00:00:00 2001 From: mr-t Date: Wed, 28 Feb 2024 20:27:37 +0100 Subject: [PATCH 1/6] move all logic to `OwnershipStore`. This way `cw-ownable` is still a singleton. But can be re-use e.g. by defining `CREATOR: OwnershipStore = OwnershipStore::new("creator")` --- README.md | 2 +- packages/ownable/README.md | 10 + packages/ownable/src/lib.rs | 357 +++++++++++++++++++++--------------- 3 files changed, 221 insertions(+), 148 deletions(-) diff --git a/README.md b/README.md index 26f3cd9..f6ad969 100644 --- a/README.md +++ b/README.md @@ -9,7 +9,7 @@ A collection of [CosmWasm][1] utilities and helper libraries. | [cw-address-like][2] | v1.0.4 | A trait that marks unchecked or checked address strings | | [cw-item-set][3] | v0.7.1 | Set of non-duplicate items for storage | | [cw-optional-indexes][4] | v0.1.1 | Index types for `IndexedMap` where an item may or may not have an index | -| [cw-ownable][5] | v0.5.1 | Utility for controlling contract ownership | +| [cw-ownable][5] | v0.6.0 | Utility for controlling contract ownership | | [cw-paginate][6] | v0.2.1 | Helper function for interating maps | ## License diff --git a/packages/ownable/README.md b/packages/ownable/README.md index 710248d..19eab04 100644 --- a/packages/ownable/README.md +++ b/packages/ownable/README.md @@ -123,6 +123,16 @@ pub fn query(deps: Deps, env: Env, msg: QueryMsg) -> StdResult { } ``` +You can use ownership for other purposes: + +```rust +use cw_ownable::OwnershipStore; + +pub const CREATOR: OwnershipStore = OwnershipStore::new("creator"); +``` + +`CREATOR` has all functions in place: `initialize_owner`, `is_owner`, `assert_owner`, and `get_ownership`. + ## License Contents of this crate at or prior to version `0.5.0` are published under [GNU Affero General Public License v3](https://github.com/steak-enjoyers/cw-plus-plus/blob/9c8fcf1c95b74dd415caf5602068c558e9d16ecc/LICENSE) or later; contents after the said version are published under [Apache-2.0](../../LICENSE) license. diff --git a/packages/ownable/src/lib.rs b/packages/ownable/src/lib.rs index a88b6cc..94c43a2 100644 --- a/packages/ownable/src/lib.rs +++ b/packages/ownable/src/lib.rs @@ -28,6 +28,183 @@ pub struct Ownership { pub pending_expiry: Option, } +pub struct OwnershipStore<'a> { + pub item: Item<'a, Ownership>, +} + +impl<'a> OwnershipStore<'a> { + pub const fn new(key: &'a str) -> Self { + Self { + item: Item::new(key), + } + } + + /// Set the given address as the contract owner. + /// + /// This function is only intended to be used only during contract instantiation. + pub fn initialize_owner( + &self, + storage: &mut dyn Storage, + api: &dyn Api, + owner: Option<&str>, + ) -> StdResult> { + let ownership = Ownership { + owner: owner.map(|h| api.addr_validate(h)).transpose()?, + pending_owner: None, + pending_expiry: None, + }; + self.item.save(storage, &ownership)?; + Ok(ownership) + } + + /// Return Ok(true) if the contract has an owner and it's the given address. + /// Return Ok(false) if the contract doesn't have an owner, of if it does but + /// it's not the given address. + /// Return Err if fails to load ownership info from storage. + pub fn is_owner(&self, store: &dyn Storage, addr: &Addr) -> StdResult { + let ownership = self.item.load(store)?; + + if let Some(owner) = ownership.owner { + if *addr == owner { + return Ok(true); + } + } + + Ok(false) + } + + /// Assert that an account is the contract's current owner. + pub fn assert_owner(&self, store: &dyn Storage, sender: &Addr) -> Result<(), OwnershipError> { + let ownership = self.item.load(store)?; + self.check_owner(&ownership, sender) + } + + /// Assert that an account is the contract's current owner. + fn check_owner( + &self, + ownership: &Ownership, + sender: &Addr, + ) -> Result<(), OwnershipError> { + // the contract must have an owner + let Some(current_owner) = &ownership.owner else { + return Err(OwnershipError::NoOwner); + }; + + // the sender must be the current owner + if sender != current_owner { + return Err(OwnershipError::NotOwner); + } + + Ok(()) + } + + /// Update the contract's ownership info based on the given action. + /// Return the updated ownership. + pub fn update_ownership( + &self, + deps: DepsMut, + block: &BlockInfo, + sender: &Addr, + action: Action, + ) -> Result, OwnershipError> { + match action { + Action::TransferOwnership { + new_owner, + expiry, + } => self.transfer_ownership(deps, sender, &new_owner, expiry), + Action::AcceptOwnership => self.accept_ownership(deps.storage, block, sender), + Action::RenounceOwnership => self.renounce_ownership(deps.storage, sender), + } + } + + /// Get the current ownership value. + pub fn get_ownership(&self, storage: &dyn Storage) -> StdResult> { + self.item.load(storage) + } + + /// Propose to transfer the contract's ownership to the given address, with an + /// optional deadline. + fn transfer_ownership( + &self, + deps: DepsMut, + sender: &Addr, + new_owner: &str, + expiry: Option, + ) -> Result, OwnershipError> { + self.item.update(deps.storage, |ownership| { + // the contract must have an owner + self.check_owner(&ownership, sender)?; + + // NOTE: We don't validate the expiry, i.e. asserting it is later than + // the current block time. + // + // This is because if the owner submits an invalid expiry, it won't have + // any negative effect - it's just that the pending owner won't be able + // to accept the ownership. + // + // By not doing the check, we save a little bit of gas. + // + // To fix the erorr, the owner can simply invoke `transfer_ownership` + // again with the correct expiry and overwrite the invalid one. + Ok(Ownership { + pending_owner: Some(deps.api.addr_validate(new_owner)?), + pending_expiry: expiry, + ..ownership + }) + }) + } + + /// Accept a pending ownership transfer. + fn accept_ownership( + &self, + store: &mut dyn Storage, + block: &BlockInfo, + sender: &Addr, + ) -> Result, OwnershipError> { + self.item.update(store, |ownership| { + // there must be an existing ownership transfer + let Some(pending_owner) = &ownership.pending_owner else { + return Err(OwnershipError::TransferNotFound); + }; + + // the sender must be the pending owner + if sender != pending_owner { + return Err(OwnershipError::NotPendingOwner); + }; + + // if the transfer has a deadline, it must not have been reached + if let Some(expiry) = &ownership.pending_expiry { + if expiry.is_expired(block) { + return Err(OwnershipError::TransferExpired); + } + } + + Ok(Ownership { + owner: ownership.pending_owner, + pending_owner: None, + pending_expiry: None, + }) + }) + } + + /// Set the contract's ownership as vacant permanently. + fn renounce_ownership( + &self, + store: &mut dyn Storage, + sender: &Addr, + ) -> Result, OwnershipError> { + self.item.update(store, |ownership| { + self.check_owner(&ownership, sender)?; + + Ok(Ownership { + owner: None, + pending_owner: None, + pending_expiry: None, + }) + }) + } +} + /// Actions that can be taken to alter the contract's ownership #[cw_serde] pub enum Action { @@ -79,7 +256,8 @@ pub enum OwnershipError { } /// Storage constant for the contract's ownership -const OWNERSHIP: Item> = Item::new("ownership"); +pub const OWNERSHIP_KEY: &str = "ownership"; +pub const OWNERSHIP: OwnershipStore = OwnershipStore::new(OWNERSHIP_KEY); /// Set the given address as the contract owner. /// @@ -89,13 +267,7 @@ pub fn initialize_owner( api: &dyn Api, owner: Option<&str>, ) -> StdResult> { - let ownership = Ownership { - owner: owner.map(|h| api.addr_validate(h)).transpose()?, - pending_owner: None, - pending_expiry: None, - }; - OWNERSHIP.save(storage, &ownership)?; - Ok(ownership) + OWNERSHIP.initialize_owner(storage, api, owner) } /// Return Ok(true) if the contract has an owner and it's the given address. @@ -103,36 +275,12 @@ pub fn initialize_owner( /// it's not the given address. /// Return Err if fails to load ownership info from storage. pub fn is_owner(store: &dyn Storage, addr: &Addr) -> StdResult { - let ownership = OWNERSHIP.load(store)?; - - if let Some(owner) = ownership.owner { - if *addr == owner { - return Ok(true); - } - } - - Ok(false) + OWNERSHIP.is_owner(store, addr) } /// Assert that an account is the contract's current owner. pub fn assert_owner(store: &dyn Storage, sender: &Addr) -> Result<(), OwnershipError> { - let ownership = OWNERSHIP.load(store)?; - check_owner(&ownership, sender) -} - -/// Assert that an account is the contract's current owner. -fn check_owner(ownership: &Ownership, sender: &Addr) -> Result<(), OwnershipError> { - // the contract must have an owner - let Some(current_owner) = &ownership.owner else { - return Err(OwnershipError::NoOwner); - }; - - // the sender must be the current owner - if sender != current_owner { - return Err(OwnershipError::NotOwner); - } - - Ok(()) + OWNERSHIP.assert_owner(store, sender) } /// Update the contract's ownership info based on the given action. @@ -143,19 +291,12 @@ pub fn update_ownership( sender: &Addr, action: Action, ) -> Result, OwnershipError> { - match action { - Action::TransferOwnership { - new_owner, - expiry, - } => transfer_ownership(deps, sender, &new_owner, expiry), - Action::AcceptOwnership => accept_ownership(deps.storage, block, sender), - Action::RenounceOwnership => renounce_ownership(deps.storage, sender), - } + OWNERSHIP.update_ownership(deps, block, sender, action) } /// Get the current ownership value. pub fn get_ownership(storage: &dyn Storage) -> StdResult> { - OWNERSHIP.load(storage) + OWNERSHIP.get_ownership(storage) } impl Ownership { @@ -202,89 +343,11 @@ impl Ownership { } } -fn none_or(or: Option<&T>) -> String { +// This is a nice helper, maybe move to dedicated utils package? +pub fn none_or(or: Option<&T>) -> String { or.map_or_else(|| "none".to_string(), |or| or.to_string()) } -/// Propose to transfer the contract's ownership to the given address, with an -/// optional deadline. -fn transfer_ownership( - deps: DepsMut, - sender: &Addr, - new_owner: &str, - expiry: Option, -) -> Result, OwnershipError> { - OWNERSHIP.update(deps.storage, |ownership| { - // the contract must have an owner - check_owner(&ownership, sender)?; - - // NOTE: We don't validate the expiry, i.e. asserting it is later than - // the current block time. - // - // This is because if the owner submits an invalid expiry, it won't have - // any negative effect - it's just that the pending owner won't be able - // to accept the ownership. - // - // By not doing the check, we save a little bit of gas. - // - // To fix the erorr, the owner can simply invoke `transfer_ownership` - // again with the correct expiry and overwrite the invalid one. - Ok(Ownership { - pending_owner: Some(deps.api.addr_validate(new_owner)?), - pending_expiry: expiry, - ..ownership - }) - }) -} - -/// Accept a pending ownership transfer. -fn accept_ownership( - store: &mut dyn Storage, - block: &BlockInfo, - sender: &Addr, -) -> Result, OwnershipError> { - OWNERSHIP.update(store, |ownership| { - // there must be an existing ownership transfer - let Some(pending_owner) = &ownership.pending_owner else { - return Err(OwnershipError::TransferNotFound); - }; - - // the sender must be the pending owner - if sender != pending_owner { - return Err(OwnershipError::NotPendingOwner); - }; - - // if the transfer has a deadline, it must not have been reached - if let Some(expiry) = &ownership.pending_expiry { - if expiry.is_expired(block) { - return Err(OwnershipError::TransferExpired); - } - } - - Ok(Ownership { - owner: ownership.pending_owner, - pending_owner: None, - pending_expiry: None, - }) - }) -} - -/// Set the contract's ownership as vacant permanently. -fn renounce_ownership( - store: &mut dyn Storage, - sender: &Addr, -) -> Result, OwnershipError> { - OWNERSHIP.update(store, |ownership| { - check_owner(&ownership, sender)?; - - Ok(Ownership { - owner: None, - pending_owner: None, - pending_expiry: None, - }) - }) -} - //------------------------------------------------------------------------------ // Tests //------------------------------------------------------------------------------ @@ -317,10 +380,10 @@ mod tests { let [larry, _, _] = mock_addresses(&deps.api); let ownership = - initialize_owner(&mut deps.storage, &deps.api, Some(larry.as_str())).unwrap(); + OWNERSHIP.initialize_owner(&mut deps.storage, &deps.api, Some(larry.as_str())).unwrap(); // ownership returned is same as ownership stored. - assert_eq!(ownership, OWNERSHIP.load(deps.as_ref().storage).unwrap()); + assert_eq!(ownership, OWNERSHIP.item.load(deps.as_ref().storage).unwrap()); assert_eq!( ownership, @@ -335,7 +398,7 @@ mod tests { #[test] fn initialize_ownership_no_owner() { let mut deps = mock_dependencies(); - let ownership = initialize_owner(&mut deps.storage, &deps.api, None).unwrap(); + let ownership = OWNERSHIP.initialize_owner(&mut deps.storage, &deps.api, None).unwrap(); assert_eq!( ownership, Ownership { @@ -353,20 +416,20 @@ mod tests { // case 1. owner has not renounced { - initialize_owner(&mut deps.storage, &deps.api, Some(larry.as_str())).unwrap(); + OWNERSHIP.initialize_owner(&mut deps.storage, &deps.api, Some(larry.as_str())).unwrap(); - let res = assert_owner(deps.as_ref().storage, &larry); + let res = OWNERSHIP.assert_owner(deps.as_ref().storage, &larry); assert!(res.is_ok()); - let res = assert_owner(deps.as_ref().storage, &jake); + let res = OWNERSHIP.assert_owner(deps.as_ref().storage, &jake); assert_eq!(res.unwrap_err(), OwnershipError::NotOwner); } // case 2. owner has renounced { - renounce_ownership(deps.as_mut().storage, &larry).unwrap(); + OWNERSHIP.renounce_ownership(deps.as_mut().storage, &larry).unwrap(); - let res = assert_owner(deps.as_ref().storage, &larry); + let res = OWNERSHIP.assert_owner(deps.as_ref().storage, &larry); assert_eq!(res.unwrap_err(), OwnershipError::NoOwner); } } @@ -376,11 +439,11 @@ mod tests { let mut deps = mock_dependencies(); let [larry, jake, pumpkin] = mock_addresses(&deps.api); - initialize_owner(&mut deps.storage, &deps.api, Some(larry.as_str())).unwrap(); + OWNERSHIP.initialize_owner(&mut deps.storage, &deps.api, Some(larry.as_str())).unwrap(); // non-owner cannot transfer ownership { - let err = update_ownership( + let err = OWNERSHIP.update_ownership( deps.as_mut(), &mock_block_at_height(12345), &jake, @@ -395,7 +458,7 @@ mod tests { // owner properly transfers ownership { - let ownership = update_ownership( + let ownership = OWNERSHIP.update_ownership( deps.as_mut(), &mock_block_at_height(12345), &larry, @@ -414,7 +477,7 @@ mod tests { }, ); - let saved_ownership = OWNERSHIP.load(deps.as_ref().storage).unwrap(); + let saved_ownership = OWNERSHIP.item.load(deps.as_ref().storage).unwrap(); assert_eq!(saved_ownership, ownership); } } @@ -424,11 +487,11 @@ mod tests { let mut deps = mock_dependencies(); let [larry, jake, pumpkin] = mock_addresses(&deps.api); - initialize_owner(&mut deps.storage, &deps.api, Some(larry.as_str())).unwrap(); + OWNERSHIP.initialize_owner(&mut deps.storage, &deps.api, Some(larry.as_str())).unwrap(); // cannot accept ownership when there isn't a pending ownership transfer { - let err = update_ownership( + let err = OWNERSHIP.update_ownership( deps.as_mut(), &mock_block_at_height(12345), &pumpkin, @@ -438,7 +501,7 @@ mod tests { assert_eq!(err, OwnershipError::TransferNotFound); } - transfer_ownership( + OWNERSHIP.transfer_ownership( deps.as_mut(), &larry, pumpkin.as_str(), @@ -448,7 +511,7 @@ mod tests { // non-pending owner cannot accept ownership { - let err = update_ownership( + let err = OWNERSHIP.update_ownership( deps.as_mut(), &mock_block_at_height(12345), &jake, @@ -460,7 +523,7 @@ mod tests { // cannot accept ownership if deadline has passed { - let err = update_ownership( + let err = OWNERSHIP.update_ownership( deps.as_mut(), &mock_block_at_height(69420), &pumpkin, @@ -472,7 +535,7 @@ mod tests { // pending owner properly accepts ownership before deadline { - let ownership = update_ownership( + let ownership = OWNERSHIP.update_ownership( deps.as_mut(), &mock_block_at_height(10000), &pumpkin, @@ -488,7 +551,7 @@ mod tests { }, ); - let saved_ownership = OWNERSHIP.load(deps.as_ref().storage).unwrap(); + let saved_ownership = OWNERSHIP.item.load(deps.as_ref().storage).unwrap(); assert_eq!(saved_ownership, ownership); } } @@ -503,11 +566,11 @@ mod tests { pending_owner: Some(pumpkin), pending_expiry: None, }; - OWNERSHIP.save(deps.as_mut().storage, &ownership).unwrap(); + OWNERSHIP.item.save(deps.as_mut().storage, &ownership).unwrap(); // non-owner cannot renounce { - let err = update_ownership( + let err = OWNERSHIP.update_ownership( deps.as_mut(), &mock_block_at_height(12345), &jake, @@ -519,7 +582,7 @@ mod tests { // owner properly renounces { - let ownership = update_ownership( + let ownership = OWNERSHIP.update_ownership( deps.as_mut(), &mock_block_at_height(12345), &larry, @@ -528,7 +591,7 @@ mod tests { .unwrap(); // ownership returned is same as ownership stored. - assert_eq!(ownership, OWNERSHIP.load(deps.as_ref().storage).unwrap()); + assert_eq!(ownership, OWNERSHIP.item.load(deps.as_ref().storage).unwrap()); assert_eq!( ownership, @@ -542,7 +605,7 @@ mod tests { // cannot renounce twice { - let err = update_ownership( + let err = OWNERSHIP.update_ownership( deps.as_mut(), &mock_block_at_height(12345), &larry, From c688a5d402283f892d47b1b94e9b9bced8f028e7 Mon Sep 17 00:00:00 2001 From: mr-t Date: Wed, 28 Feb 2024 20:30:22 +0100 Subject: [PATCH 2/6] cargo fmt --- packages/ownable/src/lib.rs | 162 +++++++++++++++++++----------------- 1 file changed, 86 insertions(+), 76 deletions(-) diff --git a/packages/ownable/src/lib.rs b/packages/ownable/src/lib.rs index 94c43a2..4ae7465 100644 --- a/packages/ownable/src/lib.rs +++ b/packages/ownable/src/lib.rs @@ -443,31 +443,33 @@ mod tests { // non-owner cannot transfer ownership { - let err = OWNERSHIP.update_ownership( - deps.as_mut(), - &mock_block_at_height(12345), - &jake, - Action::TransferOwnership { - new_owner: pumpkin.to_string(), - expiry: None, - }, - ) - .unwrap_err(); + let err = OWNERSHIP + .update_ownership( + deps.as_mut(), + &mock_block_at_height(12345), + &jake, + Action::TransferOwnership { + new_owner: pumpkin.to_string(), + expiry: None, + }, + ) + .unwrap_err(); assert_eq!(err, OwnershipError::NotOwner); } // owner properly transfers ownership { - let ownership = OWNERSHIP.update_ownership( - deps.as_mut(), - &mock_block_at_height(12345), - &larry, - Action::TransferOwnership { - new_owner: pumpkin.to_string(), - expiry: Some(Expiration::AtHeight(42069)), - }, - ) - .unwrap(); + let ownership = OWNERSHIP + .update_ownership( + deps.as_mut(), + &mock_block_at_height(12345), + &larry, + Action::TransferOwnership { + new_owner: pumpkin.to_string(), + expiry: Some(Expiration::AtHeight(42069)), + }, + ) + .unwrap(); assert_eq!( ownership, Ownership { @@ -491,57 +493,62 @@ mod tests { // cannot accept ownership when there isn't a pending ownership transfer { - let err = OWNERSHIP.update_ownership( - deps.as_mut(), - &mock_block_at_height(12345), - &pumpkin, - Action::AcceptOwnership, - ) - .unwrap_err(); + let err = OWNERSHIP + .update_ownership( + deps.as_mut(), + &mock_block_at_height(12345), + &pumpkin, + Action::AcceptOwnership, + ) + .unwrap_err(); assert_eq!(err, OwnershipError::TransferNotFound); } - OWNERSHIP.transfer_ownership( - deps.as_mut(), - &larry, - pumpkin.as_str(), - Some(Expiration::AtHeight(42069)), - ) - .unwrap(); + OWNERSHIP + .transfer_ownership( + deps.as_mut(), + &larry, + pumpkin.as_str(), + Some(Expiration::AtHeight(42069)), + ) + .unwrap(); // non-pending owner cannot accept ownership { - let err = OWNERSHIP.update_ownership( - deps.as_mut(), - &mock_block_at_height(12345), - &jake, - Action::AcceptOwnership, - ) - .unwrap_err(); + let err = OWNERSHIP + .update_ownership( + deps.as_mut(), + &mock_block_at_height(12345), + &jake, + Action::AcceptOwnership, + ) + .unwrap_err(); assert_eq!(err, OwnershipError::NotPendingOwner); } // cannot accept ownership if deadline has passed { - let err = OWNERSHIP.update_ownership( - deps.as_mut(), - &mock_block_at_height(69420), - &pumpkin, - Action::AcceptOwnership, - ) - .unwrap_err(); + let err = OWNERSHIP + .update_ownership( + deps.as_mut(), + &mock_block_at_height(69420), + &pumpkin, + Action::AcceptOwnership, + ) + .unwrap_err(); assert_eq!(err, OwnershipError::TransferExpired); } // pending owner properly accepts ownership before deadline { - let ownership = OWNERSHIP.update_ownership( - deps.as_mut(), - &mock_block_at_height(10000), - &pumpkin, - Action::AcceptOwnership, - ) - .unwrap(); + let ownership = OWNERSHIP + .update_ownership( + deps.as_mut(), + &mock_block_at_height(10000), + &pumpkin, + Action::AcceptOwnership, + ) + .unwrap(); assert_eq!( ownership, Ownership { @@ -570,25 +577,27 @@ mod tests { // non-owner cannot renounce { - let err = OWNERSHIP.update_ownership( - deps.as_mut(), - &mock_block_at_height(12345), - &jake, - Action::RenounceOwnership, - ) - .unwrap_err(); + let err = OWNERSHIP + .update_ownership( + deps.as_mut(), + &mock_block_at_height(12345), + &jake, + Action::RenounceOwnership, + ) + .unwrap_err(); assert_eq!(err, OwnershipError::NotOwner); } // owner properly renounces { - let ownership = OWNERSHIP.update_ownership( - deps.as_mut(), - &mock_block_at_height(12345), - &larry, - Action::RenounceOwnership, - ) - .unwrap(); + let ownership = OWNERSHIP + .update_ownership( + deps.as_mut(), + &mock_block_at_height(12345), + &larry, + Action::RenounceOwnership, + ) + .unwrap(); // ownership returned is same as ownership stored. assert_eq!(ownership, OWNERSHIP.item.load(deps.as_ref().storage).unwrap()); @@ -605,13 +614,14 @@ mod tests { // cannot renounce twice { - let err = OWNERSHIP.update_ownership( - deps.as_mut(), - &mock_block_at_height(12345), - &larry, - Action::RenounceOwnership, - ) - .unwrap_err(); + let err = OWNERSHIP + .update_ownership( + deps.as_mut(), + &mock_block_at_height(12345), + &larry, + Action::RenounceOwnership, + ) + .unwrap_err(); assert_eq!(err, OwnershipError::NoOwner); } } From 5f35ce37c42866501412584ec36e0978ff4be28d Mon Sep 17 00:00:00 2001 From: mr-t Date: Wed, 28 Feb 2024 20:31:38 +0100 Subject: [PATCH 3/6] cargo clippy --tests --- packages/ownable/derive/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/ownable/derive/src/lib.rs b/packages/ownable/derive/src/lib.rs index bf1e82d..8d205ce 100644 --- a/packages/ownable/derive/src/lib.rs +++ b/packages/ownable/derive/src/lib.rs @@ -40,7 +40,7 @@ fn merge_variants(metadata: TokenStream, left: TokenStream, right: TokenStream) }; // insert variants from the right to the left - variants.extend(to_add.into_iter()); + variants.extend(to_add); quote! { #left }.into() } From 4658ec91f4cab2576875af1d0b389b15cda94ee0 Mon Sep 17 00:00:00 2001 From: mr-t Date: Mon, 18 Mar 2024 12:24:30 +0100 Subject: [PATCH 4/6] use api and storage (instead of DepsMut) for being less restrictive --- packages/ownable/derive/src/lib.rs | 6 ++-- packages/ownable/src/lib.rs | 57 +++++++++++++++++++----------- 2 files changed, 40 insertions(+), 23 deletions(-) diff --git a/packages/ownable/derive/src/lib.rs b/packages/ownable/derive/src/lib.rs index 8d205ce..d962847 100644 --- a/packages/ownable/derive/src/lib.rs +++ b/packages/ownable/derive/src/lib.rs @@ -22,7 +22,8 @@ fn merge_variants(metadata: TokenStream, left: TokenStream, right: TokenStream) let Enum(DataEnum { variants, .. - }) = &mut left.data else { + }) = &mut left.data + else { return syn::Error::new(left.ident.span(), "only enums can accept variants") .to_compile_error() .into(); @@ -33,7 +34,8 @@ fn merge_variants(metadata: TokenStream, left: TokenStream, right: TokenStream) let Enum(DataEnum { variants: to_add, .. - }) = right.data else { + }) = right.data + else { return syn::Error::new(left.ident.span(), "only enums can provide variants") .to_compile_error() .into(); diff --git a/packages/ownable/src/lib.rs b/packages/ownable/src/lib.rs index 4ae7465..a529a47 100644 --- a/packages/ownable/src/lib.rs +++ b/packages/ownable/src/lib.rs @@ -3,7 +3,7 @@ use std::fmt::Display; use cosmwasm_schema::cw_serde; -use cosmwasm_std::{Addr, Api, Attribute, BlockInfo, DepsMut, StdError, StdResult, Storage}; +use cosmwasm_std::{Addr, Api, Attribute, BlockInfo, StdError, StdResult, Storage}; use cw_address_like::AddressLike; use cw_storage_plus::Item; @@ -102,7 +102,8 @@ impl<'a> OwnershipStore<'a> { /// Return the updated ownership. pub fn update_ownership( &self, - deps: DepsMut, + api: &dyn Api, + storage: &mut dyn Storage, block: &BlockInfo, sender: &Addr, action: Action, @@ -111,9 +112,9 @@ impl<'a> OwnershipStore<'a> { Action::TransferOwnership { new_owner, expiry, - } => self.transfer_ownership(deps, sender, &new_owner, expiry), - Action::AcceptOwnership => self.accept_ownership(deps.storage, block, sender), - Action::RenounceOwnership => self.renounce_ownership(deps.storage, sender), + } => self.transfer_ownership(api, storage, sender, &new_owner, expiry), + Action::AcceptOwnership => self.accept_ownership(storage, block, sender), + Action::RenounceOwnership => self.renounce_ownership(storage, sender), } } @@ -126,12 +127,13 @@ impl<'a> OwnershipStore<'a> { /// optional deadline. fn transfer_ownership( &self, - deps: DepsMut, + api: &dyn Api, + storage: &mut dyn Storage, sender: &Addr, new_owner: &str, expiry: Option, ) -> Result, OwnershipError> { - self.item.update(deps.storage, |ownership| { + self.item.update(storage, |ownership| { // the contract must have an owner self.check_owner(&ownership, sender)?; @@ -147,7 +149,7 @@ impl<'a> OwnershipStore<'a> { // To fix the erorr, the owner can simply invoke `transfer_ownership` // again with the correct expiry and overwrite the invalid one. Ok(Ownership { - pending_owner: Some(deps.api.addr_validate(new_owner)?), + pending_owner: Some(api.addr_validate(new_owner)?), pending_expiry: expiry, ..ownership }) @@ -286,12 +288,13 @@ pub fn assert_owner(store: &dyn Storage, sender: &Addr) -> Result<(), OwnershipE /// Update the contract's ownership info based on the given action. /// Return the updated ownership. pub fn update_ownership( - deps: DepsMut, + api: &dyn Api, + storage: &mut dyn Storage, block: &BlockInfo, sender: &Addr, action: Action, ) -> Result, OwnershipError> { - OWNERSHIP.update_ownership(deps, block, sender, action) + OWNERSHIP.update_ownership(api, storage, block, sender, action) } /// Get the current ownership value. @@ -445,7 +448,8 @@ mod tests { { let err = OWNERSHIP .update_ownership( - deps.as_mut(), + &deps.api.clone(), + deps.as_mut().storage, &mock_block_at_height(12345), &jake, Action::TransferOwnership { @@ -461,7 +465,8 @@ mod tests { { let ownership = OWNERSHIP .update_ownership( - deps.as_mut(), + &deps.api.clone(), + deps.as_mut().storage, &mock_block_at_height(12345), &larry, Action::TransferOwnership { @@ -489,13 +494,16 @@ mod tests { let mut deps = mock_dependencies(); let [larry, jake, pumpkin] = mock_addresses(&deps.api); - OWNERSHIP.initialize_owner(&mut deps.storage, &deps.api, Some(larry.as_str())).unwrap(); + OWNERSHIP + .initialize_owner(&mut deps.storage, &deps.api.clone(), Some(larry.as_str())) + .unwrap(); // cannot accept ownership when there isn't a pending ownership transfer { let err = OWNERSHIP .update_ownership( - deps.as_mut(), + &deps.api.clone(), + deps.as_mut().storage, &mock_block_at_height(12345), &pumpkin, Action::AcceptOwnership, @@ -506,7 +514,8 @@ mod tests { OWNERSHIP .transfer_ownership( - deps.as_mut(), + &deps.api.clone().clone(), + deps.as_mut().storage, &larry, pumpkin.as_str(), Some(Expiration::AtHeight(42069)), @@ -517,7 +526,8 @@ mod tests { { let err = OWNERSHIP .update_ownership( - deps.as_mut(), + &deps.api.clone(), + deps.as_mut().storage, &mock_block_at_height(12345), &jake, Action::AcceptOwnership, @@ -530,7 +540,8 @@ mod tests { { let err = OWNERSHIP .update_ownership( - deps.as_mut(), + &deps.api.clone(), + deps.as_mut().storage, &mock_block_at_height(69420), &pumpkin, Action::AcceptOwnership, @@ -543,7 +554,8 @@ mod tests { { let ownership = OWNERSHIP .update_ownership( - deps.as_mut(), + &deps.api.clone(), + deps.as_mut().storage, &mock_block_at_height(10000), &pumpkin, Action::AcceptOwnership, @@ -579,7 +591,8 @@ mod tests { { let err = OWNERSHIP .update_ownership( - deps.as_mut(), + &deps.api.clone(), + deps.as_mut().storage, &mock_block_at_height(12345), &jake, Action::RenounceOwnership, @@ -592,7 +605,8 @@ mod tests { { let ownership = OWNERSHIP .update_ownership( - deps.as_mut(), + &deps.api.clone(), + deps.as_mut().storage, &mock_block_at_height(12345), &larry, Action::RenounceOwnership, @@ -616,7 +630,8 @@ mod tests { { let err = OWNERSHIP .update_ownership( - deps.as_mut(), + &deps.api.clone(), + deps.as_mut().storage, &mock_block_at_height(12345), &larry, Action::RenounceOwnership, From eb4445e07cd0da2859c7313de569b83a773cc407 Mon Sep 17 00:00:00 2001 From: mintthemoon <105956535+mintthemoon@users.noreply.github.com> Date: Mon, 12 Aug 2024 13:32:27 -0400 Subject: [PATCH 5/6] add multi-owner `cw-ownable` with cosmwasm v2 (#1) * move all logic to `OwnershipStore`. This way `cw-ownable` is still a singleton. But can be re-use e.g. by defining `CREATOR: OwnershipStore = OwnershipStore::new("creator")` * cargo fmt * cargo clippy --tests * use api and storage (instead of DepsMut) for being less restrictive * fix ownable * remove unneccessary lifetime (clippy warning) --------- Co-authored-by: mr-t --- packages/optional-indexes/src/unique.rs | 2 +- packages/ownable/src/lib.rs | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/optional-indexes/src/unique.rs b/packages/optional-indexes/src/unique.rs index e29cd4f..691d962 100644 --- a/packages/optional-indexes/src/unique.rs +++ b/packages/optional-indexes/src/unique.rs @@ -22,7 +22,7 @@ pub struct OptionalUniqueIndex { phantom: PhantomData, } -impl<'a, IK, T, PK> OptionalUniqueIndex { +impl OptionalUniqueIndex { pub const fn new(idx_fn: fn(&T) -> Option, idx_namespace: &'static str) -> Self { Self { index: idx_fn, diff --git a/packages/ownable/src/lib.rs b/packages/ownable/src/lib.rs index a529a47..a7c424a 100644 --- a/packages/ownable/src/lib.rs +++ b/packages/ownable/src/lib.rs @@ -28,12 +28,12 @@ pub struct Ownership { pub pending_expiry: Option, } -pub struct OwnershipStore<'a> { - pub item: Item<'a, Ownership>, +pub struct OwnershipStore { + pub item: Item>, } -impl<'a> OwnershipStore<'a> { - pub const fn new(key: &'a str) -> Self { +impl OwnershipStore { + pub const fn new(key: &'static str) -> Self { Self { item: Item::new(key), } From a094824e6a190df2449662556a640417db1404ac Mon Sep 17 00:00:00 2001 From: mintthemoon <105956535+mintthemoon@users.noreply.github.com> Date: Tue, 13 Aug 2024 17:03:42 -0400 Subject: [PATCH 6/6] updates to cw-ownable from PR feedback (#2) * update ownable exports and revert API breaking change * fully revert update_ownership breaking API change and fix relevant tests --- packages/ownable/src/lib.rs | 49 ++++++++++++++----------------------- 1 file changed, 19 insertions(+), 30 deletions(-) diff --git a/packages/ownable/src/lib.rs b/packages/ownable/src/lib.rs index a7c424a..43ee96a 100644 --- a/packages/ownable/src/lib.rs +++ b/packages/ownable/src/lib.rs @@ -3,7 +3,7 @@ use std::fmt::Display; use cosmwasm_schema::cw_serde; -use cosmwasm_std::{Addr, Api, Attribute, BlockInfo, StdError, StdResult, Storage}; +use cosmwasm_std::{Addr, Api, Attribute, BlockInfo, DepsMut, StdError, StdResult, Storage}; use cw_address_like::AddressLike; use cw_storage_plus::Item; @@ -102,8 +102,7 @@ impl OwnershipStore { /// Return the updated ownership. pub fn update_ownership( &self, - api: &dyn Api, - storage: &mut dyn Storage, + deps: DepsMut, block: &BlockInfo, sender: &Addr, action: Action, @@ -112,9 +111,9 @@ impl OwnershipStore { Action::TransferOwnership { new_owner, expiry, - } => self.transfer_ownership(api, storage, sender, &new_owner, expiry), - Action::AcceptOwnership => self.accept_ownership(storage, block, sender), - Action::RenounceOwnership => self.renounce_ownership(storage, sender), + } => self.transfer_ownership(deps.api, deps.storage, sender, &new_owner, expiry), + Action::AcceptOwnership => self.accept_ownership(deps.storage, block, sender), + Action::RenounceOwnership => self.renounce_ownership(deps.storage, sender), } } @@ -258,8 +257,8 @@ pub enum OwnershipError { } /// Storage constant for the contract's ownership -pub const OWNERSHIP_KEY: &str = "ownership"; -pub const OWNERSHIP: OwnershipStore = OwnershipStore::new(OWNERSHIP_KEY); +const OWNERSHIP_KEY: &str = "ownership"; +const OWNERSHIP: OwnershipStore = OwnershipStore::new(OWNERSHIP_KEY); /// Set the given address as the contract owner. /// @@ -288,13 +287,12 @@ pub fn assert_owner(store: &dyn Storage, sender: &Addr) -> Result<(), OwnershipE /// Update the contract's ownership info based on the given action. /// Return the updated ownership. pub fn update_ownership( - api: &dyn Api, - storage: &mut dyn Storage, + deps: DepsMut, block: &BlockInfo, sender: &Addr, action: Action, ) -> Result, OwnershipError> { - OWNERSHIP.update_ownership(api, storage, block, sender, action) + OWNERSHIP.update_ownership(deps, block, sender, action) } /// Get the current ownership value. @@ -347,7 +345,7 @@ impl Ownership { } // This is a nice helper, maybe move to dedicated utils package? -pub fn none_or(or: Option<&T>) -> String { +fn none_or(or: Option<&T>) -> String { or.map_or_else(|| "none".to_string(), |or| or.to_string()) } @@ -448,8 +446,7 @@ mod tests { { let err = OWNERSHIP .update_ownership( - &deps.api.clone(), - deps.as_mut().storage, + deps.as_mut(), &mock_block_at_height(12345), &jake, Action::TransferOwnership { @@ -465,8 +462,7 @@ mod tests { { let ownership = OWNERSHIP .update_ownership( - &deps.api.clone(), - deps.as_mut().storage, + deps.as_mut(), &mock_block_at_height(12345), &larry, Action::TransferOwnership { @@ -502,8 +498,7 @@ mod tests { { let err = OWNERSHIP .update_ownership( - &deps.api.clone(), - deps.as_mut().storage, + deps.as_mut(), &mock_block_at_height(12345), &pumpkin, Action::AcceptOwnership, @@ -526,8 +521,7 @@ mod tests { { let err = OWNERSHIP .update_ownership( - &deps.api.clone(), - deps.as_mut().storage, + deps.as_mut(), &mock_block_at_height(12345), &jake, Action::AcceptOwnership, @@ -540,8 +534,7 @@ mod tests { { let err = OWNERSHIP .update_ownership( - &deps.api.clone(), - deps.as_mut().storage, + deps.as_mut(), &mock_block_at_height(69420), &pumpkin, Action::AcceptOwnership, @@ -554,8 +547,7 @@ mod tests { { let ownership = OWNERSHIP .update_ownership( - &deps.api.clone(), - deps.as_mut().storage, + deps.as_mut(), &mock_block_at_height(10000), &pumpkin, Action::AcceptOwnership, @@ -591,8 +583,7 @@ mod tests { { let err = OWNERSHIP .update_ownership( - &deps.api.clone(), - deps.as_mut().storage, + deps.as_mut(), &mock_block_at_height(12345), &jake, Action::RenounceOwnership, @@ -605,8 +596,7 @@ mod tests { { let ownership = OWNERSHIP .update_ownership( - &deps.api.clone(), - deps.as_mut().storage, + deps.as_mut(), &mock_block_at_height(12345), &larry, Action::RenounceOwnership, @@ -630,8 +620,7 @@ mod tests { { let err = OWNERSHIP .update_ownership( - &deps.api.clone(), - deps.as_mut().storage, + deps.as_mut(), &mock_block_at_height(12345), &larry, Action::RenounceOwnership,