Contract output on reverted executions

[xermicus]

As per the docs, returning an Err from a message will always revert the contract execution. Now that makes me wondering as to why ink! contracts always return a MessageResult, instead of the actual type?

Given that:

• Returning Ok will never revert the execution
• Return Err will always revert the execution

Unless I'm missing something here, it does seem unnecessary to always encode the contract output as a Result; this is known anyways based on the revert flag. Additionally, the current model implies that we need to support situations where a contract would return Err but not revert it's state and vice versa, and I'm not sure when that would be useful.

I think the whole situation could be simplified by defining that:

• Contracts return two types O and E as their output
• Type O is expected if the execution did not revert
• Type E is expected if the execution did revert

Which would:

• Simplify the ink! metadata. There would be no need do define a "special" LanguageError type or wrapping the message ouptuts in nested types like MessageResult. Instead, each contract message would just define their O and E types.
• Remove the need to encode and encode these nested Result types at runtime. Yes it's only 1 or 2 bytes but why not getting rid of any redundancy we can. Requiring all contracts to always encode their message results prefixed with 1 or 2 bytes, which encode no meaningful (because redundant) data, just because ink! does expect that, signals inelegance.
• Make the metadata no longer agnostic to Rusts Result type.

Of course, this would be a major breaking change requiring careful consideration.

[xermicus]

Alternatively: There isn't even a need to change the metadata at all to make this work. If we just specify that front-ends and calling contracts always must decode the lang_error for Solidity contracts if the contract reverted (the Language is in the metadata). We achieve practically the same, so ink! can still always wrap the contract outputs in (possibly nested) result types if it wants to do so - but others are not forced to do that.

[xgreenx]

The MessageResult is the only way to pass an error from the ink! level to the caller(from auto-generated code). In some cases, the caller wants to know the reason why execution failed and reverted. Even if the cross-contract call reverted, it still can be a positive sign.

An example is the before_received function called by the PSP22 (until it was removed). The before_received function is optional to implement, and some contracts may not implement it. The MessageResult::CouldNotReadInput error only indicates that the contract doesn't implement this method -> we can continue execution successfully.

Another example is the allow_reentrancy attribute. A supporting attribute like this also requires returning an error from ink! side to the caller. It can be used in the before_received case too.

Yeah, the examples use removed before_received method, but supporting the MessageResult is about flexibility. The contract creators may need to handle some errors from ink!, not only from the business logic(Result<O, E>).

[xermicus]

I see why contracts want to return errors. But I'm still not convinced that a contract output can be the same in the revert case and in the non-revert case. This is strongly reflected in ink! codegen: When an Err is returned, the contract always reverts. So why not reflect that in the metadata too?

[xgreenx]

But I'm still not convinced that a contract output can be the same in the revert case and in the non-revert case.

I'm not saying that the output is the same in the revert and non-revert cases. It is clear that in the non-revert case, the output is only Ok(O), while in the revert case, it is either Err(LangError) or Err(UsersError) or CalleeTrapped(panic).

The problem in your proposal comes with:

Simplify the ink! metadata. There would be no need do define a "special" LanguageError type or wrapping the message ouptuts in nested types like MessageResult. Instead, each contract message would just define their O and E types.

We need to propagate errors from ink! auto-generated code to the end-user. It means that the message's defined type E should include UserError and LangError. We could create a new enum under the hood and return this new type:

enum CombinedErrorForMessageX {
    UserError(UserError),
    LangError(LangError),
}

But it is the same in terms of performance because you spend one byte to make the difference between LangError or UserError, but it increases the size of the contracts and creates new weird types per message.

I also want to highlight that we create/return MessageResult even before we started the execution of the message itself. We don't know which CombinedErrorForMessageX we need to return because the can't decode input(selector of the message).

[xermicus]

but it increases the size of the contracts and creates new weird types per message.

Right now, Result, a generic type, is a common appearance in our metadata, and if any message returns a Result, it'll even get nested. So if we'd have the following Error type instead:

#[non_exhaustive]
enum RevertReason<T> {
  ContractReverted(T),  // Holding whatever the contract message returns if it decides to revert.
  SelectorInvalid,
  /// maybe more in the future
}

How does this generic type increase contract sizes and create "weird" types in the metadata, when we already have nested generic types as the contract outputs?

[xermicus]

However, seems like for now I don't even need such breaking changes to the metadata. We have the language in the metadata for a reason. For my use case it'd most likely be sufficient to specify that for Solang Solidity contracts, the contracts only return their message return type if they didn't revert. Otherwise, the output will be the a standartd Solidity error and the lang_err found in the metadata provides the information needed on how to parse it.

[xgreenx]

Right now, Result, a generic type, is a common appearance in our metadata, and if any message returns a Result, it'll even get nested. So if we'd have the following Error type instead:

#[non_exhaustive]
enum RevertReason<T> {
  ContractReverted(T),  // Holding whatever the contract message returns if it decides to revert.
  SelectorInvalid,
  /// maybe more in the future
}

Yeah, you are right. I forgot that we can define generic type on ink! level instead defining it for each output(like CombinedErrorForMessageX), my bad=D

Hmm, this feature should be implemented in this way initially based on the issue.

I remember we discussed it here. And the final decision was to have one Result.

@HCastano Was there any reason why you selected the approach with two Results?

[xermicus]

Ah that is a good find. Well I think in #1207 and following discussions and PRs were solving a (very slightly) different problem; namely allowing to include something like language errors in the Metadata. And we just now started talking about whether it makes sense at all to "allow" ink! contracts to revert and return Ok and vice versa in this issue. I guess the addition of lang_error just made this more apparent. It looks like this was not something the original implementation intended (explicitly), at least I didn't think of it back then. Anyways we should be very careful with such breaking changes, could be there was a strong reason to do it like that.

I mean for example before #1446 we didn't have fallible constructors. Things like this might have played a role in designing the metadata in regards to the language error.

[SkymanOne]

@xermicus what's the status of this issue?

[xermicus]

@SkymanOne to me the question as to why the contract execution output is wrapped inside a Result, while this seems unnecessary (because the revert flag already should tell you if the contract reverted or not, and seemingly the Error variant can not be returned in the Ok case and vice versa) is still open.

After all, if the metadata would allow to explicitly specify distinct return types for revert and non-revert execution outputs, it'd allow languages without the Result type to re-use the ink-metadata properly.

[deuszx]

To me it makes sense that there are two layers of Result type: one for lang_error and one for the application (contract) layer. These are two different "channels" for the error that might be handled by a different layers of the consumer app. A specific lang_error can indicate that the problem is "bigger" - like an incompatiblity between client and the chain it interacts with and requires manual intervention while a "layer error" (internal Err in the Result sandwich) can have automatic recovery. The "domain-specific" part of the client doesn't have to know about lang_errors and vice versa. This also explains why it makes sense to have Ok(Err) (but maybe not Err(Ok)).

This is similar to how you can have different error types when interacting with a REST service:

• request timeout
• 404
• domain-specific error in the app you're talking to.

In my experience, bundling all of these (otherwise unrelated) concepts together just makes it harder to the other side.

[xermicus]

it makes sense to have Ok(Err)

Sorry, I can't hold this off

On a more serious note. I'm not questioning the concept of a lang_error per se. I'm just wondering whether it makes sense to even assume that contracts might produce the same execution output regardless whether they reverted or not. Which so far doesn't seem to be the case, and apparently isn't even possible to do in ink! v4. So I thought, why not just reflect that fact properly in the metadata.

I mean if there are dedicated types for reverted and non-reverted executions, you can still have lang_error. The difference would just be that instead of the rather silly Ok(Err) type, it'd could just be some Enum Variant instead.

[deuszx]

@xermicus I cannot comment on internals of the pallet contracts, I'm not that familiar with it yet at the moment. I'm talking about a general approach for "signalling" and handling errors. It's pretty common in IT to have different "channels" for different domains - separation of concerns.

Although, now that I think of it again, I think I've misinterpreted the "inner" Err of the Ok(Err) case. Inner is not the dApp's error but a pallet contracts https://paritytech.github.io/ink/ink_env/enum.Error.html .