Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add an optional way to handle contract errors with Result #745

Merged
merged 6 commits into from
Mar 10, 2022
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 2 additions & 34 deletions near-sdk-macros/src/core_impl/code_generator/attr_sig_info.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@ use proc_macro2::TokenStream as TokenStream2;
use crate::core_impl::info_extractor::{
ArgInfo, AttrSigInfo, BindgenArgType, InputStructType, SerializerType,
};
use crate::core_impl::utils;
use quote::quote;
use syn::{GenericArgument, Path, PathArguments, Type};

impl AttrSigInfo {
/// Create struct representing input arguments.
Expand Down Expand Up @@ -192,7 +192,7 @@ impl AttrSigInfo {
}
}
BindgenArgType::CallbackResultArg => {
let ok_type = if let Some(ok_type) = extract_ok_type(ty) {
let ok_type = if let Some(ok_type) = utils::extract_ok_type(ty) {
ok_type
} else {
return syn::Error::new_spanned(ty, "Function parameters marked with \
Expand Down Expand Up @@ -262,38 +262,6 @@ impl AttrSigInfo {
}
}

/// Checks whether the given path is literally "Result".
/// Note that it won't match a fully qualified name `core::result::Result` or a type alias like
/// `type StringResult = Result<String, String>`.
fn path_is_result(path: &Path) -> bool {
path.leading_colon.is_none()
&& path.segments.len() == 1
&& path.segments.iter().next().unwrap().ident == "Result"
}

/// Extracts the Ok type from a `Result` type.
///
/// For example, given `Result<String, u8>` type it will return `String` type.
fn extract_ok_type(ty: &Type) -> Option<&Type> {
match ty {
Type::Path(type_path) if type_path.qself.is_none() && path_is_result(&type_path.path) => {
// Get the first segment of the path (there should be only one, in fact: "Result"):
let type_params = &type_path.path.segments.first()?.arguments;
// We are interested in the first angle-bracketed param responsible for Ok type ("<String, _>"):
let generic_arg = match type_params {
PathArguments::AngleBracketed(params) => Some(params.args.first()?),
_ => None,
}?;
// This argument must be a type:
match generic_arg {
GenericArgument::Type(ty) => Some(ty),
_ => None,
}
}
_ => None,
}
}

pub fn deserialize_data(ty: &SerializerType) -> TokenStream2 {
match ty {
SerializerType::JSON => quote! {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use crate::core_impl::info_extractor::{
AttrSigInfo, ImplItemMethodInfo, InputStructType, MethodType, SerializerType,
};
use crate::core_impl::utils;
use proc_macro2::TokenStream as TokenStream2;
use quote::quote;
use syn::{ReturnType, Signature};
Expand Down Expand Up @@ -53,6 +54,7 @@ impl ImplItemMethodInfo {
method_type,
is_payable,
is_private,
is_returns_result,
..
} = attr_signature_info;
let deposit_check = if *is_payable || matches!(method_type, &MethodType::View) {
Expand Down Expand Up @@ -136,6 +138,30 @@ impl ImplItemMethodInfo {
#method_invocation;
#contract_ser
},
ReturnType::Type(_, return_type)
if utils::type_is_result(return_type) && *is_returns_result =>
{
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.");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ticket: #746

},
SerializerType::Borsh => quote! {
let result = near_sdk::borsh::BorshSerialize::try_to_vec(&result).expect("Failed to serialize the return value using Borsh.");
},
};
quote! {
#contract_deser
let result = #method_invocation;
match result {
Ok(result) => {
#value_ser
near_sdk::env::value_return(&result);
#contract_ser
}
Err(err) => near_sdk::FunctionError::panic(&err)
}
}
itegulov marked this conversation as resolved.
Show resolved Hide resolved
}
ReturnType::Type(_, _) => {
let value_ser = match result_serializer {
SerializerType::JSON => quote! {
Expand All @@ -146,11 +172,11 @@ impl ImplItemMethodInfo {
},
};
quote! {
#contract_deser
let result = #method_invocation;
#value_ser
near_sdk::env::value_return(&result);
#contract_ser
#contract_deser
let result = #method_invocation;
#value_ser
near_sdk::env::value_return(&result);
#contract_ser
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ pub struct AttrSigInfo {
pub is_payable: bool,
/// Whether method can accept calls from self (current account)
pub is_private: bool,
/// Whether method returns Result type where only Ok type is serialized
pub is_returns_result: bool,
/// The serializer that we use for `env::input()`.
pub input_serializer: SerializerType,
/// The serializer that we use for the return type.
Expand Down Expand Up @@ -61,6 +63,7 @@ impl AttrSigInfo {
let mut method_type = MethodType::Regular;
let mut is_payable = false;
let mut is_private = false;
let mut is_returns_result = false;
// By the default we serialize the result with JSON.
let mut result_serializer = SerializerType::JSON;

Expand All @@ -87,6 +90,9 @@ impl AttrSigInfo {
let serializer: SerializerAttr = syn::parse2(attr.tokens.clone())?;
result_serializer = serializer.serializer_type;
}
"return_result" => {
is_returns_result = true;
}
_ => {
non_bindgen_attrs.push((*attr).clone());
}
Expand Down Expand Up @@ -136,6 +142,7 @@ impl AttrSigInfo {
method_type,
is_payable,
is_private,
is_returns_result,
result_serializer,
receiver,
returns,
Expand Down
1 change: 1 addition & 0 deletions near-sdk-macros/src/core_impl/mod.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
mod code_generator;
mod info_extractor;
mod metadata;
mod utils;
pub use code_generator::*;
pub use info_extractor::*;
pub use metadata::metadata_visitor::MetadataVisitor;
41 changes: 41 additions & 0 deletions near-sdk-macros/src/core_impl/utils/mod.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
use syn::{GenericArgument, Path, PathArguments, Type};

/// Checks whether the given path is literally "Result".
/// Note that it won't match a fully qualified name `core::result::Result` or a type alias like
/// `type StringResult = Result<String, String>`.
pub(crate) fn path_is_result(path: &Path) -> bool {
path.leading_colon.is_none()
&& path.segments.len() == 1
&& path.segments.iter().next().unwrap().ident == "Result"
}

/// Equivalent to `path_is_result` except that it works on `Type` values.
pub(crate) fn type_is_result(ty: &Type) -> bool {
match ty {
Type::Path(type_path) if type_path.qself.is_none() => path_is_result(&type_path.path),
_ => false,
}
}

/// Extracts the Ok type from a `Result` type.
///
/// For example, given `Result<String, u8>` type it will return `String` type.
pub(crate) fn extract_ok_type(ty: &Type) -> Option<&Type> {
match ty {
Type::Path(type_path) if type_path.qself.is_none() && path_is_result(&type_path.path) => {
// Get the first segment of the path (there should be only one, in fact: "Result"):
let type_params = &type_path.path.segments.first()?.arguments;
// We are interested in the first angle-bracketed param responsible for Ok type ("<String, _>"):
let generic_arg = match type_params {
PathArguments::AngleBracketed(params) => Some(params.args.first()?),
_ => None,
}?;
// This argument must be a type:
match generic_arg {
GenericArgument::Type(ty) => Some(ty),
_ => None,
}
}
_ => None,
}
}
27 changes: 27 additions & 0 deletions near-sdk-macros/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -184,3 +184,30 @@ pub fn borsh_storage_key(item: TokenStream) -> TokenStream {
impl near_sdk::BorshIntoStorageKey for #name {}
})
}

/// `FunctionError` generates implementation for `near_sdk::FunctionError` trait.
/// It allows contract runtime to panic with the type using its `ToString` implementation
/// as the message.
#[proc_macro_derive(FunctionError)]
pub fn function_error(item: TokenStream) -> TokenStream {
let name = if let Ok(input) = syn::parse::<ItemEnum>(item.clone()) {
input.ident
} else if let Ok(input) = syn::parse::<ItemStruct>(item) {
input.ident
} else {
return TokenStream::from(
syn::Error::new(
Span::call_site(),
"FunctionError can only be used as a derive on enums or structs.",
)
.to_compile_error(),
);
};
TokenStream::from(quote! {
impl near_sdk::FunctionError for #name {
fn panic(&self) {
near_sdk::env::panic_str(&std::string::ToString::to_string(&self))
itegulov marked this conversation as resolved.
Show resolved Hide resolved
}
}
})
}
1 change: 1 addition & 0 deletions near-sdk/compilation_tests/all.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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.

}
50 changes: 50 additions & 0 deletions near-sdk/compilation_tests/function_error.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
//! Testing FunctionError macro.

use borsh::{BorshDeserialize, BorshSerialize};
use near_sdk::{near_bindgen, FunctionError};
use std::fmt;

#[derive(FunctionError, BorshSerialize)]
struct ErrorStruct {
message: String,
}

impl fmt::Display for ErrorStruct {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
write!(f, "error ocurred: {}", self.message)
}
}

#[derive(FunctionError, BorshSerialize)]
enum ErrorEnum {
NotFound,
Banned { account_id: String },
}

impl fmt::Display for ErrorEnum {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
match self {
ErrorEnum::NotFound => write!(f, "not found"),
ErrorEnum::Banned { account_id } => write!(f, "account {} is banned", account_id)
}
}
}

#[near_bindgen]
#[derive(BorshDeserialize, BorshSerialize, Default)]
struct Contract {}

#[near_bindgen]
impl Contract {
#[return_result]
pub fn set(&self, value: String) -> Result<String, ErrorStruct> {
Err(ErrorStruct { message: format!("Could not set to {}", value) })
}

#[return_result]
pub fn get(&self) -> Result<String, ErrorEnum> {
Err(ErrorEnum::NotFound)
}
}

fn main() {}
2 changes: 1 addition & 1 deletion near-sdk/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ extern crate quickcheck;

pub use near_sdk_macros::{
callback, callback_vec, ext_contract, init, metadata, near_bindgen, result_serializer,
serializer, BorshStorageKey, PanicOnDefault,
serializer, BorshStorageKey, FunctionError, PanicOnDefault,
};

#[cfg(feature = "unstable")]
Expand Down
36 changes: 36 additions & 0 deletions near-sdk/src/types/error.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
use std::borrow::Borrow;

/// Enables contract runtime to panic with the given type. Any error type used in conjunction
/// with `#[return_result]` has to implement this trait.
///
/// ```
/// use near_sdk::FunctionError;
///
/// enum Error {
/// NotFound,
/// Unexpected { message: String },
/// }
///
/// impl FunctionError for Error {
/// fn panic(&self) {
/// match self {
/// Error::NotFound =>
/// near_sdk::env::panic_str("not found"),
/// Error::Unexpected { message } =>
/// near_sdk::env::panic_str(&format!("unexpected error: {}", message))
/// }
/// }
/// }
/// ```
pub trait FunctionError {
fn panic(&self);
itegulov marked this conversation as resolved.
Show resolved Hide resolved
}

impl<T> FunctionError for T
where
T: Borrow<str>,
itegulov marked this conversation as resolved.
Show resolved Hide resolved
{
fn panic(&self) {
crate::env::panic_str(self.borrow())
}
}
3 changes: 3 additions & 0 deletions near-sdk/src/types/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@ pub use self::account_id::{AccountId, ParseAccountIdError};
mod gas;
pub use self::gas::Gas;

mod error;
pub use self::error::FunctionError;

/// Raw type for duration in nanoseconds
pub type Duration = u64;

Expand Down