Return Result from external program calls

[iFrostizz]

Returning a faillible type from external program calls has been a concern since some time because two of these issues have been raised: #666 and #595 and because of the recent updates, we are probably more than ever ready for this to be implemented.
Currently, external calls will panic in the runtime and this is probably not a great way of doing since we cannot handle errors at all in the execution as they will halt the whole execution.

If we look at the implementation of program-to-program calls, they return a single 32 bits value which is the pointer to the return data. We currently cannot use multi values to return an error code as the 2nd value of the tuple, as they are not implemented in the official target.

This discussion is still at the research state, so I will add possible paths that we can explore further.

wasm-bindgen does something similar so we might want to do that:

• https://rustwasm.github.io/docs/wasm-bindgen/reference/types/result.html
• Replace Result<T, JsValue> with Result<T, Error> rustwasm/wasm-bindgen#1742
• Allow for returning custom error type in Result rustwasm/wasm-bindgen#1004

[iFrostizz]

This is how wasm-bindgen implements their error handling: https://github.com/rustwasm/wasm-bindgen/blob/c108c7520a13fa124038b28db6fb759b35576dd4/src/convert/impls.rs#L451 custom error types can be returned as long as their implement Into<JsValue>. We could do something similar by defining a custom error type with some default variants for system errors. The only caveat is that it will allow programs to return these system errors. Is that okay ?

[iFrostizz]

We could probably have system error codes defined as negatives. That way, the simulator will know if a program created an error that is reserved and return a specific error for that.
The problem with using only error codes is that program errors won't be able to return context, for instance a variable in the error message.
For this reason, we should probably have a custom error trait that holds a message and a code method.
Once this works well, some macros will be used to make the implementation less work, as well as avoiding the use of system errors in programs.

[richardpringle]

we cannot handle errors at all in the execution as they will halt the whole execution.

What do you mean here?

And wasm-bindgen isn't a good example because the problem space differs. In a wasm-module executed from JS in the browser (the typical wasm-bindgen usage), there is no need for determinism. Most errors we could encounter are non-deterministic on the host side, and thus we cannot return them in a Result.

Maybe we should come up with a list of potential error cases so that we have a better defined problem space.

[iFrostizz]

What do you mean here?

It was an oversight, program calls are instantiating a new runtime, even when they are in the context of a program execution (program to program call).

About non-determinism, I agree. But if we take one of the possible errors which is an error when interacting with the database, it would probably not help regarding determinism and it is fair that the execution should return an error if it happens, handling this error would probably not make a difference.

The errors that can happen in the import_program.go file are:

• Deserialization of the callProgramInput
• The call is requesting too much fuel
• Execution errors
  • Program is not found in the db
  • Compiling the module in the engine
  • Issue during the instantiation of the program in the runtime
  • Execution error if the function does not exist, or that the execution panicked

All of these errors are not fatal and it would probably make sense to be able to catch them. For instance a lending protocol which calls an oracle and can fallback to another if the first one fails. Another example would be a multicall contract, where it would make sense to either stop execution if some fatal error happened, or keep executing if there was a program error.
I have opened a branch that catches these errors so that they can be manipulated on the host, but they might not be the "endgame" of faillible public functions main...result_from_public

[richardpringle]

• Deserialization of the callProgramInput
  Is fatal. It means the user isn't using the wasmlanche-sdk or there's a bug in the runtime
• The call is requesting too much fuel
  Yes, in theory would could catch this error, but why would you want to, what value does it add? It would be better if the program does this check beforehand
• Program is not found in the db
  Yeah, this one is potentially recoverable, but it would definitely not be a common case.
• Compiling the module in the engine
  This would be a larger problem. When someone creates a program, the runtime should check that it compiles before storing that program in state. We don't want to fill VM state with a bunch of garbage that doesn't work... so if we actually hit that case, we would have larger concerns.
• Issue during the instantiation of the program in the runtime
  Essentially the same concerns as above
• Execution error if the function does not exist, or that the execution panicked
  If a function doesn't exist, that's an interesting case. I personally don't think that should be recoverable as we should really be able to dynamically generate function names. I realize that we can't prevent that from happening, but making the error recoverable would kind of be encouraging something that doesn't really fit all that well with the type-safety we try our best to provide. As for execution panicking, I agree that this should be recoverable.

But having #[public] functions return a Result is not just about programs calling programs. It's the public API. I would expect it to be just as common for #[public] functions to be called directly by user transactions. How do we want to handle possible errors in those cases? Currently, a user can return their own result, and if either the Result::Error or the Result::Ok fails to serialize, the module will panic, and we won't return anything meaningful to the caller (either an action or a program).

To develop a solution that properly solves the problem, you need to define the problem and its scope fully. Only then can you devise a solution, let alone implement it.

You need to take a few steps back and think about this from a higher level. Please don't look at the current runtime implementation; it's subject to change and it should necessarily be relevant to the solution if the solution is generic enough.

[iFrostizz]

Thanks @richardpringle , so it becomes clear that we need to frame this feature more clearly. I will take popular projects and explain why these may need faillible external calls.

• The ERC20 standard is based on that. When transferring a token, for a transfer to be considered succesful, the external call has to succeed, and the token has to return true. In SafeERC20, openzeppelin does a workaround because some tokens don't return anything but instead just revert on fail. So here the requirement is relaxed to: success transfer = the call didn't reverted and there is no return data, or the call didn't reverted and the return data is true. We could arguably just use a "high-level call" (with an interface) for that though.
• MakerDAO calls untrusted contract and has a runtime check to make sure that the src() function is present (we have an error case that is very similar in the runtime!!)
• The tryAggregate function in the implementation of a Multicall contract which may be allowed to be faillible, so that it won't block other calls to be made.
• It can also be used to early return in the case of the call not being successful, or that the return data is too long, something that we couldn't do if the error state could not be caught.

It's becoming quite clear that some of these errors are fatal, and that we may probably not want them in the user space. Programs don't need to deal with potential bugs in the hypersdk or if they are doing really unsafe stuff, with the wasmlanche-sdk we want to give them the maximum freedom without footguns. If there is something that cannot be done, then we should implement it.

It probably doesn't make sense to be able to catch the "out of gas" error, since if we meter every instruction later, then there is nothing else that the program can do once it runs out of fuel. I'm not sure that a fuel check before a call would be very helpful, it would even be quite redundant and probably require a host call that crosses the WASM boundary, which costs fuel.

I think that we are left with the last case being either an inexistent function if the program is calling untrusted code, or that the execution panicked.

There is a last question left:

Should program authors be encouraged to panic vs returning an error ?

My guess is that they should not. Catching a panic is helpful, but doing pattern matching on a panic is cumbersome. For instance, if an ERC20 returns a library-defined TransferFailed, it's much better and largely more efficient than doing if result.error.is_some_and(|err| err == "transfer failed: reason not enough balance")

[richardpringle]

Thanks @iFrostizz!

So... it seems to me that two things arise here.

Firstly, we probably need to return a result from external calls. An out-of-fuel error seems like it's "catchable"; even a panic could probably be handled. When making external calls, we should probably return a Result<T, ExternalCallError> where the error is a simple enum (no data) that allows the caller to handle the error accordingly.

From what I can tell, I would do the following:

#[derive(thiserror::Error)]
#[non_exhaustive]
#[repr(u8)]
enum ExternalCallError {
    ExecutionFailure = 0,
    OutOfFuel = 1,
    CallPanicked = 2,
}

The ExecutionFailure would be all-encompassing, basically any trap errors that aren't related to fuel or panicking. Though, I'm not sure that it's a good idea. We should probably have an error variant like that, even if we never actually produce that error. Its purpose is explicitly omitting the details of the error but allows the calling contract to carry on knowing that the error wasn't fatal for the entire action. The other two are self-evident. Specifically, signalling a panic makes room for authors who decide to use assert to handle things like balance checking without the foresight of knowing what types of other programs will be calling their program in the future.

Secondly, it seems from your examples that Solidity does not provide great primitives for recoverable errors. Fortunately, since Result<T, E> is already borsh-serializable, there's nothing precluding us from allowing users to define their own Result<T, E>, and we should not opine further. We should not do anything special with the response either. Force the client or the caller to deal with deserialization, etc.

With all that said, serialization is something that we should encourage users to test, and if they experience any failures, panicking is the only course of action.

[iFrostizz]

It looks like we are getting closer to a solution!

I just figured that my observation on the OutOfFuel error didn't made sense in the context of the one that is returned if the remaining gas is less than the one that is requested for the external call

hypersdk/x/programs/runtime/import_program.go

Line 41 in 5753287

return nil, errors.New("remaining fuel is less than requested fuel")

But this variant really makes sense to be caught though, if we are in the execution of a program. I would just like to know if instructions are properly metered, this is a subject that we have already talked about in the past so the answer might just be here.

I agree with the error enum that you provided. About catching panics, we should be able to do so but may not encourage users to do use it. We could stick to Solidity guidelines regarding panics, which is to use it in unreachable code or when an important invariant was broken: https://docs.soliditylang.org/en/latest/control-structures.html#panic-via-assert-and-error-via-require

Assert should only be used to test for internal errors, and to check invariants. Properly functioning code should never create a Panic, not even on invalid external input. If this happens, then there is a bug in your contract which you should fix. Language analysis tools can evaluate your contract to identify the conditions and function calls which will cause a Panic.

As for Solidity, it used to spend all the gas when hitting an assert but doesn't seem to anymore, it's instead just reverting now, that must have changed recently.

If we encourage users to only use panics in case of fatal errors, then I think that it would make sense to merge the CallPanicked and ExecutionFailure together. That way, if an invariant was broken during the execution of a program, we would know that the execution has entered a potentially irrecoverable state and we would only be left with

#[derive(thiserror::Error)]
#[non_exhaustive]
#[repr(u8)]
enum ExternalCallError {
    ExecutionFailure = 0,
    OutOfFuel = 1,
}

Because most users would use the ? operator, aside from edge cases, broken invariants would just revert the whole trace.

[richardpringle]

The difference with solidity is that you can still try-catch a panic (assert). I think that we should provide that ability and offer the same guidelines.

[richardpringle]

But this variant really makes sense to be caught though, if we are in the execution of a program.

I disagree. I think program authors should be responsible for this. I don't see why anyone would (on purpose) write a program where they tried to put a maximum higher than the amount of fuel that was remaining. It's also possible that we don't need to error at all since the value is just a max. We could just pass the min(remaining_fuel, max_units) and call it a day. If the call runs out of gas, too bad.

We should not encourage or make it easy for the host to perform logic that is easily manageable by the wasm-module (aka program).

[iFrostizz]

I was thinking about the OutOfFuel error that is returned by wasmtime, but yeah we probably don't want to catch the OutOfFuel that happens if the gas sent is more than what is remaining