Implement error forwarding

[cmichi]

If an ink! message returns a Result::Err, this error should be forwarded and result in e.g. the Canvas UI eventually displaying Extrinsic Failed (instead of Extrinsic Success, which it currently does).

[atenjin]

I agree this proposal, and I think this should design a protocol between pallet-contracts and ink! or other language , like what we do in Ask!.

Currently, if the ink! return error, and return the error code, the value recept in pallet-contracts and drop it

		match sandbox_result {
			// No traps were generated. Proceed normally.
			Ok(_) => {
				Ok(ExecReturnValue { flags: ReturnFlags::empty(), data: Vec::new() })
			}

https://github.com/paritytech/substrate/blob/master/frame/contracts/src/wasm/runtime.rs#L364-L368

And under this protocol, any other contract framework (include ink!, Ask!, Solang and so on) should obey this convention

[hychen]

@cmichi Does this means that If an ink! message returns a Result::Error, it emits ExtrinsicFailed event which contains errors defined by a contract?

[Robbepop]

In a meeting @cmichi and me concluded to put forward with the following design for ink!:

• Introduce a new ink! attribute, called revert_on_err: bool that is optional and which can be applied to both, ink! constructors and messages.
• If applied to ink! constructors or messages ink! includes a compile-time check to guard that the return type of the applied to method is in fact a Result type and furthermore ink! will revert the contract execution upon yielding Result::Err with appropriate return flags to SEAL.
• A new optional metadata field is added to ink! messages and constructors: revert_on_err: Option<bool>. Optional means that it can be missing. Note that it is kind of specific to ink!/Rust and not very useful for other languages like Solang which is why it cannot be mandatory. When the field is missing cargo-contract will assert that the associated ink! constructor or message in fact does NOT return a Result. This means that whenver a user returns a Result type having this new ink! attribute is mandatory to set. If the user wants to revert the execution upon Result::Err the message or constructors should be flagged with either revert_on_err or revert_on_err: true which is both equivalent. If the execution should not be reverted upon Result::Err the user is mandated to flag the ink! constructor or message with revert_on_err: false.
• Besides this the currently generated signatures of call and deploy no longer return a u32 to make the ink! codegen compatible with the most recent SEAL versions again.

This design allows for an efficient design that ink! and cargo-contact can statically verify and which pleasantly fits into the current ink! design from a syntactical point of view.

An example follows:

/// It is still possible to create normal non-Result messages or constructors.
#[ink(message)]
fn foo(&self) -> i32 { ... }

/// Long-form notation, mandates revert upon returning `Result::Err`.
#[ink(message, revert_on_err: true)]
fn try_foo_1(&self) -> Result<i32, MyError> { ... }

/// Short-form notation, mandates revert upon returning `Result::Err`.
///
/// Absolutely the same as `try_foo_1`.
#[ink(message, revert_on_err)]
fn try_foo_2(&self) -> Result<i32, MyError> { ... }

/// Long-form notation, does NOT revert upon returning `Result::Err`.
#[ink(message, revert_on_err: false)]
fn foo_2(&self) -> Result<i32, MyError> { ... }

/// This is invalid ink! code and will yield a compile error via Rust compiler
/// since the message is not returning a Result type.
#[ink(message, revert_on_err: true)]
fn foo_3(&self) -> i32 { ... }

/// This is invalid ink! code and will yield an error via cargo-contract when being compiled
/// since the mandatory revert_on_err for Result type returns is missing.
#[ink(message)]
fn foo_4(&self) -> Result<i32, MyError> { ... }

[atenjin]

In a meeting @cmichi and me concluded to put forward with the following design for ink!:

• Introduce a new ink! attribute, called revert_on_err: bool that is optional and which can be applied to both, ink! constructors and messages.
• If applied to ink! constructors or messages ink! includes a compile-time check to guard that the return type of the applied to method is in fact a Result type and furthermore ink! will revert the contract execution upon yielding Result::Err with appropriate return flags to SEAL.
• A new optional metadata field is added to ink! messages and constructors: revert_on_err: Option<bool>. Optional means that it can be missing. Note that it is kind of specific to ink!/Rust and not very useful for other languages like Solang which is why it cannot be mandatory. When the field is missing cargo-contract will assert that the associated ink! constructor or message in fact does NOT return a Result. This means that whenver a user returns a Result type having this new ink! attribute is mandatory to set. If the user wants to revert the execution upon Result::Err the message or constructors should be flagged with either revert_on_err or revert_on_err: true which is both equivalent. If the execution should not be reverted upon Result::Err the user is mandated to flag the ink! constructor or message with revert_on_err: false.
• Besides this the currently generated signatures of call and deploy no longer return a u32 to make the ink! codegen compatible with the most recent SEAL versions again.

This design allows for an efficient design that ink! and cargo-contact can statically verify and which pleasantly fits into the current ink! design from a syntactical point of view.

An example follows:

/// It is still possible to create normal non-Result messages or constructors.
#[ink(message)]
fn foo(&self) -> i32 { ... }

/// Long-form notation, mandates revert upon returning `Result::Err`.
#[ink(message, revert_on_err: true)]
fn try_foo_1(&self) -> Result<i32, MyError> { ... }

/// Short-form notation, mandates revert upon returning `Result::Err`.
///
/// Absolutely the same as `try_foo_1`.
#[ink(message, revert_on_err)]
fn try_foo_2(&self) -> Result<i32, MyError> { ... }

/// Long-form notation, does NOT revert upon returning `Result::Err`.
#[ink(message, revert_on_err: false)]
fn foo_2(&self) -> Result<i32, MyError> { ... }

/// This is invalid ink! code and will yield a compile error via Rust compiler
/// since the message is not returning a Result type.
#[ink(message, revert_on_err: true)]
fn foo_3(&self) -> i32 { ... }

/// This is invalid ink! code and will yield an error via cargo-contract when being compiled
/// since the mandatory revert_on_err for Result type returns is missing.
#[ink(message)]
fn foo_4(&self) -> Result<i32, MyError> { ... }

I don't think this solve this problem... If ink! chooses revert_on_err when Result return Err, why not chooses using assert to revert directly? And this error does not feedback to pallet-contracts layer, it just know the contract is reverted, but know why. In this view, using revert_on_err and assert are not different for outside, developers and users do not know the reason why the contract meet an error. The corrent way to report the error reason is to design a protocol between pallet-contracts and ink!, let pallet-contracts to know the logic of contracts.

In fact, all thing is:

• pallet-contracts knows if executing wasm is failed
• pallet-contracts does not know if the contract business logic is failed. Thus need a way to let contract part to tell pallet-contracts, it meets a business logic fail.

[xgreenx]

I agree with @atenjin . The behavior of revert_on_err is similar to assert or panic. If we revert on error during child call it doesn't allow us to handle the error in the parent contract and process it correctly in case of business logic.

So the most simple way is to use assert and panic in contracts instead of returning Result. It also will reduce the code generated for the contract, because we don't need to support Result, Error, Encode/Decode and etc. Also in the case of each contract, we need to have our own Error, which means a lot of generated code to work with them all.

I think we still can define errors in the contract and return them via assert. For example assert!(Error::ZeroAddress::Encode()).
It will work in the same way but will simplify the signature of functions.
As @atenjin mentioned, we need only to support returning of assert message on pallet-contracts level and it will be enough for usage.

[Robbepop]

I agree with @atenjin . The behavior of revert_on_err is similar to assert or panic. If we revert on error during child call it doesn't allow us to handle the error in the parent contract and process it correctly in case of business logic.

This is a very good point! So a contract should ideally behave differently in this regard depending on if it was called by another contract (to allow for error handling) or an origin user (to properly forward the error).

[xgreenx]

This is a very good point! So a contract should ideally behave differently in this regard depending on if it was called by another contract (to allow for error handling) or an origin user (to properly forward the error).

Yes, if you plan to add support of it into contract-pallet. For example, only revert the transaction if the return from function - initiator(first function in the stack of pallet) returned Revert flag. Then the introduction of Result and Error semantic will have the reason. Otherwise better to use assert and panic.

[Robbepop]

This is a very good point! So a contract should ideally behave differently in this regard depending on if it was called by another contract (to allow for error handling) or an origin user (to properly forward the error).

Yes, if you plan to add support of it into contract-pallet. For example, only revert the transaction if the return from function - initiator(first function in the stack of pallet) returned Revert flag. Then the introduction of Result and Error semantic will have the reason. Otherwise better to use assert and panic.

No need to implement on contracts pallet level. We should be able to fully control this in ink! itself.

[xgreenx]

This is a very good point! So a contract should ideally behave differently in this regard depending on if it was called by another contract (to allow for error handling) or an origin user (to properly forward the error).

Yes, if you plan to add support of it into contract-pallet. For example, only revert the transaction if the return from function - initiator(first function in the stack of pallet) returned Revert flag. Then the introduction of Result and Error semantic will have the reason. Otherwise better to use assert and panic.

No need to implement on contracts pallet level. We should be able to fully control this in ink! itself.

How do you want to distinguish the child's contract call vs initial contract call?

[Robbepop]

ink! smart contracts are compiled in different levels whether they are at root or not. So if you are compiling your ink! smart contract normally you will get the behavior that acts as if it can only be called as root. If you compile it via ink-as-dependency you can only use its codegen from within another contract and therefore know that only other contract (the root one) can call it.

[xgreenx]

Okay, if it is already possible(to understand where is root call where is not), it is cool. Seems I have ideas on how it can be done in the current scope of codegen. During the generation of code for cross calling, we can ignore revert_on_err and use it only during the generation of entry... Oh, wait. How contract-pallet will process seal_call correctly(to not revert it because we want to process error in root contract) if it contains revert flag?

Another actual question. Do we need to allow to handle errors in business logic, or better to do always 'assert' or 'panic'? Because it really adds a lot of additional code to handle/pass/work with Result and Error.

[Robbepop]

Oh, wait. How contract-pallet will process seal_call correctly(to not revert it because we want to process error in root contract) if it contains revert flag?

Contract pallet will just do what ink! commands it to.

Do we need to allow to handle errors in business logic, or better to do always 'assert' or 'panic'?

Depends on your use case. For Rust recoverable errors are returned via Result and unrecoverable errors are done with panic! and the likes.

[xgreenx]

Contract pallet will just do what ink! commands it to.

Yes, but I meant that we have already deployed another contract(with feature revert_on_error), and this contract contains the logic which returns a result with REVERT flag. What will do contract-pallet in this case? It will ignore the flag and will return back to the context of the root contract or will abord the execution?=)

Depends on your use case. For Rust recoverable errors are returned via Result and unrecoverable errors are done with panic! and the likes.

You are right. I'm thinking about the standardization of tokens and other useful stuff. And the question do we need to return errors in case of Erc* or better to assert=)
The problem is that when the library will be popular, people will use patterns from there, and if we don't use Result and Error, they will also ignore them.

[Robbepop]

Yes, but I meant that we have already deployed another contract(with feature revert_on_error), and this contract contains the logic which returns a result with REVERT flag. What will do contract-pallet in this case? It will ignore the flag and will return back to the context of the root contract or will abord the execution?=)

The ink! version you are using does not yet properly support revert on error. So it will just do not revert on Err.

You are right. I'm thinking about the standardization of tokens and other useful stuff. And the question do we need to return errors in case of Erc* or better to assert=)
The problem is that when the library will be popular, people will use patterns from there, and if we don't use Result and Error, they will also ignore them.

This is not decided by ink! but purely decided by how you want people to operate on your API. So if you want them to handle errors go for Result, if the errors cause inacceptable state that cannot be recovered, go with panic! and the likes.

[xgreenx]

The ink! version you are using does not yet properly support revert on error. So it will just do not revert on Err.

I know that. I meant let's imagine, that we already implemented that=) And the question is what will do contract-pallet when a child's call will return with REVERT flag?

This is not decided by ink! but purely decided by how you want people to operate on your API. So if you want them to handle errors go for Result, if the errors cause inacceptable state that cannot be recovered, go with panic! and the likes.

Okey, we will define that during PSP for standard tokens=)

[Robbepop]

I know that. I meant let's imagine, that we already implemented that=) And the question is what will do contract-pallet when a child's call will return with REVERT flag?

It will revert.
However, I am not sure if the planned ink! design for this feature is in a good shape yet.

[xgreenx]

Hello again=) We thought about how this feature can be implemented. It is clear how it must work in the case of single contract execution. But we faced several issues which don't allow (we think so) to implement revert_on_err, and also we found a bug in the design of cross calls (when parent contract calls a child contract). We will describe them here and let's discuss together can we implement this feature or not.

Firstly, we have one restriction which must be covered during cross-contract calls. If the child contract decided to revert the transaction, the contract pallet SHOULD revert it.

It is needed because during contract execution we can flush into storage, but later we can decide to revert the execution. We shouldn't force the user to work in the paradigm "check everything before modification of the storage". !ink and contract pallet must be agnostic in this case.

But at the moment it's impossible with the current design of the cross calls in the contract pallet. In the current implementation, the parent contract must decide how to process the CalleeReverted error. It means that the parent contract can IGNORE the revert flag, and end the execution of the transaction successfully. @athei

1. Based on the rule above, it means that if we decide to revert the transaction, we don't have the reason to continue the execution of the transaction, because any result in the future will be reverted.

2. We can't directly return the encoded result during revert, because:
   The ContractA can return the Result<_, ErrorA>. The ContractB can return the Result<_, ErrorB>.
   If the ContractA calls the ContractB, but ContractB decides to revert the transaction, it means that the method from the ContractA will return the Result<_, ErrorB>. But ABI says that the return type is the Result<_, ErrorA>. So revert always must use the same output type, for example, string.

Points 1 and 2 mean that no reason to return a result from the function, which is marked with revert_on_err = true.
So the developer must use the Result only in cases when he can really return an error before modification of state without reverting.

BTW, the second point is related to error propagation from child contract to parent and to end-user. ContractReverted should use DispatchError::Custom to propagate an error. (e.g. ContractReverted.InsufficientBalance)

We need to decide what we want to return in return_value method. The most simple case is return string with error and didn't try to support other types like Result(I meant don't try to decode the result from ContractReverted on UI side and always work only with strings during revert, overwise it can cause an issue with different return types during cross-contract calls described in point 2).

[athei]

It means that the parent contract can IGNORE the revert flag, and end the execution of the transaction successfully. @athei

Changes applied by the child contract are reverted on failure (setting the REVERT flags falls under this) and the parent has no way to prevent this.

[seanyoung]

Why ContractReverted can't store the output buffer inside? Like String or Vec, and UI will be able to decode the result based on metadata(Let's call it ABI in this context to be more clear).

Because we are not allowed to emit arbitrary data to the block. Any data emitted there would become part of the block. The same reason why errors are not strings but just indices. It is just a convention in substrate that additional data must be emitted in events. Emitting one event per contract execution with all the data is something that is way to heavyweight. It is just something we need to live without. That said, any contract is free to emit custom events.

Just because it isn't stored in the block, doesn't mean it can't be done. The return buffer can be re-computed, maybe through an RPC which reruns the wasm with the right input state?

[xgreenx]

@athei

In which non-error state do you think it is useful to revert?

I can't imagine that=D But the developer still can return a result with Ok and some value, but mark it like a REVERT and then I can't say that it is "error". It is "revert"=)

Any data emitted there would become part of the block. The same reason why errors are not strings but just indices

I see. If you are using that errors to store information about the execution of the call in the blockchain, then it makes sense.

Right now that isn't implemented at all (see linked issue above). What you are suggesting is that the output buffer is independent of Ok vs Err (right now it is only available on Ok).

Yes, because the developer can return something in the Ok case and in the Err case. And the only rule which he must follow: The user of the contract must be able to decode the output based on the ABI, otherwise it is not a usable contract.

@seanyoung

Just because it isn't stored in the block, doesn't mean it can't be done. The return buffer can be re-computed, maybe through an RPC which reruns the wasm with the right input state?

To compute the output you need to have a state of the whole blockchain(or only for contracts that can be affected during contract call executed) at block X before execution of this transaction in this block. It is not a trivial task. + in contracts you can call some methods of pallets via ChainExtension so, it means that we also need the state of other extensions at block X. I think it impossible to implement=)

---

It is just a convention in substrate that additional data must be emitted in events. Emitting one event per contract execution with all the data is something that is way to heavyweight. It is just something we need to live without. That said, any contract is free to emit custom events.

Returning only of the status of the execution similar to method get_transaction_status. But the user of the contract is waiting via RPC for the end of execution of the transaction. I think it must be two different behaviors(get_transaction_status and "send on-chain transaction via RPC") because in the case of on-chain RPC we are waiting for a block with the result for our transaction, and when we will get the block, we will verify it, and during verification(I didn't check the logic of verification, but I think you are executing each transaction in the block, Am I right @athei?), we can compute the output of the transaction and we can return in via RPC call.

So in this case we will have the same behavior as for off-chain RPC call. And if someone will ask the blockchain about the status of the transaction, he will get only the result from the block without buffer and additional stuff.

[athei]

Yes, because the developer can return something in the Ok case and in the Err case. And the only rule which he must follow: The user of the contract must be able to decode the output based on the ABI, otherwise it is not a usable contract.

That makes sense.

Returning only of the status of the execution similar to method get_transaction_status. But the user of the contract is waiting via RPC for the end of execution of the transaction. I think it must be two different behaviors(get_transaction_status and "send on-chain transaction via RPC") because in the case of on-chain RPC we are waiting for a block with the result for our transaction, and when we will get the block, we will verify it, and during verification(I didn't check the logic of verification, but I think you are executing each transaction in the block, Am I right @athei?), we can compute the output of the transaction and we can return in via RPC call.

So in this case we will have the same behavior as for off-chain RPC call. And if someone will ask the blockchain about the status of the transaction, he will get only the result from the block without buffer and additional stuff.

I didn't understand this at all. Sorry. Maybe this helps: No matter whether a call dispatchable is submitted via an extrinsic (on-chain) or an RPC (off-chain) the same code is run. The only difference is that the RPC can return data while the extrinsic cannot. Returning data from a contract has two uses:

• Return data to a a calling contract. (i.e the returning contract is not the top level contract)
• Conveying a custom error to any UI which pre-runs any dispatchable before submitting it as RPC (i.e the contract is the top level contract)

It is not for emitting data to off chain observers. This is what events are for. Yes it can be annoying that this is different from ethereum where any output of a contract is automatically put into the permanent record.

Just because it isn't stored in the block, doesn't mean it can't be done. The return buffer can be re-computed, maybe through an RPC which reruns the wasm with the right input state?

This would only work on an archive node because it would need historical state. I don't think we should depend on that for essential functionality. If depending on archive nodes is OK you could just emit an event for any returned data which can be queried from archive nodes at historical blocks (I think).

[xgreenx]

Maybe this helps: No matter whether a call dispatchable is submitted via an extrinsic (on-chain) or an RPC (off-chain) the same code is run. The only difference is that the RPC can return data while the extrinsic cannot. Returning data from a contract has two uses:

• Return data to a a calling contract. (i.e the returning contract is not the top level contract)
• Conveying a custom error to any UI which pre-runs any dispatchable before submitting it as RPC (i.e the contract is the top level contract)

I understand that. I'll try to describe what I meant=)
We have a "Send transaction" action. At the moment the output for this action is extrinsic. Under the hood, this action still does a request with the transaction to the blockchain and waits for the response, when the transaction will be confirmed.

And here we can add some additional features like: "Hey, blockchain. I sent you a transaction and I'm waiting for confirmation. Could you include the output of this transaction also in the response, please? I know that you can do that because you will verify the block which contains this transaction and you will compute the output during verification. So, you can add this output to the response, but not include it into the block."

It will be only behavior for the case if someone sent the transaction to the node, this node will return the full response back. But if someone asks for information about the result of execution of the transaction separately, it will get only information available in extrinsic(So behavior only for the initiator of the transaction).

But it is one idea. Another is more general, to add a configuration to the pallet to store the output of execution in some storage(off-chain data), and support additional RPC requests to get this data. So, if some node owner decided to provide more info(not only extrinsic information stored on-chain but also additional information), he can enable it in the config of runtime. In this case, common block producers/verifiers don't need to enable it and provide this info. But some API nodes can enable it and provide more information from the box. It is untrusted information, anyone can modify it, but at the moment it is only information for debugging.

This would only work on an archive node because it would need historical state. I don't think we should depend on that for essential functionality. If depending on archive nodes is OK you could just emit an event for any returned data which can be queried from archive nodes at historical blocks (I think).

BTW, does an archive node also store state for each pallet? If yes, it is cool, but seems it will take a lot of space=D

[athei]

And here we can add some additional features like: "Hey, blockchain. I sent you a transaction and I'm waiting for confirmation. Could you include the output of this transaction also in the response, please? I know that you can do that because you will verify the block which contains this transaction and you will compute the output during verification. So, you can add this output to the response, but not include it into the block."

In theory this would be possible. In practice an extrinsic is not allowed to return arbitrary data and therefore this isn't possible on polkadot. One reason I can imagine why this is is because this would not work with light clients which do not execute imported blocks. Polkadot tries really hard to not break light clients.

But it is one idea. Another is more general, to add a configuration to the pallet to store the output of execution in some storage(off-chain data), and support additional RPC requests to get this data. So, if some node owner decided to provide more info(not only extrinsic information stored on-chain but also additional information), he can enable it in the config of runtime. In this case, common block producers/verifiers don't need to enable it and provide this info. But some API nodes can enable it and provide more information from the box. It is untrusted information, anyone can modify it, but at the moment it is only information for debugging.

This a a lot of complexity for something for which the solution already exist: Just emit an event if you want to observe something happened on-chain. That works with light clients, too. If there is a off-chain solution I am strictly against adding complexity to the runtime. Because the runtime needs to be safe and audited and adding stuff there is highly expensive for that reason. Complexity should be in the off-chain tools. I know that sucks of you are the one developing those tools but is for the best.

[xgreenx]

This a a lot of complexity for something for which the solution already exist: Just emit an event if you want to observe something happened on-chain. That works with light clients, too. If there is a off-chain solution I am strictly against adding complexity to the runtime. Because the runtime needs to be safe and audited and adding stuff there is highly expensive for that reason. Complexity should be in the off-chain tools. I know that sucks of you are the one developing those tools but is for the best.

We are trying to resolve the question, how better to propagate the error to the end-user. If each developer(or ink!) uses events to do that, it will the same like store the error message on-chain. But this solution will be more complicated than variant to store output buffer inside of error.

I prefer to avoid storing any data related to error propagation, on-chain(Like events or buffer in the body of the error).

In the case of storing this data optionally off-chain allows us to use the same API for RPC calls and for "Send transaction" actions(As you mentioned before, we can return Option<(Flags, Buffer)>). In the case of a light client, it will be None, in the case of the node with an enabled flag to store output, it will be a Some with data.

This variant allows us to not spam the network with events about errors. And still, handle it simply in UI/DApps.

[xgreenx]

@cmichi I think we need to describe something about the progress of this task=)
Because #665 implements revert on an error by default.

BTW, #665 doesn't cover the case with cross contract calls. Because contract-pallet doesn't pass the encoded result after return_value.

[athei]

I prefer to avoid storing any data related to error propagation, on-chain(Like events or buffer in the body of the error).

Events are generally not considered as storing data on-chain because they don't became part of the state.

For the rest of your posting I can't quite follow. Are you suggesting to return the output buffer in case of an RPC call?

[xgreenx]

Events are generally not considered as storing data on-chain because they don't became part of the state.

But does the full node still store them somewhere?

For the rest of your posting I can't quite follow. Are you suggesting to return the output buffer in case of an RPC call?

Yes, Send transaction RPC call can return Option<(Flags, Buffer)>. It is Some if the node cached the result of the execution otherwise None.

[Robbepop]

From my perspective this feature has been implemented on the ink! side as stated above with PR #665 and can therefore be closed.
Please correct me if I am wrong @xgreenx since I do not really understand your objections to this claim.

[athei]

But does the full node still store them somewhere?

No. It gets deleted from state after the block was executed it was part of. Only archive nodes hold events forever.

Yes, Send transaction RPC call can return Option<(Flags, Buffer)>. It is Some if the node cached the result of the execution otherwise None.

The call and instantiate RPC already return the output buffer. So I do not really get what you are suggesting. You can't do the same for transactions. Transactions can't return any data.

[xgreenx]

From my perspective this feature has been implemented on the ink! side as stated above with PR #665 and can therefore be closed.
Please correct me if I am wrong @xgreenx since I do not really understand your objections to this claim.

During cross contract call contract-pallet returns CalleeReverted. But we expect Result with Err. In theory, this is the responsibility of the contract-pallet to return an encoded output of the child contract, not CalleeReverted . But we still need to clean up in ink! the code related to CalleeReverted.

[athei]

This is just wrong. Even in case of CalleeReverted the output buffer is copied to the calling contract. That is the whole point of this issue.

[xgreenx]

But does the full node still store them somewhere?

No. It gets deleted from state after the block was executed it was part of. Only archive nodes hold events forever.

Yes, Send transaction RPC call can return Option<(Flags, Buffer)>. It is Some if the node cached the result of the execution otherwise None.

The call and instantiate RPC already return the output buffer. So I do not really get what you are suggesting. You can't do the same for transactions. Transactions can't return any data.

I described in this comment the idea similar to the behavior of events - [1] if the node is an archive node, then it can store the output of the execution.

And this comment described some simplification - [2] if the node is an archive node and stores the output of the execution, it can return this output as a result of Send transaction(It is why it is optional)=)

Implementation of [1] will be enough to show the error on the UI side. If the Send transaction call returned an error, UI will do an additional call to the node to get an output.

The difference between [1] and events, is that UI will know how it can request a detailed description of the error(output).

But maybe in UI, we can show all events of the execution and the user will be able to see the event related to the error.
So maybe in this case we don't need [1], but we need proper UI=)

[Robbepop]

Do I derive correctly that this discussion should be held in the issue tracker for the contracts-pallet and not in ink!'s issue tracker since this functionality seems not related to ink! ? I'd then like to close this issue.

[xgreenx]

This is just wrong. Even in case of CalleeReverted the output buffer is copied to the calling contract. That is the whole point of this issue.

Okey, so it must be implemented on ink! side. @Robbepop After cross contract call ink! receives CalleeReverted , does expect, and fails with ContractTrapped.

Based on the comment from the @athei ink! should handle CalleeReverted and decode the output buffer.

[Robbepop]

As can be seen here ink! always provides the result buffer to SEAL properly:
https://github.com/paritytech/ink/blob/e8d4739649293a458c3958bec802d2d750067d98/crates/lang/src/codegen/dispatch/execution.rs#L167

[xgreenx]

As can be seen here ink! always provides the result buffer to SEAL properly:

https://github.com/paritytech/ink/blob/e8d4739649293a458c3958bec802d2d750067d98/crates/lang/src/codegen/dispatch/execution.rs#L167

Yes, it is right. The problem is on the side of the root contract.
Here you will get CalleeReverted and you will forward this error upper, but it is a valid case and you can decode the output.
Here it will fail with ContractTrapped.

[Robbepop]

Ah I think I finally get what you mean. I will inspect it.

[athei]

I described in this comment the idea similar to the behavior of events - [1] if the node is an archive node, then it can store the output of the execution.

And this comment described some simplification - [2] if the node is an archive node and stores the output of the execution, it can return this output as a result of Send transaction(It is why it is optional)=)

I think we are mixing two separate concerns in this discussion:

1. Error handling between contracts.
2. Presenting the return value of the top level contract to the UI.

I will comment on 2:

It won't work because the transaction won't return anything. Even if if the dispatchable would return anything (when your client executes the transaction before submission to verify it) you can't be sure that you get the same result when a validator receives and executes the transaction later. With the exception of emitting stuff to storage (events are a lighter variant of this) of course which we have discussed is way to expensive. At that point this isn't any better than use the result of the dry-run RPC to present rich error messages to the user. In fact it is exactly what you are asking for. It happily returns you a debug buffer and the output buffer. The thing is that UIs today are not doing this. But it is possible and how we intent to provide error reporting.

[xgreenx]

I described in this comment the idea similar to the behavior of events - [1] if the node is an archive node, then it can store the output of the execution.
And this comment described some simplification - [2] if the node is an archive node and stores the output of the execution, it can return this output as a result of Send transaction(It is why it is optional)=)

I think we are mixing two separate concerns in this discussion:

1. Error handling between contracts.
2. Presenting the return value of the top level contract to the UI.

I will comment on 2:

It won't work because the transaction won't return anything. Even if if the dispatchable would return anything (when your client executes the transaction before submission to verify it) you can't be sure that you get the same result when a validator receives and executes the transaction later. With the exception of emitting stuff to storage (events are a lighter variant of this) of course which we have discussed is way to expensive. At that point this isn't any better than use the result of the dry-run RPC to present rich error messages to the user. In fact it is exactly what you are asking for. It happily returns you a debug buffer and the output buffer. The thing is that UIs today are not doing this. But it is possible and how we intent to provide error reporting.

Yea, we have severals topics here=D

Sorry, let me clarify. In this comment I agreed with you, that events can cover everything, that we need(and my idea to store separately output is useless). We only need a proper UI that will show all events generated by the transaction. And if the transaction is marked as ContractReverted, the user must check events by self and find the reason=)

So let me summarize. We have two ways to propagate the error to the user.

1. RPC call returns (Flags, Buffer)(or it is not implemented yet?). The UI(or any framework) always can do an RPC call before Send transaction. If RPC fails, UI can show the output buffer(the same schema is used in Ethereum).
2. The developer of the contract(or it can be the default behavior in the ink!) can emit an event on panic(he should override panic_handler, but it will increase the size of the contract).
   2.a UI can show the list of events generated by the contract, and the user must find the "error" event by himself.
   2.b Or we can define the error event in ink!. In this case, UI will look only for the signature of this event to show it.

The question here we prefer 2.a or 2.b? And who will add events to UI?=)

[athei]

You are completely right about 1. This is already implemented. The UIs just don't make use of it right now.

Events are something that are in the domain of the individual Dapps that build on ink!. A generic UI would just show all events generated by a contract. They can be decoded automatically by using the ABI file. Please keep in mind that the target audience of those generic UIs are contract developers and not users. For developers those the decoded events make sense.

The question here we prefer 2.a or 2.b? And who will add events to UI?=)

Users will interact with contracts through custom Dapps that can make sense of those events and display them in a friendly manner.

[xgreenx]

Events are something that are in the domain of the individual Dapps that build on ink!. A generic UI would just show all events generated by a contract. They can be decoded automatically by using the ABI file. Please keep in mind that the target audience of those generic UIs are contract developers and not users. For developers those the decoded events make sense.

Yes, I'm only thinking about UI for developers=) I meant that we need to introduce the list of decoded events into canvas-ui(is it the main UI for developers?). And do an RPC call before Send transaction.
Maybe you can create an issue for that in the according repository(or I can do that, but in which repository? Because canvas-ui's last commit was in July). Resolving this new issue and paritytech/substrate#9338 will fully cover error propagation flow.

#975 is merged so revert_on_error is implemented and we can close this issue.
Small question to @cmichi and @Robbepop : Do we want to add some feature to ink_env like event-on-error which will dispatch error event generated by ink! with a message from the panic function? If the developer wants to get an error event, he will enable this feature.

[Robbepop]

Do we want to add some feature to ink_env like event-on-error which will dispatch error event generated by ink! with a message from the panic function? If the developer wants to get an error event, he will enable this feature.

Please open an issue where we can discuss the specifics of this feature request. Thank you for your help today with the revert_on_error issues!
I will close this issue now since it has been implemented by both:

• Implement new trait definition codegen 🚀  #665
• Always revert state upon encountering Result::Err #975