feat: add an optional way to handle contract errors with Result
#745
Conversation
| @@ -19,4 +19,5 @@ fn compilation_tests() { | |||
| t.pass("compilation_tests/cond_compilation.rs"); | |||
| t.compile_fail("compilation_tests/payable_view.rs"); | |||
| t.pass("compilation_tests/borsh_storage_key.rs"); | |||
| t.pass("compilation_tests/function_error.rs"); | |||
There was a problem hiding this comment.
this test is awesome, perhaps it's worth adding a test with invalid format(s) to assert it fails consistently and with a valid error?
There was a problem hiding this comment.
Ok so, I have spent half a day trying to figure out why a negative test I wrote was not failing at compile-time and finally realized that we are compiling all these files with the host target rather than wasm32-unknown-unknown, so none of the wasm-specific code we generate is tested here. I have tried making trybuild use a target different to the one we compile near-sdk crate with, but it does not seem like it is possible? I am a bit stumped here as to how to proceed. Maybe moving compilation tests into a different crate would help, but then we would still have to compile trybuild with wasm target and I do not know if it will play out well.
There was a problem hiding this comment.
Ok so, I have spent half a day trying to figure out why a negative test I wrote was not failing at compile-time and finally realized that we are compiling all these files with the host target rather than wasm32-unknown-unknown, so none of the wasm-specific code we generate is tested here. I have tried making
trybuilduse a target different to the one we compilenear-sdkcrate with, but it does not seem like it is possible? I am a bit stumped here as to how to proceed. Maybe moving compilation tests into a different crate would help, but then we would still have to compiletrybuildwith wasm target and I do not know if it will play out well.
Oh, interesting. Yeah I wasn't aware of this. Yeah I’d say just leave it and we can create an issue and tackle it separately not to block this one.
Yeah, Also, not related to this PR in particular, but I don't think it makes sense that our only integration tests are essentially examples. I have had a few occasions where I wanted to add an e2e test but didn't want anyone to treat it as an example to follow. |
Absolutely, this is a good idea to build out this scaffolding. I just suggested in examples since we do want to indicate to users that this pattern exists |
|
Oh also a changelog entry would be awesome, but nbd it can come after |
| { | ||
| let value_ser = match result_serializer { | ||
| SerializerType::JSON => quote! { | ||
| let result = near_sdk::serde_json::to_vec(&result).expect("Failed to serialize the return value using JSON."); |
There was a problem hiding this comment.
I see that we're making similar calls to expect in a couple places, but @austinabell do we want to eventually get rid of these expect calls since they should be bringing the formatting mechanism. Or is there some reason we're keeping them?
There was a problem hiding this comment.
iirc this expect doesn't embed file-specific data in the error message when used in a proc macro, but I could be remembering wrong why I didn't switch these.
Might be worth opening an issue and double-checking before next release, but wouldn't be a breaking change to come in after anyway
Fixes #574
This PR provides an opt-in way to treat
Resultfunction return types as an error-handling mechanism. Meaning that only the underlyingOkvalues are serialized while theErrvalues result in panic. To do this,FunctionErrormust be implemented by theErrtype (there are default implementations for types that haveBorrow<str>and a derivation macro that generates aToString-powered implementation).The legacy behavior still works by default (
Resultis treated as a normal value and goes through full serialization), but it is supposed to generate compilation warnings (I am still trying to figure out a proper way to do this ascompile_warning!seems to be missing rn, any suggestions are welcome). The user will have to mark the function with#[return_result]to opt-in for the new behavior.