Proposal: return Result from contract rather than panicking

[Tarnadas]

Hey!

I am still fairly new to writing Smart Contracts in Rust, but one thing that makes me feel really uncomfortable is the panicking.
I don’t feel like this is a very “rusty” way. In idiomatic Rust, I would rather expect to return a Result<T, std::error::Error + Display> and I don’t see a downside here, except that it needs to be supported by the SDK.

Returning a Result:

• would allow using ? operator
• has same fail early mechanisms
• would let me use thiserror, which I absolutely love
• could use the Display trait for returning the error message

[austinabell]

Thanks for opening this! I've been thinking a lot about this and would like to add support for it, but it's hard to make a change that works well in a backwards-compatible way.

The main issue is just that Result needs to be explicitly handled, which can be a hidden breaking change for those who use a serialized result (I know this may seem like a strange pattern, but I've seen at least a few times).

The other consideration is that for those who want to minimize contract sizes, Display ties pretty close with core formatting machinery, which generally bloats compiled code size. Yes, the SDK is already tied close to this, but we are trying to move away from it and would want to be careful introducing something functionality so core.

My intuition is that the bounds for the error type might be constrained by a new trait that has auto impl from std::error::Error or maybe more generally Display (but want to be careful to not tie impls that don't require the overhead). For example, &str would require no overhead because it can just panic with that string, but with the auto-impl for Display, it would tie it with formatting machinery and require allocating a string.

[Tarnadas]

Hm I see. Code size matters a lot, so maybe try replacing Display with ToString or does it have the same code size problems?
I suppose a modified version of thiserror would be possible or maybe something similar already exists.

Regarding the serialized result, yeah that’s definitely a strange pattern, but it would introduce a breaking change. There would probably need to be some sort of backward compatibility for the old behavior, which would cause returning a Result not to panic implicitly. I could imagine adding an attribute to the function like #[result_non_panic].

[austinabell]

Hm I see. Code size matters a lot, so maybe try replacing Display with ToString or does it have the same code size problems?

No this would be fine, but it may create some overhead still for types that don't need to be heap-allocated to a String like &str, and just noting my train of thought for when this is progressed. Also, might be a hidden connection to fmt if developers don't know it's going through formatting machinery. I'm actually not certain what would be the best path for this error trait bounds long term, and input from others would be welcomed.

Regarding the serialized result, yeah that’s definitely a strange pattern, but it would introduce a breaking change. There would probably need to be some sort of backward compatibility for the old behavior, which would cause returning a Result not to panic implicitly. I could imagine adding an attribute to the function like #[result_non_panic].

Yeah, this or just recommend just using a nested Result if they really want to use a serialized result (Result<Result<_, _>, _>) to avoid having to maintain the annotation. Either way, they would need to change, and it wouldn't be obvious unless they noticed this in changelog/docs.

If you or anyone reading would like to take this on, more than welcome to PR and I'm happy to help however I can. Other than this, not sure how soon we will have the bandwidth to support this, but it's high on my wants personally :)

[itegulov]

Regarding the serialized result, yeah that’s definitely a strange pattern, but it would introduce a breaking change. There would probably need to be some sort of backward compatibility for the old behavior, which would cause returning a Result not to panic implicitly. I could imagine adding an attribute to the function like #[result_non_panic].

Yeah, this or just recommend just using a nested Result if they really want to use a serialized result (Result<Result<_, _>, _>) to avoid having to maintain the annotation. Either way, they would need to change, and it wouldn't be obvious unless they noticed this in changelog/docs.

My proposal for 4.x:

1. The new Result-return style works only when the function is marked with an annotation #[return_result] (suggestions for a better name are welcome).
2. The legacy style works by default but generates a warning saying that the user should switch over to #[return_result] with a nested Result type if they really need a serialized result value.

In 5.x we get rid of the marker annotation and make the new behaviour the default one. I think this would provide the smoothest migration experience for everyone.

My intuition is that the bounds for the error type might be constrained by a new trait that has auto impl from std::error::Error or maybe more generally Display (but want to be careful to not tie impls that don't require the overhead). For example, &str would require no overhead because it can just panic with that string, but with the auto-impl for Display, it would tie it with formatting machinery and require allocating a string.

By "auto impl" are you referring to auto traits or something different?

[austinabell]

My proposal for 4.x:

1. The new Result-return style works only when the function is marked with an annotation #[return_result] (suggestions for a better name are welcome).
2. The legacy style works by default but generates a warning saying that the user should switch over to #[return_result] with a nested Result type if they really need a serialized result value.
   In 5.x we get rid of the marker annotation and make the new behaviour the default one. I think this would provide the smoothest migration experience for everyone.

Keep in mind that this requires us to maintain the #[return_result] annotation in our macros without being able to deprecate and phase out. Afaik there isn't a way to generate deprecations for this manually.

Returning a result and using that serialized data is an extremely uncommon pattern (I've only seen it once) so I don't think there is a large cost to switching this behaviour given it's coming with a major version bump as long as it's documented well.

By "auto impl" are you referring to auto traits or something different?

Just referring to something like:

impl SdkFunctionError for T where T: std::error::Error { ... }

I guess a more accurate term would be generic implementation

[itegulov]

Keep in mind that this requires us to maintain the #[return_result] annotation in our macros without being able to deprecate and phase out. Afaik there isn't a way to generate deprecations for this manually.

Why wouldn't we be able to deprecate/remove it in 5.x? Is there some technical problem with this as I think we follow semver and make breaking changes in major versions?

Returning a result and using that serialized data is an extremely uncommon pattern (I've only seen it once) so I don't think there is a large cost to switching this behaviour given it's coming with a major version bump as long as it's documented well.

Right, I was just trying to put myself in the place of a user and I must admit that I rarely fully read changelogs when I upgrade a major version of a library I am using unless I see some kind of a compilation error/warning :)

But if you think it is okay to do it this way I am not against it either.

Just referring to something like:

impl SdkFunctionError for T where T: std::error::Error { ... }

I guess a more accurate term would be generic implementation

Ah, I see. I have a naive implementation ready, just playing with different error contraints right now to see what makes the most amount of sense.

[austinabell]

Keep in mind that this requires us to maintain the #[return_result] annotation in our macros without being able to deprecate and phase out. Afaik there isn't a way to generate deprecations for this manually.

Why wouldn't we be able to deprecate/remove it in 5.x? Is there some technical problem with this as I think we follow semver and make breaking changes in major versions?

We can only remove it, which is fine, I'm just indicating that this will be an abrupt change when it comes and people have to think about yet another custom annotation during 4.0. To be clear I'm just bringing this up to vocalize the cons of the change, not to share a preference.

Right, I was just trying to put myself in the place of a user and I must admit that I rarely fully read changelogs when I upgrade a major version of a library I am using unless I see some kind of a compilation error/warning :)

But if you think it is okay to do it this way I am not against it either.

This is a very good point, and I think your plan of adding the annotation for 4.0 and changing default behaviour/removing the annotation in 5.0 makes sense. Just in the same pattern as above, just trying to make sure we consider all drawbacks :)

[itegulov]

I have looked into the ways to invoke panic_str with arbitrary error types and here are my findings:

It does not seem like it is possible to come up with a trait that has a generic implementation from Display/std::error::Error/ToString/anything-else-tied-to-formatting while also having a custom implementation that just panics for &str. This leads to conflicting trait implementations and to avoid this we would have to use unstable features, either auto_traits with negative_impls or specialization/min_min_specialization.

Some alternatives I have considered up to now:

1. Creating a special case for &str in macro implementation and using ToString for all other types. Would not be difficult, but arguably hacky and might cause confusion for end users.
2. Using an alternative minimalistic formatting machinery such as https://github.com/japaric/ufmt. I have not explored this option too much because μfmt has not been maintained for the last couple of years, but it is there.
3. Creating a trait SdkFunctionError with fn panic(&self);. The trait will be implemented for String and &str by default, but will also come with a procedural macro impl_sdk_function_error_via_display!($ty) that could be used to derive Display-powered implementation for $ty. This way the user will be explicitly aware if they are using formatting machinery.
4. As @Tarnadas suggests, we could just use ToString for everything and lose one extra String allocation for &str. The users might unknowingly use formatting machinery this way though.

To me it seems like 3 is the most robust choice, but in the end it is also going to be the most awkward to use. Would love to hear your guys' thoughts on this before I proceed with the final implementation.

[austinabell]

3. Creating a trait SdkFunctionError with fn panic(&self);. The trait will be implemented for String and &str by default, but will also come with a procedural macro impl_sdk_function_error_via_display!($ty) that could be used to derive Display-powered implementation for $ty. This way the user will be explicitly aware if they are using formatting machinery.

To me it seems like 3 is the most robust choice, but in the end it is also going to be the most awkward to use. Would love to hear your guys' thoughts on this before I proceed with the final implementation.

One other benefit about 3 is that it allows specific customization of how an error would be printed out, instead of having to do things like overriding the ToString implementation of any error, which might be auto-implemented for certain error types.

Also, instead of using a function-like macro to implement this, can just do a derive macro to make it cleaner to implement.

My intuition is similar to yours where 3 seems like the best path given our constraints/goals