Make `decl_error!` slightly more friendly for `frame_system`

[shawntabrizi]

Error messages propagating from one pallet to another is not handled that well right now.

If you use ensure_signed for example, the best you can do is:

let sender = ensure_signed(origin).map_err(|e| e.as_str())?;

This will convert the frame_system::Error object into a string, and as a result you will lose all of the great UI experiences that these errors are meant to provide.

One solution is to re-implement and map to a custom error message.

For example:

let sender = ensure_signed(origin).map_err(|_| Error::RequireSignedOrigin)?;

But then the user would need to define extra error types for every single module, since they all rely on some combination of ensure_signed or ensure_root or ensure_none.

decl_error! {
    enum Error {
        MyCustomError,
        RequireSignedOrigin,
        RequireRootOrigin,
        RequireNoOrigin,
    }
}

A proposed short term fix is to have decl_error! automatically add the 3 error types from frame_system for every new pallet:

• RequireSignedOrigin
• RequireRootOrigin
• RequireNoOrigin

Then to simply implement From between frame_system and the custom pallet being created.

At that point, the errors and UI should work as expected.

Long term, the solution here may need to be more robust and allow for verbose error tracing across multiple modules.

@thiolliere @bkchr

[gui1117]

yes we can do that behind an attribute for frame_system::Error as they are very common. Though I agree we probably need an error mecanism which is more scalable, and can wrap around other modules error.

EDIT: we can make mandatory or not, I'm more in favor of optional after an attribute but I'm ok either way.

[shawntabrizi]

Optional sounds good :)

[xlc]

My original idea for decl_error was make it similar to Event, each module have its own Error type and there is a Runtime Error type created from construct_runtime (#2246 (comment)). Currently only the first part is in Substrate and the (outdated) second part is in #2880. I created #4040 for the runtime error type.

With this, runtime module can define an Error type in Trait like this

https://github.com/paritytech/substrate/pull/2880/files#diff-c41ce90d658462c61a0814b7279026a6R239

type Error: From<Error> + From<system::Error> + From<&'static str>;

And then the error type can be just converted from system::Error into T::Error.

If this runtime module uses other runtime module, the definition can be changed to

type Error: From<Error> + From<system::Error> + From<balances::Error> + From<&'static str>;

This will allow the SDK to know the origin module of the error (without intermediate parts).

[xlc]

FYI decl_error already add CannotLookup

substrate/frame/support/src/error.rs

Line 79 in c6fc2e6

CannotLookup,

But I don't like the idea of have few error variants hardcoded for all module errors. They should just be system::Error instead or somewhere more generalized crate.

[shawntabrizi]

This will allow the SDK to know the origin module of the error (without intermediate parts).

This is def better than what exists today, but does not seem nearly as complete as the proposal I have made in the other thread. I think due to the nature of "errors" having some trace stack is pretty important, whereas events can be emitted on their own time, and pretty much whenever.

This issue I hope is more of a band-aid. I think it makes sense to update all the modules to use decl_event before 2.0 if we can do it real quick.

This was something that jumped out to me as an obvious improvement without doing a lot of work.

If we are going to overhaul the direction of decl_error, I think it would be outside of the scope of what we can do at this time.

@xlc in that case, do you think that doing this patch is worse than doing nothing at all?

[gui1117]

maybe we could do something in between this little fix and #4385 by having:

decl_error!{
  pub enum MyError {
    AuthentificationFail(frame_system::Error),
    SomeOtherLogic(frame_system::Error),
  }
}

And this will generate one enum with only final variant such as:

pub enum MyError {
    AuthentificationFailRequireRootOrigin,
    AuthentificationFailRequireNoOrigin,
    AuthentificationFailRequireSignedOrigin,
    SomeOtherLogicRequireNoOrigin
    SomeOtherLogicRequire..Origin
    SomeOtherLogicRequire..Origin
}

In the metadata there will be some additional information so UI can know that AuthentificationFailRequireNoOrigin is also the second error of a module named "System" and that's all.

I'm not sure how much does this fits into one u8 thought, if one module has 3 variants another module has 3 variants wrapping this one and a latter module has 3 variants wrapping the second one the last module has then 3*3*3 variants.

[xlc]

@thiolliere Yeah I don't think this approach scales. The whole reason I proposed the initial decl_error design is with the constraints that we want to limit the amount of bytes used for error code. The decision was use two bytes, one for module identifier, and one for module specific error code.

It doesn't solve all the issues, but an improvement of what we have, without too much overhead, and reasonably easy to implement.

BTW we seems discuss same thing across three places... should we pick one and continue all the discussion there?

[gui1117]

The whole reason I proposed the initial decl_error design is with the constraints that we want to limit the amount of bytes used for error code

I agree but if we leverage this constrain to u16 or compact encoding then we could scale a bit, although the metadata can grow quite big :-/

[shawntabrizi]

@xlc let's continue the discussion here to make two decisions:

1. What will we do with the eminent release of 2.0
2. What will we do in the long term knowing we can support another breaking change

My feeling is that by 2.0, all modules should use decl_error. That is the goal of #4365

The only thing we know for certain is that basically every module will use some combination of ensure_signed/none/root. This means, without any change to our current system, we will have a lot of code like this:

let sender = ensure_signed(origin).map_err(|e| e.as_str())?;

Which will provide no error user experience in our user interfaces. I know that this can be fixed with the PR that @thiolliere opened, and allow us to do a mostly better job for this 2.0 push.

What kind of work would be required to support Errors like Events? Would we be able to get it in fast?

[xlc]

Implement #4040 is not hard as I did it once already and it is similar to decl_event so we just mostly follow the code of decl_event.

Another possible approach which I did in my modules is use decl_error but omit type Error = Error in decl_module.

So that the module error type is still &'static str but at least when reading the code, you don't see it.

Example: https://github.com/laminar-protocol/open-runtime-module-library/blob/master/auction/src/lib.rs

And then later we can easily switch to the new solutions with minimal code change.

[shawntabrizi]

Fixed with #4449