fix: fail poll on empty list of execute_msgs

[ezaanm]

• fail poll if the execute_msgs list is empty
• solves Executed messages may be empty, but without error #13
• thanks @pronvis for pointing this out

[ezaanm]

As @MSNTCS pointed out in the comment above we should not implement this change. The reason is because Poll's should be allowed to exist with empty execute_msgs such as a text only based poll. This change would have marked those Poll's as failed while they really should be executed

This will close #13 as well.

[pronvis]

@MSNTCS Hi! You wrote:

We have text proposals that do not include execute messages. with this new changes we expire them instead of putting them in execution status. In this case, do we need to add this new change?

Okey, but in code you still have:

    if let Some(all_msgs) = a_poll.execute_data {
        // some code here
    } else {
        return Err(ContractError::NoExecuteData {});
    }

So, you still have this error. And, because of quote that I mention about, you have some "hidden logic" like:
"If you create text proposal - then Poll's execute_data: Option<Vec<ExecuteData>> should be Some(vec![])". This logic is:

1. not documented anywhere
2. looks wrong to me

Issue that I created (#13) fills this "gap" in logic. Yep, maybe it fills it in a wrong way, but even if so - you need to have same logic for:

1. execute_data == None
2. execute_data == Some(vec![])
   now, the logic is different for those 2 cases, and this looks 100% wrong to me.

[pronvis]

I think you need to change field execute_data to be: execute_data: Vec<ExecuteData>. You could still create empty execute_data = vec![] and will not have any "gaps" in logic between None & empty vec

[MSNTCS]

@pronvis thanks. I agree with you on some points, I do not see having problems with None and it is better not to touch the interface due to several dependencies that we have right now on contracts. The point that I agree with you is treating the empty vector, like None. I think the code needs some changes for empty vector/None. I mentioned required changes here #9 (comment)

[pronvis]

Also, I found the same logic in Mirror Gov contract: https://github.com/Mirror-Protocol/mirror-contracts/blob/3c182c6d32e4083b13838cc5aba570e4ae279879/contracts/mirror_gov/src/contract.rs#L502

Carlos (Mirror team member) respond with:

For text polls, Passed is the final state, they dont need to be executed

[MSNTCS]

@pronvis of course they do not need to be executed. The point is, with the current design, if somebody executes it, the poll status would not stay in passed, it would be an expired/failed poll and it changes the history of a poll. This issue is a minor issue, but still, I think it is better to keep the states as executed. Look at this example that has been passed in reality but the status of the poll has been a stored to failed https://bombay-lcd.terra.dev/wasm/contracts/terra16ckeuu7c6ggu52a8se005mg5c0kd2kmuun63cu/store?query_msg={%22poll%22:{%22poll_id%22:119}}

[pronvis]

@MSNTCS but why, if execute_poll returns error - that means Poll status will not be changed.
(I am a bit away from my working PC, may be wrong here)

[MSNTCS]

@pronvis with the current design, if you send an execute message for a text proposal even if it is passed, the current design change the status to failed. I am referring to this function:

anchor-token-contracts/contracts/gov/src/contract.rs

Line 487 in 93dcc49

pub fn execute_poll_messages(

[pronvis]

@MSNTCS on reply handler we can check if Poll have empty execute messages or not and if so - do not mark it as failed. (or you can set some flag on poll, or solve this somehow - to do the same logic)

[pronvis]

@MSNTCS this is how I solve it: Nexus-Protocol/services-contracts#14