Skip to content

Commit

Permalink
Comparison fixes (#762)
Browse files Browse the repository at this point in the history 
* Fix comparisons of Vec and Map with unequal lengths

* Order object discriminants the same as XDR

* Make Ord for Status agree with XDR comparison
  • Loading branch information
@brson
brson committed Apr 11, 2023
1 parent 2ceaad2 commit 5c7c415
Show file tree
Hide file tree
Showing 2 changed files with 283 additions and 31 deletions.
60 changes: 58 additions & 2 deletions soroban-env-common/src/status.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,8 @@ impl PartialOrd for Status {
impl Ord for Status {
#[inline(always)]
fn cmp(&self, other: &Self) -> Ordering {
let self_tup = (self.as_raw().get_major(), self.as_raw().get_minor());
let other_tup = (other.as_raw().get_major(), other.as_raw().get_minor());
let self_tup = (self.as_raw().get_minor(), self.as_raw().get_major());
let other_tup = (other.as_raw().get_minor(), other.as_raw().get_major());
self_tup.cmp(&other_tup)
}
}
Expand Down Expand Up @@ -327,3 +327,59 @@ impl From<core::convert::Infallible> for crate::Status {
unreachable!()
}
}

#[cfg(all(test, feature = "std"))]
mod tests {
use super::*;

#[test]
fn status_ord_same_as_scstatus() {
// The impl `Ord for Status` must agree with `Ord for ScStatus`,
// re https://github.com/stellar/rs-soroban-env/issues/743.
//
// This test creates pairs of corresponding ScStatus/Status values,
// puts them all into a list, and sorts them with each comparison function,
// then checks that both lists are sorted the same.

use crate::xdr::*;

let xdr_vals = &[
ScStatus::Ok,
ScStatus::UnknownError(ScUnknownErrorCode::General),
ScStatus::UnknownError(ScUnknownErrorCode::Xdr),
ScStatus::HostValueError(ScHostValErrorCode::UnknownError),
ScStatus::HostValueError(ScHostValErrorCode::ReservedTagValue),
ScStatus::HostObjectError(ScHostObjErrorCode::UnknownError),
ScStatus::HostObjectError(ScHostObjErrorCode::UnknownReference),
ScStatus::HostFunctionError(ScHostFnErrorCode::UnknownError),
ScStatus::HostFunctionError(ScHostFnErrorCode::UnexpectedHostFunctionAction),
ScStatus::HostStorageError(ScHostStorageErrorCode::UnknownError),
ScStatus::HostStorageError(ScHostStorageErrorCode::ExpectContractData),
ScStatus::HostContextError(ScHostContextErrorCode::UnknownError),
ScStatus::HostContextError(ScHostContextErrorCode::NoContractRunning),
ScStatus::VmError(ScVmErrorCode::Unknown),
ScStatus::VmError(ScVmErrorCode::Validation),
ScStatus::ContractError(0),
ScStatus::ContractError(1),
ScStatus::HostAuthError(ScHostAuthErrorCode::UnknownError),
ScStatus::HostAuthError(ScHostAuthErrorCode::NonceError),
];

let pairs: Vec<_> = xdr_vals
.into_iter()
.map(|xdr_val| {
let host_val = Status::try_from(xdr_val.clone()).unwrap();
(xdr_val, host_val)
})
.collect();

let mut pairs_xdr_sorted = pairs.clone();
let mut pairs_host_sorted = pairs_xdr_sorted.clone();

pairs_xdr_sorted.sort_by(|&(v1, _), &(v2, _)| v1.cmp(&v2));

pairs_host_sorted.sort_by(|&(_, v1), &(_, v2)| v1.cmp(&v2));

assert_eq!(pairs_xdr_sorted, pairs_host_sorted);
}
}
254 changes: 225 additions & 29 deletions soroban-env-host/src/host/comparison.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use soroban_env_common::{
LedgerKeyAccount, LedgerKeyClaimableBalance, LedgerKeyConfigSetting, LedgerKeyContractCode,
LedgerKeyData, LedgerKeyLiquidityPool, LedgerKeyOffer, LedgerKeyTrustLine,
LiquidityPoolEntry, OfferEntry, PublicKey, ScAddress, ScContractExecutable,
ScHostValErrorCode, ScMap, ScNonceKey, ScVal, ScVec, TimePoint, TrustLineAsset,
ScHostValErrorCode, ScMap, ScMapEntry, ScNonceKey, ScVal, ScVec, TimePoint, TrustLineAsset,
TrustLineEntry, Uint256,
},
Compare, SymbolStr, I256, U256,
Expand All @@ -24,23 +24,26 @@ use super::declared_size::DeclaredSizeForMetering;
// We can't use core::mem::discriminant here because it returns an opaque type
// that only supports Eq, not Ord, to reduce the possibility of an API breakage
// based on reordering enums: https://github.com/rust-lang/rust/issues/51561
//
// Note that these must have the same order as the impl
// of Ord for ScVal, re https://github.com/stellar/rs-soroban-env/issues/743
fn host_obj_discriminant(ho: &HostObject) -> usize {
match ho {
HostObject::Vec(_) => 0,
HostObject::Map(_) => 1,
HostObject::U64(_) => 2,
HostObject::I64(_) => 3,
HostObject::TimePoint(_) => 4,
HostObject::Duration(_) => 5,
HostObject::U128(_) => 6,
HostObject::I128(_) => 7,
HostObject::U256(_) => 8,
HostObject::I256(_) => 9,
HostObject::Bytes(_) => 10,
HostObject::String(_) => 11,
HostObject::Symbol(_) => 12,
HostObject::Address(_) => 13,
HostObject::ContractExecutable(_) => 14,
HostObject::U64(_) => 0,
HostObject::I64(_) => 1,
HostObject::TimePoint(_) => 2,
HostObject::Duration(_) => 3,
HostObject::U128(_) => 4,
HostObject::I128(_) => 5,
HostObject::U256(_) => 6,
HostObject::I256(_) => 7,
HostObject::Bytes(_) => 8,
HostObject::String(_) => 9,
HostObject::Symbol(_) => 10,
HostObject::Vec(_) => 11,
HostObject::Map(_) => 12,
HostObject::ContractExecutable(_) => 13,
HostObject::Address(_) => 14,
HostObject::NonceKey(_) => 15,
}
}
Expand Down Expand Up @@ -211,27 +214,30 @@ impl Compare<ScVec> for Budget {
type Error = HostError;

fn compare(&self, a: &ScVec, b: &ScVec) -> Result<Ordering, Self::Error> {
for (a, b) in a.iter().zip(b.iter()) {
match self.compare(a, b)? {
Ordering::Equal => (),
unequal => return Ok(unequal),
}
}
Ok(Ordering::Equal)
let a: &Vec<ScVal> = &*a;
let b: &Vec<ScVal> = &*b;
self.compare(a, b)
}
}

impl Compare<ScMap> for Budget {
type Error = HostError;

fn compare(&self, a: &ScMap, b: &ScMap) -> Result<Ordering, Self::Error> {
for (a, b) in a.iter().zip(b.iter()) {
match self.compare(&(&a.key, &a.val), &(&b.key, &b.val))? {
Ordering::Equal => (),
unequal => return Ok(unequal),
}
let a: &Vec<ScMapEntry> = &*a;
let b: &Vec<ScMapEntry> = &*b;
self.compare(a, b)
}
}

impl Compare<ScMapEntry> for Budget {
type Error = HostError;

fn compare(&self, a: &ScMapEntry, b: &ScMapEntry) -> Result<Ordering, Self::Error> {
match self.compare(&a.key, &b.key)? {
Ordering::Equal => self.compare(&a.val, &b.val),
cmp => Ok(cmp),
}
Ok(Ordering::Equal)
}
}

Expand Down Expand Up @@ -362,3 +368,193 @@ impl Compare<LedgerEntryData> for Budget {
}
}
}

#[cfg(test)]
mod tests {
use super::*;

#[test]
fn test_scvec_unequal_lengths() {
{
let v1 = ScVec::try_from((0, 1)).unwrap();
let v2 = ScVec::try_from((0, 1, 2)).unwrap();
let expected_cmp = Ordering::Less;
let budget = Budget::default();
let actual_cmp = budget.compare(&v1, &v2).unwrap();
assert_eq!(expected_cmp, actual_cmp);
}
{
let v1 = ScVec::try_from((0, 1, 2)).unwrap();
let v2 = ScVec::try_from((0, 1)).unwrap();
let expected_cmp = Ordering::Greater;
let budget = Budget::default();
let actual_cmp = budget.compare(&v1, &v2).unwrap();
assert_eq!(expected_cmp, actual_cmp);
}
{
let v1 = ScVec::try_from((0, 1)).unwrap();
let v2 = ScVec::try_from((0, 0, 2)).unwrap();
let expected_cmp = Ordering::Greater;
let budget = Budget::default();
let actual_cmp = budget.compare(&v1, &v2).unwrap();
assert_eq!(expected_cmp, actual_cmp);
}
{
let v1 = ScVec::try_from((0, 0, 2)).unwrap();
let v2 = ScVec::try_from((0, 1)).unwrap();
let expected_cmp = Ordering::Less;
let budget = Budget::default();
let actual_cmp = budget.compare(&v1, &v2).unwrap();
assert_eq!(expected_cmp, actual_cmp);
}
}

#[test]
fn test_scmap_unequal_lengths() {
{
let v1 = ScMap::sorted_from([
(ScVal::U32(0), ScVal::U32(0)),
(ScVal::U32(1), ScVal::U32(1)),
])
.unwrap();
let v2 = ScMap::sorted_from([
(ScVal::U32(0), ScVal::U32(0)),
(ScVal::U32(1), ScVal::U32(1)),
(ScVal::U32(2), ScVal::U32(2)),
])
.unwrap();
let expected_cmp = Ordering::Less;
let budget = Budget::default();
let actual_cmp = budget.compare(&v1, &v2).unwrap();
assert_eq!(expected_cmp, actual_cmp);
}
{
let v1 = ScMap::sorted_from([
(ScVal::U32(0), ScVal::U32(0)),
(ScVal::U32(1), ScVal::U32(1)),
(ScVal::U32(2), ScVal::U32(2)),
])
.unwrap();
let v2 = ScMap::sorted_from([
(ScVal::U32(0), ScVal::U32(0)),
(ScVal::U32(1), ScVal::U32(1)),
])
.unwrap();
let expected_cmp = Ordering::Greater;
let budget = Budget::default();
let actual_cmp = budget.compare(&v1, &v2).unwrap();
assert_eq!(expected_cmp, actual_cmp);
}
{
let v1 = ScMap::sorted_from([
(ScVal::U32(0), ScVal::U32(0)),
(ScVal::U32(1), ScVal::U32(1)),
])
.unwrap();
let v2 = ScMap::sorted_from([
(ScVal::U32(0), ScVal::U32(0)),
(ScVal::U32(1), ScVal::U32(0)),
(ScVal::U32(2), ScVal::U32(2)),
])
.unwrap();
let expected_cmp = Ordering::Greater;
let budget = Budget::default();
let actual_cmp = budget.compare(&v1, &v2).unwrap();
assert_eq!(expected_cmp, actual_cmp);
}
{
let v1 = ScMap::sorted_from([
(ScVal::U32(0), ScVal::U32(0)),
(ScVal::U32(1), ScVal::U32(0)),
(ScVal::U32(2), ScVal::U32(2)),
])
.unwrap();
let v2 = ScMap::sorted_from([
(ScVal::U32(0), ScVal::U32(0)),
(ScVal::U32(1), ScVal::U32(1)),
])
.unwrap();
let expected_cmp = Ordering::Less;
let budget = Budget::default();
let actual_cmp = budget.compare(&v1, &v2).unwrap();
assert_eq!(expected_cmp, actual_cmp);
}
}

#[test]
fn host_obj_discriminant_order() {
// The HostObject discriminants need to be ordered the same
// as the ScVal discriminants so that Compare<HostObject>
// produces the same results as `Ord for ScVal`,
// re https://github.com/stellar/rs-soroban-env/issues/743.
//
// This test creates pairs of corresponding ScVal/HostObjects,
// puts them all into a list, and sorts them 2 ways:
// comparing ScVals, and comparing the HostObject discriminants;
// then tests that the two lists are the same.

use crate::ScValObjRef;
use soroban_env_common::xdr;

let host = Host::default();

let xdr_vals = &[
ScVal::U64(u64::MAX),
ScVal::I64(i64::MAX),
ScVal::Timepoint(xdr::TimePoint(u64::MAX)),
ScVal::Duration(xdr::Duration(u64::MAX)),
ScVal::U128(xdr::Int128Parts {
lo: u64::MAX,
hi: u64::MAX,
}),
ScVal::I128(xdr::Int128Parts {
lo: u64::MAX,
hi: 0,
}),
ScVal::U256(xdr::Uint256([0xff; 32])),
ScVal::I256(xdr::Uint256([0xf0; 32])),
ScVal::Bytes(xdr::ScBytes::try_from(vec![]).unwrap()),
ScVal::String(xdr::ScString::try_from(vec![]).unwrap()),
ScVal::Symbol(xdr::ScSymbol::try_from("big-symbol").unwrap()),
ScVal::Vec(Some(xdr::ScVec::try_from((0,)).unwrap())),
ScVal::Map(Some(xdr::ScMap::try_from(vec![]).unwrap())),
ScVal::ContractExecutable(xdr::ScContractExecutable::Token),
ScVal::Address(xdr::ScAddress::Contract(xdr::Hash([0; 32]))),
];

let pairs: Vec<_> = xdr_vals
.into_iter()
.map(|xdr_val| {
let xdr_obj = ScValObjRef::classify(&xdr_val).unwrap();
let host_obj = host.to_host_obj(&xdr_obj).unwrap();
(xdr_obj, host_obj)
})
.collect();

let mut pairs_xdr_sorted = pairs.clone();
let mut pairs_host_sorted = pairs_xdr_sorted.clone();

pairs_xdr_sorted.sort_by(|&(v1, _), &(v2, _)| v1.cmp(&v2));

pairs_host_sorted.sort_by(|&(_, v1), &(_, v2)| unsafe {
host.unchecked_visit_val_obj(v1, |v1| {
host.unchecked_visit_val_obj(v2, |v2| {
let v1 = v1.unwrap();
let v2 = v2.unwrap();
let v1d = host_obj_discriminant(v1);
let v2d = host_obj_discriminant(v2);
Ok(v1d.cmp(&v2d))
})
})
.unwrap()
});

let iter = pairs_xdr_sorted
.into_iter()
.zip(pairs_host_sorted.into_iter());

for ((xdr1, _), (xdr2, _)) in iter {
assert_eq!(xdr1, xdr2);
}
}
}

0 comments on commit 5c7c415

Please sign in to comment.