Propagate extrinsic errors over JSON-RPC for locally submitted transactions

[foby]

Extrinsic errors raised within module functions are not propagated over the JSON-RPC interface. The executor indeed handles those errors by sending an ExtrinsicFailed event. But that event doesn't carry any further information about the error.

Example:

In our module transaction we raise this error:

return Err("my_error_code")

And on the client side the transaction result is like:

{
  "events": [
    {
      "phase": {
        "ApplyExtrinsic": 1
      },
      "event": {
        "index": "0x0001",
        "data": []
      }
    }
  ],
  "status": {
    "Finalised": "0xf813791343ba383cb28e4a4b2df2e2948bff1c973de93c1ce404d5d78c335d70"
  },
  "type": "Finalised"
}

So we get the ExtrinsicFailed event (0x0001) but with an empty data field.

Suggestion:
Is it possible to encode the error object into the data field of the event record?

[xlc]

We also really wants this feature because otherwise it is very hard for client side to figure out what's going on.

I can implement this if that's ok. Just let me know if there are anything I need to be caution about.

[bkchr]

CC @tomusdrw

[tomusdrw]

It's related more to how srml/system notes the applied extrinsics than the extrinsic itself, I'm not sure though if it's a good idea to store arbitrary (unbounded) strings inside the event, it seems it could be heavily abused.
We could maybe store few first byte of the hash of the (trimmed) message or better just error codes.

[xlc]

I think the easy way is trunk the error message if it is more than X chars (30?).
Hash will work but it is hard to consume on SDK side and it is not human readable.
Error codes will be the proper way but I don't know how to do it without change the dispatch::Result type and refactor every modules.

[tomusdrw]

I think the easy way is trunk the error message if it is more than X chars (30?).

Sure, but it's really wasteful cause we are talking about 30 bytes vs 1 byte for every failed transaction in the chain, hence why I thought on some other ideas. Maybe we could truncate to a single byte actually :)

[foby]

I'm in favor of numeric error codes (u16 should be large enough). Processing numbers is straightforward and less error prone. Translation into something human readable can be made on the client side.

[bkchr]

Yeah, I would also vote for u16 or similar as error code. (u8 should probably be enough).

[xlc]

Ok. So everyone wants u16 or u8 error code. Then it should really be an enum.
Now the question is, how to do it? Currently the errors are &'static str. Should I change support::dispatch::Result to Result<(), u8> and refactor every modules to convert static string to enum?

Otherwise hash and trunk to u16 may work. If I did math right, with 50 different errors, chance of collision is 0.08%.
But the value of the error code itself will be meaningless and not stable as if people changes the error string, the hash will change. Whereas the enum approach can provide a somewhat readable error values. And it can use compact encoding if people for whatever reasons want large error number.

And this needs to be in 1.0 right? as it is a breaking change.

[bkchr]

I don't think that we will get this into 1.0.

I think that hashing is not a good idea. For the actual implementation, we probably need some more time to think about it. Some simple search & replace of &'static str with u16 will probably not be the implementation we want to have.

[xlc]

I am thinking have decl_module defines a type DispatchError which bounded to Into<u16>

and also macro defines/rewrite Result to be Result<(), DispatchError>.

Not sure if something like Result<(), impl Into<DispatchError>> will work.
Then make DispatchError have a variant Unknown(&'static str) which encode to 0, that basically means unknown error.

So existing code will have error code of 0 without rewrite, not good enough but not a breaking change. New code can define new errors and return a proper error code.

Let me know if this sounds feasible.

[bkchr]

Sounds like a plan, but how do we make error codes unique across modules?

Okay I think got some sort of idea:

enum ModuleError {
     Unknow(&'static str),
     MyError0
}

First we need to expose the error in the metadata as well. Then, when we encounter some error, we calculate the error code in the following way:

1. Module index * 100 (I think 100 errors per module are more than enough).
2. Error as u16 + index from 1 = error code

Thus we have unique indices + a way to get an error description (via metadata).

[bkchr]

If the user does not specifies any error, we would just create the following error:

enum ModuleError {
     Unknow(&'static str),
}

[xlc]

I don't think we have to make error code unique because we know which tx cause the error hence we know which module the error belongs to. But it does make thing bit simpler.

Also this approach means a module cannot return the error code from another module directly, which is not a big deal I think.

Shared error type may need some thoughts like Currency::transfer. We can have a CurrencyError enum contains bunch predefined errors but the Currency implementing module won't be able to return custom error code to indicate some custom error. I guess we can make a ErrorType generic in Currency, not sure what is the best practice of Rust error propagation.