From ec6172e80b3eaab780d4bec5cc29612620351ede Mon Sep 17 00:00:00 2001 From: Noah Saso Date: Thu, 31 Oct 2024 18:43:45 -0400 Subject: [PATCH] pr review fixes --- .../schema/dao-voting-cw721-staked.json | 49 +++++++++++++--- .../dao-voting-cw721-staked/src/contract.rs | 58 +++++++++---------- .../voting/dao-voting-cw721-staked/src/msg.rs | 17 ++++-- .../src/testing/execute.rs | 10 ++-- packages/cw721-controllers/src/nft_claim.rs | 35 ++++++----- 5 files changed, 106 insertions(+), 63 deletions(-) diff --git a/contracts/voting/dao-voting-cw721-staked/schema/dao-voting-cw721-staked.json b/contracts/voting/dao-voting-cw721-staked/schema/dao-voting-cw721-staked.json index 0be42988b..731433436 100644 --- a/contracts/voting/dao-voting-cw721-staked/schema/dao-voting-cw721-staked.json +++ b/contracts/voting/dao-voting-cw721-staked/schema/dao-voting-cw721-staked.json @@ -265,7 +265,7 @@ "additionalProperties": false }, { - "description": "Claim NFTs that have been unstaked for the specified duration. If none are provided, it attempts to claim all legacy claims. If token IDs are provided, only those are claimed. If an empty vector is provided, it attempts to claim all non-legacy claims.", + "description": "Claim NFTs that have been unstaked for the specified duration.", "type": "object", "required": [ "claim_nfts" @@ -273,15 +273,12 @@ "properties": { "claim_nfts": { "type": "object", + "required": [ + "type" + ], "properties": { - "token_ids": { - "type": [ - "array", - "null" - ], - "items": { - "type": "string" - } + "type": { + "$ref": "#/definitions/ClaimType" } }, "additionalProperties": false @@ -440,6 +437,40 @@ "description": "Binary is a wrapper around Vec to add base64 de/serialization with serde. It also adds some helper methods to help encode inline.\n\nThis is only needed as serde-json-{core,wasm} has a horrible encoding for Vec. See also .", "type": "string" }, + "ClaimType": { + "oneOf": [ + { + "description": "Claims all legacy claims.", + "type": "string", + "enum": [ + "legacy" + ] + }, + { + "description": "Claims all non-legacy claims.", + "type": "string", + "enum": [ + "all" + ] + }, + { + "description": "Claims specific non-legacy NFTs.", + "type": "object", + "required": [ + "specific" + ], + "properties": { + "specific": { + "type": "array", + "items": { + "type": "string" + } + } + }, + "additionalProperties": false + } + ] + }, "Cw721ReceiveMsg": { "description": "Cw721ReceiveMsg should be de/serialized under `Receive()` variant in a ExecuteMsg", "type": "object", diff --git a/contracts/voting/dao-voting-cw721-staked/src/contract.rs b/contracts/voting/dao-voting-cw721-staked/src/contract.rs index ff8444cbe..493225702 100644 --- a/contracts/voting/dao-voting-cw721-staked/src/contract.rs +++ b/contracts/voting/dao-voting-cw721-staked/src/contract.rs @@ -17,7 +17,7 @@ use dao_voting::threshold::{ ActiveThresholdResponse, }; -use crate::msg::{ExecuteMsg, InstantiateMsg, MigrateMsg, NftContract, QueryMsg}; +use crate::msg::{ClaimType, ExecuteMsg, InstantiateMsg, MigrateMsg, NftContract, QueryMsg}; use crate::state::{ register_staked_nft, register_unstaked_nfts, Config, ACTIVE_THRESHOLD, CONFIG, DAO, HOOKS, INITIAL_NFTS, LEGACY_NFT_CLAIMS, NFT_BALANCES, NFT_CLAIMS, STAKED_NFTS_PER_OWNER, @@ -205,7 +205,7 @@ pub fn execute( match msg { ExecuteMsg::ReceiveNft(msg) => execute_stake(deps, env, info, msg), ExecuteMsg::Unstake { token_ids } => execute_unstake(deps, env, info, token_ids), - ExecuteMsg::ClaimNfts { token_ids } => execute_claim_nfts(deps, env, info, token_ids), + ExecuteMsg::ClaimNfts { r#type } => execute_claim_nfts(deps, env, info, r#type), ExecuteMsg::UpdateConfig { duration } => execute_update_config(info, deps, duration), ExecuteMsg::AddHook { addr } => execute_add_hook(deps, info, addr), ExecuteMsg::RemoveHook { addr } => execute_remove_hook(deps, info, addr), @@ -337,47 +337,43 @@ pub fn execute_claim_nfts( deps: DepsMut, env: Env, info: MessageInfo, - token_ids: Option>, + claim_type: ClaimType, ) -> Result { - let token_ids = match token_ids { + let token_ids = match claim_type { // attempt to claim all legacy NFTs - None => { - // claim all legacy NFTs - let legacy_nfts = - LEGACY_NFT_CLAIMS.claim_nfts(deps.storage, &info.sender, &env.block)?; + ClaimType::Legacy => { + LEGACY_NFT_CLAIMS.claim_nfts(deps.storage, &info.sender, &env.block)? + } + // attempt to claim all non-legacy NFTs + ClaimType::All => { + let token_ids = NFT_CLAIMS + .query_claims(deps.as_ref(), &info.sender, None, None)? + .nft_claims + .into_iter() + .filter(|nft| nft.release_at.is_expired(&env.block)) + .map(|nft| nft.token_id) + .collect::>(); - if legacy_nfts.is_empty() { - return Err(ContractError::NothingToClaim {}); + if !token_ids.is_empty() { + NFT_CLAIMS.claim_nfts(deps.storage, &info.sender, &token_ids, &env.block)?; } - legacy_nfts + token_ids } - // attempt to claim non-legacy NFTs - Some(token_ids) => { - let token_ids = if token_ids.is_empty() { - // query all NFT claims if none are provided - NFT_CLAIMS - .query_claims(deps.as_ref(), &info.sender, None, None)? - .nft_claims - .into_iter() - .filter(|nft| nft.release_at.is_expired(&env.block)) - .map(|nft| nft.token_id) - .collect::>() - } else { - // use provided NFTs if any - token_ids - }; - - if token_ids.is_empty() { - return Err(ContractError::NothingToClaim {}); + // attempt to claim specific non-legacy NFTs + ClaimType::Specific(token_ids) => { + if !token_ids.is_empty() { + NFT_CLAIMS.claim_nfts(deps.storage, &info.sender, &token_ids, &env.block)?; } - NFT_CLAIMS.claim_nfts(deps.storage, &info.sender, &token_ids, &env.block)?; - token_ids } }; + if token_ids.is_empty() { + return Err(ContractError::NothingToClaim {}); + } + let config = CONFIG.load(deps.storage)?; let msgs = token_ids diff --git a/contracts/voting/dao-voting-cw721-staked/src/msg.rs b/contracts/voting/dao-voting-cw721-staked/src/msg.rs index 8643d6ddc..6722d0262 100644 --- a/contracts/voting/dao-voting-cw721-staked/src/msg.rs +++ b/contracts/voting/dao-voting-cw721-staked/src/msg.rs @@ -54,11 +54,8 @@ pub enum ExecuteMsg { /// sender. token_ids must have unique values and have non-zero /// length. Unstake { token_ids: Vec }, - /// Claim NFTs that have been unstaked for the specified duration. If none - /// are provided, it attempts to claim all legacy claims. If token IDs are - /// provided, only those are claimed. If an empty vector is provided, it - /// attempts to claim all non-legacy claims. - ClaimNfts { token_ids: Option> }, + /// Claim NFTs that have been unstaked for the specified duration. + ClaimNfts { r#type: ClaimType }, /// Updates the contract configuration, namely unstaking duration. /// Only callable by the DAO that initialized this voting contract. UpdateConfig { duration: Option }, @@ -75,6 +72,16 @@ pub enum ExecuteMsg { }, } +#[cw_serde] +pub enum ClaimType { + /// Claims all legacy claims. + Legacy, + /// Claims all non-legacy claims. + All, + /// Claims specific non-legacy NFTs. + Specific(Vec), +} + #[active_query] #[voting_module_query] #[cw_serde] diff --git a/contracts/voting/dao-voting-cw721-staked/src/testing/execute.rs b/contracts/voting/dao-voting-cw721-staked/src/testing/execute.rs index a80b0b90c..acfbeea1c 100644 --- a/contracts/voting/dao-voting-cw721-staked/src/testing/execute.rs +++ b/contracts/voting/dao-voting-cw721-staked/src/testing/execute.rs @@ -5,7 +5,7 @@ use cw_multi_test::{App, AppResponse, Executor}; use anyhow::Result as AnyResult; use cw_utils::Duration; -use crate::msg::ExecuteMsg; +use crate::msg::{ClaimType, ExecuteMsg}; // Shorthand for an unchecked address. macro_rules! addr { @@ -111,7 +111,7 @@ pub fn claim_nfts(app: &mut App, module: &Addr, sender: &str) -> AnyResult AnyResul app.execute_contract( addr!(sender), module.clone(), - &ExecuteMsg::ClaimNfts { token_ids: None }, + &ExecuteMsg::ClaimNfts { + r#type: ClaimType::Legacy, + }, &[], ) } diff --git a/packages/cw721-controllers/src/nft_claim.rs b/packages/cw721-controllers/src/nft_claim.rs index f2fe60b9e..ca1c186fe 100644 --- a/packages/cw721-controllers/src/nft_claim.rs +++ b/packages/cw721-controllers/src/nft_claim.rs @@ -12,8 +12,8 @@ pub enum NftClaimError { #[error("NFT claim not found for {token_id}")] NotFound { token_id: String }, - #[error("One or more NFTs is not ready to be claimed")] - NotReady {}, + #[error("NFT with ID {token_id} is not ready to be claimed")] + NotReady { token_id: String }, } #[cw_serde] @@ -80,12 +80,15 @@ impl<'a> NftClaims<'a> { .map(|token_id| -> Result<(), NftClaimError> { match self.0.may_load(storage, (addr, token_id)) { Ok(Some(expiration)) => { - // if claim is expired, continue + // if claim is expired, remove it and continue if expiration.is_expired(block) { + self.0.remove(storage, (addr, token_id)); Ok(()) } else { // if claim is not expired, error - Err(NftClaimError::NotReady {}) + Err(NftClaimError::NotReady { + token_id: token_id.to_string(), + }) } } // if claim is not found, error @@ -95,14 +98,8 @@ impl<'a> NftClaims<'a> { Err(e) => Err(e.into()), } }) - .collect::, NftClaimError>>()?; - - // remove all now that we've confirmed they're mature - token_ids - .iter() - .for_each(|token_id| self.0.remove(storage, (addr, token_id))); - - Ok(()) + .collect::, NftClaimError>>() + .map(|_| ()) } pub fn query_claims( @@ -120,7 +117,12 @@ impl<'a> NftClaims<'a> { .prefix(address) .range(deps.storage, start, None, Order::Ascending) .take(limit) - .map(|item| item.map(|(token_id, v)| NftClaim::new(token_id, v))) + .map(|item| { + item.map(|(token_id, release_at)| NftClaim { + token_id, + release_at, + }) + }) .collect::>>()?; Ok(NftClaimsResponse { nft_claims }) @@ -314,7 +316,12 @@ mod test { &env.block, ) .unwrap_err(); - assert_eq!(error, NftClaimError::NotReady {}); + assert_eq!( + error, + NftClaimError::NotReady { + token_id: TEST_CRYPTO_PUNKS_TOKEN_ID.to_string() + } + ); let saved_claims = claims .0