Add optional executor restriction to cw3-flex#741
Conversation
ethanfrey
left a comment
ethanfrey
left a comment
There was a problem hiding this comment.
Wow. Thanks for the quick reaction here.
Code looks good. I had an idea how to extend it a bit, left comments on the Executor variant and making the check a method.
The other idea (for a different PR) was to use the same pattern for who can submit a proposal, but have that default to "Member" rather than "Everyone" if the InstantiateMsg has None in the field.
| // - None: Anyone can execute message | ||
| let cfg = CONFIG.load(deps.storage)?; | ||
| if let Some(executor) = cfg.executor { | ||
| match executor { |
There was a problem hiding this comment.
Is this something that could be pulled out to a method on Executor?
Already thinking of reusing it for who can make proposals. (but default is Member)
There was a problem hiding this comment.
I moved it under Config struct. I think it's more versatile this way.
What do you think?
| pub group_addr: Cw4Contract, | ||
| // who is able to execute passed proposals | ||
| // None means that anyone can execute | ||
| pub executor: Option<Executor>, |
There was a problem hiding this comment.
Disregard the above comment. This is great as it is drop-in state compatible with previous version, not requiring an explicit migration (we should have some placeholder migrate function, but no state change needed). No need to make breaking changes for some aesthetic opinion of mine.
There was a problem hiding this comment.
Yes, I considered that but as you pointed out - this way it's completely backward compatible.
| } | ||
| } | ||
| } | ||
| cfg.authorize(&deps.querier, &info.sender)?; |
| pub executor: Option<Executor>, | ||
| } | ||
|
|
||
| impl Config { |
2 participants
closes #739
Test are bloated, but all of them are... Some proper refactoring would be useful here.