Skip to content

Commit

Permalink
Make try_ fns return smaller SDK error type (#1175)
Browse files Browse the repository at this point in the history
### What
Make try_ fns return smaller SDK error type instead of the large host
error type.

### Why
The host error type contains a massive number of possible errors.
However, the host obfuscates the error when it is passed to the guest,
and so the guest and sdk only ever see the Context/InvalidAction error.

It's confusing for developers if we expose the host error type, then
don't actually surface most of the errors on it. A developer might think
they can distingish between different error states, when in reality they
cannot.


Close #1136
  • Loading branch information
leighmcculloch authored Nov 23, 2023
1 parent 8c5eba9 commit 464fb58
Show file tree
Hide file tree
Showing 7 changed files with 444 additions and 23 deletions.
48 changes: 44 additions & 4 deletions soroban-sdk-macros/src/derive_error_enum_int.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ pub fn derive_type_error_enum_int(
let mut errors = Vec::<Error>::new();

let variants = &data.variants;
let (spec_cases, try_froms, intos): (Vec<_>, Vec<_>, Vec<_>) = variants
let (spec_cases, try_froms, into_errors, into_invoke_errors): (Vec<_>, Vec<_>, Vec<_>, Vec<_>) = variants
.iter()
.map(|v| {
let ident = &v.ident;
Expand Down Expand Up @@ -49,9 +49,11 @@ pub fn derive_type_error_enum_int(
value: discriminant,
};
let try_from = quote! { #discriminant => Self::#ident };
let into =
let into_error =
quote! { #enum_ident::#ident => #path::Error::from_contract_error(#discriminant) };
(spec_case, try_from, into)
let into_invoke_error =
quote! { #enum_ident::#ident => #path::InvokeError::Contract(#discriminant) };
(spec_case, try_from, into_error, into_invoke_error)
})
.multiunzip();

Expand Down Expand Up @@ -119,7 +121,7 @@ pub fn derive_type_error_enum_int(
#[inline(always)]
fn from(val: #enum_ident) -> #path::Error {
match val {
#(#intos,)*
#(#into_errors,)*
}
}
}
Expand All @@ -131,6 +133,44 @@ pub fn derive_type_error_enum_int(
}
}

impl TryFrom<#path::InvokeError> for #enum_ident {
type Error = #path::InvokeError;
#[inline(always)]
fn try_from(error: #path::InvokeError) -> Result<Self, #path::InvokeError> {
match error {
#path::InvokeError::Abort => Err(error),
#path::InvokeError::Contract(code) => Ok(match code {
#(#try_froms,)*
_ => return Err(error),
}),
}
}
}

impl TryFrom<&#path::InvokeError> for #enum_ident {
type Error = #path::InvokeError;
#[inline(always)]
fn try_from(error: &#path::InvokeError) -> Result<Self, #path::InvokeError> {
<_ as TryFrom<#path::InvokeError>>::try_from(*error)
}
}

impl From<#enum_ident> for #path::InvokeError {
#[inline(always)]
fn from(val: #enum_ident) -> #path::InvokeError {
match val {
#(#into_invoke_errors,)*
}
}
}

impl From<&#enum_ident> for #path::InvokeError {
#[inline(always)]
fn from(val: &#enum_ident) -> #path::InvokeError {
<_ as From<#enum_ident>>::from(*val)
}
}

impl #path::TryFromVal<#path::Env, #path::Val> for #enum_ident {
type Error = #path::ConversionError;
#[inline(always)]
Expand Down
2 changes: 1 addition & 1 deletion soroban-sdk-macros/src/syn_ext.rs
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ impl<'a> Fn<'a> {
Type::Verbatim(quote! {
Result<
Result<#t, <#t as #crate_path::TryFromVal<#crate_path::Env, #crate_path::Val>>::Error>,
Result<#e, <#e as TryFrom<#crate_path::Error>>::Error>
Result<#e, #crate_path::InvokeError>
>
})
}
Expand Down
20 changes: 13 additions & 7 deletions soroban-sdk/src/env.rs
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@ where
use crate::auth::InvokerContractAuthEntry;
use crate::unwrap::UnwrapInfallible;
use crate::unwrap::UnwrapOptimized;
use crate::InvokeError;
use crate::{
crypto::Crypto, deploy::Deployer, events::Events, ledger::Ledger, logs::Logs, prng::Prng,
storage::Storage, Address, Vec,
Expand Down Expand Up @@ -386,10 +387,11 @@ impl Env {
contract_address: &Address,
func: &crate::Symbol,
args: Vec<Val>,
) -> Result<Result<T, T::Error>, Result<E, E::Error>>
) -> Result<Result<T, T::Error>, Result<E, InvokeError>>
where
T: TryFromVal<Env, Val>,
E: TryFrom<Error>,
E::Error: Into<InvokeError>,
{
let rv = internal::Env::try_call(
self,
Expand All @@ -399,7 +401,7 @@ impl Env {
)
.unwrap_infallible();
match internal::Error::try_from_val(self, &rv) {
Ok(err) => Err(E::try_from(err)),
Ok(err) => Err(E::try_from(err).map_err(Into::into)),
Err(ConversionError) => Ok(T::try_from_val(self, &rv)),
}
}
Expand Down Expand Up @@ -1139,10 +1141,10 @@ impl Env {
/// // as long as a valid error type used.
/// Err(Ok(NoopAccountError::SomeError))
/// );
/// // Successful call of `__check_auth` with a `soroban_sdk::Error`
/// // Successful call of `__check_auth` with a `soroban_sdk::InvokeError`
/// // error - this should be compatible with any error type.
/// assert_eq!(
/// e.try_invoke_contract_check_auth::<soroban_sdk::Error>(
/// e.try_invoke_contract_check_auth::<soroban_sdk::InvokeError>(
/// &account_contract.address,
/// &BytesN::from_array(&e, &[0; 32]),
/// 0_i32.into(),
Expand All @@ -1152,13 +1154,17 @@ impl Env {
/// );
/// }
/// ```
pub fn try_invoke_contract_check_auth<E: TryFrom<Error>>(
pub fn try_invoke_contract_check_auth<E>(
&self,
contract: &Address,
signature_payload: &BytesN<32>,
signature: Val,
auth_context: &Vec<auth::Context>,
) -> Result<(), Result<E, E::Error>> {
) -> Result<(), Result<E, InvokeError>>
where
E: TryFrom<Error>,
E::Error: Into<InvokeError>,
{
let args = Vec::from_array(
self,
[signature_payload.to_val(), signature, auth_context.to_val()],
Expand All @@ -1168,7 +1174,7 @@ impl Env {
.call_account_contract_check_auth(contract.to_object(), args.to_object());
match res {
Ok(rv) => Ok(rv.into_val(self)),
Err(e) => Err(e.error.try_into()),
Err(e) => Err(e.error.try_into().map_err(Into::into)),
}
}

Expand Down
37 changes: 37 additions & 0 deletions soroban-sdk/src/error.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
use core::convert::Infallible;

use crate::xdr;

/// InvokeError captures errors returned from the invocation of another
/// contract.
#[derive(Debug, Copy, Clone, Eq, PartialEq)]
pub enum InvokeError {
/// Abort occurs if the invoke contract panicks with a [`panic!`], or a host
/// function of the environment has a failure, or a runtime error occurs.
Abort,
/// Contract error occurs if the invoked contract function exited returning
/// an error or called [`panic_with_error!`][crate::panic_with_error!] with
/// a [`contracterror`][crate::contracterror].
///
/// If the contract defines a [`contracterror`][crate::contracterror] type
/// as part of its interface, this variant of the error will be convertible
/// to that type, but if that conversion failed then this variant of the
/// error would be used to represent the error.
Contract(u32),
}

impl From<crate::Error> for InvokeError {
fn from(e: crate::Error) -> Self {
if e.is_type(xdr::ScErrorType::Contract) {
InvokeError::Contract(e.get_code())
} else {
InvokeError::Abort
}
}
}

impl From<Infallible> for InvokeError {
fn from(_: Infallible) -> Self {
unreachable!()
}
}
2 changes: 2 additions & 0 deletions soroban-sdk/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -765,6 +765,8 @@ pub mod auth;
mod bytes;
pub mod crypto;
pub mod deploy;
mod error;
pub use error::InvokeError;
pub mod events;
pub mod iter;
pub mod ledger;
Expand Down
87 changes: 76 additions & 11 deletions tests/errors/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ impl Contract {
panic_with_error!(&env, Error::AnError)
} else if flag == 3 {
panic!("an error")
} else if flag == 4 {
panic_with_error!(&env, soroban_sdk::Error::from_contract_error(9))
} else {
unimplemented!()
}
Expand All @@ -42,11 +44,7 @@ impl Contract {

#[cfg(test)]
mod test {
use soroban_sdk::{
symbol_short,
xdr::{ScErrorCode, ScErrorType},
Env,
};
use soroban_sdk::{symbol_short, xdr, Env, InvokeError};

use crate::{Contract, ContractClient, Error};

Expand Down Expand Up @@ -101,13 +99,80 @@ mod test {
let client = ContractClient::new(&e, &contract_id);

let res = client.try_hello(&3);
assert_eq!(res, Err(Err(InvokeError::Abort)));
assert!(!client.persisted());
}

#[test]
fn try_hello_error_unexpected_contract_error() {
let e = Env::default();
let contract_id = e.register_contract(None, Contract);
let client = ContractClient::new(&e, &contract_id);

let res = client.try_hello(&4);
assert_eq!(res, Err(Err(InvokeError::Contract(9))));
assert!(!client.persisted());
}

#[test]
fn type_conversion() {
// Error can be converted into InvokeError.
assert_eq!(
res,
Err(Err(soroban_sdk::Error::from_type_and_code(
ScErrorType::Context,
ScErrorCode::InvalidAction
)))
<_ as Into<InvokeError>>::into(Error::AnError),
InvokeError::Contract(1),
);

// InvokeError can be converted into Error or itself.
assert_eq!(
<_ as TryInto<Error>>::try_into(InvokeError::Contract(1)),
Ok(Error::AnError),
);
assert_eq!(
<_ as TryInto<Error>>::try_into(InvokeError::Contract(2)),
Err(InvokeError::Contract(2)),
);
assert_eq!(
<_ as TryInto<Error>>::try_into(InvokeError::Abort),
Err(InvokeError::Abort),
);

// Error can be converted into Env Error.
assert_eq!(
<_ as Into<soroban_sdk::Error>>::into(Error::AnError),
soroban_sdk::Error::from_contract_error(1),
);

// Env Error can be converted into Error.
assert_eq!(
<_ as TryInto<Error>>::try_into(soroban_sdk::Error::from_contract_error(1)),
Ok(Error::AnError),
);
assert_eq!(
<_ as TryInto<Error>>::try_into(soroban_sdk::Error::from_contract_error(2)),
Err(soroban_sdk::Error::from_contract_error(2)),
);
assert_eq!(
<_ as TryInto<Error>>::try_into(soroban_sdk::Error::from_type_and_code(
xdr::ScErrorType::Context,
xdr::ScErrorCode::InvalidAction
)),
Err(soroban_sdk::Error::from_type_and_code(
xdr::ScErrorType::Context,
xdr::ScErrorCode::InvalidAction
)),
);

// Env Error can be converted into InvokeError.
assert_eq!(
<_ as Into<InvokeError>>::into(soroban_sdk::Error::from_contract_error(1)),
InvokeError::Contract(1),
);
assert_eq!(
<_ as Into<InvokeError>>::into(soroban_sdk::Error::from_type_and_code(
xdr::ScErrorType::Context,
xdr::ScErrorCode::InvalidAction
)),
InvokeError::Abort,
);
assert!(!client.persisted());
}
}
Loading

0 comments on commit 464fb58

Please sign in to comment.