Allow admin to pause/unpause subdao #744
Conversation
Also fixed some typos and removed unused msg.rs from dao-dao-core
The only way the DAO can unpause is through an admin that is not itself
There was a problem hiding this comment.
sick! i think i found one line that needs fixing, and it seems like you need to format (cargo fmt --all -- --check)
also, it wouldn't hurt to add a test that random person cannot unpause as well, not just the DAO itself
update: i was wrong, no fix needed other than the format
There was a problem hiding this comment.
overall lgtm!
that being said, it would be great to add some extended testing for this. as mentioned in comment, imho adding some tests that would combine the pausing/unpausing logic with a veto committee would be a good way to make sure we aren't introducing some potential chaos here.
|
also now that i think of it, i'm not sure how i feel about the fact that pausing / unpausing can be done by different parties. don't have a solution off the top of my head right now, but something to think about. |
Had similar thoughts initially, but eventually warmed up to it as a nice change that Neutron introduced. This feature is most relevant in the context of SubDAOs. For DAOs without an admin, nothing changes. As a parent DAO is in full control of its SubDAOs, it makes sense that that control should also extend to pausing and unpausing a SubDAO. For example, the Juno Charter has departments that may become inactive due to lack of members or activity. Rather than having to do all the work of moving around treasuries, vesting, etc., the SubDAO can simply enter a paused state until the parent DAO decides what to do with it. The Parent DAO ( |
|
that makes sense! |
|
this is really interesting; i have some thoughts! sorry for the long ass brain dump but i want to make sure we know exactly what we're doing, and this is me working it out out loud 😁 both allowing the parent DAO and DAO itself to bypass the pause, AND moving the paused check to before: "lock contract, frozen until expiration or forever if no expiration set" interestingly, proposals can still be created and voted on. they just won't be executable until the DAO is unpaused. i think this is fine, and we can disable proposal creation in the UI while paused to enforce the expected behavior that you shouldn't be creating proposals in a paused DAO. our goals (what we want from pause):
all of these are achieved by both allowing the parent and DAO itself to call execute while paused (and no one else), AND moving the pause logic to the proposal execution hook. something else to consider though: authz allows an account to execute on behalf of another account. if the DAO has authorized (authz granted) a wallet the authority to do something as the DAO, there is no way for us to prevent action being taken while paused, since the wallet authz executes as the DAO itself. we cannot prevent the wallet from updating membership, for example, since the voting module would allow that if it comes from the DAO when authz executed by the wallet. we also couldn't prevent the wallet from calling messages on the DAO itself, IFF we allow the DAO to execute messages on itself (which we have to allow if we want to let the parent tell the DAO to execute messages while paused, like to add/remove a voting or proposal module). now, it'd be very silly for a DAO to give generic authz power on or as the DAO to a wallet—that effectively sets them as admin, so maybe it's not a case we really need to design for, but we should definitely know how it will interact with this pause logic. most likely, DAO-given authz will be for very specific actions, like trading on a DEX or managing staking. in these instances, we can't do anything about them anyways, and the DAO/parent needs to know to revoke any outstanding authz grants when they go to pause. that is something the UI can help solve, and there's nothing the contracts can do about it either way. additionally, a DAO may want to give a trusted wallet or DAO (maybe a security SubDAO) the ability to pause/unpause using authz, so in all cases we must allow both the parent DAO and DAO itself to bypass the pause and execute things, as long as the proposal hook is blocked. so the question really is: should pausing simply mean halting the governance process (by disallowing proposal module execution)? i think that sounds right, but now i'm curious what @0xekez had in mind at the beginning when putting the auth logic at the top level like this. since there was no ability to unpause early and no ability for the parent to execute while paused, it acts as a complete emergency freeze until the expiration (or indefinitely if no expiration). if we want to give the parent DAO the power to execute as the DAO and on the DAO, which it seems like we do, we are changing what paused means—and we need to allow the parent DAO to execute at least it seems that both keeping the paused logic in the top-level execute function (with the addition that the parent and DAO itself can still execute anything) and simply moving the paused logic to the wdyt @ismellike @JakeHartnell @bekauz ? |
really great point about authz. completely flew over my head. my immediate thought after reading through this again would be to design this flow with the assumption that DAO gave generic (or some more restrictive but still critical) authz power to some wallet. while it's unlikely and unwise, i totally echo this:
@NoahSaso could we revoke all authorizations on pause? should we? just pondering here. |
|
one more doomer thought regarding the authz revoking above: the number of things that we need to consider when adding features like this makes me somewhat paranoid of getting to the point where adding something new will require us explicitly addressing numerous other features previously added without considering something like this. not sure if this constitutes feature creep, internal dependency hell (is that a thing?), or something else, but i think you get what i mean. veto feature was giving me similar feelings at the start, but ended up being quite encapsulated. |
nope 🤩 i don't think we can force the DAO to revoke all authorizations on pause without adding some way to register all of the authorizations that already exist. even if we did that, i suspect we'd run into gas limit issues if too many authorizations had been granted, thereby preventing pausing. instead, this is something we should surface in the UI when attempting to pause. we can display all outstanding authorizations to make it clear that certain executions will still be possible, and even prompt to add actions to revoke them all in the same proposal.
yeah... i think we are choosing to play the game on hard mode here. we can't avoid this problem if we want to add features. we should strongly seek to solve problems without modifying the core contract when possible, but pause has to be done directly like this. i mean, pause should really be the only feature that has to be implemented so bluntly like this. most other features have to do with proposal and voting modules. we should be paranoid. we're building smart contracts that give the users full power to change anything about the contract and its relationship to other accounts. most other contracts aren't susceptible to this because they don't allow generic message executions, but we're basically creating a passthrough contract with some wrapper logic. i think this is the best we can do 🤷🏻♂️ |
|
alright @ismellike, i think we're good to go on moving the paused check back to the beginning of the execution function, and skipping it if the sender is the admin or the DAO itself :D |
Co-authored-by: noah <[email protected]>
|
seems like you need to recreate the schema and then the linter will pass |
Also fixed some typos and removed unused msg.rs from dao-dao-core
#737