Graydon code review

soroban-env-common/src/arbitrary.rs

@@ -10,9 +10,8 @@ use arbitrary::{Arbitrary, Unstructured};
 
 impl<'a> Arbitrary<'a> for Error {
     fn arbitrary(u: &mut Unstructured<'a>) -> arbitrary::Result<Self> {
-        let scstatus = ScError::arbitrary(u)?;
-        let status = Error::from(scstatus);
-
-        Ok(status)
+        let scerror = ScError::arbitrary(u)?;
+        let error = Error::from(scerror);
+        Ok(error)
     }
 }

soroban-env-common/src/bytes.rs

@@ -1,5 +1,7 @@
 use crate::{
-    declare_tag_based_object_wrapper, ConversionError, Env, Error, TryFromVal, TryIntoVal, Val,
+    declare_tag_based_object_wrapper,
+    xdr::{ScErrorCode, ScErrorType},
+    Env, Error, TryFromVal, TryIntoVal, Val,
 };
 
 declare_tag_based_object_wrapper!(BytesObject);
@@ -8,14 +10,17 @@ impl<E: Env, const N: usize> TryFromVal<E, BytesObject> for [u8; N] {
     type Error = Error;
 
     fn try_from_val(env: &E, val: &BytesObject) -> Result<Self, Self::Error> {
-        let len: u32 = env.bytes_len(*val).map_err(|_| ConversionError)?.into();
+        let len: u32 = env.bytes_len(*val).map_err(Into::into)?.into();
         let len = len as usize;
         if len != N {
-            return Err(ConversionError.into());
+            return Err(Error::from_type_and_code(
+                ScErrorType::Value,
+                ScErrorCode::UnexpectedSize,
+            ));
         }
         let mut arr = [0u8; N];
         env.bytes_copy_to_slice(*val, Val::U32_ZERO, &mut arr)
-            .map_err(|_| ConversionError)?;
+            .map_err(Into::into)?;
         Ok(arr)
     }
 }
@@ -34,11 +39,11 @@ impl<E: Env> TryFromVal<E, BytesObject> for Vec<u8> {
     type Error = Error;
 
     fn try_from_val(env: &E, val: &BytesObject) -> Result<Self, Self::Error> {
-        let len: u32 = env.bytes_len(*val).map_err(|_| ConversionError)?.into();
+        let len: u32 = env.bytes_len(*val).map_err(Into::into)?.into();
         let len = len as usize;
         let mut vec = vec![0u8; len];
         env.bytes_copy_to_slice(*val, Val::U32_ZERO, &mut vec)
-            .map_err(|_| ConversionError)?;
+            .map_err(Into::into)?;
         Ok(vec)
     }
 }
@@ -57,7 +62,7 @@ impl<E: Env> TryFromVal<E, &[u8]> for BytesObject {
     type Error = Error;
     #[inline(always)]
     fn try_from_val(env: &E, v: &&[u8]) -> Result<BytesObject, Self::Error> {
-        Ok(env.bytes_new_from_slice(v).map_err(|_| ConversionError)?)
+        env.bytes_new_from_slice(v).map_err(Into::into)
     }
 }
 

soroban-env-common/src/compare.rs

@@ -1,7 +1,11 @@
 #[cfg(feature = "std")]
 use std::rc::Rc;
 
-use crate::{val::ValConvert, Env, Tag, Val};
+use crate::{
+    val::ValConvert,
+    xdr::{ScErrorCode, ScErrorType},
+    Env, Error, Tag, Val,
+};
 use core::cmp::Ordering;
 
 /// General trait representing the ability to compare two values of some type.
@@ -51,11 +55,9 @@ macro_rules! impl_compare_for_tuple {
 
             fn compare(&self, a: &($($T,)+), b: &($($T,)+)) -> Result<Ordering, Self::Error> {
                 $(
-                    if let Ordering::Less = <C as Compare<$T>>::compare(self, &a.$idx, &b.$idx)? {
-                        return Ok(Ordering::Less)
-                    }
-                    if let Ordering::Greater = <C as Compare<$T>>::compare(self, &a.$idx, &b.$idx)? {
-                        return Ok(Ordering::Greater)
+                    match <C as Compare<$T>>::compare(self, &a.$idx, &b.$idx)? {
+                        unequal @ (Ordering::Less | Ordering::Greater) => return Ok(unequal),
+                        _ => ()
                     }
                 )*
                 Ok(Ordering::Equal)
@@ -194,7 +196,11 @@ impl<E: Env> Compare<Val> for E {
                 | Tag::SymbolObject
                 | Tag::VecObject
                 | Tag::MapObject
-                | Tag::AddressObject => unreachable!(),
+                | Tag::AddressObject => Err(Error::from_type_and_code(
+                    ScErrorType::Context,
+                    ScErrorCode::InternalError,
+                )
+                .into()),
 
                 Tag::ObjectCodeUpperBound => Ok(Ordering::Equal),
                 Tag::Bad => Ok(Ordering::Equal),