diff --git a/crates/xrpl-host-functions-macros/src/enums.rs b/crates/xrpl-host-functions-macros/src/enums.rs new file mode 100644 index 0000000000..2e4cfac36d --- /dev/null +++ b/crates/xrpl-host-functions-macros/src/enums.rs @@ -0,0 +1,289 @@ +//! `#[coded_enum]`: an enum of wire codes, and the `ALL`/`code`/`from_code` set +//! that must not fall behind its variants. +//! +//! Rust cannot enumerate an enum's variants — an exhaustive `match` forces an arm per +//! variant but gives nothing to iterate — so `ALL` is trustworthy only by being +//! generated from them, as `HostFunctionSpec::ALL` is from the `host_functions!` block. + +use proc_macro2::TokenStream; +use quote::quote; +use syn::{Expr, ExprLit, ExprUnary, Fields, Ident, ItemEnum, Lit, UnOp, Variant}; + +use crate::errors; + +/// One variant as the expansion reads it: the name `ALL` lists and the code +/// `from_code` matches. +struct WireVariant<'a> { + ident: &'a Ident, + code: &'a Expr, +} + +pub(crate) fn expand(args: TokenStream, item: TokenStream) -> syn::Result { + if !args.is_empty() { + return Err(syn::Error::new_spanned( + args, + "`#[coded_enum]` takes no arguments", + )); + } + + let item: ItemEnum = syn::parse2(item)?; + let variants = wire_variants(&item)?; + + let attrs = &item.attrs; + let vis = &item.vis; + let name = &item.ident; + let declarations = item.variants.iter(); + let identifiers = variants.iter().map(|variant| variant.ident); + let arms = variants.iter().map(|variant| { + let (ident, code) = (variant.ident, variant.code); + quote! { #code => Some(Self::#ident) } + }); + + Ok(quote! { + #(#attrs)* + #[derive(Debug, Clone, Copy, PartialEq, Eq)] + #[repr(i32)] + #vis enum #name { + #(#declarations,)* + } + + impl #name { + /// Every variant, in declaration order — the whole set, and whole by + /// construction. + pub const ALL: &'static [Self] = &[#(Self::#identifiers,)*]; + + /// The wire value that names this variant. + #[inline] + pub const fn code(self) -> i32 { + self as i32 + } + + /// The variant `code` names, or `None` if no variant does — what an + /// unnamed code means is the caller's to decide. + pub const fn from_code(code: i32) -> Option { + match code { + #(#arms,)* + _ => None, + } + } + } + }) +} + +/// Every variant, checked against what the expansion needs of it, or every +/// mistake in the list. +fn wire_variants(item: &ItemEnum) -> syn::Result>> { + // `#[repr(i32)]` is rejected on a variantless enum, and there is nothing for a + // wire enum with no codes to mean anyway. + if item.variants.is_empty() { + return Err(syn::Error::new_spanned( + item, + "`#[coded_enum]` needs at least one variant", + )); + } + + let mut errors = Vec::new(); + let variants = item + .variants + .iter() + .filter_map(|variant| { + let code = errors::record(code_of(variant), &mut errors)?; + Some(WireVariant { + ident: &variant.ident, + code, + }) + }) + .collect(); + + errors::into_result(variants, errors) +} + +/// The code a variant is declared with. +fn code_of(variant: &Variant) -> syn::Result<&Expr> { + if !matches!(variant.fields, Fields::Unit) { + return Err(syn::Error::new_spanned( + &variant.fields, + "a wire enum's variants carry no data: the code is the whole of what crosses", + )); + } + + let Some((_, code)) = &variant.discriminant else { + return Err(syn::Error::new_spanned( + variant, + "missing `= `: a wire value is declared, never implied by position", + )); + }; + + if !is_integer_literal(code) { + return Err(syn::Error::new_spanned( + code, + "a wire value must be an integer literal, since `from_code` matches it as a pattern", + )); + } + + Ok(code) +} + +/// `-2147483648` and `7`, but not `i32::MIN` or `1 + 1`: the discriminant is emitted +/// into pattern position unchanged, where an expression means something else or nothing. +fn is_integer_literal(code: &Expr) -> bool { + match code { + Expr::Lit(ExprLit { + lit: Lit::Int(_), .. + }) => true, + Expr::Unary(ExprUnary { + op: UnOp::Neg(_), + expr, + .. + }) => is_integer_literal(expr), + _ => false, + } +} + +#[cfg(test)] +#[cfg_attr(coverage_nightly, coverage(off))] +mod tests { + use super::*; + + /// The whole expansion for the smallest list that exercises every generated + /// item, negative codes included. + #[test] + fn generates_the_enum_and_the_three_items_over_it() { + let generated = generated(quote! { + /// How a trace buffer is read. + pub enum TraceDataType { + /// Eight little-endian bytes. + Int64 = 1, + Unnamed = -2, + } + }); + + for expected in [ + // The declaration reaches the output as written, doc comments and all, + // under the derives and the representation `code` casts through. + "# [doc = r\" How a trace buffer is read.\"] \ + # [derive (Debug , Clone , Copy , PartialEq , Eq)] # [repr (i32)] \ + pub enum TraceDataType { # [doc = r\" Eight little-endian bytes.\"] Int64 = 1 , \ + Unnamed = - 2 , }", + "pub const ALL : & 'static [Self] = & [Self :: Int64 , Self :: Unnamed ,] ;", + "pub const fn code (self) -> i32 { self as i32 }", + "pub const fn from_code (code : i32) -> Option < Self > \ + { match code { 1 => Some (Self :: Int64) , - 2 => Some (Self :: Unnamed) , \ + _ => None , } }", + ] { + assert!(generated.contains(expected), "missing {expected:?}"); + } + } + + /// The visibility is the caller's: a `pub` the macro supplied would be one the + /// declaration could not take back. + #[test] + fn keeps_the_declared_visibility() { + assert!( + generated(quote! { + enum Private { + One = 1, + } + }) + .contains("enum Private"), + "the expansion should not widen a private enum" + ); + } + + /// A variant with no code would take one from its position, which is the + /// mistake that silently renumbers a wire value. + #[test] + fn rejects_a_variant_without_a_code() { + let messages = messages(quote! { + pub enum Ordering { + Equal = 0, + Greater, + } + }); + + assert_eq!(messages.len(), 1, "{messages:?}"); + assert!(messages[0].contains("missing `= `"), "{messages:?}"); + } + + /// The two shapes a code is tempting to write as and cannot be: a constant's + /// path, and arithmetic. + #[test] + fn rejects_a_code_that_is_not_an_integer_literal() { + let messages = messages(quote! { + pub enum Ordering { + Equal = i32::MIN, + Greater = 1 + 1, + } + }); + + assert_eq!(messages.len(), 2, "{messages:?}"); + for message in &messages { + assert!(message.contains("must be an integer literal"), "{message}"); + } + } + + #[test] + fn rejects_a_variant_carrying_data() { + let messages = messages(quote! { + pub enum Ordering { + Equal(u8) = 0, + } + }); + + assert_eq!(messages.len(), 1, "{messages:?}"); + assert!(messages[0].contains("carry no data"), "{messages:?}"); + } + + /// `#[repr(i32)]` is rejected on a variantless enum, so the diagnostic has to + /// be this one rather than rustc's. + #[test] + fn rejects_an_enum_with_no_variants() { + let messages = messages(quote! { + pub enum Nothing {} + }); + + assert_eq!(messages.len(), 1, "{messages:?}"); + assert!(messages[0].contains("at least one variant"), "{messages:?}"); + } + + /// Every mistake in one build, as `host_functions!` reports a block. + #[test] + fn reports_every_mistake_in_the_list() { + let messages = messages(quote! { + pub enum Ordering { + Equal, + Greater = 1 + 1, + } + }); + + assert_eq!(messages.len(), 2, "{messages:?}"); + } + + #[test] + fn rejects_arguments() { + let error = expand(quote!(i64), quote! { pub enum Ordering { Equal = 0, } }) + .expect_err("expected the argument to be refused"); + + assert!(error.to_string().contains("takes no arguments")); + } + + #[test] + fn rejects_an_item_that_is_not_an_enum() { + expand(quote!(), quote! { pub struct Ordering; }) + .expect_err("expected a struct to be refused"); + } + + fn generated(item: TokenStream) -> String { + expand(quote!(), item) + .expect("the enum should expand") + .to_string() + } + + /// The messages of every diagnostic recorded by one failed `expand`. + fn messages(item: TokenStream) -> Vec { + let Err(error) = expand(quote!(), item) else { + panic!("expected expansion to fail"); + }; + error.into_iter().map(|error| error.to_string()).collect() + } +} diff --git a/crates/xrpl-host-functions-macros/src/lib.rs b/crates/xrpl-host-functions-macros/src/lib.rs index ccd674cccc..1ddabc68c6 100644 --- a/crates/xrpl-host-functions-macros/src/lib.rs +++ b/crates/xrpl-host-functions-macros/src/lib.rs @@ -1,5 +1,6 @@ #![cfg_attr(coverage_nightly, feature(coverage_attribute))] +mod enums; mod errors; mod glue; mod lowering; @@ -115,9 +116,9 @@ use parsed_host_function::ParsedHostFunction; /// generics: it maps to exactly one wasm import signature. Its parameters must be /// `i32`, `i64`, `u32`, `&[u8]`, `&mut [u8]`, `&str` or `TraceDataType`, and it /// must return `HostResult` if it writes an output region, -/// `HostResult` if it answers a value directly, or `HostResult<()>` if it -/// answers nothing. Two declarations may not share a `wasm_name`, nor collapse to -/// the same PascalCase variant. +/// `HostResult` or `HostResult` if it answers a value directly, +/// or `HostResult<()>` if it answers nothing. Two declarations may not share a +/// `wasm_name`, nor collapse to the same PascalCase variant. #[proc_macro] pub fn host_functions(input: proc_macro::TokenStream) -> proc_macro::TokenStream { expand(input.into()) @@ -125,6 +126,48 @@ pub fn host_functions(input: proc_macro::TokenStream) -> proc_macro::TokenStream .into() } +/// Declares an enum of wire codes, together with the `ALL`, `code` and +/// `from_code` set that must not fall behind its variants. +/// +/// The enum is written as an ordinary one — its own doc comment, its own +/// visibility, one `Variant = code,` per line — and the attribute supplies the +/// derives and the `#[repr(i32)]` that `code` casts through. Rust cannot enumerate +/// an enum's variants, so `ALL` is the only complete set a test can iterate. +/// +/// A variant carries no data and states its code as an integer literal, since that +/// literal is also the pattern `from_code` matches it by. What an unnamed code means +/// is the caller's to decide, being a different condition per enum. +/// +/// ``` +/// use xrpl_host_functions_macros::coded_enum; +/// +/// /// How a trace buffer is to be read. +/// #[coded_enum] +/// pub enum TraceDataType { +/// /// 8 little-endian bytes, rendered as a signed decimal. +/// Int64 = 1, +/// /// A 20-byte account ID, rendered as base58. +/// Account = 4, +/// } +/// +/// assert_eq!(TraceDataType::Account.code(), 4); +/// assert_eq!(TraceDataType::from_code(4), Some(TraceDataType::Account)); +/// assert_eq!(TraceDataType::from_code(2), None); +/// assert_eq!( +/// TraceDataType::ALL, +/// &[TraceDataType::Int64, TraceDataType::Account], +/// ); +/// ``` +#[proc_macro_attribute] +pub fn coded_enum( + args: proc_macro::TokenStream, + item: proc_macro::TokenStream, +) -> proc_macro::TokenStream { + enums::expand(args.into(), item.into()) + .unwrap_or_else(syn::Error::into_compile_error) + .into() +} + fn expand(input: TokenStream) -> syn::Result { let functions = parse_block(input)?; let abi = abi_items(&functions); diff --git a/crates/xrpl-host-functions-macros/src/lowering.rs b/crates/xrpl-host-functions-macros/src/lowering.rs index d6bcfa70fe..c89577c780 100644 --- a/crates/xrpl-host-functions-macros/src/lowering.rs +++ b/crates/xrpl-host-functions-macros/src/lowering.rs @@ -12,7 +12,7 @@ //! little-endian bytes, which is how the guest SDK passes a sequence number. //! - **`usize` and `i32` results are the same on the wire and not //! interchangeable**: the first is the length of what was written to an output -//! region, the second the answer itself. +//! region, the second the answer itself — as is `FloatOrdering`, a third spelling. //! //! Matching is on types as they are spelled — a proc macro resolves nothing, so //! `type Bytes = u32; … x: Bytes` is unrecognizable — but on a path's last @@ -54,7 +54,9 @@ pub(crate) enum ResultType { /// the engine turns into the wire's `i32` or into `BufferTooSmall` / /// `DataFieldTooLarge`. Never itself the wire type. BufferLength, - /// `i32`: the answer, from a function that writes no region. + /// `i32` or `FloatOrdering`: the answer, from a function that writes no region. + /// One variant for both — one wire result, and the declared type still reaches the + /// trait verbatim. Value, /// `()`: no wasm result at all — the call's whole effect is on the host, and /// an `Err` reaches the guest in no form. @@ -182,8 +184,9 @@ impl ResultType { /// refuses it against its own span. pub(crate) fn parse(success: &Type) -> syn::Result { const ALLOWED: &str = "a host function must return `HostResult` for a value it \ - writes to an output region, `HostResult` for one it answers \ - directly, or `HostResult<()>` for none at all"; + writes to an output region, `HostResult` or \ + `HostResult` for one it answers directly, or \ + `HostResult<()>` for none at all"; if let Type::Tuple(tuple) = success && tuple.elems.is_empty() @@ -193,7 +196,7 @@ impl ResultType { match last_path_segment(success) { Some(name) if name == "usize" => Ok(Self::BufferLength), - Some(name) if name == "i32" => Ok(Self::Value), + Some(name) if name == "i32" || name == "FloatOrdering" => Ok(Self::Value), _ => Err(syn::Error::new_spanned(success, ALLOWED)), } } @@ -395,13 +398,14 @@ mod tests { } } - /// The three success types, and the wasm result each becomes. `usize` and - /// `i32` agree on the wire and are separate rows. + /// The declared success types, and the wasm result each becomes. `usize` and `i32` + /// agree on the wire and are separate rows; `FloatOrdering` shares `i32`'s. #[test] fn lowers_every_success_type() { - let mapping: [(Type, ResultType, Option); 3] = [ + let mapping: [(Type, ResultType, Option); 4] = [ (parse_quote!(usize), ResultType::BufferLength, Some(I32)), (parse_quote!(i32), ResultType::Value, Some(I32)), + (parse_quote!(FloatOrdering), ResultType::Value, Some(I32)), (parse_quote!(()), ResultType::Nothing, None), ]; diff --git a/crates/xrpl-host-functions/src/lib.rs b/crates/xrpl-host-functions/src/lib.rs index 2861e3821e..64734de519 100644 --- a/crates/xrpl-host-functions/src/lib.rs +++ b/crates/xrpl-host-functions/src/lib.rs @@ -5,28 +5,25 @@ //! wasm engine registers from. //! //! The split: hand-written here is the vocabulary the declarations are written in — -//! [`HostError`], [`TraceDataType`], [`HostResult`], [`HASH_LEN`] — and everything -//! derived from the declarations is generated. The expansion names nothing this file -//! does not, so the two sides meet only in the block below. +//! [`HostError`], [`TraceDataType`], [`FloatOrdering`], [`HostResult`], [`HASH_LEN`] — +//! and everything derived from the declarations is generated. The expansion names +//! nothing this file does not, so the two sides meet only in the block below. //! //! Three items cross that split the other way, named by the expansion but by no //! declaration: [`WasmValType`], which the derived wasm signatures are spelled in, //! and `FromWasmRegion`/`FromWasmScalar`, which `wasmi_glue!` builds a marshalled //! argument through. -//! -//! So this file is lists — error codes, trace data types, functions. The `macro_rules!` -//! that expand the first two into enums live in `macros.rs`. #![no_std] #![cfg_attr(coverage_nightly, feature(coverage_attribute))] -#[macro_use] -mod macros; - // Not re-exported: the ABI is declared once, here, and this is the only call site. -use xrpl_host_functions_macros::host_functions; +use xrpl_host_functions_macros::{coded_enum, host_functions}; -host_errors! { +/// Error codes a host function may return. Every code is negative, which is what lets +/// a failure and an answer share one `i32` on the wire. +#[coded_enum] +pub enum HostError { Unimplemented = -1, FieldNotFound = -2, BufferTooSmall = -3, @@ -58,7 +55,12 @@ pub type HostResult = Result; /// A `sha512Half` digest: the first 32 bytes of a SHA-512, as XRPL uses it. pub const HASH_LEN: usize = 32; -trace_data_types! { +/// How [`HostFunctions::trace`] is to read its data buffer. Wire values shared with the +/// guest stdlib: append only, never renumber, and starting at 1 so a zeroed argument +/// names no type. `xrpl-wasm-vm-ffi` holds the second declaration, the one C++ compiles +/// against — this crate links into the guest too, so it cannot depend on `cxx`. +#[coded_enum] +pub enum TraceDataType { /// 8 little-endian bytes, rendered as a signed decimal. Int64 = 1, /// 8 little-endian bytes, rendered as an unsigned decimal. @@ -75,6 +77,17 @@ trace_data_types! { AsText = 7, } +/// The verdict [`HostFunctions::float_compare`] answers, read as the placing of `x` +/// against `y`. **Not C's `memcmp` convention**: the wire's negative range belongs to +/// [`HostError`], so every code here is non-negative — append only, never renumber. +/// `WasmCommon.h` holds the second declaration, as [`TraceDataType`] has one. +#[coded_enum] +pub enum FloatOrdering { + Equal = 0, + Greater = 1, + Less = 2, +} + /// The wasm module name a guest imports these functions under: /// `(import "host_lib" "ldgr_index" …)`. pub const HOST_MODULE: &str = "host_lib"; @@ -121,7 +134,8 @@ pub trait FromWasmScalar { // marshalled.** `&[u8]`/`&str` and `&mut [u8]` are `(ptr, len)` pairs, `TraceDataType` // is an `i32` code the engine names before a host sees it, and **`u32` is four // little-endian bytes in a region**, not a scalar, which is how the guest SDK passes a -// sequence number. +// sequence number. A result is `usize` for the length of what was written to an output +// region, `i32` or `FloatOrdering` for the answer itself, or `()` for none. host_functions! { /// The sequence number of the ledger being built, as 4 little-endian bytes. #[gas = 60] @@ -539,11 +553,11 @@ host_functions! { mode: i32, ) -> HostResult; - /// Compares floats `x` and `y`, returning a negative, zero, or positive scalar as - /// `x` is less than, equal to, or greater than `y`. + /// Compares floats `x` and `y`, answering the [`FloatOrdering`] that places `x` + /// against `y`. Reaches the guest as that variant's code, **not `memcmp`'s sign**. #[gas = 80] #[wasm_name = "float_cmp"] - fn float_compare(&self, x: &[u8], y: &[u8]) -> HostResult; + fn float_compare(&self, x: &[u8], y: &[u8]) -> HostResult; /// The float sum `x + y` under rounding `mode`. #[gas = 160] diff --git a/crates/xrpl-host-functions/src/macros.rs b/crates/xrpl-host-functions/src/macros.rs deleted file mode 100644 index b077526784..0000000000 --- a/crates/xrpl-host-functions/src/macros.rs +++ /dev/null @@ -1,102 +0,0 @@ -//! The `macro_rules!` behind the two hand-listed enums, [`crate::HostError`] and -//! [`crate::TraceDataType`]. -//! -//! Each takes one list of `Variant = code,` and expands the enum together with the -//! `ALL`/`code`/`from_code` set that must not fall behind it. The lists themselves stay -//! in `lib.rs`, beside the `host_functions!` block. - -/// Declares [`crate::HostError`] from one list: the variants, `HostError::ALL` and -/// `HostError::from_code`'s table all expand from the codes given. -/// -/// One list is what makes `ALL` complete. Rust cannot enumerate an enum's -/// variants — an exhaustive `match` forces an arm per variant but gives nothing to -/// iterate — so a hand-written `ALL` beside a hand-written enum could only be kept -/// in step by review, and `ALL`'s whole purpose is to be the set a test can trust. -/// A code added to the list gains its `ALL` entry and its `from_code` arm by -/// construction. `HostFunctionSpec::ALL` is complete the same way, from the -/// `host_functions!` block. -macro_rules! host_errors { - ($($(#[$doc:meta])* $variant:ident = $code:literal,)+) => { - /// Error codes a host function may return. - /// - #[derive(Debug, Clone, Copy, PartialEq, Eq)] - #[repr(i32)] - pub enum HostError { - $($(#[$doc])* $variant = $code,)+ - } - - impl HostError { - /// Every error a host function may return, in code order. - /// - /// The complete set, and complete by construction: a wasm engine's - /// split between the codes it hands the guest and the conditions it - /// traps on is a decision per variant, so the test that checks the - /// split iterates this and a code added to the ABI cannot slip past it. - pub const ALL: &'static [HostError] = &[$(HostError::$variant,)+]; - - /// The negative wire value a failed call returns. Every code but - /// `InternalFatal` is one a guest reads off that value. - #[inline] - pub const fn code(self) -> i32 { - self as i32 - } - - /// Reconstruct a `HostError` from its wire code. - /// - /// A code this ABI does not define is `InternalFatal`: an answer the - /// caller cannot act on is the call not having been served, and that is - /// the variant which says so. Positive values are not errors at all and go - /// the same way, since this is reached only once a negative return has - /// been read as a failure. - pub const fn from_code(code: i32) -> HostError { - match code { - $($code => HostError::$variant,)+ - _ => HostError::InternalFatal, - } - } - } - }; -} - -/// Declares [`crate::TraceDataType`] from one list, so `TraceDataType::ALL`, -/// `TraceDataType::code` and `TraceDataType::from_code` cannot fall behind the -/// variants — the reason `host_errors!` above is written this way. -macro_rules! trace_data_types { - ($($(#[$doc:meta])* $variant:ident = $code:literal,)+) => { - /// How [`HostFunctions::trace`] is to read its data buffer. - /// - /// The discriminants are wire values shared with the guest stdlib: append only, - /// never renumber. They start at 1, so a zeroed argument names no type rather - /// than the first one. - /// - /// This is the declaration a guest and a host both compile against. The host - /// side needs a second one — `cxx` cannot be a dependency here, since this - /// crate also links into the guest — so `xrpl-wasm-vm-ffi` declares a shared - /// enum for C++ and converts, exhaustively, from this. - #[derive(Debug, Clone, Copy, PartialEq, Eq)] - #[repr(i32)] - pub enum TraceDataType { - $($(#[$doc])* $variant = $code,)+ - } - - impl TraceDataType { - /// Every data type a guest may name, in code order. - pub const ALL: &'static [TraceDataType] = &[$(TraceDataType::$variant,)+]; - - /// The wire value a guest passes to name this type. - #[inline] - pub const fn code(self) -> i32 { - self as i32 - } - - /// The type `code` names, or `None`: the engine drops a call it cannot - /// read rather than guessing at a rendering the guest did not ask for. - pub const fn from_code(code: i32) -> Option { - match code { - $($code => Some(TraceDataType::$variant),)+ - _ => None, - } - } - } - }; -} diff --git a/crates/xrpl-host-functions/tests/generated_abi.rs b/crates/xrpl-host-functions/tests/generated_abi.rs index b782d07741..d6ce38dd06 100644 --- a/crates/xrpl-host-functions/tests/generated_abi.rs +++ b/crates/xrpl-host-functions/tests/generated_abi.rs @@ -1,14 +1,17 @@ //! Exercises the API that `host_functions!` generates, not the macro itself: //! the `HostFunctions` trait is implementable and callable both directly and -//! through `&dyn`, and the generated `HostFunctionSpec` and `TraceDataType` -//! tables agree with the declarations in `src/lib.rs`. The macro's own parsing -//! and diagnostics are covered by the unit tests in `xrpl-host-functions-macros`. +//! through `&dyn`, and the generated `HostFunctionSpec` table and the hand-listed +//! `TraceDataType` / `FloatOrdering` enums agree with the declarations in +//! `src/lib.rs`. The macro's own parsing and diagnostics are covered by the unit +//! tests in `xrpl-host-functions-macros`. use std::cell::RefCell; +use std::cmp::Ordering; use std::collections::HashSet; use xrpl_host_functions::{ - HASH_LEN, HostError, HostFunctionSpec, HostFunctions, HostResult, TraceDataType, WasmValType, + FloatOrdering, HASH_LEN, HostError, HostFunctionSpec, HostFunctions, HostResult, TraceDataType, + WasmValType, }; /// Records what it was asked to do; enough to prove the trait is usable. @@ -558,12 +561,18 @@ impl HostFunctions for FakeHost { put(out, &[mantissa as u8]) } - /// Reads two floats and returns a scalar; `InvalidParams` if either is empty. - fn float_compare(&self, x: &[u8], y: &[u8]) -> HostResult { - if x.is_empty() || y.is_empty() { + /// Reads two floats and answers a [`FloatOrdering`]; `InvalidParams` if either is + /// empty. First bytes only — what matters is that the verdict is a named variant. + fn float_compare(&self, x: &[u8], y: &[u8]) -> HostResult { + let (Some(x), Some(y)) = (x.first(), y.first()) else { return Err(HostError::InvalidParams); - } - Ok(i32::from(x[0]) - i32::from(y[0])) + }; + + Ok(match x.cmp(y) { + Ordering::Equal => FloatOrdering::Equal, + Ordering::Greater => FloatOrdering::Greater, + Ordering::Less => FloatOrdering::Less, + }) } /// A binary float operator; `InvalidParams` if either operand is empty. @@ -866,7 +875,14 @@ fn the_trait_is_implementable() { let mut exp = [0u8; 4]; assert_eq!(host.float_to_mant_exp(&[3; 8], &mut mant, &mut exp), Ok(2)); assert_eq!(host.float_from_mant_exp(5, 0, &mut out, 0), Ok(1)); - assert_eq!(host.float_compare(&[9; 8], &[4; 8]), Ok(5)); + assert_eq!( + host.float_compare(&[9; 8], &[4; 8]), + Ok(FloatOrdering::Greater) + ); + assert_eq!( + host.float_compare(&[4; 8], &[9; 8]), + Ok(FloatOrdering::Less) + ); assert_eq!( host.float_compare(&[], &[4; 8]), Err(HostError::InvalidParams) @@ -1141,6 +1157,41 @@ fn an_unnamed_trace_data_type_code_is_refused() { } } +/// `float_cmp`'s verdicts are what a guest branches on, so they are pinned as literals +/// here; `ALL` is in code order, so the round trip pins the discriminants too. Zero is +/// `Equal`, not reserved as in [`TraceDataType`]: a comparison always has an answer. +#[test] +fn every_float_ordering_survives_the_wire() { + let codes: Vec = FloatOrdering::ALL.iter().map(|o| o.code()).collect(); + + assert_eq!(codes, [0, 1, 2]); + for &ordering in FloatOrdering::ALL { + assert_eq!(FloatOrdering::from_code(ordering.code()), Some(ordering)); + } +} + +/// **The property that keeps a verdict from being read as a failure.** A verdict and an +/// error code share one `i32`, split by sign, so no ordering may be negative however the +/// enum is extended — `Less` is `2`, not `memcmp`'s `-1`, which is `Unimplemented`. +#[test] +fn no_float_ordering_collides_with_an_error_code() { + for &ordering in FloatOrdering::ALL { + assert!(ordering.code() >= 0, "{ordering:?} is negative"); + } + + assert_eq!(HostError::from_code(-1), Some(HostError::Unimplemented)); + assert_eq!(FloatOrdering::from_code(-1), None); +} + +/// A value no variant names is refused rather than rounded to a neighbouring verdict: a +/// total order has exactly three outcomes, and the negative codes are `HostError`'s. +#[test] +fn an_unnamed_float_ordering_code_is_refused() { + for code in [-1, 3, 5, i32::MAX, i32::MIN] { + assert_eq!(FloatOrdering::from_code(code), None, "code {code}"); + } +} + /// `ALL` is what a wasm engine iterates to register imports, so no two declarations /// may collapse to the same wire name. The table above pins membership and order; /// this adds only uniqueness, and restates nothing. diff --git a/crates/xrpl-host-functions/tests/host_errors.rs b/crates/xrpl-host-functions/tests/host_errors.rs index 7e77fcdc56..a9716be501 100644 --- a/crates/xrpl-host-functions/tests/host_errors.rs +++ b/crates/xrpl-host-functions/tests/host_errors.rs @@ -1,5 +1,5 @@ -//! Exercises what `host_errors!` generates: the wire codes, the set -//! [`HostError::ALL`] names, and the round trip between them. +//! Exercises what `#[coded_enum]` generates for [`HostError`]: the wire codes, the +//! set [`HostError::ALL`] names, and the round trip between them. //! //! The codes are consensus input — they are what a guest reads off a failed host //! call — so they are pinned here as literals and derived everywhere else. @@ -78,25 +78,18 @@ fn every_code_but_the_sentinel_is_in_the_shared_range() { #[test] fn every_wire_code_round_trips_back_to_its_error() { for &error in HostError::ALL { - assert_eq!(HostError::from_code(error.code()), error, "{error:?}"); + assert_eq!(HostError::from_code(error.code()), Some(error), "{error:?}"); } } -/// A code from outside the set is `InternalFatal`: a host answering something this ABI -/// does not define has not served the call, whatever it meant by it, and success is not -/// an error at all. +/// A code from outside the set names no error and is not rounded to a neighbouring one; +/// what it means instead is the crossing's to decide, in `xrpl-wasm-vm-ffi`'s `host_error`. /// -/// `-21` is the code xrpld would append next, so it is the one that decides whether a -/// list this crate has not caught up with reaches a guest or stops the run. `i32::MIN + -/// 1` is next to the sentinel and unassigned, which is what makes the sentinel a value -/// rather than a range. +/// `-21` is the code xrpld would append next; `i32::MIN + 1` is next to the sentinel and +/// unassigned, which is what makes the sentinel a value rather than a range. #[test] -fn a_code_outside_the_set_is_internal_fatal() { +fn a_code_outside_the_set_names_no_error() { for code in [-21, i32::MIN + 1, 0, 1, i32::MAX] { - assert_eq!( - HostError::from_code(code), - HostError::InternalFatal, - "{code}" - ); + assert_eq!(HostError::from_code(code), None, "{code}"); } } diff --git a/crates/xrpl-wasm-vm-ffi/src/lib.rs b/crates/xrpl-wasm-vm-ffi/src/lib.rs index 25f50cf302..28022be661 100644 --- a/crates/xrpl-wasm-vm-ffi/src/lib.rs +++ b/crates/xrpl-wasm-vm-ffi/src/lib.rs @@ -31,7 +31,7 @@ use std::any::Any; use std::panic::{AssertUnwindSafe, catch_unwind}; -use xrpl_host_functions::{HostError, HostFunctions, HostResult, TraceDataType}; +use xrpl_host_functions::{FloatOrdering, HostError, HostFunctions, HostResult, TraceDataType}; use xrpl_wasm_vm::{CheckError, RunError, RunFailure, RunOutcome, check, run}; /// [`guarded`] must be able to stop an unwind. Under `panic = "abort"` it cannot, @@ -554,6 +554,15 @@ struct CxxHost<'a> { ctx: &'a ffi::HostContext, } +/// The error a negative code names, or `InternalFatal`. +/// +/// **The one place the fallback is decided.** A code outside the ABI is xrpld's +/// `HostFunctionError` list having outrun this one — the call was not served, whatever +/// the host meant by it, so the run stops on the one code that says so. +fn host_error(n: i32) -> HostError { + HostError::from_code(n).unwrap_or(HostError::InternalFatal) +} + /// A byte-producing call's answer: the value's true length, or its error code. /// /// The conversion *is* the sign test — it fails on exactly the negative values — so @@ -563,7 +572,7 @@ struct CxxHost<'a> { /// involved — `i32`, `Result`, `HostError` — is foreign to this crate, so the orphan /// rule forbids the impl. fn bytes_written(n: i32) -> HostResult { - usize::try_from(n).map_err(|_| HostError::from_code(n)) + usize::try_from(n).map_err(|_| host_error(n)) } /// The ABI's data type as the shared enum C++ was given a definition of. @@ -587,11 +596,21 @@ fn crossed(data_type: TraceDataType) -> ffi::TraceDataType { /// a non-negative value is that answer, a negative one its error code. fn scalar(n: i32) -> HostResult { if n < 0 { - return Err(HostError::from_code(n)); + return Err(host_error(n)); } Ok(n) } +/// A call whose answer is a named verdict: [`scalar`]'s split first, then the code must +/// name a variant. +/// +/// **This is where the C++ side is held to the ABI.** A code naming no variant is +/// `WasmCommon.h`'s `FloatOrdering` having drifted from this one — nothing a contract can +/// act on, hence `InternalFatal` and a stopped run. +fn float_ordering(n: i32) -> HostResult { + FloatOrdering::from_code(scalar(n)?).ok_or(HostError::InternalFatal) +} + impl HostFunctions for CxxHost<'_> { fn get_ledger_sqn(&self, out: &mut [u8]) -> HostResult { bytes_written(self.ctx.get_ledger_sqn(out)) @@ -904,8 +923,8 @@ impl HostFunctions for CxxHost<'_> { bytes_written(self.ctx.float_from_mant_exp(mantissa, exponent, mode, out)) } - fn float_compare(&self, x: &[u8], y: &[u8]) -> HostResult { - scalar(self.ctx.float_compare(x, y)) + fn float_compare(&self, x: &[u8], y: &[u8]) -> HostResult { + float_ordering(self.ctx.float_compare(x, y)) } fn float_add(&self, x: &[u8], y: &[u8], out: &mut [u8], mode: i32) -> HostResult { @@ -1125,6 +1144,70 @@ mod tests { assert_eq!(crossed.result, 0, "a failed run returned no value"); } + /// Every code a guest can be handed comes back as the error that produced it, not as + /// a neighbouring one. + #[test] + fn every_wire_code_crosses_back_as_its_error() { + for &error in HostError::ALL { + assert_eq!(host_error(error.code()), error, "{error:?}"); + } + } + + /// A code from outside the set is `InternalFatal`, the fallback this crate owns. + /// + /// `-21` is the code xrpld would append next; `i32::MIN + 1` is next to the sentinel + /// and unassigned, which is what makes the sentinel a value rather than a range. The + /// non-negative codes reach here only once the sign has been read elsewhere. + #[test] + fn a_code_outside_the_set_is_internal_fatal() { + for code in [-21, i32::MIN + 1, 0, 1, i32::MAX] { + assert_eq!(host_error(code), HostError::InternalFatal, "{code}"); + } + } + + /// The split every scalar answer crosses on: non-negative is the value, negative is + /// the code that names why there is none. + #[test] + fn a_scalar_splits_its_answer_from_its_error_on_the_sign() { + assert_eq!(scalar(0), Ok(0)); + assert_eq!(scalar(7), Ok(7)); + assert_eq!(scalar(-19), Err(HostError::FloatInputMalformed)); + } + + /// `float_cmp`'s three verdicts survive the crossing as themselves. + #[test] + fn every_verdict_crosses_back_as_itself() { + for &ordering in FloatOrdering::ALL { + assert_eq!( + float_ordering(ordering.code()), + Ok(ordering), + "{ordering:?}" + ); + } + } + + /// **Where the two `FloatOrdering` declarations are held together**, rather than a + /// contract being handed a `3` that matches none of its three branches. `0` is not in + /// this set: it is `Equal`, not an absent answer. + #[test] + fn a_code_naming_no_verdict_is_internal_fatal() { + for code in [3, 4, 99, i32::MAX] { + assert_eq!( + float_ordering(code), + Err(HostError::InternalFatal), + "code {code}" + ); + } + } + + /// An error still crosses as an error, and does not become the drift sentinel: the + /// sign is read before the code is matched against the variants. + #[test] + fn a_refused_comparison_keeps_its_own_error() { + assert_eq!(float_ordering(-19), Err(HostError::FloatInputMalformed)); + assert_eq!(float_ordering(-1), Err(HostError::Unimplemented)); + } + /// The `RunError` set as the test *expects* it, not as the conversion reports it: /// deriving it from the code under test would make the assertion vacuous. fn every_run_error() -> Vec { diff --git a/crates/xrpl-wasm-vm/src/abi.rs b/crates/xrpl-wasm-vm/src/abi.rs index 1b38280422..03142d223e 100644 --- a/crates/xrpl-wasm-vm/src/abi.rs +++ b/crates/xrpl-wasm-vm/src/abi.rs @@ -343,7 +343,7 @@ mod tests { use crate::vm::TRANSFER_LIMIT_BYTES; use std::cell::Cell; use wasmi::StoreLimitsBuilder; - use xrpl_host_functions::TraceDataType; + use xrpl_host_functions::{FloatOrdering, TraceDataType}; /// `charge_transfer` takes the store data, which has to hold a host. struct UncalledHost; @@ -632,7 +632,7 @@ mod tests { ) -> HostResult { unreachable!("no unit test in this module calls the host") } - fn float_compare(&self, _x: &[u8], _y: &[u8]) -> HostResult { + fn float_compare(&self, _x: &[u8], _y: &[u8]) -> HostResult { unreachable!("no unit test in this module calls the host") } fn float_add( diff --git a/crates/xrpl-wasm-vm/src/register.rs b/crates/xrpl-wasm-vm/src/register.rs index 9df719cf77..ee7412dc4f 100644 --- a/crates/xrpl-wasm-vm/src/register.rs +++ b/crates/xrpl-wasm-vm/src/register.rs @@ -647,7 +647,9 @@ impl HostFunctionBodies for Bodies { ) -> CallResult { let memory = guest_memory(caller)?; let host = caller.data().host; - Ok(host.float_compare(x.read(memory)?, y.read(memory)?)?) + // `FloatOrdering`'s codes are non-negative, which is what lets the verdict share + // this `i32` with a negative `HostError`. + Ok(host.float_compare(x.read(memory)?, y.read(memory)?)?.code()) } fn float_add( diff --git a/crates/xrpl-wasm-vm/tests/host_calls.rs b/crates/xrpl-wasm-vm/tests/host_calls.rs index b984133a7d..27e6184273 100644 --- a/crates/xrpl-wasm-vm/tests/host_calls.rs +++ b/crates/xrpl-wasm-vm/tests/host_calls.rs @@ -8,7 +8,7 @@ use support::{ COMPLETED, EMPTY_REGION, FakeHost, ONE_PAGE, Trace, code, failure, import, module, run, status, traced, }; -use xrpl_host_functions::{HASH_LEN, HostError, TraceDataType}; +use xrpl_host_functions::{FloatOrdering, HASH_LEN, HostError, TraceDataType}; use xrpl_wasm_vm::RunError; /// A value the host writes must be readable by the guest at the pointer it gave, @@ -1038,23 +1038,61 @@ fn float_to_mant_exp_with_a_short_exponent_region_writes_neither() { assert_eq!(status(&wat, &host), 0, "neither region should be written"); } -/// A comparison that reads two float regions and returns a scalar verdict, no output -/// region involved. +/// A comparison that reads two float regions and answers with no output region involved: +/// the host's [`FloatOrdering`] reaches the guest as its code. #[test] fn float_cmp_reads_both_and_returns_the_verdict() { - let host = FakeHost::new().answering_float_compare(Ok(-1)); + let host = FakeHost::new().answering_float_compare(Ok(FloatOrdering::Less)); let wat = module( &[import::FLOAT_CMP, ONE_PAGE], "(call $float_cmp (i32.const 0) (i32.const 8) (i32.const 8) (i32.const 8))", ); - assert_eq!(status(&wat, &host), -1, "the comparison verdict"); + assert_eq!( + status(&wat, &host), + FloatOrdering::Less.code(), + "the comparison verdict" + ); assert_eq!( *host.float_compare_asked.borrow(), vec![(vec![0u8; 8], vec![0u8; 8])] ); } +/// Every verdict lowers to its own code, so a guest reads the ordering the host named and +/// not a sibling. +#[test] +fn every_float_cmp_verdict_reaches_the_guest_unchanged() { + for &verdict in FloatOrdering::ALL { + let host = FakeHost::new().answering_float_compare(Ok(verdict)); + + let wat = module( + &[import::FLOAT_CMP, ONE_PAGE], + "(call $float_cmp (i32.const 0) (i32.const 8) (i32.const 8) (i32.const 8))", + ); + assert_eq!(status(&wat, &host), verdict.code(), "{verdict:?}"); + } +} + +/// The other half of that `i32`: a refused comparison is a negative code, which no verdict +/// can be mistaken for. +#[test] +fn float_cmp_error_reaches_the_guest_as_a_negative_code() { + let host = FakeHost::new().answering_float_compare(Err(HostError::FloatInputMalformed)); + + let wat = module( + &[import::FLOAT_CMP, ONE_PAGE], + "(call $float_cmp (i32.const 0) (i32.const 8) (i32.const 8) (i32.const 8))", + ); + let code = status(&wat, &host); + assert_eq!(code, HostError::FloatInputMalformed.code()); + assert_eq!( + FloatOrdering::from_code(code), + None, + "an error code must not read back as a verdict" + ); +} + /// A binary operator that reads two float regions and a mode, and writes the result: /// both operands and the mode reach the host, tagged by operator. #[test] diff --git a/crates/xrpl-wasm-vm/tests/support/mod.rs b/crates/xrpl-wasm-vm/tests/support/mod.rs index 03e15d1812..ef5bf58abe 100644 --- a/crates/xrpl-wasm-vm/tests/support/mod.rs +++ b/crates/xrpl-wasm-vm/tests/support/mod.rs @@ -10,7 +10,7 @@ use std::cell::RefCell; use std::collections::HashMap; -use xrpl_host_functions::{HostError, HostFunctions, HostResult, TraceDataType}; +use xrpl_host_functions::{FloatOrdering, HostError, HostFunctions, HostResult, TraceDataType}; use xrpl_wasm_vm::{RunFailure, RunOutcome}; /// The entry point every test module exports. @@ -379,7 +379,7 @@ pub struct FakeHost { /// Every `(mantissa, exponent, mode)` `float_from_mant_exp` was asked for. pub float_from_mant_exp_asked: RefCell>, /// What `float_compare` answers, whatever floats it is given. - pub float_compare_answer: HostResult, + pub float_compare_answer: HostResult, /// Every `(x, y)` `float_compare` was asked for. pub float_compare_asked: RefCell, Vec)>>, /// Every `(x, y, mode)` the four binary float operators were asked for, tagged by @@ -505,7 +505,7 @@ impl Default for FakeHost { float_mant_exp_answer: (vec![0u8; 8], vec![0u8; 4]), float_to_mant_exp_asked: RefCell::new(Vec::new()), float_from_mant_exp_asked: RefCell::new(Vec::new()), - float_compare_answer: Ok(0), + float_compare_answer: Ok(FloatOrdering::Equal), float_compare_asked: RefCell::new(Vec::new()), float_binary_ops_asked: RefCell::new(Vec::new()), float_unary_ops_asked: RefCell::new(Vec::new()), @@ -890,7 +890,7 @@ impl FakeHost { self } - pub fn answering_float_compare(mut self, answer: HostResult) -> FakeHost { + pub fn answering_float_compare(mut self, answer: HostResult) -> FakeHost { self.float_compare_answer = answer; self } @@ -1444,7 +1444,7 @@ impl HostFunctions for FakeHost { self.float_answer.fill(out) } - fn float_compare(&self, x: &[u8], y: &[u8]) -> HostResult { + fn float_compare(&self, x: &[u8], y: &[u8]) -> HostResult { self.float_compare_asked .borrow_mut() .push((x.to_vec(), y.to_vec())); diff --git a/include/xrpl/tx/wasm/HostFunc.h b/include/xrpl/tx/wasm/HostFunc.h index fc8133abb7..83a6e50c45 100644 --- a/include/xrpl/tx/wasm/HostFunc.h +++ b/include/xrpl/tx/wasm/HostFunc.h @@ -42,7 +42,7 @@ floatToMantExpImpl(Slice const& x); std::expected floatFromMantExpImpl(int64_t mantissa, int32_t exponent, int32_t mode); -std::expected +std::expected floatCompareImpl(Slice const& x, Slice const& y); std::expected @@ -439,7 +439,7 @@ public: return std::unexpected(HostFunctionError::Unimplemented); } - [[nodiscard]] [[nodiscard]] virtual std::expected + [[nodiscard]] virtual std::expected floatCompare(Slice const& x, Slice const& y) const { return std::unexpected(HostFunctionError::Unimplemented); diff --git a/include/xrpl/tx/wasm/HostFuncImpl.h b/include/xrpl/tx/wasm/HostFuncImpl.h index d7d999005f..e6cbb20187 100644 --- a/include/xrpl/tx/wasm/HostFuncImpl.h +++ b/include/xrpl/tx/wasm/HostFuncImpl.h @@ -274,7 +274,7 @@ public: std::expected floatFromMantExp(int64_t mantissa, int32_t exponent, int32_t mode) const override; - std::expected + std::expected floatCompare(Slice const& x, Slice const& y) const override; std::expected diff --git a/include/xrpl/tx/wasm/WasmCommon.h b/include/xrpl/tx/wasm/WasmCommon.h index 421dd84b29..0dee73853b 100644 --- a/include/xrpl/tx/wasm/WasmCommon.h +++ b/include/xrpl/tx/wasm/WasmCommon.h @@ -56,6 +56,16 @@ enum class HostFunctionError : int32_t { InternalFatal = std::numeric_limits::min(), }; +// The verdict `floatCompare` answers, read as the placing of `x` against `y`. Wire values +// shared with the guest: append only, never renumber, never negative — a verdict and a +// `HostFunctionError` share one `i32`, split by sign. The second declaration of +// `xrpl_host_functions::FloatOrdering`, which links into the guest and so cannot use `cxx`. +enum class FloatOrdering : int32_t { + Equal = 0, + Greater = 1, + Less = 2, +}; + template struct WasmResult { @@ -160,6 +170,12 @@ hfErrorToInt(HostFunctionError e) return static_cast(e); } +constexpr int32_t +floatOrderingToInt(FloatOrdering o) +{ + return static_cast(o); +} + template std::invoke_result_t guarded( diff --git a/src/libxrpl/tx/wasm/HostContext.cpp b/src/libxrpl/tx/wasm/HostContext.cpp index 6e0212c8f3..d301f165d7 100644 --- a/src/libxrpl/tx/wasm/HostContext.cpp +++ b/src/libxrpl/tx/wasm/HostContext.cpp @@ -347,7 +347,16 @@ invoke(Functor&& functor) return hfErrorToInt(value.error()); } - return *value; + // `FloatOrdering` is the one answer not already an `i32`, and a scoped enum does not + // convert on its own. Its codes are non-negative, so the two returns stay distinct. + if constexpr (std::is_enum_v>) + { + return static_cast(*value); + } + else + { + return *value; + } } // A traced integer, which the guest sends as bytes rather than as a wasm scalar so that one diff --git a/src/libxrpl/tx/wasm/HostFuncImplFloat.cpp b/src/libxrpl/tx/wasm/HostFuncImplFloat.cpp index 4ec93eb2d9..cde9187d16 100644 --- a/src/libxrpl/tx/wasm/HostFuncImplFloat.cpp +++ b/src/libxrpl/tx/wasm/HostFuncImplFloat.cpp @@ -256,7 +256,7 @@ floatFromMantExpImpl(int64_t mantissa, int32_t exponent, int32_t mode) } } -std::expected +std::expected floatCompareImpl(Slice const& x, Slice const& y) { try @@ -271,10 +271,10 @@ floatCompareImpl(Slice const& x, Slice const& y) if (!yy) return std::unexpected(HostFunctionError::FloatInputMalformed); if (*xx < *yy) - return 2; + return FloatOrdering::Less; if (*xx == *yy) - return 0; - return 1; + return FloatOrdering::Equal; + return FloatOrdering::Greater; } // LCOV_EXCL_START catch (...) @@ -459,7 +459,7 @@ WasmHostFunctionsImpl::floatFromMantExp(int64_t mantissa, int32_t exponent, int3 return wasm_float::floatFromMantExpImpl(mantissa, exponent, mode); } -std::expected +std::expected WasmHostFunctionsImpl::floatCompare(Slice const& x, Slice const& y) const { return wasm_float::floatCompareImpl(x, y); diff --git a/src/tests/libxrpl/tx/wasm/fixtures/MockHostFunctions.h b/src/tests/libxrpl/tx/wasm/fixtures/MockHostFunctions.h index 024713d1f7..f44f6dd60e 100644 --- a/src/tests/libxrpl/tx/wasm/fixtures/MockHostFunctions.h +++ b/src/tests/libxrpl/tx/wasm/fixtures/MockHostFunctions.h @@ -387,7 +387,7 @@ struct MockHostFunctions : HostFunctions (const, override)); MOCK_METHOD( - (std::expected), + (std::expected), floatCompare, (Slice const& x, Slice const& y), (const, override)); diff --git a/src/tests/libxrpl/tx/wasm/host_context/FloatCompare.cpp b/src/tests/libxrpl/tx/wasm/host_context/FloatCompare.cpp index 381dc58e6f..3912ded7e0 100644 --- a/src/tests/libxrpl/tx/wasm/host_context/FloatCompare.cpp +++ b/src/tests/libxrpl/tx/wasm/host_context/FloatCompare.cpp @@ -24,9 +24,24 @@ struct FloatCompareCall : HostContextTest TEST_F(FloatCompareCall, XAndYAreForwardedResultReturnedDirectly) { EXPECT_CALL(host, floatCompare(BytesAre("cmp-x"), BytesAre("cmp-yy"))) - .WillOnce(testing::Return(1)); + .WillOnce(testing::Return(FloatOrdering::Greater)); - EXPECT_EQ(hostContext.floatCompare(bytesOf(x), bytesOf(y)), 1); + EXPECT_EQ( + hostContext.floatCompare(bytesOf(x), bytesOf(y)), + floatOrderingToInt(FloatOrdering::Greater)); +} + +// This layer is where `FloatOrdering` stops being a type and becomes the `i32` a contract +// reads. Every variant, so a mis-lowered one cannot hide behind a sibling. +TEST_F(FloatCompareCall, EveryVerdictIsLoweredToItsWireCode) +{ + for (auto const verdict : {FloatOrdering::Equal, FloatOrdering::Greater, FloatOrdering::Less}) + { + EXPECT_CALL(host, floatCompare(BytesAre("cmp-x"), BytesAre("cmp-yy"))) + .WillOnce(testing::Return(verdict)); + + EXPECT_EQ(hostContext.floatCompare(bytesOf(x), bytesOf(y)), floatOrderingToInt(verdict)); + } } TEST_F(FloatCompareCall, HostErrorBecomesContractReturnValue) @@ -56,9 +71,12 @@ TEST_F(FloatCompareCall, HostExceptionBecomesInternalFatalAndIsLogged) TEST_F(FloatCompareCall, OddSizedOperandReachesHostUnchanged) { Bytes const oddX{0x2a}; - EXPECT_CALL(host, floatCompare(testing::_, BytesAre("cmp-yy"))).WillOnce(testing::Return(0)); + EXPECT_CALL(host, floatCompare(testing::_, BytesAre("cmp-yy"))) + .WillOnce(testing::Return(FloatOrdering::Equal)); - EXPECT_EQ(hostContext.floatCompare(bytesOf(oddX), bytesOf(y)), 0); + EXPECT_EQ( + hostContext.floatCompare(bytesOf(oddX), bytesOf(y)), + floatOrderingToInt(FloatOrdering::Equal)); } } // namespace xrpl::test diff --git a/src/tests/libxrpl/tx/wasm/host_functions/FloatCompare.cpp b/src/tests/libxrpl/tx/wasm/host_functions/FloatCompare.cpp index 21acd3f915..916e0bb4cb 100644 --- a/src/tests/libxrpl/tx/wasm/host_functions/FloatCompare.cpp +++ b/src/tests/libxrpl/tx/wasm/host_functions/FloatCompare.cpp @@ -23,17 +23,40 @@ TEST_F(FloatCompareImpl, MalformedInputs) TEST_F(FloatCompareImpl, Less) { - expectValue(makeHost()->floatCompare(slice(FloatTest::kIntMin), slice(FloatTest::kIntZero)), 2); + expectValue( + makeHost()->floatCompare(slice(FloatTest::kIntMin), slice(FloatTest::kIntZero)), + FloatOrdering::Less); } TEST_F(FloatCompareImpl, Greater) { - expectValue(makeHost()->floatCompare(slice(FloatTest::kIntMax), slice(FloatTest::kIntZero)), 1); + expectValue( + makeHost()->floatCompare(slice(FloatTest::kIntMax), slice(FloatTest::kIntZero)), + FloatOrdering::Greater); } TEST_F(FloatCompareImpl, Equal) { - expectValue(makeHost()->floatCompare(slice(FloatTest::kOne), slice(FloatTest::kOne)), 0); + expectValue( + makeHost()->floatCompare(slice(FloatTest::kOne), slice(FloatTest::kOne)), + FloatOrdering::Equal); +} + +// The wire codes a contract branches on, pinned as literals because that is what the guest +// compiles against — `FloatOrdering` is declared twice, so each side pins its own numbers +// as `HostFunctionError` already does. Non-negativity is the invariant, not an accident of +// the numbering: `Less` is `2`, not `memcmp`'s `-1`, which is `Unimplemented`. +TEST_F(FloatCompareImpl, VerdictCodesAreTheOnesTheGuestReads) +{ + EXPECT_EQ(floatOrderingToInt(FloatOrdering::Equal), 0); + EXPECT_EQ(floatOrderingToInt(FloatOrdering::Greater), 1); + EXPECT_EQ(floatOrderingToInt(FloatOrdering::Less), 2); + + for (auto const verdict : {FloatOrdering::Equal, FloatOrdering::Greater, FloatOrdering::Less}) + { + EXPECT_GE(floatOrderingToInt(verdict), 0); + EXPECT_NE(floatOrderingToInt(verdict), hfErrorToInt(HostFunctionError::Unimplemented)); + } } // A non-canonical encoding of 10 (mantissa 100000, exponent -4) is normalized on decode, so @@ -42,7 +65,9 @@ TEST_F(FloatCompareImpl, NonCanonicalNormalizes) { Bytes const nonCanonicalTen{ 0x00, 0x00, 0x00, 0x00, 0x00, 0x01, 0x86, 0xA0, 0xFF, 0xFF, 0xFF, 0xFC}; - expectValue(makeHost()->floatCompare(slice(nonCanonicalTen), slice(FloatTest::kTen)), 0); + expectValue( + makeHost()->floatCompare(slice(nonCanonicalTen), slice(FloatTest::kTen)), + FloatOrdering::Equal); } } // namespace xrpl::test