Public EnvBase functions should be infallible in Guest

[dmkozh]

Currently the SDK implements EnvBase functions and exposes them publicly as is. However, these functions are declared as returning Result<>, so this interface creates a false impression that these functions can return an error. This has been confusing enough to even slip into our own tests (

rs-soroban-env/soroban-test-wasms/wasm-workspace/linear_memory/src/lib.rs

Line 50 in 7435744

assert!(e.vec_unpack_to_slice(vec, &mut short_buf).is_err());

).

We should hide EnvBase and add public infallible wrappers around its functions.

[leighmcculloch]

I think the issue needs addressing in the soroban-env repo, because the same issue occurs in the soroban-env-guest. We shouldn't fix this in a way that only fixes the problem in the SDK and not there also. Any changes in the SDK should just be downstream updates that need applying after fixing this in the env.

[dmkozh]

I don't think we can make the interface infallible because it can be fallible depending on implementation. But we also shouldn't expose the fallible functions via SDK's Env.

[leighmcculloch]

Ah interesting, so I was going to say EnvBase isn't exported from the SDK and so contract developers should never see it or know about it so from the SDK interface pov it doesn't matter. However, it looks like we did export the EnvBase recently to get access to some EnvBase functionality in generated code.

I'll create an issue to rehide it, as it's all intended to be internal functionality.

I still think this issue is primarily concerned with the guest, which is probably going to become an advanced low-level SDK in the future.

[leighmcculloch]

Opened:

• EnvBase is exposed in docs rs-soroban-sdk#1171

[dmkozh]

Hiding the EnvBase is what this issue basically suggests; I just thought some of these methods might be useful to expose from Env as well (as infallible). If we don't think that's the case, we could probably update the tests to use the appropriate SDK wrappers.

[leighmcculloch]

The methods should be exposed on SDK types yeah, I created an issue to refactor that in:

• EnvBase is exported but shouldn't be rs-soroban-sdk#1173

But the infallible vs fallible issue is still relevant I think and specifically seems like a problem in the guest. This exact issue exists in the guest today:

these functions are declared as returning Result<>, so this interface creates a false impression

[dmkozh]

Yeah, I guess that's a fair concern if someone uses guest. However, I don't have a good idea as to how this can be fixed. Maybe we should generate the infallible guest wrapper and hide the fallible versions. But I'm not sure we have time for that now.

[leighmcculloch]

The interface for the guest doesn't have to be locked in today, as I think it makes no API guarantees. I'll transfer this issue to env and make it about the guest, and we can revisit in the future.

[graydon]

I think EnvBase is a bit of a red herring here. All soroban_env_commom::Env methods are also "technically fallible", returning Result<..., EnvBase::Error> which has 3 significantly-different ways it gets used:

1. Result<..., Infallible> when building against Guest: the type system is telling you it's impossible to observe Err(..) because your enclosing VM will trap before you see it.
2. Result<..., HostError> when building against Host natively for local testing, where the methods get wrapped in an SDK method that rejects any error with reject_err in the SDK that calls EnvBase::escalate_error_to_panic and unwinds, transforming Result<..., HostError> into Result<..., Infallible> and thereby emulating the Guest mode.
3. Result<..., HostError> when being called from another host function, including but not limited to a native contract or a unit test. In all these cases we actually want to observe Err(...) values, log them, inspect them, return them, etc.

The SDK doesn't (visibly?) re-export EnvBase but it also doesn't (visibly?) re-export the fallible soroban_env_common::Env interface. It exports its own soroban_sdk::Env type, which has inherent methods that are not fallible, and also has a whole other set of wrapper types it provides (it doesn't expose most of the types used by, or functions mentioned in, the soroban_env_common::Env interface at all).

But we can (and some of our tests do) explicitly import soroban_env_common::Env and use it to furnish that interface on the SDK's Env (eg. look at tests that use both the SDK and soroban_env_common::Env as _, such as hostile.rs)

[leighmcculloch]

@graydon So in the example at the top of the issue where .is_err() is being called on a fn that'll never return error, that's a Result<_, Infallible>?

It's a shame that clippy isn't reporting that we're calling is_err on a infallible.

[graydon]

@leighmcculloch Yes, it's a Result<_, Infallible> (you can check by inserting a temporary variable to hold the result, and annotating it with that type).

It's a shame that clippy isn't reporting that we're calling is_err on a infallible.

Yeah (not even with --target=wasm32-unknown-unknown which was my first guess: enable Guest mode in the first place). Even https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021 requires nightly and an exhaustive_patterns setting to notice the Err(...) is unreachable. If you look at how we implement unwrap_infallible we do something quite weird and manual, matching the unreachable Err(never:Infallible) constructor and then destructuring it into an empty match never {}, which rustc finally looks at and says "oh, yeah, that's the never-type".

WHO KNOWS.

[graydon]

(given we're pretty much unable to "fix" this, closing)