From f971d2ae9b46fbb592b9620699669aa9bd4fba6e Mon Sep 17 00:00:00 2001 From: Buckram Date: Fri, 25 Aug 2023 19:11:55 +0300 Subject: [PATCH 1/9] make proxy the admin of sub-accounts --- .../contracts/account/manager/src/commands.rs | 22 +++++++++++++------ .../contracts/account/manager/src/contract.rs | 11 +--------- .../account/manager/tests/subaccount.rs | 21 ++++++------------ .../abstract-core/src/objects/gov_type.rs | 16 +++++++------- 4 files changed, 31 insertions(+), 39 deletions(-) diff --git a/framework/contracts/account/manager/src/commands.rs b/framework/contracts/account/manager/src/commands.rs index 90188abb31..9608b342de 100644 --- a/framework/contracts/account/manager/src/commands.rs +++ b/framework/contracts/account/manager/src/commands.rs @@ -14,6 +14,7 @@ use abstract_core::version_control::ModuleResponse; use abstract_macros::abstract_response; use abstract_sdk::cw_helpers::AbstractAttributes; +use abstract_sdk::namespaces::ADMIN_NAMESPACE; use abstract_sdk::{ core::{ manager::state::DEPENDENTS, @@ -199,7 +200,6 @@ pub fn exec_on_module( /// Creates a sub-account for this account, pub fn create_subaccount( deps: DepsMut, - env: Env, msg_info: MessageInfo, name: String, description: Option, @@ -211,9 +211,9 @@ pub fn create_subaccount( assert_admin_right(deps.as_ref(), &msg_info.sender)?; let create_account_msg = &abstract_core::account_factory::ExecuteMsg::CreateAccount { - /// this contract (the manager) will be the account owner + /// proxy of this manager will be the account owner governance: GovernanceDetails::SubAccount { - manager: env.contract.address.to_string(), + proxy: ACCOUNT_MODULES.load(deps.storage, PROXY)?.into_string(), }, name, description, @@ -807,19 +807,27 @@ fn assert_admin_right(deps: Deps, sender: &Addr) -> ManagerResult<()> { let account_info = INFO.load(deps.storage)?; match account_info.governance_details { // If the account has SubAccount governance, the owner of the manager also has admin rights over this account - GovernanceDetails::SubAccount { manager } => { + GovernanceDetails::SubAccount { proxy } => { // We try to query the ownership of the manager monarch account if the first query failed - let mut current = manager; + let mut current = proxy; let mut i = 0; while i < MAX_ADMIN_RECURSION { - let owner = query_ownership(deps, current) + // admin of a proxy is a manager + let manager = deps + .querier + .query_wasm_raw(current, ADMIN_NAMESPACE.as_bytes())? + .ok_or(ManagerError::SubAccountAdminVerification)?; + let manager: Addr = from_binary(&Binary(manager))?; + let owner = query_ownership(deps, manager.clone()) .map_err(|_| ManagerError::SubAccountAdminVerification)?; + + println!("sender: {sender}, owner: {owner}"); if *sender == owner { // If the owner of the current contract is the sender, the admin test is passed return Ok(()); } else { // If not, we try again with the queried owner - current = deps.api.addr_validate(&owner)? + current = Addr::unchecked(owner); } i += 1; } diff --git a/framework/contracts/account/manager/src/contract.rs b/framework/contracts/account/manager/src/contract.rs index f5f5d33799..53c3266c20 100644 --- a/framework/contracts/account/manager/src/contract.rs +++ b/framework/contracts/account/manager/src/contract.rs @@ -123,16 +123,7 @@ pub fn execute(deps: DepsMut, env: Env, info: MessageInfo, msg: ExecuteMsg) -> M link, base_asset, namespace, - } => create_subaccount( - deps, - env, - info, - name, - description, - link, - base_asset, - namespace, - ), + } => create_subaccount(deps, info, name, description, link, base_asset, namespace), ExecuteMsg::Upgrade { modules } => upgrade_modules(deps, env, info, modules), ExecuteMsg::UpdateInfo { name, diff --git a/framework/contracts/account/manager/tests/subaccount.rs b/framework/contracts/account/manager/tests/subaccount.rs index 58ecbd4935..afc925f8b5 100644 --- a/framework/contracts/account/manager/tests/subaccount.rs +++ b/framework/contracts/account/manager/tests/subaccount.rs @@ -45,31 +45,24 @@ fn updating_on_subaccount_should_succeed() -> AResult { } #[test] -fn manager_updating_on_subaccount_should_succeed() -> AResult { +fn proxy_updating_on_subaccount_should_succeed() -> AResult { let sender = Addr::unchecked(common::OWNER); let chain = Mock::new(&sender); let deployment = Abstract::deploy_on(chain, sender.to_string())?; let account = create_default_account(&deployment.account_factory)?; - let manager_address = account.manager.address()?; + let proxy_address = account.proxy.address()?; account .manager .create_sub_account("My subaccount".to_string(), None, None, None, None)?; // Subaccount should have id 2 in this test, we try to update the config of this module - let account_contracts = get_account_contracts(&deployment.version_control, Some(2)); + let (sub_manager, _sub_proxy) = get_account_contracts(&deployment.version_control, Some(2)); let new_desc = "new desc"; - // We call as the manager, it should also be possible - account_contracts.0.call_as(&manager_address).update_info( - Some(new_desc.to_string()), - None, - None, - )?; - - assert_eq!( - Some(new_desc.to_string()), - account_contracts.0.info()?.info.description - ); + // We call as the proxy, it should also be possible + sub_manager + .call_as(&proxy_address) + .update_info(Some(new_desc.to_owned()), None, None)?; Ok(()) } diff --git a/framework/packages/abstract-core/src/objects/gov_type.rs b/framework/packages/abstract-core/src/objects/gov_type.rs index 88be81174f..c54b6db36c 100644 --- a/framework/packages/abstract-core/src/objects/gov_type.rs +++ b/framework/packages/abstract-core/src/objects/gov_type.rs @@ -21,8 +21,8 @@ pub enum GovernanceDetails { }, /// Used when the account is a sub-account of another account. SubAccount { - /// The manager of the account of which this account is the sub-account. - manager: T, + /// The proxy of the account of which this account is the sub-account. + proxy: T, }, /// An external governance source External { @@ -41,9 +41,9 @@ impl GovernanceDetails { let addr = api.addr_validate(&monarch)?; Ok(GovernanceDetails::Monarchy { monarch: addr }) } - GovernanceDetails::SubAccount { manager } => { - let addr = api.addr_validate(&manager)?; - Ok(GovernanceDetails::SubAccount { manager: addr }) + GovernanceDetails::SubAccount { proxy } => { + let addr = api.addr_validate(&proxy)?; + Ok(GovernanceDetails::SubAccount { proxy: addr }) } GovernanceDetails::External { governance_address, @@ -95,7 +95,7 @@ impl GovernanceDetails { pub fn owner_address(&self) -> Addr { match self { GovernanceDetails::Monarchy { monarch } => monarch.clone(), - GovernanceDetails::SubAccount { manager } => manager.clone(), + GovernanceDetails::SubAccount { proxy } => proxy.clone(), GovernanceDetails::External { governance_address, .. } => governance_address.clone(), @@ -109,8 +109,8 @@ impl From> for GovernanceDetails { GovernanceDetails::Monarchy { monarch } => GovernanceDetails::Monarchy { monarch: monarch.to_string(), }, - GovernanceDetails::SubAccount { manager } => GovernanceDetails::SubAccount { - manager: manager.to_string(), + GovernanceDetails::SubAccount { proxy } => GovernanceDetails::SubAccount { + proxy: proxy.to_string(), }, GovernanceDetails::External { governance_address, From 81e380fdd3df7e8452d438edb33175f61db5a647 Mon Sep 17 00:00:00 2001 From: Buckram Date: Fri, 25 Aug 2023 19:17:42 +0300 Subject: [PATCH 2/9] use raw query for getting owner --- .../contracts/account/manager/src/commands.rs | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/framework/contracts/account/manager/src/commands.rs b/framework/contracts/account/manager/src/commands.rs index 9608b342de..b682648bd9 100644 --- a/framework/contracts/account/manager/src/commands.rs +++ b/framework/contracts/account/manager/src/commands.rs @@ -1,12 +1,12 @@ use crate::{contract::ManagerResult, error::ManagerError, queries::query_module_cw2}; use crate::{validation, versioning}; -use abstract_core::manager::state::ACCOUNT_FACTORY; +use abstract_core::manager::state::{ACCOUNT_FACTORY, OWNER}; use abstract_core::adapter::{ AuthorizedAddressesResponse, BaseExecuteMsg, BaseQueryMsg, ExecuteMsg as AdapterExecMsg, QueryMsg as AdapterQuery, }; -use abstract_core::manager::{InternalConfigAction, QueryMsg}; +use abstract_core::manager::InternalConfigAction; use abstract_core::objects::gov_type::GovernanceDetails; use abstract_core::objects::AssetEntry; @@ -788,13 +788,11 @@ pub fn update_internal_config(deps: DepsMut, info: MessageInfo, config: Binary) } } -fn query_ownership(deps: Deps, maybe_manager: Addr) -> ManagerResult { - let Ownership:: { owner, .. } = deps - .querier - .query_wasm_smart(maybe_manager.clone(), &QueryMsg::Ownership {}) +fn query_ownership(deps: Deps, maybe_manager: Addr) -> ManagerResult { + let Ownership { owner, .. } = OWNER + .query(&deps.querier, maybe_manager.clone()) .map_err(|_| ManagerError::NoContractOwner(maybe_manager.to_string()))?; - - owner.ok_or(ManagerError::NoContractOwner(maybe_manager.to_string())) + owner.ok_or(ManagerError::NoContractOwner(maybe_manager.into_string())) } fn assert_admin_right(deps: Deps, sender: &Addr) -> ManagerResult<()> { From ff277f7f43c93d783df4fa3804fc9be839ad32e2 Mon Sep 17 00:00:00 2001 From: Buckram Date: Fri, 25 Aug 2023 19:20:30 +0300 Subject: [PATCH 3/9] remove debug string --- framework/contracts/account/manager/src/commands.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/framework/contracts/account/manager/src/commands.rs b/framework/contracts/account/manager/src/commands.rs index b682648bd9..30f2e8b0d0 100644 --- a/framework/contracts/account/manager/src/commands.rs +++ b/framework/contracts/account/manager/src/commands.rs @@ -819,7 +819,6 @@ fn assert_admin_right(deps: Deps, sender: &Addr) -> ManagerResult<()> { let owner = query_ownership(deps, manager.clone()) .map_err(|_| ManagerError::SubAccountAdminVerification)?; - println!("sender: {sender}, owner: {owner}"); if *sender == owner { // If the owner of the current contract is the sender, the admin test is passed return Ok(()); From 15ac8e755919031ab0371dff86471fee655d9ae3 Mon Sep 17 00:00:00 2001 From: Buckram Date: Fri, 25 Aug 2023 19:28:10 +0300 Subject: [PATCH 4/9] update errors --- framework/contracts/account/manager/src/commands.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/framework/contracts/account/manager/src/commands.rs b/framework/contracts/account/manager/src/commands.rs index 30f2e8b0d0..ee88b7ecfc 100644 --- a/framework/contracts/account/manager/src/commands.rs +++ b/framework/contracts/account/manager/src/commands.rs @@ -815,7 +815,8 @@ fn assert_admin_right(deps: Deps, sender: &Addr) -> ManagerResult<()> { .querier .query_wasm_raw(current, ADMIN_NAMESPACE.as_bytes())? .ok_or(ManagerError::SubAccountAdminVerification)?; - let manager: Addr = from_binary(&Binary(manager))?; + let manager: Option = from_binary(&Binary(manager))?; + let manager = manager.ok_or(ManagerError::SubAccountAdminVerification)?; let owner = query_ownership(deps, manager.clone()) .map_err(|_| ManagerError::SubAccountAdminVerification)?; From a85ad72211e97acd432da4a197d91cf0d9258c3c Mon Sep 17 00:00:00 2001 From: Mykhailo Donchenko <91957742+Buckram123@users.noreply.github.com> Date: Fri, 25 Aug 2023 19:47:59 +0300 Subject: [PATCH 5/9] Update CHANGELOG.md --- framework/CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/framework/CHANGELOG.md b/framework/CHANGELOG.md index 291f764fa1..4fb625a75f 100644 --- a/framework/CHANGELOG.md +++ b/framework/CHANGELOG.md @@ -11,6 +11,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ### Changed - Updated fetch_data arguments of CwStakingCommand +- Owner of the sub-accounts now Proxy, allowing modules to interact with sub-accounts ### Fixed - Partially fixed cw-staking for Osmosis From e11032d21203bc4864b939c7ede01c09aa109910 Mon Sep 17 00:00:00 2001 From: Buckram Date: Fri, 25 Aug 2023 20:17:18 +0300 Subject: [PATCH 6/9] clean up --- .../contracts/account/manager/src/commands.rs | 27 ++++++++++--------- 1 file changed, 15 insertions(+), 12 deletions(-) diff --git a/framework/contracts/account/manager/src/commands.rs b/framework/contracts/account/manager/src/commands.rs index ee88b7ecfc..fabbfcad2d 100644 --- a/framework/contracts/account/manager/src/commands.rs +++ b/framework/contracts/account/manager/src/commands.rs @@ -788,11 +788,20 @@ pub fn update_internal_config(deps: DepsMut, info: MessageInfo, config: Binary) } } -fn query_ownership(deps: Deps, maybe_manager: Addr) -> ManagerResult { +fn query_ownership(deps: Deps, maybe_proxy: Addr) -> ManagerResult { + // query admin of the proxy (it's manager) + let manager_bytes = deps + .querier + .query_wasm_raw(maybe_proxy, ADMIN_NAMESPACE.as_bytes())? + .ok_or(ManagerError::SubAccountAdminVerification)?; + let manager = from_binary::>(&Binary(manager_bytes))? + .ok_or(ManagerError::SubAccountAdminVerification)?; + + // get owner of the manager let Ownership { owner, .. } = OWNER - .query(&deps.querier, maybe_manager.clone()) - .map_err(|_| ManagerError::NoContractOwner(maybe_manager.to_string()))?; - owner.ok_or(ManagerError::NoContractOwner(maybe_manager.into_string())) + .query(&deps.querier, manager.clone()) + .map_err(|_| ManagerError::NoContractOwner(manager.to_string()))?; + owner.ok_or(ManagerError::NoContractOwner(manager.into_string())) } fn assert_admin_right(deps: Deps, sender: &Addr) -> ManagerResult<()> { @@ -811,13 +820,7 @@ fn assert_admin_right(deps: Deps, sender: &Addr) -> ManagerResult<()> { let mut i = 0; while i < MAX_ADMIN_RECURSION { // admin of a proxy is a manager - let manager = deps - .querier - .query_wasm_raw(current, ADMIN_NAMESPACE.as_bytes())? - .ok_or(ManagerError::SubAccountAdminVerification)?; - let manager: Option = from_binary(&Binary(manager))?; - let manager = manager.ok_or(ManagerError::SubAccountAdminVerification)?; - let owner = query_ownership(deps, manager.clone()) + let owner = query_ownership(deps, current.clone()) .map_err(|_| ManagerError::SubAccountAdminVerification)?; if *sender == owner { @@ -825,7 +828,7 @@ fn assert_admin_right(deps: Deps, sender: &Addr) -> ManagerResult<()> { return Ok(()); } else { // If not, we try again with the queried owner - current = Addr::unchecked(owner); + current = owner; } i += 1; } From 3ea1dcb3da156aad288ea8ee8afce0e0273b7135 Mon Sep 17 00:00:00 2001 From: Buckram Date: Fri, 25 Aug 2023 21:08:27 +0300 Subject: [PATCH 7/9] check that apps have ownership over sub-account --- .../account/manager/tests/subaccount.rs | 54 ++++++++++++++++++- 1 file changed, 53 insertions(+), 1 deletion(-) diff --git a/framework/contracts/account/manager/tests/subaccount.rs b/framework/contracts/account/manager/tests/subaccount.rs index afc925f8b5..9a41be9304 100644 --- a/framework/contracts/account/manager/tests/subaccount.rs +++ b/framework/contracts/account/manager/tests/subaccount.rs @@ -2,7 +2,7 @@ mod common; use abstract_interface::*; use common::*; -use cosmwasm_std::Addr; +use cosmwasm_std::{wasm_execute, Addr}; use cw_orch::deploy::Deploy; use cw_orch::prelude::*; // use cw_multi_test::StakingInfo; @@ -64,6 +64,11 @@ fn proxy_updating_on_subaccount_should_succeed() -> AResult { .call_as(&proxy_address) .update_info(Some(new_desc.to_owned()), None, None)?; + assert_eq!( + Some(new_desc.to_string()), + sub_manager.info()?.info.description + ); + Ok(()) } @@ -102,3 +107,50 @@ fn recursive_updating_on_subaccount_should_succeed() -> AResult { Ok(()) } + +#[test] +fn installed_app_updating_on_subaccount_should_succeed() -> AResult { + let sender = Addr::unchecked(common::OWNER); + let chain = Mock::new(&sender); + let deployment = Abstract::deploy_on(chain, sender.to_string())?; + let account = create_default_account(&deployment.account_factory)?; + account + .manager + .create_sub_account("My subaccount".to_string(), None, None, None, None)?; + let first_proxy_addr = account.proxy.address()?; + + let mock_app = Addr::unchecked("mock_app"); + account + .proxy + .call_as(&account.manager.address()?) + .add_module(mock_app.to_string())?; + + let (sub_manager, _sub_proxy) = get_account_contracts(&deployment.version_control, Some(2)); + let new_desc = "new desc"; + + // recover address on first proxy + account.proxy.set_address(&first_proxy_addr); + // adding mock_app to whitelist on proxy + + // We call as installed app of the owner-proxy, it should also be possible + account + .proxy + .call_as(&mock_app) + .module_action(vec![wasm_execute( + sub_manager.addr_str()?, + &abstract_core::manager::ExecuteMsg::UpdateInfo { + name: None, + description: Some(new_desc.to_owned()), + link: None, + }, + vec![], + )? + .into()])?; + + assert_eq!( + Some(new_desc.to_string()), + sub_manager.info()?.info.description + ); + + Ok(()) +} From ba38f12d4cb21393f2b5f980db5259d8605b0c2a Mon Sep 17 00:00:00 2001 From: Buckram Date: Mon, 28 Aug 2023 14:25:59 +0300 Subject: [PATCH 8/9] Query info instead of ownership --- .../contracts/account/manager/src/commands.rs | 77 +++++++++---------- .../contracts/account/manager/src/contract.rs | 11 ++- .../abstract-core/src/objects/gov_type.rs | 20 +++-- 3 files changed, 58 insertions(+), 50 deletions(-) diff --git a/framework/contracts/account/manager/src/commands.rs b/framework/contracts/account/manager/src/commands.rs index fabbfcad2d..7feeaa6092 100644 --- a/framework/contracts/account/manager/src/commands.rs +++ b/framework/contracts/account/manager/src/commands.rs @@ -1,6 +1,6 @@ use crate::{contract::ManagerResult, error::ManagerError, queries::query_module_cw2}; use crate::{validation, versioning}; -use abstract_core::manager::state::{ACCOUNT_FACTORY, OWNER}; +use abstract_core::manager::state::ACCOUNT_FACTORY; use abstract_core::adapter::{ AuthorizedAddressesResponse, BaseExecuteMsg, BaseQueryMsg, ExecuteMsg as AdapterExecMsg, @@ -14,7 +14,6 @@ use abstract_core::version_control::ModuleResponse; use abstract_macros::abstract_response; use abstract_sdk::cw_helpers::AbstractAttributes; -use abstract_sdk::namespaces::ADMIN_NAMESPACE; use abstract_sdk::{ core::{ manager::state::DEPENDENTS, @@ -41,7 +40,6 @@ use cosmwasm_std::{ DepsMut, Empty, Env, MessageInfo, Response, StdError, StdResult, Storage, WasmMsg, }; use cw2::{get_contract_version, ContractVersion}; -use cw_ownable::Ownership; use cw_storage_plus::Item; use semver::Version; @@ -201,6 +199,7 @@ pub fn exec_on_module( pub fn create_subaccount( deps: DepsMut, msg_info: MessageInfo, + env: Env, name: String, description: Option, link: Option, @@ -213,6 +212,7 @@ pub fn create_subaccount( let create_account_msg = &abstract_core::account_factory::ExecuteMsg::CreateAccount { /// proxy of this manager will be the account owner governance: GovernanceDetails::SubAccount { + manager: env.contract.address.into_string(), proxy: ACCOUNT_MODULES.load(deps.storage, PROXY)?.into_string(), }, name, @@ -788,56 +788,51 @@ pub fn update_internal_config(deps: DepsMut, info: MessageInfo, config: Binary) } } -fn query_ownership(deps: Deps, maybe_proxy: Addr) -> ManagerResult { - // query admin of the proxy (it's manager) - let manager_bytes = deps - .querier - .query_wasm_raw(maybe_proxy, ADMIN_NAMESPACE.as_bytes())? - .ok_or(ManagerError::SubAccountAdminVerification)?; - let manager = from_binary::>(&Binary(manager_bytes))? - .ok_or(ManagerError::SubAccountAdminVerification)?; - - // get owner of the manager - let Ownership { owner, .. } = OWNER - .query(&deps.querier, manager.clone()) - .map_err(|_| ManagerError::NoContractOwner(manager.to_string()))?; - owner.ok_or(ManagerError::NoContractOwner(manager.into_string())) -} - fn assert_admin_right(deps: Deps, sender: &Addr) -> ManagerResult<()> { let ownership_test = cw_ownable::assert_owner(deps.storage, sender); - if ownership_test.is_ok() { - return Ok(()); - } + let mut ownership_error: ManagerError = match ownership_test { + Ok(()) => return Ok(()), + Err(err) => err.into(), + }; - let account_info = INFO.load(deps.storage)?; - match account_info.governance_details { - // If the account has SubAccount governance, the owner of the manager also has admin rights over this account - GovernanceDetails::SubAccount { proxy } => { - // We try to query the ownership of the manager monarch account if the first query failed - let mut current = proxy; - let mut i = 0; - while i < MAX_ADMIN_RECURSION { - // admin of a proxy is a manager - let owner = query_ownership(deps, current.clone()) + let mut current: AccountInfo = INFO.load(deps.storage)?; + // Get sub-accounts until we get non-sub-account governance or reach recursion limit + for _ in 0..MAX_ADMIN_RECURSION { + match current.governance_details { + GovernanceDetails::SubAccount { manager, .. } => { + current = INFO + .query(&deps.querier, manager) .map_err(|_| ManagerError::SubAccountAdminVerification)?; - if *sender == owner { - // If the owner of the current contract is the sender, the admin test is passed - return Ok(()); - } else { - // If not, we try again with the queried owner - current = owner; - } - i += 1; + // Change error type if it was sub-account + ownership_error = ManagerError::SubAccountAdminVerification; + } + _ => break, + } + } + + match current.governance_details { + GovernanceDetails::Monarchy { monarch: owner } + | GovernanceDetails::External { + governance_address: owner, + .. + } => { + // If the owner of the current contract is the sender, the admin test is passed + if *sender == owner { + Ok(()) + } else { + Err(ownership_error) } + } + // MAX_ADMIN_RECURSION levels deep still sub account + GovernanceDetails::SubAccount { .. } => { Err(ManagerError::Std(StdError::generic_err(format!( "Admin recursion error, too much recursion, maximum allowed admin recursion : {}", MAX_ADMIN_RECURSION )))) } - _ => Ok(ownership_test?), + _ => Err(ownership_error), } } diff --git a/framework/contracts/account/manager/src/contract.rs b/framework/contracts/account/manager/src/contract.rs index 53c3266c20..d717f9de9f 100644 --- a/framework/contracts/account/manager/src/contract.rs +++ b/framework/contracts/account/manager/src/contract.rs @@ -123,7 +123,16 @@ pub fn execute(deps: DepsMut, env: Env, info: MessageInfo, msg: ExecuteMsg) -> M link, base_asset, namespace, - } => create_subaccount(deps, info, name, description, link, base_asset, namespace), + } => create_subaccount( + deps, + info, + env, + name, + description, + link, + base_asset, + namespace, + ), ExecuteMsg::Upgrade { modules } => upgrade_modules(deps, env, info, modules), ExecuteMsg::UpdateInfo { name, diff --git a/framework/packages/abstract-core/src/objects/gov_type.rs b/framework/packages/abstract-core/src/objects/gov_type.rs index c54b6db36c..bbdd333417 100644 --- a/framework/packages/abstract-core/src/objects/gov_type.rs +++ b/framework/packages/abstract-core/src/objects/gov_type.rs @@ -21,6 +21,8 @@ pub enum GovernanceDetails { }, /// Used when the account is a sub-account of another account. SubAccount { + /// The manager of the account of which this account is the sub-account. + manager: T, /// The proxy of the account of which this account is the sub-account. proxy: T, }, @@ -41,9 +43,10 @@ impl GovernanceDetails { let addr = api.addr_validate(&monarch)?; Ok(GovernanceDetails::Monarchy { monarch: addr }) } - GovernanceDetails::SubAccount { proxy } => { - let addr = api.addr_validate(&proxy)?; - Ok(GovernanceDetails::SubAccount { proxy: addr }) + GovernanceDetails::SubAccount { manager, proxy } => { + let manager = api.addr_validate(&manager)?; + let proxy = api.addr_validate(&proxy)?; + Ok(GovernanceDetails::SubAccount { manager, proxy }) } GovernanceDetails::External { governance_address, @@ -95,7 +98,7 @@ impl GovernanceDetails { pub fn owner_address(&self) -> Addr { match self { GovernanceDetails::Monarchy { monarch } => monarch.clone(), - GovernanceDetails::SubAccount { proxy } => proxy.clone(), + GovernanceDetails::SubAccount { proxy, .. } => proxy.clone(), GovernanceDetails::External { governance_address, .. } => governance_address.clone(), @@ -107,16 +110,17 @@ impl From> for GovernanceDetails { fn from(value: GovernanceDetails) -> Self { match value { GovernanceDetails::Monarchy { monarch } => GovernanceDetails::Monarchy { - monarch: monarch.to_string(), + monarch: monarch.into_string(), }, - GovernanceDetails::SubAccount { proxy } => GovernanceDetails::SubAccount { - proxy: proxy.to_string(), + GovernanceDetails::SubAccount { manager, proxy } => GovernanceDetails::SubAccount { + manager: manager.into_string(), + proxy: proxy.into_string(), }, GovernanceDetails::External { governance_address, governance_type, } => GovernanceDetails::External { - governance_address: governance_address.to_string(), + governance_address: governance_address.into_string(), governance_type, }, } From 0d66e5c4250fcd23470f78da701b6db98fdf1cc7 Mon Sep 17 00:00:00 2001 From: cyberhoward Date: Mon, 28 Aug 2023 14:43:55 +0200 Subject: [PATCH 9/9] add some comments --- framework/contracts/account/manager/src/commands.rs | 12 ++++++++++-- .../contracts/account/manager/tests/subaccount.rs | 1 + 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/framework/contracts/account/manager/src/commands.rs b/framework/contracts/account/manager/src/commands.rs index 6975573bf5..d5d037d443 100644 --- a/framework/contracts/account/manager/src/commands.rs +++ b/framework/contracts/account/manager/src/commands.rs @@ -851,18 +851,25 @@ pub fn update_internal_config( } } +/// Function that guards all the admin rights. +/// This function should return `Ok` when called by: +/// - The owner of the contract (i.e. account). +/// - The top-level owner of the account that owns this account. I.e. the first account for which the `GovernanceDetails` is not `SubAccount`. fn assert_admin_right(deps: Deps, sender: &Addr) -> ManagerResult<()> { let ownership_test = cw_ownable::assert_owner(deps.storage, sender); + // If the sender is the owner, the admin test is passed let mut ownership_error: ManagerError = match ownership_test { Ok(()) => return Ok(()), Err(err) => err.into(), }; + // In case it fails we get the account info and check if the current(this) account is a sub-account. let mut current: AccountInfo = INFO.load(deps.storage)?; // Get sub-accounts until we get non-sub-account governance or reach recursion limit for _ in 0..MAX_ADMIN_RECURSION { match current.governance_details { + // As long as the accounts are sub-accounts, we check the owner of the parent account GovernanceDetails::SubAccount { manager, .. } => { current = INFO .query(&deps.querier, manager) @@ -881,7 +888,8 @@ fn assert_admin_right(deps: Deps, sender: &Addr) -> ManagerResult<()> { governance_address: owner, .. } => { - // If the owner of the current contract is the sender, the admin test is passed + // If the owner of the top-level account is the sender, the admin test is passed. + // This gives the top-level owner the ability to manage all sub-accounts. if *sender == owner { Ok(()) } else { @@ -891,7 +899,7 @@ fn assert_admin_right(deps: Deps, sender: &Addr) -> ManagerResult<()> { // MAX_ADMIN_RECURSION levels deep still sub account GovernanceDetails::SubAccount { .. } => { Err(ManagerError::Std(StdError::generic_err(format!( - "Admin recursion error, too much recursion, maximum allowed admin recursion : {}", + "Admin recursion error, too much recursion, maximum allowed sub-account admin recursion : {}", MAX_ADMIN_RECURSION )))) } diff --git a/framework/contracts/account/manager/tests/subaccount.rs b/framework/contracts/account/manager/tests/subaccount.rs index 61da695737..60fe187439 100644 --- a/framework/contracts/account/manager/tests/subaccount.rs +++ b/framework/contracts/account/manager/tests/subaccount.rs @@ -119,6 +119,7 @@ fn recursive_updating_on_subaccount_should_succeed() -> AResult { account_contracts .0 + .call_as(&sender) .update_info(Some(new_desc.to_string()), None, None)?; assert_eq!(