make cw721 claims paginatable with backwards compatibility

Cargo.toml

@@ -158,3 +158,6 @@ dao-proposal-multiple-v241 = { package = "dao-proposal-multiple", version = "=2.
 dao-proposal-single-v241 = { package = "dao-proposal-single", version = "=2.4.1" }
 dao-voting-cw4-v241 = { package = "dao-voting-cw4", version = "=2.4.1" }
 dao-voting-v241 = { package = "dao-voting", version = "=2.4.1" }
+
+# v2.5.0 dependencies.
+cw721-controllers-v250 = { package = "cw721-controllers", version = "=2.5.0" }

ci/integration-tests/src/tests/dao_voting_cw721_staked_test.rs

@@ -1,4 +1,4 @@
-use cosm_orc::orchestrator::{ExecReq, SigningKey};
+use cosm_orc::orchestrator::SigningKey;
 use cosmwasm_std::{Binary, Empty, Uint128};
 use cw_utils::Duration;
 use test_context::test_context;
@@ -126,7 +126,9 @@ pub fn claim_nfts(chain: &mut Chain, sender: &SigningKey) {
         .execute(
             CONTRACT_NAME,
             "claim_nfts",
-            &module::msg::ExecuteMsg::ClaimNfts {},
+            &module::msg::ExecuteMsg::ClaimNfts {
+                token_ids: Some(vec![]),
+            },
             sender,
             vec![],
         )
@@ -189,79 +191,3 @@ fn cw721_stake_tokens(chain: &mut Chain) {
     let voting_power = query_voting_power(chain, &user_addr, None);
     assert_eq!(voting_power, Uint128::zero());
 }
-
-#[test_context(Chain)]
-#[test]
-#[ignore]
-fn cw721_stake_max_claims_works(chain: &mut Chain) {
-    use module::state::MAX_CLAIMS;
-
-    let user_addr = chain.users["user1"].account.address.clone();
-    let user_key = chain.users["user1"].key.clone();
-
-    let CommonTest { module, .. } =
-        setup_test(chain, Some(Duration::Height(1)), &user_key, &user_addr);
-
-    // Create `MAX_CLAIMS` claims.
-
-    // batch_size * 3 = the number of msgs to be batched per tx.
-    // We cant batch all of the msgs under a single tx because we hit MAX_BLOCK_GAS limits.
-    let batch_size = 10;
-    let mut total_msgs = 0;
-
-    let mut reqs = vec![];
-    for i in 0..MAX_CLAIMS {
-        let token_id = i.to_string();
-
-        reqs.push(ExecReq {
-            contract_name: CW721_NAME.to_string(),
-            msg: Box::new(cw721_base::ExecuteMsg::Mint::<Empty, Empty> {
-                token_id: token_id.clone(),
-                owner: user_addr.to_string(),
-                token_uri: None,
-                extension: Empty::default(),
-            }),
-            funds: vec![],
-        });
-
-        reqs.push(ExecReq {
-            contract_name: CW721_NAME.to_string(),
-            msg: Box::new(cw721::Cw721ExecuteMsg::SendNft {
-                contract: module.to_string(),
-                token_id: token_id.clone(),
-                msg: Binary::default(),
-            }),
-            funds: vec![],
-        });
-
-        reqs.push(ExecReq {
-            contract_name: CONTRACT_NAME.to_string(),
-            msg: Box::new(module::msg::ExecuteMsg::Unstake {
-                token_ids: vec![token_id],
-            }),
-            funds: vec![],
-        });
-
-        if (i != 0 && i % batch_size == 0) || i == MAX_CLAIMS - 1 {
-            total_msgs += reqs.len();
-
-            chain
-                .orc
-                .execute_batch("batch_cw721_stake_max_claims", reqs, &user_key)
-                .unwrap();
-
-            reqs = vec![];
-        }
-    }
-
-    assert_eq!(total_msgs as u64, MAX_CLAIMS * 3);
-
-    chain
-        .orc
-        .poll_for_n_blocks(1, core::time::Duration::from_millis(20_000), false)
-        .unwrap();
-
-    // If this works, we're golden. Other tests make sure that the
-    // NFTs get returned as a result of this.
-    claim_nfts(chain, &user_key);
-}

contracts/voting/dao-voting-cw721-roles/Cargo.toml

@@ -22,7 +22,6 @@ dao-cw721-extensions = { workspace = true }
 dao-dao-macros = { workspace = true }
 dao-interface = { workspace = true }
 cw721-base = { workspace = true, features = ["library"] }
-cw721-controllers = { workspace = true }
 cw-ownable = { workspace = true }
 cw721 = { workspace = true }
 cw-utils = { workspace = true }

contracts/voting/dao-voting-cw721-staked/Cargo.toml

@@ -31,6 +31,7 @@ cw-hooks = { workspace = true }
 cw721 = { workspace = true }
 cw721-base = { workspace = true, features = ["library"] }
 cw721-controllers = { workspace = true }
+cw721-controllers-v250 = { workspace = true }
 cw-utils = { workspace = true }
 cw2 = { workspace = true }
 dao-dao-macros = { workspace = true }

contracts/voting/dao-voting-cw721-staked/src/contract.rs

@@ -20,7 +20,8 @@ use dao_voting::threshold::{
 use crate::msg::{ExecuteMsg, InstantiateMsg, MigrateMsg, NftContract, QueryMsg};
 use crate::state::{
     register_staked_nft, register_unstaked_nfts, Config, ACTIVE_THRESHOLD, CONFIG, DAO, HOOKS,
-    INITIAL_NFTS, MAX_CLAIMS, NFT_BALANCES, NFT_CLAIMS, STAKED_NFTS_PER_OWNER, TOTAL_STAKED_NFTS,
+    INITIAL_NFTS, LEGACY_NFT_CLAIMS, NFT_BALANCES, NFT_CLAIMS, STAKED_NFTS_PER_OWNER,
+    TOTAL_STAKED_NFTS,
 };
 use crate::ContractError;
 
@@ -204,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 {} => execute_claim_nfts(deps, env, info),
+        ExecuteMsg::ClaimNfts { token_ids } => execute_claim_nfts(deps, env, info, token_ids),
         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),
@@ -314,13 +315,6 @@ pub fn execute_unstake(
         }
 
         Some(duration) => {
-            let outstanding_claims = NFT_CLAIMS
-                .query_claims(deps.as_ref(), &info.sender)?
-                .nft_claims;
-            if outstanding_claims.len() + token_ids.len() > MAX_CLAIMS as usize {
-                return Err(ContractError::TooManyClaims {});
-            }
-
             // Out of gas here is fine - just try again with fewer
             // tokens.
             NFT_CLAIMS.create_nft_claims(
@@ -343,22 +337,57 @@ pub fn execute_claim_nfts(
     deps: DepsMut,
     env: Env,
     info: MessageInfo,
+    token_ids: Option<Vec<String>>,
 ) -> Result<Response, ContractError> {
-    let nfts = NFT_CLAIMS.claim_nfts(deps.storage, &info.sender, &env.block)?;
-    if nfts.is_empty() {
-        return Err(ContractError::NothingToClaim {});
-    }
+    let token_ids = match token_ids {
+        // 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)?;
+
+            if legacy_nfts.is_empty() {
+                return Err(ContractError::NothingToClaim {});
+            }
+
+            legacy_nfts
+        }
+        // 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::<Vec<_>>()
+            } else {
+                // use provided NFTs if any
+                token_ids
+            };
+
+            if token_ids.is_empty() {
+                return Err(ContractError::NothingToClaim {});
+            }
+
+            NFT_CLAIMS.claim_nfts(deps.storage, &info.sender, &token_ids, &env.block)?;
+
+            token_ids
+        }
+    };
 
     let config = CONFIG.load(deps.storage)?;
 
-    let msgs = nfts
+    let msgs = token_ids
         .into_iter()
-        .map(|nft| -> StdResult<CosmosMsg> {
+        .map(|token_id| -> StdResult<CosmosMsg> {
             Ok(WasmMsg::Execute {
                 contract_addr: config.nft_address.to_string(),
                 msg: to_json_binary(&cw721::Cw721ExecuteMsg::TransferNft {
                     recipient: info.sender.to_string(),
-                    token_id: nft,
+                    token_id,
                 })?,
                 funds: vec![],
             }
@@ -485,7 +514,11 @@ pub fn query(deps: Deps, env: Env, msg: QueryMsg) -> StdResult<Binary> {
         QueryMsg::Dao {} => query_dao(deps),
         QueryMsg::Info {} => query_info(deps),
         QueryMsg::IsActive {} => query_is_active(deps, env),
-        QueryMsg::NftClaims { address } => query_nft_claims(deps, address),
+        QueryMsg::NftClaims {
+            address,
+            start_after,
+            limit,
+        } => query_nft_claims(deps, address, start_after, limit),
         QueryMsg::Hooks {} => query_hooks(deps),
         QueryMsg::StakedNfts {
             address,
@@ -606,8 +639,31 @@ pub fn query_dao(deps: Deps) -> StdResult<Binary> {
     to_json_binary(&dao)
 }
 
-pub fn query_nft_claims(deps: Deps, address: String) -> StdResult<Binary> {
-    to_json_binary(&NFT_CLAIMS.query_claims(deps, &deps.api.addr_validate(&address)?)?)
+pub fn query_nft_claims(
+    deps: Deps,
+    address: String,
+    start_after: Option<String>,
+    limit: Option<u32>,
+) -> StdResult<Binary> {
+    let addr = deps.api.addr_validate(&address)?;
+
+    // load all legacy claims since it does not support pagination
+    let legacy_claims = LEGACY_NFT_CLAIMS
+        .query_claims(deps, &addr)?
+        .nft_claims
+        .into_iter()
+        .map(|c| cw721_controllers::NftClaim::new(c.token_id, c.release_at))
+        .collect::<Vec<_>>();
+
+    // paginate all new claims
+    let claims = NFT_CLAIMS
+        .query_claims(deps, &addr, start_after.as_ref(), limit)?
+        .nft_claims;
+
+    // combine legacy and new claims
+    let nft_claims = legacy_claims.into_iter().chain(claims).collect();
+
+    to_json_binary(&cw721_controllers::NftClaimsResponse { nft_claims })
 }
 
 pub fn query_hooks(deps: Deps) -> StdResult<Binary> {

contracts/voting/dao-voting-cw721-staked/src/error.rs

@@ -20,6 +20,9 @@ pub enum ContractError {
     #[error(transparent)]
     UnstakingDurationError(#[from] dao_voting::duration::UnstakingDurationError),
 
+    #[error(transparent)]
+    NftClaimError(#[from] cw721_controllers::NftClaimError),
+
     #[error("Can not stake that which has already been staked")]
     AlreadyStaked {},
 
@@ -44,9 +47,6 @@ pub enum ContractError {
     #[error("Can not unstake that which you have not staked (unstaking {token_id})")]
     NotStaked { token_id: String },
 
-    #[error("Too many outstanding claims. Claim some tokens before unstaking more.")]
-    TooManyClaims {},
-
     #[error("Unauthorized")]
     Unauthorized {},
 

contracts/voting/dao-voting-cw721-staked/src/msg.rs

@@ -54,8 +54,11 @@ pub enum ExecuteMsg {
     /// sender. token_ids must have unique values and have non-zero
     /// length.
     Unstake { token_ids: Vec<String> },
-    /// Claim NFTs that have been unstaked for the specified duration.
-    ClaimNfts {},
+    /// 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<Vec<String>> },
     /// Updates the contract configuration, namely unstaking duration.
     /// Only callable by the DAO that initialized this voting contract.
     UpdateConfig { duration: Option<Duration> },
@@ -80,7 +83,11 @@ pub enum QueryMsg {
     #[returns(crate::state::Config)]
     Config {},
     #[returns(::cw721_controllers::NftClaimsResponse)]
-    NftClaims { address: String },
+    NftClaims {
+        address: String,
+        start_after: Option<String>,
+        limit: Option<u32>,
+    },
     #[returns(::cw_controllers::HooksResponse)]
     Hooks {},
     // List the staked NFTs for a given address.

contracts/voting/dao-voting-cw721-staked/src/state.rs

@@ -42,9 +42,14 @@ pub const TOTAL_STAKED_NFTS: SnapshotItem<Uint128> = SnapshotItem::new(
     Strategy::EveryBlock,
 );
 
-/// The maximum number of claims that may be outstanding.
-pub const MAX_CLAIMS: u64 = 70;
-pub const NFT_CLAIMS: NftClaims = NftClaims::new("nft_claims");
+/// The legacy NFT claims storage uses a non-paginatable vector, which limits
+/// the number of claims that may be outstanding. This is horrible UX,
+/// especially for large NFT collections. To allow DAOs to upgrade, we must keep
+/// the legacy NFT claims storage, but we can paginate the new storage.
+pub const LEGACY_NFT_CLAIMS: cw721_controllers_v250::NftClaims =
+    cw721_controllers_v250::NftClaims::new("nft_claims");
+
+pub const NFT_CLAIMS: NftClaims = NftClaims::new("nc");
 
 // Hooks to contracts that will receive staking and unstaking
 // messages.