conversion.rs code review fixes (#1117)

Nothing deeply complicated in here, just a bunch of error plumbing,
error code and check consolidation, double checks of internal logic,
renames and comment gardening.

The most annoying parts of this PR are the bad errors we get from
"anything going wrong during ScVal conversion" which is already noted as
bug #1046 which I would love to fix but we probably don't have time; or
at least last time I tried I gave up because it required too much SDK
proc-macro hacking and error routing. Maybe I can try again sometime?

soroban-env-common/src/convert.rs

@@ -474,6 +474,10 @@ where
 {
     type Error = ConversionError;
     fn try_from_val(env: &E, val: &ScVal) -> Result<Val, Self::Error> {
+        if !Val::can_represent_scval(val) {
+            return Err(ConversionError);
+        }
+
         if let Some(scvo) = ScValObjRef::classify(val) {
             let obj = Object::try_from_val(env, &scvo)?;
             return Ok(obj.into());
@@ -529,7 +533,8 @@ where
             }
 
             // These should all have been classified as ScValObjRef above, or are
-            // reserved ScVal types that are never passed as vals at all.
+            // reserved ScVal types Val::can_represent_scval would have returned
+            // false from above.
             ScVal::Bytes(_)
             | ScVal::String(_)
             | ScVal::Vec(_)

soroban-env-common/src/error.rs

@@ -1,6 +1,6 @@
 use crate::xdr::{ScError, ScErrorCode, ScErrorType, ScVal};
 use crate::{
-    impl_wrapper_as_and_to_rawval, impl_wrapper_tag_based_constructors,
+    impl_wrapper_as_and_to_val, impl_wrapper_tag_based_constructors,
     impl_wrapper_tag_based_valconvert, impl_wrapper_wasmi_conversions, Compare, ConversionError,
     Env, SymbolError, Val,
 };
@@ -22,7 +22,7 @@ pub struct Error(Val);
 
 impl_wrapper_tag_based_valconvert!(Error);
 impl_wrapper_tag_based_constructors!(Error);
-impl_wrapper_as_and_to_rawval!(Error);
+impl_wrapper_as_and_to_val!(Error);
 impl_wrapper_wasmi_conversions!(Error);
 
 impl Hash for Error {

soroban-env-common/src/object.rs

@@ -1,6 +1,6 @@
 use crate::xdr::{Duration, ScVal, TimePoint};
 use crate::{
-    impl_rawval_wrapper_base, num, val::ValConvert, Compare, ConversionError, Convert, Env, Tag,
+    impl_val_wrapper_base, num, val::ValConvert, Compare, ConversionError, Convert, Env, Tag,
     TryFromVal, Val,
 };
 use core::{cmp::Ordering, fmt::Debug};
@@ -11,7 +11,7 @@ use core::{cmp::Ordering, fmt::Debug};
 #[repr(transparent)]
 #[derive(Copy, Clone)]
 pub struct Object(pub(crate) Val);
-impl_rawval_wrapper_base!(Object);
+impl_val_wrapper_base!(Object);
 
 impl ValConvert for Object {
     fn is_val_type(v: Val) -> bool {

soroban-env-common/src/val.rs

@@ -3,9 +3,9 @@
 
 use crate::xdr::{ScError, ScValType};
 use crate::{
-    declare_tag_based_object_wrapper, declare_tag_based_wrapper, impl_rawval_wrapper_base,
-    impl_tryfroms_and_tryfromvals_delegating_to_valconvert, Compare, I32Val, SymbolSmall,
-    SymbolStr, U32Val,
+    declare_tag_based_object_wrapper, declare_tag_based_wrapper,
+    impl_tryfroms_and_tryfromvals_delegating_to_valconvert, impl_val_wrapper_base, Compare, I32Val,
+    SymbolSmall, SymbolStr, U32Val,
 };
 
 use super::{Env, Error, TryFromVal};
@@ -158,12 +158,12 @@ pub enum Tag {
 
 impl Tag {
     #[inline(always)]
-    pub const fn rawval_mask() -> i64 {
+    pub const fn val_mask() -> i64 {
         TAG_MASK as i64
     }
 
     #[inline(always)]
-    pub fn rawval_const(&self) -> i64 {
+    pub fn val_const(&self) -> i64 {
         *self as i64
     }
 
@@ -294,7 +294,7 @@ impl<E: Env> Compare<Void> for E {
 #[repr(transparent)]
 #[derive(Copy, Clone)]
 pub struct Bool(Val);
-impl_rawval_wrapper_base!(Bool);
+impl_val_wrapper_base!(Bool);
 
 impl From<bool> for Bool {
     fn from(value: bool) -> Self {
@@ -574,6 +574,40 @@ impl From<&ScError> for Val {
 // Utility methods
 
 impl Val {
+    /// Some ScVals are not representable as Vals at all,
+    /// and only exist in the XDR to serve as special storage
+    /// system key or value payloads managed by the Host.
+    pub const fn can_represent_scval_type(scv_ty: ScValType) -> bool {
+        match scv_ty {
+            ScValType::Bool
+            | ScValType::Void
+            | ScValType::Error
+            | ScValType::U32
+            | ScValType::I32
+            | ScValType::U64
+            | ScValType::I64
+            | ScValType::Timepoint
+            | ScValType::Duration
+            | ScValType::U128
+            | ScValType::I128
+            | ScValType::U256
+            | ScValType::I256
+            | ScValType::Bytes
+            | ScValType::String
+            | ScValType::Symbol
+            | ScValType::Vec
+            | ScValType::Map
+            | ScValType::Address => true,
+            ScValType::ContractInstance
+            | ScValType::LedgerKeyContractInstance
+            | ScValType::LedgerKeyNonce => false,
+        }
+    }
+
+    pub const fn can_represent_scval(scv: &crate::xdr::ScVal) -> bool {
+        Self::can_represent_scval_type(scv.discriminant())
+    }
+
     #[inline(always)]
     pub const fn get_payload(self) -> u64 {
         self.0

soroban-env-common/src/wrapper_macros.rs

@@ -105,7 +105,7 @@ macro_rules! impl_wrapper_wasmi_conversions {
 
 #[doc(hidden)]
 #[macro_export]
-macro_rules! impl_wrapper_as_and_to_rawval {
+macro_rules! impl_wrapper_as_and_to_val {
     ($wrapper:ty) => {
         // AsRef / AsMut to Val.
         impl AsRef<$crate::Val> for $wrapper {
@@ -168,14 +168,14 @@ macro_rules! impl_wrapper_from_other_type {
 /// Macro for base implementation of a type wrapping a [`Val`]
 #[doc(hidden)]
 #[macro_export]
-macro_rules! impl_rawval_wrapper_base {
+macro_rules! impl_val_wrapper_base {
     ($T:ident) => {
         impl core::fmt::Debug for $T {
             fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
                 self.0.fmt(f)
             }
         }
-        $crate::impl_wrapper_as_and_to_rawval!($T);
+        $crate::impl_wrapper_as_and_to_val!($T);
         $crate::impl_wrapper_wasmi_conversions!($T);
         $crate::impl_tryfroms_and_tryfromvals_delegating_to_valconvert!($T);
     };
@@ -215,7 +215,7 @@ macro_rules! declare_tag_based_wrapper {
         #[derive(Copy, Clone)]
         pub struct $T($crate::Val);
 
-        $crate::impl_rawval_wrapper_base!($T);
+        $crate::impl_val_wrapper_base!($T);
         $crate::impl_wrapper_tag_based_valconvert!($T);
         $crate::impl_wrapper_tag_based_constructors!($T);
     };
@@ -272,7 +272,7 @@ macro_rules! declare_tag_based_small_and_object_wrappers {
         #[repr(transparent)]
         #[derive(Copy, Clone)]
         pub struct $GENERAL($crate::Val);
-        $crate::impl_rawval_wrapper_base!($GENERAL);
+        $crate::impl_val_wrapper_base!($GENERAL);
 
         impl $crate::val::ValConvert for $GENERAL {
             fn is_val_type(v: $crate::Val) -> bool {

soroban-env-host/benches/common/experimental/map_ops.rs

@@ -14,7 +14,7 @@ impl HostCostMeasurement for MapEntryMeasure {
 
     fn new_random_case(host: &Host, rng: &mut StdRng, input: u64) -> MapEntrySample {
         let input = 1 + input * Self::STEP_SIZE;
-        let mut keys: Vec<_> = util::to_rawval_u32(0..(input as u32)).collect();
+        let mut keys: Vec<_> = util::u32_iter_to_val_iter(0..(input as u32)).collect();
         let om = keys.iter().cloned().zip(keys.iter().cloned()).collect();
         let map: MeteredOrdMap<_, _, _> = MeteredOrdMap::from_map(om, host).unwrap();
         keys.shuffle(rng);

soroban-env-host/benches/common/experimental/vec_ops.rs

@@ -15,7 +15,7 @@ impl HostCostMeasurement for VecEntryMeasure {
     // Random case is worst case.
     fn new_random_case(_host: &Host, rng: &mut StdRng, input: u64) -> VecEntrySample {
         let input = 1 + input * Self::STEP_SIZE;
-        let ov = util::to_rawval_u32(0..(input as u32)).collect();
+        let ov = util::u32_iter_to_val_iter(0..(input as u32)).collect();
         let vec: MeteredVector<_> = MeteredVector::from_vec(ov).unwrap();
         let mut idxs: Vec<usize> = (0..input as usize).collect();
         idxs.shuffle(rng);

soroban-env-host/benches/common/util.rs

@@ -25,7 +25,7 @@ pub(crate) const TEST_WASMS: [&'static [u8]; 10] = [
     soroban_test_wasms::COMPLEX,
 ];
 
-pub(crate) fn to_rawval_u32<I: Iterator<Item = u32>>(vals: I) -> impl Iterator<Item = Val> {
+pub(crate) fn u32_iter_to_val_iter<I: Iterator<Item = u32>>(vals: I) -> impl Iterator<Item = Val> {
     vals.map(move |v| Val::from_u32(v).into())
 }
 

soroban-env-host/src/auth.rs

@@ -364,7 +364,7 @@ impl AuthorizedFunction {
                 AuthorizedFunction::ContractFn(ContractFunction {
                     contract_address: host.add_host_object(xdr_contract_fn.contract_address)?,
                     function_name: Symbol::try_from_val(host, &xdr_contract_fn.function_name)?,
-                    args: host.scvals_to_rawvals(xdr_contract_fn.args.as_slice())?,
+                    args: host.scvals_to_val_vec(xdr_contract_fn.args.as_slice())?,
                 })
             }
             SorobanAuthorizedFunction::CreateContractHostFn(xdr_args) => {
@@ -391,7 +391,7 @@ impl AuthorizedFunction {
                 Ok(SorobanAuthorizedFunction::ContractFn(InvokeContractArgs {
                     contract_address: host.scaddress_from_address(contract_fn.contract_address)?,
                     function_name,
-                    args: host.rawvals_to_sc_val_vec(contract_fn.args.as_slice())?,
+                    args: host.vals_to_scval_vec(contract_fn.args.as_slice())?,
                 }))
             }
             AuthorizedFunction::CreateContractHostFn(create_contract_args) => {
@@ -413,7 +413,7 @@ impl AuthorizedFunction {
                         })?,
                     function_name: contract_fn.function_name.try_into_val(host)?,
                     // why is this intentionally non-metered? the visit_obj above is metered.
-                    args: host.rawvals_to_sc_val_vec_non_metered(contract_fn.args.as_slice())?,
+                    args: host.vals_to_scval_vec_non_metered(contract_fn.args.as_slice())?,
                 }))
             }
             AuthorizedFunction::CreateContractHostFn(create_contract_args) => Ok(

soroban-env-host/src/events/internal.rs

@@ -25,7 +25,7 @@ pub struct InternalContractEvent {
 impl InternalContractEvent {
     // Metering: covered by components
     pub fn to_xdr(&self, host: &Host) -> Result<xdr::ContractEvent, HostError> {
-        let topics = host.call_args_to_sc_val_vec(self.topics)?;
+        let topics = host.vecobject_to_scval_vec(self.topics)?;
         let data = host.from_host_val(self.data)?;
         let contract_id = match self.contract_id {
             Some(id) => Some(host.hash_from_bytesobj_input("contract_id", id)?),

soroban-env-host/src/host.rs

@@ -1619,7 +1619,7 @@ impl VmCallerEnv for Host {
         self.check_val_integrity(k)?;
         let res = match t {
             StorageType::Temporary | StorageType::Persistent => {
-                let key = self.storage_key_from_rawval(k, t.try_into()?)?;
+                let key = self.storage_key_from_val(k, t.try_into()?)?;
                 self.try_borrow_storage_mut()?
                     .has(&key, self.as_budget())
                     .map_err(|e| self.decorate_contract_data_storage_error(e, k))?
@@ -1642,7 +1642,7 @@ impl VmCallerEnv for Host {
         self.check_val_integrity(k)?;
         match t {
             StorageType::Temporary | StorageType::Persistent => {
-                let key = self.storage_key_from_rawval(k, t.try_into()?)?;
+                let key = self.storage_key_from_val(k, t.try_into()?)?;
                 let entry = self
                     .try_borrow_storage_mut()?
                     .get(&key, self.as_budget())
@@ -1683,7 +1683,7 @@ impl VmCallerEnv for Host {
         self.check_val_integrity(k)?;
         match t {
             StorageType::Temporary | StorageType::Persistent => {
-                let key = self.contract_data_key_from_rawval(k, t.try_into()?)?;
+                let key = self.contract_data_key_from_val(k, t.try_into()?)?;
                 self.try_borrow_storage_mut()?
                     .del(&key, self.as_budget())
                     .map_err(|e| self.decorate_contract_data_storage_error(e, k))?;
@@ -1719,7 +1719,7 @@ impl VmCallerEnv for Host {
                 &[],
             ))?;
         }
-        let key = self.contract_data_key_from_rawval(k, t.try_into()?)?;
+        let key = self.contract_data_key_from_val(k, t.try_into()?)?;
         self.try_borrow_storage_mut()?
             .extend(self, key, threshold.into(), extend_to.into())
             .map_err(|e| self.decorate_contract_data_storage_error(e, k))?;

soroban-env-host/src/host/comparison.rs

@@ -246,7 +246,7 @@ impl Compare<ScVal> for Budget {
             (Map(Some(a)), Map(Some(b))) => self.compare(a, b),
 
             (Vec(None), _) | (_, Vec(None)) | (Map(None), _) | (_, Map(None)) => {
-                Err((ScErrorType::Object, ScErrorCode::MissingValue).into())
+                Err((ScErrorType::Value, ScErrorCode::InvalidInput).into())
             }
 
             (Bytes(a), Bytes(b)) => {
@@ -589,24 +589,24 @@ mod tests {
     #[test]
     fn compare_obj_to_small() {
         let host = Host::default();
-        let rawvals: Vec<Val> = all_tags()
+        let vals: Vec<Val> = all_tags()
             .into_iter()
             .map(|t| example_for_tag(&host, t))
             .collect();
-        let scvals: Vec<ScVal> = rawvals
+        let scvals: Vec<ScVal> = vals
             .iter()
             .map(|r| ScVal::try_from_val(&host, r).expect("scval"))
             .collect();
 
-        let rawval_pairs = rawvals.iter().cartesian_product(&rawvals);
+        let val_pairs = vals.iter().cartesian_product(&vals);
         let scval_pairs = scvals.iter().cartesian_product(&scvals);
 
-        let pair_pairs = rawval_pairs.zip(scval_pairs);
+        let pair_pairs = val_pairs.zip(scval_pairs);
 
-        for ((rawval1, rawval2), (scval1, scval2)) in pair_pairs {
-            let rawval_cmp = host.compare(rawval1, rawval2).expect("compare");
+        for ((val1, val2), (scval1, scval2)) in pair_pairs {
+            let val_cmp = host.compare(val1, val2).expect("compare");
             let scval_cmp = scval1.cmp(scval2);
-            assert_eq!(rawval_cmp, scval_cmp);
+            assert_eq!(val_cmp, scval_cmp);
         }
     }