Remaining host code review

soroban-env-host/src/e2e_invoke.rs

@@ -4,16 +4,6 @@
 /// host functions.
 use std::{cmp::max, rc::Rc};
 
-use soroban_env_common::{
-    xdr::{
-        AccountId, ContractDataDurability, ContractEventType, DiagnosticEvent, HostFunction,
-        LedgerEntry, LedgerEntryData, LedgerFootprint, LedgerKey, LedgerKeyAccount,
-        LedgerKeyContractCode, LedgerKeyContractData, LedgerKeyTrustLine, ScErrorCode, ScErrorType,
-        SorobanAuthorizationEntry, SorobanResources, TtlEntry,
-    },
-    Error,
-};
-
 use crate::{
     budget::{AsBudget, Budget},
     events::Events,
@@ -25,7 +15,13 @@ use crate::{
         metered_xdr::{metered_from_xdr_with_budget, metered_write_xdr},
     },
     storage::{AccessType, Footprint, FootprintMap, SnapshotSource, Storage, StorageMap},
-    DiagnosticLevel, Host, HostError, LedgerInfo, MeteredOrdMap,
+    xdr::{
+        AccountId, ContractDataDurability, ContractEventType, DiagnosticEvent, HostFunction,
+        LedgerEntry, LedgerEntryData, LedgerFootprint, LedgerKey, LedgerKeyAccount,
+        LedgerKeyContractCode, LedgerKeyContractData, LedgerKeyTrustLine, ScErrorCode, ScErrorType,
+        SorobanAuthorizationEntry, SorobanResources, TtlEntry,
+    },
+    DiagnosticLevel, Error, Host, HostError, LedgerInfo, MeteredOrdMap,
 };
 
 pub type TtlEntryMap = MeteredOrdMap<Rc<LedgerKey>, Rc<TtlEntry>, Budget>;
@@ -101,8 +97,12 @@ pub fn get_ledger_changes<T: SnapshotSource>(
     // We return any invariant errors here as internal errors, as they would
     // typically mean inconsistency between storage and snapshot that shouldn't
     // happen in embedder environments, or simply fundamental invariant bugs.
-    let internal_error: HostError =
-        Error::from_type_and_code(ScErrorType::Storage, ScErrorCode::InternalError).into();
+    let internal_error = || {
+        HostError::from(Error::from_type_and_code(
+            ScErrorType::Storage,
+            ScErrorCode::InternalError,
+        ))
+    };
     for (key, entry_with_live_until_ledger) in storage.map.iter(budget)? {
         let mut entry_change = LedgerEntryChange::default();
         metered_write_xdr(budget, key.as_ref(), &mut entry_change.encoded_key)?;
@@ -129,14 +129,14 @@ pub fn get_ledger_changes<T: SnapshotSource>(
 
             if let Some(ref mut ttl_change) = &mut entry_change.ttl_change {
                 ttl_change.old_live_until_ledger =
-                    old_live_until_ledger.ok_or_else(|| internal_error.clone())?;
+                    old_live_until_ledger.ok_or_else(internal_error)?;
             }
         }
         if let Some((_, new_live_until_ledger)) = entry_with_live_until_ledger {
             if let Some(ref mut ttl_change) = &mut entry_change.ttl_change {
                 // Never reduce the final live until ledger.
                 ttl_change.new_live_until_ledger = max(
-                    new_live_until_ledger.ok_or_else(|| internal_error.clone())?,
+                    new_live_until_ledger.ok_or_else(internal_error)?,
                     ttl_change.old_live_until_ledger,
                 );
             }
@@ -155,7 +155,7 @@ pub fn get_ledger_changes<T: SnapshotSource>(
                 }
             }
             None => {
-                return Err(internal_error);
+                return Err(internal_error());
             }
         }
         changes.push(entry_change);

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

@@ -1,18 +1,13 @@
 use std::rc::Rc;
 
-use soroban_env_common::{
-    xdr::{Hash, ScBytes, ScString, ScVal, StringM},
-    Error, Symbol, SymbolSmall,
-};
-
 use crate::{
+    events::{
+        internal::{InternalDiagnosticArg, InternalDiagnosticEvent},
+        InternalEvent, InternalEventsBuffer,
+    },
     host::metered_clone::{MeteredAlloc, MeteredClone, MeteredContainer, MeteredIterator},
-    Host, HostError, Val,
-};
-
-use super::{
-    internal::{InternalDiagnosticArg, InternalDiagnosticEvent},
-    InternalEvent, InternalEventsBuffer,
+    xdr::{Hash, ScBytes, ScString, ScVal, StringM},
+    Error, Host, HostError, Symbol, SymbolSmall, Val,
 };
 
 #[derive(Clone, Default)]
@@ -43,7 +38,7 @@ impl Host {
         Ok(())
     }
 
-    pub fn log_diagnostics(&self, msg: &str, args: &[Val]) {
+    pub(crate) fn log_diagnostics(&self, msg: &str, args: &[Val]) {
         self.with_debug_mode(|| {
             let calling_contract = self.get_current_contract_id_opt_internal()?;
             let log_sym = SymbolSmall::try_from_str("log")?;
@@ -97,10 +92,16 @@ impl Host {
         })
     }
 
-    // Emits an event with topic = ["fn_call", called_contract_id, function_name] and
-    // data = [arg1, args2, ...]
-    // Should called prior to opening a frame for the next call so the calling contract can be inferred correctly
-    pub fn fn_call_diagnostics(&self, called_contract_id: &Hash, func: &Symbol, args: &[Val]) {
+    // Emits an event with topic = ["fn_call", called_contract_id,
+    // function_name] and data = [arg1, args2, ...]. Should called prior to
+    // opening a frame for the next call so the calling contract can be inferred
+    // correctly
+    pub(crate) fn fn_call_diagnostics(
+        &self,
+        called_contract_id: &Hash,
+        func: &Symbol,
+        args: &[Val],
+    ) {
         self.with_debug_mode(|| {
             let calling_contract = self.get_current_contract_id_opt_internal()?;
             Vec::<InternalDiagnosticArg>::charge_bulk_init_cpy(3, self)?;
@@ -121,7 +122,7 @@ impl Host {
 
     // Emits an event with topic = ["fn_return", function_name] and
     // data = [return_val]
-    pub fn fn_return_diagnostics(&self, contract_id: &Hash, func: &Symbol, res: &Val) {
+    pub(crate) fn fn_return_diagnostics(&self, contract_id: &Hash, func: &Symbol, res: &Val) {
         self.with_debug_mode(|| {
             Vec::<InternalDiagnosticArg>::charge_bulk_init_cpy(2, self)?;
             let topics = vec![

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

@@ -1,21 +1,18 @@
 use std::rc::Rc;
 
-use soroban_env_common::{BytesObject, VecObject};
-
-use super::{Events, HostEvent};
 use crate::{
     budget::AsBudget,
+    events::{Events, HostEvent},
     host::metered_clone::{MeteredClone, MeteredContainer, MeteredIterator},
-    xdr,
-    xdr::ScVal,
-    Host, HostError, Val,
+    xdr::{self, ScVal},
+    BytesObject, Host, HostError, Val, VecObject,
 };
 
-/// The internal representation of a `ContractEvent` that is stored in the events buffer
-/// and designed to be cheap to clone.
+/// The internal representation of a `ContractEvent` that is stored in the
+/// events buffer and designed to be cheap to clone.
 // This is exposed as a pub type for benches.
 #[derive(Clone, Debug)]
-pub struct InternalContractEvent {
+pub(crate) struct InternalContractEvent {
     pub type_: xdr::ContractEventType,
     pub contract_id: Option<BytesObject>,
     pub topics: VecObject,
@@ -57,7 +54,7 @@ impl InternalContractEvent {
 ///      (eg. validating a host function call's input arguments).
 
 #[derive(Clone, Debug)]
-pub struct InternalDiagnosticEvent {
+pub(crate) struct InternalDiagnosticEvent {
     pub contract_id: Option<crate::xdr::Hash>,
     pub topics: Vec<InternalDiagnosticArg>,
     pub args: Vec<InternalDiagnosticArg>,
@@ -76,6 +73,9 @@ impl std::hash::Hash for InternalEvent {
                 c.data.get_payload().hash(state);
                 c.topics.to_val().get_payload().hash(state);
             }
+            // These are not included in the hash because they not supposed to
+            // be observable, and we only have hashing to support
+            // test-observation.
             InternalEvent::Diagnostic(_) => (),
         }
     }
@@ -98,7 +98,7 @@ impl std::hash::Hash for EventError {
 // an ScVal to avoid wasting CPU in the standard case where nobody is going to
 // observe the event anyway.
 #[derive(Clone, Debug)]
-pub enum InternalDiagnosticArg {
+pub(crate) enum InternalDiagnosticArg {
     HostVal(Val),
     XdrVal(ScVal),
 }
@@ -153,7 +153,7 @@ impl InternalDiagnosticEvent {
 /// The internal representation of an `Event` that is stored in the events buffer
 /// and designed to be cheap to clone.
 #[derive(Clone, Debug)]
-pub enum InternalEvent {
+pub(crate) enum InternalEvent {
     Contract(InternalContractEvent),
     Diagnostic(Rc<InternalDiagnosticEvent>),
 }
@@ -167,13 +167,12 @@ pub(crate) enum EventError {
 /// The events buffer. Stores `InternalEvent`s in the chronological order.
 #[derive(Clone, Default)]
 pub(crate) struct InternalEventsBuffer {
-    //the bool keeps track of if the call this event was emitted in failed
     pub(crate) vec: Vec<(InternalEvent, EventError)>,
 }
 
 impl InternalEventsBuffer {
     // Records an InternalEvent
-    pub fn record(&mut self, e: InternalEvent, host: &Host) -> Result<(), HostError> {
+    pub(crate) fn record(&mut self, e: InternalEvent, host: &Host) -> Result<(), HostError> {
         let mut metered_internal_event_push = |e: InternalEvent| -> Result<(), HostError> {
             // Metering: we use the cost of instantiating a size=1 `Vec` as an
             // estimate for the cost `Vec.push(event)`.  Because the buffer length
@@ -193,35 +192,39 @@ impl InternalEventsBuffer {
         Ok(())
     }
 
-    /// Rolls back the event buffer starting at `events`.
-    pub fn rollback(&mut self, events: usize) -> Result<(), HostError> {
+    /// "Rolls back" the event buffer starting at `events` by marking all
+    /// subsequent events as failed calls.
+    pub(crate) fn rollback(&mut self, events: usize) -> Result<(), HostError> {
         // note that we first skip the events that are not being rolled back
-        // Metering: free
+        // Metering: free (or conceptually: paid for when pushing the event)
         for e in self.vec.iter_mut().skip(events) {
             e.1 = EventError::FromFailedCall;
         }
 
         Ok(())
     }
 
-    /// Converts the internal events into their external representation. This should only be called
-    /// either when the host is finished (via `try_finish`), or when an error occurs.
-    pub fn externalize(&self, host: &Host) -> Result<Events, HostError> {
-        // This line is intentionally unmetered. We want to separate out charging
-        // the main budget for `Contract` events (with "observable" costs) from
-        // charging the debug budget for `Diagnostic` events (with "non-observable"
-        // costs). Both event types are stored in the same input vector so we must
-        // not do a bulk charge based on its length, but rather walk through it
-        // charging to one budget or the other on an event-by-event basis.
+    /// Converts the internal events into their external representation. This
+    /// should only be called either when the host is finished (via
+    /// `try_finish`), or when an error occurs.
+    pub(crate) fn externalize(&self, host: &Host) -> Result<Events, HostError> {
+        // This line is intentionally unmetered. We want to separate out
+        // charging the main budget for `Contract` events (with "observable"
+        // costs) from charging the debug budget for `Diagnostic` events (with
+        // "non-observable" costs). Both event types are stored in the same
+        // input vector so we must not do a bulk charge based on its length, but
+        // rather walk through it charging to one budget or the other on an
+        // event-by-event basis.
         let mut vec = Vec::with_capacity(self.vec.len());
 
         let mut metered_external_event_push =
             |event: xdr::ContractEvent, status: &EventError| -> Result<(), HostError> {
-                // Metering: we use the cost of instantiating a size=1 `Vec` as an estimate
-                // for the cost collecting 1 `HostEvent` into the events buffer. Because
-                // the resulting buffer length may be different on different instances
-                // (due to diagnostic events) and we need a deterministic cost across all
-                // instances, the cost needs to be amortized and buffer size-independent.
+                // Metering: we use the cost of instantiating a size=1 `Vec` as
+                // an estimate for the cost collecting 1 `HostEvent` into the
+                // events buffer. Because the resulting buffer length may be
+                // different on different instances (due to diagnostic events)
+                // and we need a deterministic cost across all instances, the
+                // cost needs to be amortized and buffer size-independent.
                 Vec::<HostEvent>::charge_bulk_init_cpy(1, host)?;
                 vec.push(HostEvent {
                     event,

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

@@ -1,22 +1,19 @@
 pub(crate) mod diagnostic;
 mod internal;
 pub(crate) mod system_events;
-
 pub(crate) use internal::{
     EventError, InternalDiagnosticArg, InternalDiagnosticEvent, InternalEventsBuffer,
 };
 // expose them as pub use for benches
-pub use internal::{InternalContractEvent, InternalEvent};
-use soroban_env_common::{
+use crate::{
     num::{i256_from_pieces, u256_from_pieces},
     xdr::{
         ContractEventBody, ContractEventType, ContractExecutable, PublicKey::PublicKeyTypeEd25519,
         ScAddress, ScContractInstance, ScVal,
     },
-    Error, Val, VecObject,
+    Error, Host, HostError, Val, VecObject,
 };
-
-use crate::{Host, HostError};
+pub(crate) use internal::{InternalContractEvent, InternalEvent};
 
 /// The external representation of a host event.
 #[derive(Clone, Debug)]

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

@@ -4,7 +4,7 @@ use soroban_env_common::{xdr, EnvBase, Symbol, TryFromVal, TryIntoVal, Val, VecO
 const CONTRACT_EXECUTABLE_UPDATE_TOPIC: &str = "executable_update";
 
 impl Host {
-    pub fn system_event(&self, topics: VecObject, data: Val) -> Result<(), HostError> {
+    pub(crate) fn system_event(&self, topics: VecObject, data: Val) -> Result<(), HostError> {
         self.record_contract_event(xdr::ContractEventType::System, topics, data)?;
         Ok(())
     }