Hooks in staking contract

[ben2x4]

This PR adds the ability to add hooks to the staking contract, which are contracts that will be notified when users stake and unstake funds. This will allow cool features to be built on top of the staking contract such as delegated voting and external staking rewards.

I have also taken made some improvements to the governance of modifying hooks as it is extremly high risk as an improper hook contract would lock all funds until the hook is removed. To better enable this, I have split the admin role into an owner and manager. The owner can add/remove hooks, change the owner, change the admin, and change the unbonding duration. The manager can do all of this excepct change the owner. This allows the DAO to delegate responsibilities of managing hooks to a more reponsive party such as a multisig while maintaining authority to remove the manager in case they attempt to lock funds.

This is especially important in the wasmswap context as a staking contract would need to be instatiated and managed for each pool. The amount of management will most likely exceed the capacity of the whole DAO to make each individual contract change.

TODO:

Need to write actual tests for the hooks, but this requires a dummy contract to ensure hooks are functioning as intended.

[0xjame5]

Just double-checking, but all this contract is doing is applying a side-effect on specific behaviors on this contract to other contracts. Is there any expectation on downstream contracts? For example what if they fail on downstream side-effects?

[0xjame5]

Overall LGTM, tho I'm new to the hooks system and have some questions.

[0xekez]

This looks great. Just a couple questions around address validation. Seems like a bad hook will not stop removal of said hook which is nice.

[ben2x4]

Just double-checking, but all this contract is doing is applying a side-effect on specific behaviors on this contract to other contracts. Is there any expectation on downstream contracts? For example what if they fail on downstream side-effects?

The contract is notifying via hooks other contracts when a user modifies their amount staked. There is a critical expectations that these contracts will not fail. It is critical that whoever is governoring these hooks is not malicious as they have the ability to lock all staked funds by adding an invalid hook. Hooks can be removed if a contracts is broken.

[orkunkl]

Just double-checking, but all this contract is doing is applying a side-effect on specific behaviors on this contract to other contracts. Is there any expectation on downstream contracts? For example what if they fail on downstream side-effects?

The contract is notifying via hooks other contracts when a user modifies their amount staked. There is a critical expectations that these contracts will not fail. It is critical that whoever is governoring these hooks is not malicious as they have the ability to lock all staked funds by adding an invalid hook. Hooks can be removed if a contracts is broken.

SubMsg::new sets the reply method as Never as default. Even if the hook contract execution fails, txs will not so super safe it to use here.

[ben2x4]

Just double-checking, but all this contract is doing is applying a side-effect on specific behaviors on this contract to other contracts. Is there any expectation on downstream contracts? For example what if they fail on downstream side-effects?

The contract is notifying via hooks other contracts when a user modifies their amount staked. There is a critical expectations that these contracts will not fail. It is critical that whoever is governoring these hooks is not malicious as they have the ability to lock all staked funds by adding an invalid hook. Hooks can be removed if a contracts is broken.

SubMsg::new sets the reply method as Never as default. Even if the hook contract execution fails, txs will not so super safe it to use here.

oh interesting, I think this actually presents its own problems? Like if a sub-contract gets into a weird state and is failing, it might not be super obvious. Weird things might then occur becuase the hooked contract becomes out of sync. Probable a super edge case, but definitely some new concerns to think through here.

[vernonjohnson]

oh interesting, I think this actually presents its own problems? Like if a sub-contract gets into a weird state and is failing, it might not be super obvious. Weird things might then occur becuase the hooked contract becomes out of sync. Probable a super edge case, but definitely some new concerns to think through here.

@ben2x4 Yeah I agree. I think having the tx fail if the contract receiving the hook fails might actually be more ideal. If there's a critical bug in the dependent contract, you'd want to block execution until a fix is applied. And resume normal operations afterwards. Would prevent the dependent contract from reaching an invalid state.

Probably dependent on the use case though. For the wasmswap staking use case, you'd definitely want execution to be blocked if the downstream staking rewards contract fails to process the hook message. And apply a fix accordingly

[vernonjohnson]

Overall LGTM. The address validation checks that @ezekiiel mentioned would be nice. Also a few tests with a dummy contract that receives the hooks would be good

[ben2x4]

Changed all msg addr types to strings

[0xekez]

Happy to merge this now. Before we deploy need some tests but there is a dependency issue so I'm OK with waiting on that.

[0xjame5]

For the wasmswap staking use case, you'd definitely want execution to be blocked if the downstream staking rewards contract fails to process the hook message. And apply a fix accordingly

one other con is that if the downstream contract is broken then this contract would be locked. given the sensitivity of this hook, we'd want to fail close (meaning if down stream tx fails, we fail)