StakingMsg::Withdraw has unintended side-effect

[webmaster128]

The message

pub enum StakingMsg {
    // ...

    /// This is translated to a [MsgSetWithdrawAddress](https://github.com/cosmos/cosmos-sdk/blob/v0.40.0/proto/cosmos/distribution/v1beta1/tx.proto#L29-L37)
    /// followed by a [MsgWithdrawDelegatorReward](https://github.com/cosmos/cosmos-sdk/blob/v0.40.0/proto/cosmos/distribution/v1beta1/tx.proto#L42-L50).
    /// `delegator_address` is automatically filled with the current contract's address.
    Withdraw {
        validator: HumanAddr,
        /// this is the "withdraw address", the one that should receive the rewards
        /// if None, then use delegator address
        recipient: Option<HumanAddr>,
    },

    // ...
}

seems to be an simplification of how distribution works in Cosmos SDK. According to the distribution documentation there are multiple withdrawal events:

• Whenever withdrawing, one must withdraw the maximum amount they are entitled to, leaving nothing in the pool.
• Whenever bonding, unbonding, or re-delegating tokens to an existing account, a full withdrawal of the rewards must occur (as the rules for lazy accounting change).
• Whenever a validator chooses to change the commission on rewards, all accumulated commission rewards must be simultaneously withdrawn.

So withdrawal can occur without the delegator's explicit request.

Consider the following scenario:

1. A contract is a delegator that is earning rewards. They come by automatic withdrawl to the delegator.
2. There is a special case in the contract where a one-time event triggets a withdrawl to a different address via StakingMsg::Withdraw
3. Now all following withdrawals go to this other address instead of defaulting to the contract because MsgSetWithdrawAddress was used in 2.

[ethanfrey]

This is a valid point. I assumed any contract using this would not be manually setting the withdraw address.

The only place to change the behavior is in wasmd if you want to change the behavior, not just the comment string. It may be some work to make this different, but maybe not. If you think this is valuable, can you raise it there and link this issue, so we just change docsstrings if it is too hard.

[webmaster128]

What I would like to do is remove the combined StakingMsg::Withdraw and create two distribution messages. Then we better model how the distribution module works. If a contract never changes the withdrawal address, only MsgWithdrawDelegatorReward is needed. Otherwise the contract needs to be explicit.

[ethanfrey]

Okay, that is clear and relatively straight forward. Happy for that change