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: improve error decoding and value formatting #6286

Merged
merged 3 commits into from
Nov 11, 2023
Merged
Show file tree
Hide file tree
Changes from 2 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
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

14 changes: 7 additions & 7 deletions crates/common/src/calc.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
//! Commonly used calculations.

use alloy_primitives::U256;
use alloy_primitives::{Sign, U256};
use std::ops::Div;

/// Returns the mean of the slice
Expand Down Expand Up @@ -44,7 +44,7 @@ pub fn median_sorted(values: &[U256]) -> U256 {
/// 10000000 -> 1e7
/// ```
#[inline]
pub fn to_exp_notation(value: U256, precision: usize, trim_end_zeros: bool) -> String {
pub fn to_exp_notation(value: U256, precision: usize, trim_end_zeros: bool, sign: Sign) -> String {
let stringified = value.to_string();
let exponent = stringified.len() - 1;
let mut mantissa = stringified.chars().take(precision).collect::<String>();
Expand All @@ -61,7 +61,7 @@ pub fn to_exp_notation(value: U256, precision: usize, trim_end_zeros: bool) -> S
mantissa.insert(1, '.');
}

format!("{mantissa}e{exponent}")
format!("{sign}{mantissa}e{exponent}")
}

#[cfg(test)]
Expand Down Expand Up @@ -119,18 +119,18 @@ mod tests {
fn test_format_to_exponential_notation() {
let value = 1234124124u64;

let formatted = to_exp_notation(U256::from(value), 4, false);
let formatted = to_exp_notation(U256::from(value), 4, false, Sign::Positive);
assert_eq!(formatted, "1.234e9");

let formatted = to_exp_notation(U256::from(value), 3, true);
let formatted = to_exp_notation(U256::from(value), 3, true, Sign::Positive);
assert_eq!(formatted, "1.23e9");

let value = 10000000u64;

let formatted = to_exp_notation(U256::from(value), 4, false);
let formatted = to_exp_notation(U256::from(value), 4, false, Sign::Positive);
assert_eq!(formatted, "1.000e7");

let formatted = to_exp_notation(U256::from(value), 3, true);
let formatted = to_exp_notation(U256::from(value), 3, true, Sign::Positive);
assert_eq!(formatted, "1e7");
}
}
87 changes: 67 additions & 20 deletions crates/common/src/fmt.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,9 @@

use crate::{calc::to_exp_notation, TransactionReceiptWithRevertReason};
use alloy_dyn_abi::{DynSolType, DynSolValue};
use alloy_primitives::{hex, U256};
use alloy_primitives::{hex, Sign, I256, U256};
use eyre::Result;
use std::fmt::{self, Display};
use std::fmt;
use yansi::Paint;

pub use foundry_macros::fmt::*;
Expand All @@ -18,44 +18,55 @@ impl DynValueFormatter {
/// Recursively formats a [`DynSolValue`].
fn value(&self, value: &DynSolValue, f: &mut fmt::Formatter<'_>) -> fmt::Result {
match value {
DynSolValue::Address(inner) => Display::fmt(inner, f),
DynSolValue::Function(inner) => Display::fmt(inner, f),
DynSolValue::Address(inner) => write!(f, "{inner}"),
DynSolValue::Function(inner) => write!(f, "{inner}"),
DynSolValue::Bytes(inner) => f.write_str(&hex::encode_prefixed(inner)),
DynSolValue::FixedBytes(inner, _) => f.write_str(&hex::encode_prefixed(inner)),
DynSolValue::FixedBytes(inner, _) => write!(f, "{inner}"),
DynSolValue::Uint(inner, _) => {
if self.raw {
write!(f, "{inner}")
} else {
f.write_str(&format_uint_exp(*inner))
}
}
DynSolValue::Int(inner, _) => write!(f, "{inner}"),
DynSolValue::Int(inner, _) => {
if self.raw {
write!(f, "{inner}")
} else {
f.write_str(&format_int_exp(*inner))
}
}
DynSolValue::Array(values) | DynSolValue::FixedArray(values) => {
f.write_str("[")?;
self.list(values, f)?;
f.write_str("]")
}
DynSolValue::Tuple(values) => self.tuple(values, f),
DynSolValue::String(inner) => f.write_str(inner),
DynSolValue::Bool(inner) => Display::fmt(inner, f),
DynSolValue::String(inner) => write!(f, "{inner:?}"), // escape strings
DynSolValue::Bool(inner) => write!(f, "{inner}"),
DynSolValue::CustomStruct { name, prop_names, tuple } => {
if self.raw {
return self.tuple(tuple, f);
}

f.write_str(name)?;
f.write_str(" { ")?;

for (i, (prop_name, value)) in std::iter::zip(prop_names, tuple).enumerate() {
if i > 0 {
f.write_str(", ")?;
if prop_names.len() == tuple.len() {
f.write_str("({ ")?;

for (i, (prop_name, value)) in std::iter::zip(prop_names, tuple).enumerate() {
if i > 0 {
f.write_str(", ")?;
}
f.write_str(prop_name)?;
f.write_str(": ")?;
self.value(value, f)?;
}
f.write_str(prop_name)?;
f.write_str(": ")?;
self.value(value, f)?;
}

f.write_str(" }")
f.write_str(" })")
} else {
self.tuple(tuple, f)
}
}
}
}
Expand Down Expand Up @@ -143,7 +154,7 @@ pub fn format_token_raw(value: &DynSolValue) -> String {
/// use alloy_primitives::U256;
/// use foundry_common::fmt::format_uint_exp as f;
///
/// yansi::Paint::disable();
/// # yansi::Paint::disable();
/// assert_eq!(f(U256::from(0)), "0");
/// assert_eq!(f(U256::from(1234)), "1234");
/// assert_eq!(f(U256::from(1234567890)), "1234567890 [1.234e9]");
Expand All @@ -155,8 +166,44 @@ pub fn format_uint_exp(num: U256) -> String {
return num.to_string()
}

let exp = to_exp_notation(num, 4, true);
format!("{} {}", num, Paint::default(format!("[{exp}]")).dimmed())
let exp = to_exp_notation(num, 4, true, Sign::Positive);
format!("{num} {}", Paint::default(format!("[{exp}]")).dimmed())
}

/// Formats a U256 number to string, adding an exponential notation _hint_.
///
/// Same as [`format_uint_exp`].
///
/// # Examples
///
/// ```
/// use alloy_primitives::I256;
/// use foundry_common::fmt::format_int_exp as f;
///
/// # yansi::Paint::disable();
/// assert_eq!(f(I256::try_from(0).unwrap()), "0");
/// assert_eq!(f(I256::try_from(-1).unwrap()), "-1");
/// assert_eq!(f(I256::try_from(1234).unwrap()), "1234");
/// assert_eq!(f(I256::try_from(1234567890).unwrap()), "1234567890 [1.234e9]");
/// assert_eq!(f(I256::try_from(-1234567890).unwrap()), "-1234567890 [-1.234e9]");
/// assert_eq!(f(I256::try_from(1000000000000000000_u128).unwrap()), "1000000000000000000 [1e18]");
/// assert_eq!(
/// f(I256::try_from(10000000000000000000000_u128).unwrap()),
/// "10000000000000000000000 [1e22]"
/// );
/// assert_eq!(
/// f(I256::try_from(-10000000000000000000000_i128).unwrap()),
/// "-10000000000000000000000 [-1e22]"
/// );
/// ```
pub fn format_int_exp(num: I256) -> String {
let (sign, abs) = num.into_sign_and_abs();
if abs < U256::from(10_000) {
return format!("{sign}{abs}");
}

let exp = to_exp_notation(abs, 4, true, sign);
format!("{sign}{abs} {}", Paint::default(format!("[{exp}]")).dimmed())
}

impl UIfmt for TransactionReceiptWithRevertReason {
Expand Down
51 changes: 31 additions & 20 deletions crates/evm/core/src/decode.rs
Original file line number Diff line number Diff line change
Expand Up @@ -70,27 +70,41 @@ pub fn decode_revert(
maybe_abi: Option<&JsonAbi>,
status: Option<InstructionResult>,
) -> String {
maybe_decode_revert(err, maybe_abi, status).unwrap_or_else(|| {
if err.is_empty() {
"<no data>".to_string()
} else {
trimmed_hex(err)
}
})
}

pub fn maybe_decode_revert(
err: &[u8],
maybe_abi: Option<&JsonAbi>,
status: Option<InstructionResult>,
) -> Option<String> {
if err.len() < SELECTOR_LEN {
if let Some(status) = status {
if !status.is_ok() {
return format!("EvmError: {status:?}")
return Some(format!("EvmError: {status:?}"));
}
}
return if err.is_empty() {
"<no data>".to_string()
None
} else {
format!("custom error bytes {}", hex::encode_prefixed(err))
Some(format!("custom error bytes {}", hex::encode_prefixed(err)))
}
}

if err == crate::constants::MAGIC_SKIP {
// Also used in forge fuzz runner
return "SKIPPED".to_string()
return Some("SKIPPED".to_string());
}

// Solidity's `Error(string)` or `Panic(uint256)`
if let Ok(e) = alloy_sol_types::GenericContractError::abi_decode(err, false) {
return e.to_string()
return Some(e.to_string());
}

let (selector, data) = err.split_at(SELECTOR_LEN);
Expand All @@ -99,21 +113,18 @@ pub fn decode_revert(
match *selector {
// `CheatcodeError(string)`
Vm::CheatcodeError::SELECTOR => {
if let Ok(e) = Vm::CheatcodeError::abi_decode_raw(data, false) {
return e.message
}
let e = Vm::CheatcodeError::abi_decode_raw(data, false).ok()?;
return Some(e.message);
}
// `expectRevert(bytes)`
Vm::expectRevert_2Call::SELECTOR => {
if let Ok(e) = Vm::expectRevert_2Call::abi_decode_raw(data, false) {
return decode_revert(&e.revertData[..], maybe_abi, status)
}
let e = Vm::expectRevert_2Call::abi_decode_raw(data, false).ok()?;
return maybe_decode_revert(&e.revertData[..], maybe_abi, status);
}
// `expectRevert(bytes4)`
Vm::expectRevert_1Call::SELECTOR => {
if let Ok(e) = Vm::expectRevert_1Call::abi_decode_raw(data, false) {
return decode_revert(&e.revertData[..], maybe_abi, status)
}
let e = Vm::expectRevert_1Call::abi_decode_raw(data, false).ok()?;
return maybe_decode_revert(&e.revertData[..], maybe_abi, status);
}
_ => {}
}
Expand All @@ -123,31 +134,31 @@ pub fn decode_revert(
if let Some(abi_error) = abi.errors().find(|e| selector == e.selector()) {
// if we don't decode, don't return an error, try to decode as a string later
if let Ok(decoded) = abi_error.abi_decode_input(data, false) {
return format!(
return Some(format!(
"{}({})",
abi_error.name,
decoded.iter().map(foundry_common::fmt::format_token).format(", ")
)
));
}
}
}

// ABI-encoded `string`
if let Ok(s) = String::abi_decode(err, false) {
return s
return Some(s);
}

// UTF-8-encoded string
if let Ok(s) = std::str::from_utf8(err) {
return s.to_string()
return Some(s.to_string());
}

// Generic custom error
format!(
Some(format!(
"custom error {}:{}",
hex::encode(selector),
std::str::from_utf8(data).map_or_else(|_| trimmed_hex(data), String::from)
)
))
}

fn trimmed_hex(s: &[u8]) -> String {
Expand Down
4 changes: 3 additions & 1 deletion crates/evm/evm/src/executors/fuzz/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,9 @@ impl<'a> FuzzedExecutor<'a> {
// case.
let call_res = _counterexample.1.result.clone();
*counterexample.borrow_mut() = _counterexample;
Err(TestCaseError::fail(decode::decode_revert(&call_res, errors, Some(status))))
// HACK: we have to use an empty string here to denote `None`
let reason = decode::maybe_decode_revert(&call_res, errors, Some(status));
Err(TestCaseError::fail(reason.unwrap_or_default()))
}
}
});
Expand Down
1 change: 1 addition & 0 deletions crates/evm/fuzz/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ ethers-core.workspace = true
arbitrary = "1.3.1"
eyre = "0.6"
hashbrown = { version = "0.14", features = ["serde"] }
itertools.workspace = true
parking_lot = "0.12"
proptest = "1"
rand.workspace = true
Expand Down
5 changes: 2 additions & 3 deletions crates/evm/fuzz/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ use ethers_core::types::Log;
use foundry_common::{calc, contracts::ContractsByAddress};
use foundry_evm_coverage::HitMaps;
use foundry_evm_traces::CallTraceArena;
use itertools::Itertools;
use serde::{Deserialize, Serialize};
use std::{collections::BTreeMap, fmt};

Expand Down Expand Up @@ -92,8 +93,6 @@ impl BaseCounterExample {

impl fmt::Display for BaseCounterExample {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
let args = foundry_common::fmt::format_tokens(&self.args).collect::<Vec<_>>().join(", ");

if let Some(sender) = self.sender {
write!(f, "sender={sender} addr=")?
}
Expand All @@ -112,7 +111,7 @@ impl fmt::Display for BaseCounterExample {
write!(f, "calldata={}", self.calldata)?
}

write!(f, ", args=[{args}]")
write!(f, " args=[{}]", foundry_common::fmt::format_tokens(&self.args).format(", "))
}
}

Expand Down
4 changes: 2 additions & 2 deletions crates/forge/bin/cmd/create.rs
Original file line number Diff line number Diff line change
Expand Up @@ -205,9 +205,9 @@ impl CreateArgs {

let is_args_empty = args.is_empty();
let deployer =
factory.deploy_tokens(args.clone()).context("Failed to deploy contract").map_err(|e| {
factory.deploy_tokens(args.clone()).context("failed to deploy contract").map_err(|e| {
if is_args_empty {
e.wrap_err("No arguments provided for contract constructor. Consider --constructor-args or --constructor-args-path")
e.wrap_err("no arguments provided for contract constructor; consider --constructor-args or --constructor-args-path")
} else {
e
}
Expand Down
Loading
Loading