Return NotCallable if contract hasn't method with selector X and hasn't a fallback function

[xgreenx]

If during cross contract call the callee contract hasn't a selector, ink! will panic and cross contract call will return CalleTrapped.

I think we should return another error, for example, NotCallable in this case to identify to the caller, that call can't be processed.

In the future, ink! will have a fallback function. So the rule is: "if contract hasn't method with selector X and hasn't a fallback function it will return Some_Error_Here".

What do you think?

[athei]

Are you suggesting that ink! encoded an error in it's output buffer and sets the REVERT flag instead of panicing?

[xgreenx]

It is a good question how better to implement that.
The idea was that we will return an error that described that the handler for the method does not exist. So the idea with the REVERT flag is bad because it is CalleeReverted for the caller and the output is not expected(is not the same as in ABI, it is NotCallable).

I think maybe we can add an additional flag to return_value like set_not_callable. If it is true the seal_call will return NotCallable.

[athei]

So the idea with the REVERT flag is bad because it is CalleeReverted for the caller and the output is not expected

Wrong. It is precisely the reason for the existence of REVERT to pass output in the face of a revert. This is exactly why it exists. Otherwise we could just trap.

I think maybe we can add an additional flag to return_value like set_not_callable. If it is true the seal_call will return NotCallable.

There already exists a mechanic to pass along output. It is called REVERT. This could can easily solved in user space. Just make the encoding of the output buffer part of the standard.

[xgreenx]

There already exists a mechanic to pass along output. It is called REVERT. This could can easily solved in user space. Just make the encoding of the output buffer part of the standard.

I understand. The problem that means that we will have 3 levels of error:

• Error on Smart contract logic level
• Error on ink! logic level
• Error on contract-pallet level

And the output in that case should be contract_pallet::Result<ink::Result<contract::Result<>>>.

Right now it is contract_pallet::Result<contract::Result<>> and I prefer to not change it because it will add additional logic and will increase the size of contracts.

set_not_callable will be for ink! to show that handler does not exist. It is the same as REVERT but + some additional information for the caller.

If you think that better to create additional Result on ink! level, then okay, but it will make things more complicated=)

[athei]

I see your problem. However, adding more and more policy into the privileged code is a slippery slope when this is basically an ABI problem. Nesting Result is only one way to go about this problem. For example, the ABI could very well be changed that the output buffer is not the raw encoded bytes of the contract but some other data structure where ink! could also emit some information.

The contracts pallet has no notion of messages. On purpose. So transmitting this information makes no sense.

[xgreenx]

Yep, the problem is that other languages also should do it in the same way and It will affect current standards because return data changed.

The contracts pallet has no notion of messages. On purpose. So transmitting this information makes no sense.

I understand that and I think it is a good idea to be agnostic to that information because. It allows having a Proxy pattern.

My thoughts are that it is okay to provide an API that allows returning any kind of error from the contract like CalleeTrapped, CalleeReverted, and NotCallable. In that case callee contract decided by itself how better to inform the caller about an error. We still are agnostic, we only very flexible.

[athei]

We already have CalleeTrapped and CalleeReverted which make sense because they carry a clear enforceable semantic. NotCallable would be essentially just a return code emitted by the callee to the caller. It is just a sidechannel to the already existing output buffer.

The reason why you are asking for this is because the ABI is lacking some semantic: Having some general errors defined in the ABI in addition to the custom errors of the contract. You need to push for a new ABI version that includes this semantic. Adding redundant information to pallet contracts just to ease the pain of a ABI version migration isn't something I am aboard with.

It doesn't need to be represented as Result<Result>> at the caller. If the ABI is well designed those can be unified.

[xgreenx]

The reason why you are asking for this is because the ABI is lacking some semantic: Having some general errors defined in the ABI in addition to the custom errors of the contract. You need to push for a new ABI version that includes this semantic. Adding redundant information to pallet contracts just to ease the pain of a ABI version migration isn't something I am aboard with.

It doesn't need to be represented as Result> at the caller. If the ABI is well designed those can be unified.

I don't see how we can implement it without Result<Result>>.

The problem is that it should by additional information for the contract's language, that can be represented somehow useful. But that means that on the contract-pallet level we need to add the additional output of the seal_call method that will contain that additional information for the language(if we don't want to use Result<Result>> and change the output type). That additional information should be standardized for all languages and contracts.

Are you sure that it is better?

[athei]

The contracts pallet does not and will never deal with "languages". It just sees a wasm blob. Making a distinction between "contract errors" and "language errors" is a completely made up concept by ink!. It is within in the languages/ABIs responsibility to do things differently if that is needed. And this is exactly where it would be accomplished. All the mechanics are there.

I don't see how we can implement it without Result<Result>>.

We change the ABI so that the return type of any message/constructor is Result<T, E> where E = () in case the user defined an infallible message:

enum Error<E> {
    UnknownMessage,
    Custom(E),
}

type Result<T, E> = std::result::Result<T, Error<E>>;

We already match on Result return types and act on them by reverting on Err. So it isn't a big deal to attach further semantic to it.

[xgreenx]

type Result<T, E> = std::result::Result<T, Error>;

Is the same as Result<Result<...>> in terms of the return type. It changes the return type of each message in contracts. It changes the current flow and that means that we need to update standards like PSP22, PSP34...

It is breaking change and the main problem is that it affects standards on ABI level=(
Maybe it is okay to modify accepted standards now because ink! and contract-pallet are not ready for production and it is not stable fully. But better to avoid changes like that in the future otherwise it breaks the idea of PSPs=)

[athei]

Yes. It doesn't avoid breaking the ABI but it is less annoying in that it doesn't require a double unwrap which would be very confusing.

It is breaking change and the main problem is that it affects standards on ABI level=(
Maybe it is okay to modify accepted standards now because ink! and contract-pallet are not ready for production and it is not stable fully. But better to avoid changes like that in the future otherwise it breaks the idea of PSPs=)

I think we can agree that this would be the right thing to do even though it is messy. The mechanism you ask for to be added to pallet-contracts would only serve the purpose of avoiding this breaking change by adding a work around on top. We will add something that serves no real purpose. It is just there forever because we didn't want to break ABI.

I also don't see how your standard wouldn't change with a new side channel in pallet-contracts. You would still need to amend the standard to include this semantic.

I think now is still time to make this ABI change. It isn't as bad as it sounds.

[athei]

Replaced by #1207