From 2f4d92974fe521aaebcd04be84f1547038bf82f0 Mon Sep 17 00:00:00 2001 From: Sergey Ratiashvili Date: Mon, 25 Nov 2024 14:21:44 +0100 Subject: [PATCH 1/2] fix: review fix --- contracts/core/src/contract.rs | 2 +- contracts/factory/src/contract.rs | 123 ++++++++++++++++------------ contracts/factory/src/tests.rs | 127 ++++++++++++++++++++--------- packages/base/src/state/factory.rs | 36 +------- packages/helpers/src/phonebook.rs | 2 +- 5 files changed, 161 insertions(+), 129 deletions(-) diff --git a/contracts/core/src/contract.rs b/contracts/core/src/contract.rs index 505b268e..146a0cd7 100644 --- a/contracts/core/src/contract.rs +++ b/contracts/core/src/contract.rs @@ -2,7 +2,7 @@ use cosmwasm_schema::cw_serde; use cosmwasm_std::{ attr, ensure, ensure_eq, ensure_ne, to_json_binary, Addr, Attribute, BankQuery, Binary, Coin, CosmosMsg, CustomQuery, Decimal, Deps, DepsMut, Env, MessageInfo, Order, QueryRequest, Reply, - Response, StdError, StdResult, SubMsg, SubMsgResult, Uint128, Uint64, WasmMsg, WasmQuery, + Response, StdError, StdResult, SubMsg, SubMsgResult, Uint128, Uint64, WasmMsg, }; use cw_storage_plus::Bound; use drop_helpers::answer::response; diff --git a/contracts/factory/src/contract.rs b/contracts/factory/src/contract.rs index ccb7f204..e54ade73 100644 --- a/contracts/factory/src/contract.rs +++ b/contracts/factory/src/contract.rs @@ -2,7 +2,7 @@ use std::collections::HashMap; use cosmwasm_std::{ attr, instantiate2_address, to_json_binary, Binary, CodeInfoResponse, CosmosMsg, Deps, DepsMut, - Env, HexBinary, MessageInfo, Response, Uint128, WasmMsg, + Env, HexBinary, MessageInfo, Response, StdResult, Uint128, WasmMsg, }; use drop_helpers::answer::response; use drop_helpers::phonebook::{ @@ -12,7 +12,6 @@ use drop_helpers::phonebook::{ VALIDATORS_SET_CONTRACT, WITHDRAWAL_MANAGER_CONTRACT, WITHDRAWAL_VOUCHER_CONTRACT, }; use drop_staking_base::error::factory::ContractResult; -use drop_staking_base::state::factory::Phonebook; use drop_staking_base::state::splitter::Config as SplitterConfig; use drop_staking_base::{ msg::factory::{ @@ -209,35 +208,54 @@ pub fn instantiate( deps.api.addr_humanize(&lsm_share_bond_provider_address)?; let native_bond_provider_contract = deps.api.addr_humanize(&native_bond_provider_address)?; + STATE.save(deps.storage, CORE_CONTRACT, &core_contract.clone())?; STATE.save( deps.storage, - &Phonebook::new([ - (CORE_CONTRACT, core_contract.clone()), - ( - WITHDRAWAL_MANAGER_CONTRACT, - withdrawal_manager_contract.clone(), - ), - (REWARDS_MANAGER_CONTRACT, rewards_manager_contract.clone()), - (TOKEN_CONTRACT, token_contract.clone()), - (PUPPETEER_CONTRACT, puppeteer_contract.clone()), - ( - WITHDRAWAL_VOUCHER_CONTRACT, - withdrawal_voucher_contract.clone(), - ), - (STRATEGY_CONTRACT, strategy_contract.clone()), - (VALIDATORS_SET_CONTRACT, validators_set_contract.clone()), - (DISTRIBUTION_CONTRACT, distribution_contract.clone()), - (SPLITTER_CONTRACT, splitter_contract.clone()), - ( - LSM_SHARE_BOND_PROVIDER_CONTRACT, - lsm_share_bond_provider_contract.clone(), - ), - ( - NATIVE_BOND_PROVIDER_CONTRACT, - native_bond_provider_contract.clone(), - ), - (REWARDS_PUMP_CONTRACT, rewards_pump_contract.clone()), - ]), + WITHDRAWAL_MANAGER_CONTRACT, + &withdrawal_manager_contract.clone(), + )?; + STATE.save( + deps.storage, + REWARDS_MANAGER_CONTRACT, + &rewards_manager_contract.clone(), + )?; + STATE.save(deps.storage, TOKEN_CONTRACT, &token_contract.clone())?; + STATE.save( + deps.storage, + PUPPETEER_CONTRACT, + &puppeteer_contract.clone(), + )?; + STATE.save( + deps.storage, + WITHDRAWAL_VOUCHER_CONTRACT, + &withdrawal_voucher_contract.clone(), + )?; + STATE.save(deps.storage, STRATEGY_CONTRACT, &strategy_contract.clone())?; + STATE.save( + deps.storage, + VALIDATORS_SET_CONTRACT, + &validators_set_contract.clone(), + )?; + STATE.save( + deps.storage, + DISTRIBUTION_CONTRACT, + &distribution_contract.clone(), + )?; + STATE.save(deps.storage, SPLITTER_CONTRACT, &splitter_contract.clone())?; + STATE.save( + deps.storage, + LSM_SHARE_BOND_PROVIDER_CONTRACT, + &lsm_share_bond_provider_contract.clone(), + )?; + STATE.save( + deps.storage, + NATIVE_BOND_PROVIDER_CONTRACT, + &native_bond_provider_contract.clone(), + )?; + STATE.save( + deps.storage, + REWARDS_PUMP_CONTRACT, + &rewards_pump_contract.clone(), )?; let msgs = vec![ @@ -462,26 +480,29 @@ pub fn query(deps: Deps, _env: Env, msg: QueryMsg) -> ContractResu } fn query_state(deps: Deps) -> ContractResult { - let state = STATE.load(deps.storage)?; - Ok(to_json_binary(&state.map)?) + let state = STATE.range(deps.storage, None, None, cosmwasm_std::Order::Ascending); + let out = state + .collect::>>()? + .iter() + .map(|(k, v): &(String, cosmwasm_std::Addr)| (k.clone(), v.to_string())) + .collect::>(); + Ok(to_json_binary(&out)?) } fn query_locate(deps: Deps, items: Vec) -> ContractResult { let mut contracts: HashMap = HashMap::new(); - let state = STATE.load(deps.storage)?; + for item in items { - let item_key = item.clone(); - let contract = state.get_as_result(&item_key)?; - contracts.insert(item.clone(), contract.to_string()); + let addr = STATE.load(deps.storage, &item)?; + contracts.insert(item.clone(), addr.to_string()); } Ok(to_json_binary(&contracts)?) } fn query_pause_info(deps: Deps) -> ContractResult { - let state = STATE.load(deps.storage)?; - let core_contract = state.get_as_result(CORE_CONTRACT)?; - let withdrawal_manager_contract = state.get_as_result(WITHDRAWAL_MANAGER_CONTRACT)?; - let rewards_manager_contract = state.get_as_result(REWARDS_MANAGER_CONTRACT)?; + let core_contract = STATE.load(deps.storage, CORE_CONTRACT)?; + let withdrawal_manager_contract = STATE.load(deps.storage, WITHDRAWAL_MANAGER_CONTRACT)?; + let rewards_manager_contract = STATE.load(deps.storage, REWARDS_MANAGER_CONTRACT)?; to_json_binary(&drop_staking_base::state::factory::PauseInfoResponse { core: deps @@ -524,10 +545,9 @@ pub fn execute( fn exec_pause(deps: DepsMut, info: MessageInfo) -> ContractResult> { cw_ownable::assert_owner(deps.storage, &info.sender)?; - let state = STATE.load(deps.storage)?; - let core_contract = state.get_as_result(CORE_CONTRACT)?; - let withdrawal_manager_contract = state.get_as_result(WITHDRAWAL_MANAGER_CONTRACT)?; - let rewards_manager_contract = state.get_as_result(REWARDS_MANAGER_CONTRACT)?; + let core_contract = STATE.load(deps.storage, CORE_CONTRACT)?; + let withdrawal_manager_contract = STATE.load(deps.storage, WITHDRAWAL_MANAGER_CONTRACT)?; + let rewards_manager_contract = STATE.load(deps.storage, REWARDS_MANAGER_CONTRACT)?; let attrs = vec![attr("action", "pause")]; let messages = vec![ @@ -558,10 +578,9 @@ fn exec_pause(deps: DepsMut, info: MessageInfo) -> ContractResult ContractResult> { cw_ownable::assert_owner(deps.storage, &info.sender)?; - let state = STATE.load(deps.storage)?; - let core_contract = state.get_as_result(CORE_CONTRACT)?; - let withdrawal_manager_contract = state.get_as_result(WITHDRAWAL_MANAGER_CONTRACT)?; - let rewards_manager_contract = state.get_as_result(REWARDS_MANAGER_CONTRACT)?; + let core_contract = STATE.load(deps.storage, CORE_CONTRACT)?; + let withdrawal_manager_contract = STATE.load(deps.storage, WITHDRAWAL_MANAGER_CONTRACT)?; + let rewards_manager_contract = STATE.load(deps.storage, REWARDS_MANAGER_CONTRACT)?; let attrs = vec![attr("action", "unpause")]; let messages = vec![ get_proxied_message( @@ -608,9 +627,8 @@ fn execute_update_config( ) -> ContractResult> { let attrs = vec![attr("action", "update-config")]; cw_ownable::assert_owner(deps.storage, &info.sender)?; - let state = STATE.load(deps.storage)?; - let core_contract = state.get_as_result(CORE_CONTRACT)?; - let validators_set_contract = state.get_as_result(VALIDATORS_SET_CONTRACT)?; + let core_contract = STATE.load(deps.storage, CORE_CONTRACT)?; + let validators_set_contract = STATE.load(deps.storage, VALIDATORS_SET_CONTRACT)?; let mut messages = vec![]; match msg { UpdateConfigMsg::Core(msg) => messages.push(get_proxied_message( @@ -635,9 +653,8 @@ fn execute_proxy_msg( info: MessageInfo, msg: ProxyMsg, ) -> ContractResult> { - let state = STATE.load(deps.storage)?; - let validators_set_contract = state.get_as_result(VALIDATORS_SET_CONTRACT)?; - let puppeteer_contract = state.get_as_result(PUPPETEER_CONTRACT)?; + let validators_set_contract = STATE.load(deps.storage, VALIDATORS_SET_CONTRACT)?; + let puppeteer_contract = STATE.load(deps.storage, PUPPETEER_CONTRACT)?; let mut messages = vec![]; let attrs = vec![attr("action", "proxy-call")]; cw_ownable::assert_owner(deps.storage, &info.sender)?; diff --git a/contracts/factory/src/tests.rs b/contracts/factory/src/tests.rs index ea5beccb..87feadf0 100644 --- a/contracts/factory/src/tests.rs +++ b/contracts/factory/src/tests.rs @@ -18,7 +18,7 @@ use drop_staking_base::{ CoreParams, ExecuteMsg, FeeParams, InstantiateMsg, LsmShareBondParams, NativeBondParams, QueryMsg, UpdateConfigMsg, ValidatorSetMsg, }, - state::factory::{CodeIds, Phonebook, RemoteOpts, Timeout, STATE}, + state::factory::{CodeIds, RemoteOpts, Timeout, STATE}, }; use drop_staking_base::{ msg::{ @@ -52,45 +52,92 @@ fn set_default_factory_state(deps: DepsMut) { STATE .save( deps.storage, - &Phonebook::new([ - (TOKEN_CONTRACT, Addr::unchecked("token_contract")), - (CORE_CONTRACT, Addr::unchecked("core_contract")), - (PUPPETEER_CONTRACT, Addr::unchecked("puppeteer_contract")), - ( - WITHDRAWAL_MANAGER_CONTRACT, - Addr::unchecked("withdrawal_manager_contract"), - ), - ( - WITHDRAWAL_VOUCHER_CONTRACT, - Addr::unchecked("withdrawal_voucher_contract"), - ), - (STRATEGY_CONTRACT, Addr::unchecked("strategy_contract")), - ( - VALIDATORS_SET_CONTRACT, - Addr::unchecked("validators_set_contract"), - ), - ( - DISTRIBUTION_CONTRACT, - Addr::unchecked("distribution_contract"), - ), - ( - REWARDS_MANAGER_CONTRACT, - Addr::unchecked("rewards_manager_contract"), - ), - ( - REWARDS_PUMP_CONTRACT, - Addr::unchecked("rewards_pump_contract"), - ), - (SPLITTER_CONTRACT, Addr::unchecked("splitter_contract")), - ( - LSM_SHARE_BOND_PROVIDER_CONTRACT, - Addr::unchecked("lsm_share_bond_provider_contract"), - ), - ( - NATIVE_BOND_PROVIDER_CONTRACT, - Addr::unchecked("native_bond_provider_contract"), - ), - ]), + TOKEN_CONTRACT, + &Addr::unchecked("token_contract"), + ) + .unwrap(); + STATE + .save( + deps.storage, + CORE_CONTRACT, + &Addr::unchecked("core_contract"), + ) + .unwrap(); + STATE + .save( + deps.storage, + PUPPETEER_CONTRACT, + &Addr::unchecked("puppeteer_contract"), + ) + .unwrap(); + STATE + .save( + deps.storage, + WITHDRAWAL_MANAGER_CONTRACT, + &Addr::unchecked("withdrawal_manager_contract"), + ) + .unwrap(); + STATE + .save( + deps.storage, + WITHDRAWAL_VOUCHER_CONTRACT, + &Addr::unchecked("withdrawal_voucher_contract"), + ) + .unwrap(); + STATE + .save( + deps.storage, + STRATEGY_CONTRACT, + &Addr::unchecked("strategy_contract"), + ) + .unwrap(); + STATE + .save( + deps.storage, + VALIDATORS_SET_CONTRACT, + &Addr::unchecked("validators_set_contract"), + ) + .unwrap(); + STATE + .save( + deps.storage, + DISTRIBUTION_CONTRACT, + &Addr::unchecked("distribution_contract"), + ) + .unwrap(); + STATE + .save( + deps.storage, + REWARDS_MANAGER_CONTRACT, + &Addr::unchecked("rewards_manager_contract"), + ) + .unwrap(); + STATE + .save( + deps.storage, + REWARDS_PUMP_CONTRACT, + &Addr::unchecked("rewards_pump_contract"), + ) + .unwrap(); + STATE + .save( + deps.storage, + SPLITTER_CONTRACT, + &Addr::unchecked("splitter_contract"), + ) + .unwrap(); + STATE + .save( + deps.storage, + LSM_SHARE_BOND_PROVIDER_CONTRACT, + &Addr::unchecked("lsm_share_bond_provider_contract"), + ) + .unwrap(); + STATE + .save( + deps.storage, + NATIVE_BOND_PROVIDER_CONTRACT, + &Addr::unchecked("native_bond_provider_contract"), ) .unwrap(); } diff --git a/packages/base/src/state/factory.rs b/packages/base/src/state/factory.rs index d832823b..2d65fd2e 100644 --- a/packages/base/src/state/factory.rs +++ b/packages/base/src/state/factory.rs @@ -1,7 +1,6 @@ use cosmwasm_schema::cw_serde; use cosmwasm_std::Addr; -use cw_storage_plus::Item; -use std::hash::Hash; +use cw_storage_plus::Map; #[cw_serde] pub struct CodeIds { @@ -44,35 +43,4 @@ pub struct PauseInfoResponse { pub rewards_manager: drop_helpers::pause::PauseInfoResponse, } -#[cw_serde] -pub struct Phonebook { - pub map: std::collections::HashMap, -} - -pub const STATE: Item = Item::new("state"); - -impl Phonebook { - pub fn get_as_result<'a>( - &'a self, - key: &'a str, - ) -> Result<&Addr, crate::error::factory::ContractError> { - self.map.get(key).ok_or( - crate::error::factory::ContractError::ContractAddressNotFound { - name: key.to_string(), - }, - ) - } - - pub fn new(arr: [(K, Addr); N]) -> Self - where - // Bounds from impl: - K: Eq + Hash + Into + Clone + std::fmt::Display, - { - let map = arr - .iter() - .clone() - .map(|(k, v)| (k.to_string(), v.clone())) - .collect(); - Self { map } - } -} +pub const STATE: Map<&str, Addr> = Map::new("state"); diff --git a/packages/helpers/src/phonebook.rs b/packages/helpers/src/phonebook.rs index b8f5786e..2cbcc27a 100644 --- a/packages/helpers/src/phonebook.rs +++ b/packages/helpers/src/phonebook.rs @@ -10,7 +10,7 @@ macro_rules! get_contracts { } let contracts = $deps .querier - .query::>(&QueryRequest::Wasm(WasmQuery::Smart { + .query::>(&cosmwasm_std::QueryRequest::Wasm(cosmwasm_std::WasmQuery::Smart { contract_addr: $factory_contract.to_string(), msg: to_json_binary(&drop_staking_base::msg::factory::QueryMsg::Locate { contracts: vec![$(stringify!($field_name).to_string()),*], From 4e6219518db78cda7b0a325de581a21728f2915f Mon Sep 17 00:00:00 2001 From: Murad Karammaev Date: Mon, 25 Nov 2024 21:15:42 +0200 Subject: [PATCH 2/2] feat: avoid excess calls to clone() --- contracts/factory/src/contract.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/contracts/factory/src/contract.rs b/contracts/factory/src/contract.rs index e54ade73..2337e84e 100644 --- a/contracts/factory/src/contract.rs +++ b/contracts/factory/src/contract.rs @@ -483,8 +483,8 @@ fn query_state(deps: Deps) -> ContractResult { let state = STATE.range(deps.storage, None, None, cosmwasm_std::Order::Ascending); let out = state .collect::>>()? - .iter() - .map(|(k, v): &(String, cosmwasm_std::Addr)| (k.clone(), v.to_string())) + .into_iter() + .map(|(k, v)| (k, v.into_string())) .collect::>(); Ok(to_json_binary(&out)?) } @@ -494,7 +494,7 @@ fn query_locate(deps: Deps, items: Vec) -> ContractResult< for item in items { let addr = STATE.load(deps.storage, &item)?; - contracts.insert(item.clone(), addr.to_string()); + contracts.insert(item, addr.into_string()); } Ok(to_json_binary(&contracts)?) }