Merge pull request #595 from public-awesome/tasio/royalty-update-fix

Royalty update fix

.cargo/config

@@ -2,4 +2,4 @@
 wasm = "build --release --target wasm32-unknown-unknown --lib"
 wasm-debug = "build --target wasm32-unknown-unknown"
 unit-test = "test --lib"
-integration-test = "test --package e2e -- --ignored --test-threads 1 -Z unstable-options --report-time"
+integration-test = "test --package e2e -- --ignored --test-threads 1"

contracts/collections/sg721-base/src/contract.rs

@@ -1,5 +1,4 @@
 use cw721_base::state::TokenInfo;
-use cw721_base::Extension;
 use url::Url;
 
 use cosmwasm_std::{
@@ -20,9 +19,11 @@ use sg_std::Response;
 use crate::msg::{CollectionInfoResponse, NftParams, QueryMsg};
 use crate::{ContractError, Sg721Contract};
 
-use crate::entry::{CONTRACT_NAME, CONTRACT_VERSION, EARLIEST_VERSION, TO_VERSION};
+use crate::entry::{CONTRACT_NAME, CONTRACT_VERSION};
 
 const MAX_DESCRIPTION_LENGTH: u32 = 512;
+const MAX_SHARE_DELTA_PCT: u64 = 2;
+const MAX_ROYALTY_SHARE_PCT: u64 = 10;
 
 impl<'a, T> Sg721Contract<'a, T>
 where
@@ -31,7 +32,7 @@ where
     pub fn instantiate(
         &self,
         deps: DepsMut,
-        _env: Env,
+        env: Env,
         info: MessageInfo,
         msg: InstantiateMsg,
     ) -> Result<Response, ContractError> {
@@ -91,6 +92,9 @@ where
 
         self.frozen_collection_info.save(deps.storage, &false)?;
 
+        self.royalty_updated_at
+            .save(deps.storage, &env.block.time)?;
+
         Ok(Response::new()
             .add_attribute("action", "instantiate")
             .add_attribute("collection_name", info.name)
@@ -185,7 +189,7 @@ where
     pub fn update_collection_info(
         &self,
         deps: DepsMut,
-        _env: Env,
+        env: Env,
         info: MessageInfo,
         collection_msg: UpdateCollectionInfoMsg<RoyaltyInfoResponse>,
     ) -> Result<Response, ContractError> {
@@ -221,35 +225,44 @@ where
 
         collection.explicit_content = collection_msg.explicit_content;
 
-        // convert collection royalty info to response for comparison
-        // convert from response to royalty info for storage
-        let current_royalty_info = collection
-            .royalty_info
-            .as_ref()
-            .map(|royalty_info| royalty_info.to_response());
-
-        let new_royalty_info = collection_msg
-            .royalty_info
-            .unwrap_or_else(|| current_royalty_info.clone());
-
-        // reminder: collection_msg.royalty_info is Option<Option<RoyaltyInfoResponse>>
-        collection.royalty_info = if let Some(royalty_info) = new_royalty_info {
-            // update royalty info to equal or less, else throw error
-            if let Some(royalty_info_res) = current_royalty_info {
-                if royalty_info.share > royalty_info_res.share {
-                    return Err(ContractError::RoyaltyShareIncreased {});
+        if let Some(Some(new_royalty_info_response)) = collection_msg.royalty_info {
+            let last_royalty_update = self.royalty_updated_at.load(deps.storage)?;
+            if last_royalty_update.plus_seconds(24 * 60 * 60) > env.block.time {
+                return Err(ContractError::InvalidRoyalties(
+                    "Royalties can only be updated once per day".to_string(),
+                ));
+            }
+
+            let new_royalty_info = RoyaltyInfo {
+                payment_address: deps
+                    .api
+                    .addr_validate(&new_royalty_info_response.payment_address)?,
+                share: share_validate(new_royalty_info_response.share)?,
+            };
+
+            if let Some(old_royalty_info) = collection.royalty_info {
+                if old_royalty_info.share < new_royalty_info.share {
+                    let share_delta = new_royalty_info.share.abs_diff(old_royalty_info.share);
+
+                    if share_delta > Decimal::percent(MAX_SHARE_DELTA_PCT) {
+                        return Err(ContractError::InvalidRoyalties(format!(
+                            "Share increase cannot be greater than {}%",
+                            MAX_SHARE_DELTA_PCT
+                        )));
+                    }
+                    if new_royalty_info.share > Decimal::percent(MAX_ROYALTY_SHARE_PCT) {
+                        return Err(ContractError::InvalidRoyalties(format!(
+                            "Share cannot be greater than {}%",
+                            MAX_ROYALTY_SHARE_PCT
+                        )));
+                    }
                 }
-            } else {
-                return Err(ContractError::RoyaltyShareIncreased {});
             }
 
-            Some(RoyaltyInfo {
-                payment_address: deps.api.addr_validate(&royalty_info.payment_address)?,
-                share: share_validate(royalty_info.share)?,
-            })
-        } else {
-            None
-        };
+            collection.royalty_info = Some(new_royalty_info);
+            self.royalty_updated_at
+                .save(deps.storage, &env.block.time)?;
+        }
 
         self.collection_info.save(deps.storage, &collection)?;
 
@@ -366,40 +379,50 @@ where
         })
     }
 
-    pub fn migrate(deps: DepsMut, _env: Env, _msg: Empty) -> Result<Response, ContractError> {
-        // make sure the correct contract is being upgraded, and it's being
-        // upgraded from the correct version.
+    pub fn migrate(mut deps: DepsMut, env: Env, _msg: Empty) -> Result<Response, ContractError> {
+        let prev_contract_version = cw2::get_contract_version(deps.storage)?;
+
+        let valid_contract_names = vec![CONTRACT_NAME.to_string()];
+        if !valid_contract_names.contains(&prev_contract_version.contract) {
+            return Err(StdError::generic_err("Invalid contract name for migration").into());
+        }
 
-        if CONTRACT_VERSION < EARLIEST_VERSION {
-            return Err(
-                StdError::generic_err("Cannot upgrade to a previous contract version").into(),
-            );
+        #[allow(clippy::cmp_owned)]
+        if prev_contract_version.version >= CONTRACT_VERSION.to_string() {
+            return Err(StdError::generic_err("Must upgrade contract version").into());
         }
-        if CONTRACT_VERSION > TO_VERSION {
-            return Err(
-                StdError::generic_err("Cannot upgrade to a previous contract version").into(),
-            );
+
+        let mut response = Response::new();
+
+        #[allow(clippy::cmp_owned)]
+        if prev_contract_version.version < "3.0.0".to_string() {
+            response = crate::upgrades::v3_0_0::upgrade(deps.branch(), &env, response)?;
         }
-        // if same version return
-        if CONTRACT_VERSION == TO_VERSION {
-            return Ok(Response::new());
+
+        #[allow(clippy::cmp_owned)]
+        if prev_contract_version.version < "3.1.0".to_string() {
+            response = crate::upgrades::v3_1_0::upgrade(deps.branch(), &env, response)?;
         }
 
-        // update contract version
-        cw2::set_contract_version(deps.storage, CONTRACT_NAME, TO_VERSION)?;
+        cw2::set_contract_version(deps.storage, CONTRACT_NAME, CONTRACT_VERSION)?;
+
+        response = response.add_event(
+            Event::new("migrate")
+                .add_attribute("from_name", prev_contract_version.contract)
+                .add_attribute("from_version", prev_contract_version.version)
+                .add_attribute("to_name", CONTRACT_NAME)
+                .add_attribute("to_version", CONTRACT_VERSION),
+        );
 
-        // perform the upgrade
-        let cw17_res = cw721_base::upgrades::v0_17::migrate::<Extension, Empty, Empty, Empty>(deps)
-            .map_err(|e| ContractError::MigrationError(e.to_string()))?;
-        let mut sgz_res = Response::new();
-        sgz_res.attributes = cw17_res.attributes;
-        Ok(sgz_res)
+        Ok(response)
     }
 }
 
 pub fn share_validate(share: Decimal) -> Result<Decimal, ContractError> {
     if share > Decimal::one() {
-        return Err(ContractError::InvalidRoyalties {});
+        return Err(ContractError::InvalidRoyalties(
+            "Share cannot be greater than 100%".to_string(),
+        ));
     }
 
     Ok(share)

contracts/collections/sg721-base/src/error.rs

@@ -35,8 +35,8 @@ pub enum ContractError {
     #[error("Approval not found for: {spender}")]
     ApprovalNotFound { spender: String },
 
-    #[error("Invalid Royalties")]
-    InvalidRoyalties {},
+    #[error("InvalidRoyalties: {0}")]
+    InvalidRoyalties(String),
 
     #[error("Description too long")]
     DescriptionTooLong {},
@@ -47,9 +47,6 @@ pub enum ContractError {
     #[error("CollectionInfoFrozen")]
     CollectionInfoFrozen {},
 
-    #[error("RoyaltyShareIncreased")]
-    RoyaltyShareIncreased {},
-
     #[error("MinterNotFound")]
     MinterNotFound {},
 

contracts/collections/sg721-base/src/lib.rs

@@ -2,6 +2,7 @@ pub mod contract;
 mod error;
 pub mod msg;
 mod state;
+pub mod upgrades;
 
 pub use crate::error::ContractError;
 pub use crate::state::Sg721Contract;
@@ -26,8 +27,6 @@ pub mod entry {
     // version info for migration info
     pub const CONTRACT_NAME: &str = "crates.io:sg721-base";
     pub const CONTRACT_VERSION: &str = env!("CARGO_PKG_VERSION");
-    pub const EARLIEST_VERSION: &str = "0.16.0";
-    pub const TO_VERSION: &str = "3.0.0";
 
     #[cfg_attr(not(feature = "library"), entry_point)]
     pub fn instantiate(

contracts/collections/sg721-base/src/state.rs

@@ -1,4 +1,4 @@
-use cosmwasm_std::Empty;
+use cosmwasm_std::{Empty, Timestamp};
 use cw_storage_plus::Item;
 use serde::{de::DeserializeOwned, Serialize};
 use sg721::{CollectionInfo, RoyaltyInfo};
@@ -15,6 +15,7 @@ where
 
     /// Instantiate set to false by the minter, then true by creator to freeze collection info
     pub frozen_collection_info: Item<'a, bool>,
+    pub royalty_updated_at: Item<'a, Timestamp>,
 }
 
 impl<'a, T> Default for Sg721Contract<'a, T>
@@ -26,6 +27,7 @@ where
             parent: cw721_base::Cw721Contract::default(),
             collection_info: Item::new("collection_info"),
             frozen_collection_info: Item::new("frozen_collection_info"),
+            royalty_updated_at: Item::new("royalty_updated_at"),
         }
     }
 }

contracts/collections/sg721-base/src/upgrades/mod.rs

@@ -0,0 +1,2 @@
+pub mod v3_0_0;
+pub mod v3_1_0;

contracts/collections/sg721-base/src/upgrades/v3_0_0.rs

@@ -0,0 +1,15 @@
+use crate::ContractError;
+
+use cosmwasm_std::{DepsMut, Empty, Env, Event};
+use cw721_base::Extension;
+use sg_std::Response;
+
+pub fn upgrade(deps: DepsMut, _env: &Env, response: Response) -> Result<Response, ContractError> {
+    let cw17_res = cw721_base::upgrades::v0_17::migrate::<Extension, Empty, Empty, Empty>(deps)
+        .map_err(|e| ContractError::MigrationError(e.to_string()))?;
+
+    let mut event = Event::new("migrate-3.0.0");
+    event = event.add_attributes(cw17_res.attributes);
+
+    Ok(response.add_event(event))
+}

contracts/collections/sg721-base/src/upgrades/v3_1_0.rs

@@ -0,0 +1,20 @@
+use crate::{ContractError, Sg721Contract};
+
+use cosmwasm_std::{DepsMut, Env, Event};
+use cw721_base::Extension;
+use sg_std::Response;
+
+pub fn upgrade(deps: DepsMut, env: &Env, response: Response) -> Result<Response, ContractError> {
+    let contract = Sg721Contract::<Extension>::default();
+
+    let royalty_updated_at = env.block.time.minus_seconds(60 * 60 * 24); // 24 hours ago
+
+    contract
+        .royalty_updated_at
+        .save(deps.storage, &royalty_updated_at)?;
+
+    let event = Event::new("migrate-3.1.0")
+        .add_attribute("royalty-updated", royalty_updated_at.to_string());
+
+    Ok(response.add_event(event))
+}