feat: privileged subdaos #NTRN-211 #99
Conversation
zavgorodnii
changed the title
zavgorodnii
force-pushed
the
feat/privileged-subdaos-NTRN-211
branch
from
729347b
to
46c57f5
Compare
| ) -> Result<(), ContractError> { | ||
| match neutron_msg { | ||
| NeutronMsg::AddSchedule { | ||
| name: _, |
There was a problem hiding this comment.
you can ignore all the variables with ..
| } | ||
| } | ||
|
|
||
| fn get_allow_all_count(deps: Deps) -> Result<u64, ContractError> { |
There was a problem hiding this comment.
Do you ever need to count all allow_all? Simple find seems enough, instead of iterating over all the strategies.
| ) -> Result<Response, ContractError> { | ||
| set_contract_version(deps.storage, CONTRACT_NAME, CONTRACT_VERSION)?; | ||
|
|
||
| if let Strategy::AllowOnly(_) = msg.initial_strategy { |
There was a problem hiding this comment.
If the only AllowAll allowed as initial strategy, why do you ever need the message param at all? Just set AllowAll for initial address.
There was a problem hiding this comment.
i see your point, but I don't think that it's worth the effort.
| pub struct ProposalExecuteMessageJSON { | ||
| #[serde(rename = "@type")] | ||
| pub type_field: String, | ||
| pub authority: String, |
There was a problem hiding this comment.
You never use authority field, i think it's unnecessary field
| let typed_proposal: ProposalExecuteMessageJSON = | ||
| serde_json_wasm::from_str(proposal.message.as_str())?; | ||
|
|
||
| if typed_proposal.type_field.as_str() == "/neutron.cron.MsgUpdateParams" { |
There was a problem hiding this comment.
You could use a const for the message type value.
| Ok(strategy) => { | ||
| if let Strategy::AllowOnly(_) = strategy { | ||
| return Err(ContractError::Unauthorized {}); | ||
| } | ||
|
|
||
| Ok(()) | ||
| } | ||
| Err(_) => Err(ContractError::Unauthorized {}), |
There was a problem hiding this comment.
it's a bit easier to read, from my point of view
| Ok(strategy) => { | |
| if let Strategy::AllowOnly(_) = strategy { | |
| return Err(ContractError::Unauthorized {}); | |
| } | |
| Ok(()) | |
| } | |
| Err(_) => Err(ContractError::Unauthorized {}), | |
| Ok(Strategy::AllowAll) => Ok(()), | |
| _ => Err(ContractError::Unauthorized {}) |
There was a problem hiding this comment.
didn't even know you could do this. thanks
| ParamChangePermission(ParamChangePermission), | ||
| UpdateParamsPermission(UpdateParamsPermission), |
There was a problem hiding this comment.
Pls add a comment which describes the difference between two sets of permissions.
| Strategy::AllowOnly(permissions) => { | ||
| let mut has_permission = false; | ||
| for permission in permissions { | ||
| if let Permission::CronPermission(cron_permission) = permission { | ||
| has_permission = cron_permission.add_schedule | ||
| } | ||
| } | ||
|
|
||
| has_permission | ||
| } | ||
| } |
There was a problem hiding this comment.
we could just return cron_permission.add_schedule in the if let Permission::CronPermission(cron_permission) = permission closure similar to has_param_change_permission. this would
- shorten the code
- make an instant return on matched value (now we continue iterating over permissions even if we've already matched the respective one)
same in has_cron_remove_schedule_permission
…-refactor-privileged-subdaos
…ivileged-subdaos chore: altered strategy store scheme for chain manager (audit fix) #NTRN-251
Overview
This PR implements the privileged SubDAOs design (look up the Privileged SubDAOs RFC in Notion for full details).
neutron-chain-managercontract,init-neutrond.shscript to set theneutron-chain-managercontract as the chain admin: feat: privileged subdaos #NTRN-211 neutron#434,neutronjsplusto know the difference between the Neutron DAO and the chain admin: feat: privileged subdaos neutronjsplus#23,Important note: this PR implements an example strategy for new style parameter updates (only Cron module). However, changing the parameters of this module is not going to work until the PR to the core repo is merged and we upgrade the mainnet, because, unfortunately, we forgot to allow querying cron module parameters with stargate, and chain manager really needs to query those params. All other functionality will work without upgrading the chain.
Integration tests
neutron-org/neutron-integration-tests#266
Successful test run: https://github.com/neutron-org/neutron-tests/actions/runs/7919615589
Successful test run (post-review 1): https://github.com/neutron-org/neutron-tests/actions/runs/8055334942
cronmodule by a subdao (new style parameter updates),globalfeemodule by a subdao (old style style parameter updates).Rehearsal scenario
neutron-chain-managercontract with the ALLOW_ALLinitial_strategyfor the Neutron DAO core contract,MinimumGasPricesParam),neutron-chain-managercontract to the list of chain admins and to remove the Neutron DAO contract from the list of chain admins,MinimumGasPricesParam, then check that the parameter was actually changed.