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

Rewrite all contracts to use contracterrors #199

Open
sisuresh opened this issue Jan 27, 2023 · 4 comments
Open

Rewrite all contracts to use contracterrors #199

sisuresh opened this issue Jan 27, 2023 · 4 comments

Comments

@sisuresh
Copy link
Contributor

Related issue - stellar/rs-soroban-env#634.

During the hackathon, a couple users ran into issues where their contract hit an error in the token example, but the error didn't propagate up, making it difficult to debug simple issues like insufficient allowances. We should update all contracts (but especially the token contract) to call panic_with_error! on errors that we expect users to hit due to incorrect usage of a contract.

@leighmcculloch leighmcculloch changed the title Use contracterror and panic_with_error so error messages propagate up in all examples Use contracterror's in contracts May 16, 2023
@leighmcculloch leighmcculloch changed the title Use contracterror's in contracts Rewrite all contracts to use contracterrors May 16, 2023
@leighmcculloch
Copy link
Member

I think we need to rewrite all our examples to define contracterrors, and then ideally surface them as Result<_, E> return values on contract functions. We can also use panic_with_error! where appropriate, but returning errors creates better contract specifications.

@deep-ink-ventures
Copy link

Did this change in preview 10? Would be great to have a good example on the recommended way. panic! should not be used imho as it leaves clients clueless and leads to bad ui.

@leighmcculloch
Copy link
Member

leighmcculloch commented Aug 23, 2023

This issue is becoming more important. @brson highlighted recently that fuzzing is only meaningful if fuzz tests can distinguish between valid error conditions and unexpected error conditions. In Rust panicks are usually seen as the latter, unexpected error conditions. In our docs we promote panic_with_error! which is fine, because they get translated into returned contract errors in the same way returning an error does normally does. But in our examples we still use panic!("...") with a string a lot, which results in a generic system trap error, which a fuzz test will/should treat as an unexpected error.

Independent of the above, this is a pretty big area that our examples are teaching a practice that is inconsistent with our docs, SDK, and tooling.

I think this needs prioritizing.

cc @anupsdf

@brson
Copy link
Contributor

brson commented Sep 2, 2023

FWIW in the fuzzing I am doing on blend I am currently doing two new things that I think will help a lot:

I am not allowing contracts to implicitly overflow. Instead I am using checked math plus this code to panic with error

trait UnwrapOrOverflow {
    type Output;

    fn unwrap_or_overflow(self, env: &soroban_sdk::Env) -> Self::Output;
}

impl<T> UnwrapOrOverflow for Option<T> {
    type Output = T;

    #[inline(always)]
    fn unwrap_or_overflow(self, env: &soroban_sdk::Env) -> Self::Output {
        #[soroban_sdk::contracterror]
        #[derive(Copy, Clone)]
        #[repr(u32)]
        enum Error {
            Overflow = 0xFE,
        }

        match self {
            Some(v) => v,
            None => {
                soroban_sdk::panic_with_error!(env, Error::Overflow);
            }
        }
    }
}

And every time I call a contract method from a test I check the result using this function

/// Panic if a contract call result might have been the result of an unexpected panic.
///
/// Calls that return an error with type `ScErrorType::WasmVm` and code `ScErrorCode::InvalidAction`
/// are assumed to be unintended errors. These are the codes that result from plain `panic!` invocations,
/// thus contracts should never simply call `panic!`, but instead use `panic_with_error!`.
///
/// Other rare types of internal exception can return `InvalidAction`.
#[track_caller]
fn verify_contract_result<T>(env: &soroban_sdk::Env, r: &ContractResult<T>) {
    use soroban_sdk::{Error, ConversionError};
    use soroban_sdk::xdr::{ScErrorType, ScErrorCode};
    use soroban_sdk::testutils::Events;
    match r {
        Err(Ok(e)) => {
            if e.is_type(ScErrorType::WasmVm) && e.is_code(ScErrorCode::InvalidAction) {
                let msg = "contract failed with InvalidAction - unexpected panic?";
                eprintln!("{msg}");
                eprintln!("recent events (10):");
                for (i, event) in env.events().all().iter().rev().take(10).enumerate() {
                    eprintln!("{i}: {event:?}");
                }
                panic!("{msg}");
            }
        }
        _ => { }
    }
}

Together I think these will help distinguish most expected and unexpected panics.
I imagine that at some point I will add recommendations to the fuzzing tutorial to do similar.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants
@brson @leighmcculloch @sisuresh @deep-ink-ventures and others