soroban-examples
soroban-examples copied to clipboard
Rewrite all contracts to use contracterrors
Related issue - https://github.com/stellar/rs-soroban-env/issues/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.
I think we need to rewrite all our examples to define contracterror
s, 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.
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.
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
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.