More intelligent handling of Executor in Tally

[ethanfrey]

Is your feature request related to a problem? Please describe.

There are cases in Tally where the Tally succeeds (and can update the proposal state), but the Decoder or Executor will return an error. We don't want to return an error from Deliver, or else the state of the Proposal will never update. However, we need to handle this somehow.

Describe the solution you'd like

We need to clarify what the desired solution is.

In #647, I made a minimal solution, using a separate cached-db for the executor (with Write() on success, Discard() on error). And add any executor failures (or success) as a Log message to the DeliverResult. This solves the problem, but the Log message is not a good long-term solution.

I propose to follow @alpe 's solution to store an extra status message in the proposal itself (as well as the log).

The status codes would be persisted with the proposal only not returned. For example:

Proposal.Status=Closed  // we have this already
Proposal.Result=Accepted // we have this already
Proposal.ExecutorResult=Failed // new

The actual error message of the executor (with reason) is in the Result.Log (just like if the tally failed). No need to store in the DB. Let's just add one more field to canonically mark which proposals were and were not successfully executed after acceptance

Describe alternatives you've considered

Alternatives listed above

Additional context

None that I know of

[husio]

When making the suggestion in #633 (comment) I was not fully aware of how voting works. Now I know that in order to finalize voting, the certain deadline must pass and manual tally creation must be requested. Knowing this, the current implementation makes to me more sense than what I originally proposed.

I like the idea of different status codes depending on the combined results although I have no idea how this would work.
Today ABCI API allows us to return a code. 0 for success or anything else for an error. Would we use this code to return the result of a tally + execution?

For me the most important in this functionality is predictability. I think we must focus on what the user (government board) can understand and what makes sense for them. If this functionality is not perfect but useful, I am convinced that all minor shortcomings can be addressed offline.

[ruseinov]

Today ABCI API allows us to return a code. 0 for success or anything else for an error. Would we use this code to return the result of a tally + execution?

In my understanding statuses mentioned in the #613 have nothing to do with ABCI error codes, but rather having a status field similar to ProposalCommon.Status. I suppose that it would make sense to have such a status field for Executor with certain statuses allowing you to re-execute on failure, for example.

[alpe]

The status codes would be persisted with the proposal only not returned. For example:

Proposal.Status=Closed  // we have this already
Proposal.Result=Accepted // we have this already
Proposal.ExecutorResult=Failed // new

A question would be: Do we
A new tally msg would fail when Status=Closed but a new ExecuteMsg msg (after tally) can succeed. With a separate executor status I would also not mind to skip the ExecuteMsg and just have the tally msg sent again. The client side needs to query the executor status anyway after the first tally...
Long term I hope for an automated solution based on block callbacks like in cosmos-sdk.

[ethanfrey]

I would like to only allow one Tally and Execute attempt.

I mean, if you voted for something to happen (eg. update the validators), but didn't have permission, this fails. If a month later you are given permission, can anyone then re-Execute this closed but never succeeded proposal and update the Validators?

Seems to open up to many cans of worms. I would prefer to have just one attempt to Execute it. The New status code looks good to me. I would both store it upon execution and return it in the Data field of the transaction, so you get an abci error code if you couldn't actually tally (too early), and abci success if the vote was tallied. and can check the return data for Result and ExecutorResult (just define the encoding of the Data blob so easily parseable by the client)