Separate EnvBase testutils functions into a separate trait

[leighmcculloch]

We should break EnvBase up into two traits, one that is non-testutils functionality, and one that is testutils functionality.

So that when dependencies import the soroban-env-host with testutils enabled, or not enabled, the function set of EnvBase doesn't change, which requires upstream dependencies to propogate the testutils feature upwards.

This issue was encountered in stellar/rs-soroban-sdk#1476:

Features on traits, like EnvBase, inherently create situations where dependencies can get stuck in situations where they are incompatible because another dep might be dependent on the trait in the opposite state. We should probably revisit EnvBase's liberal use of features, breaking it up into separate traits. Rust features should always be, where possible, additively compatible.

cc @stellar/contract-committers @fnando

[graydon]

I've done a sketch of this in #1623 but I don't actually understand the motivation. What do you mean by "incompatible" and why isn't the same thing a problem with a conditional trait?

[leighmcculloch]

What do you mean by "incompatible" and why isn't the same thing a problem with a conditional trait?

Doing my best to remember what had happened...

The problem with feature-gated methods on a trait is that Rust's Cargo unifies features across the entire dependency graph. When any crate in your dependency tree enables a feature on a shared dependency, that feature is enabled for everyone.

For gated functions on a trait, this means every user of that trait needs to implement exactly the functions that match the feature unification, which is impossible for every dependency to predict since it is no single dependency on its own can guarantee to control what features get enabled on another crate, because of feature unification.

Today EnvBase looks different depending on whether testutils is enabled:

// Without testutils
pub trait EnvBase {
    fn check_val_integrity(&self, val: Val) -> Result<(), Self::Error>;
    fn bytes_copy_from_slice(&self, ...) -> ...;
    // ... core methods only
}

// With testutils
pub trait EnvBase {
    fn check_val_integrity(&self, val: Val) -> Result<(), Self::Error>;
    fn bytes_copy_from_slice(&self, ...) -> ...;
    // ... core methods
    fn escalate_error_to_panic(&self, e: Self::Error) -> !;  // NEW
    fn env_call_hook(&self, ...) -> ...;                      // NEW
    fn env_ret_hook(&self, ...) -> ...;                       // NEW
}

Consider this dependency scenario:

my-app
├── soroban-sdk (without testutils)
│   └── soroban-env-common (EnvBase used without testutils)
└── soroban-ledger-snapshot (with testutils)
    └── soroban-env-host (testutils enabled)
        └── soroban-env-common (testutils enabled)

Cargo unifies soroban-env-common's features, so testutils becomes enabled for the entire build. Now soroban-sdk's Env type must implement escalate_error_to_panic, env_call_hook, etc. - methods it didn't expect to need to implement because the soroban-sdk had been built without its own testutils feature.

Historically this wasn't a problem, but in the following PRs storage functions on the Host were moved behind the soroban-env-host's testutil feature. The soroban-ledger-snapshot crate was updated to require testutils on soroban-env-host. And that made the soroban-ledger-snapshot crate unusable with other crates also using the soroban-env-host and using EnvBase that were not always requiring the testutils feature

• Add support for automatic restoration for the 'recording' mode. #1523
  • ☝🏻 Included a refactor removing pub fn with_mut_storage that the soroban-ledger-snapshots crate used.
• Add test utilities to compensate hiding the storage interface. #1556
  • ☝🏻 Exported a new fn get_ledger_entries but only behind testutils.
• Bump XDR and env to v23.0.0-rc.1.1 rs-soroban-sdk#1467
  • ☝🏻 Updated soroban-ledger-entries to always require the soroban-env-host with testutils..

Separate traits shouldn't present the same problem, since when the soroban-env-host has testutils enabled the EnvBase trait won't change, meaning any crate using that trait won't need to manually configured to match.

I guess this comes back to the Rust principle that features should be additive. Adding functions to a trait with a feature while it sounds additive, can't actually be coordinated as additive across multiple crates.