Introduce new types to ensure that storage is rollbacked before ignoring or handling a `DispatchError`

[gui1117]

Issue

Calls do rollback when they return a DispatchError.

When implementing traits or function which returns DispatchResult, people tends to assume that the storage will be rollbacked if an error is returned. At the same time some code do ignore the error when using those traits or function.

We should bring some new safety, to avoid dirty storage.

Solution 1: New type with a runtime check.

We can have a new error type: DispatchErrorMustRollback or DispatchErrorTainted, which must not be ignored, and must rollback the storage to handle gracefully.

It has a method fn rollback which handles the rollback. And implements Drop with a debug_assert! and log::error.

At least we have a runtime check.

Solution 2: Stop assuming storage is rollbacked. (or keep not assuming storage is rollbacked)

Incite people to use with_transaction in their code, or transactional macro on top of their function.

Solution 3: Assume DispatchError must be rollbacked.

Same as 1 but without new type.

[xlc]

Have a new macro with_transaction that automatically wrap the function block into a transaction.

we already have transactional does exactly that

[gui1117]

Have a new macro with_transaction that automatically wrap the function block into a transaction.

we already have transactional does exactly that

Thanks I forgot, I updated the main post

[xlc]

I dont' think we can do 3 as there is no easy way to enforce it. I will say we should go with 2, which is always the case. Calls are transactional recently (relative to the history of Substrate) and it that happens to return DispatchError but by no means returning DispatchError means rollback. I am not sure why people are having the wrong impression.

[bkchr]

Agreeing with what @xlc said.