Runtime: Don't convert syscall errors

[arajasek]

filecoin-project/ref-fvm#1020

Open questions:

• Is it fine to make the runtime always depend on the SDK? (and less importantly, for test_vm to do so?
  • No, we're just gonna move the relevant types to shared (Refactor: Move Response from SDK to shared ref-fvm#1445)
• EVM: Is converting the SyscallError into an ActorError at the interpreter's system level fine (for now)? I'm guessing we'll want to actually propagate and handle differently at the callsites (or at least do more nuanced handling here), but that can be deferred so long as this isn't a regression.
• We can reduce the code duplication in the test runtimes by having them do the same thing that the "actual" FVM runtime does (and moving that logic into the trait itself). The only loss is some information in test failures (since ErrorNumbers don't contain message strings).
  • @Stebalien is in favour of making this change, so gonna do that.

[anorth]

I didn't see the introduction of send_generalised, but when this comes to master I will strongly argue for collapsing to a single send method (possibly with a trait helper with default impl that fills in the flags). I think different return types from different methods is technical debt we should not take. Instead, we should do the work to update existing callers to the new, better way. That will solve the duplication item.

Given the current package architecture, we should not add fvm_sdk as a dependency of crates that don't use the actual FVM. Instead, move the required types to fvm_shared, along with many similar. I could be convinced that a different package architecture would make sense in which the SDK is always a dependency, but then would want to see things moved back the other way so there's a consistent decision logic for what lives where.

[anorth]

This TODO isn't helpful - there's nothing concretely to do or a condition upon which to do it. It's fine that gas limit is ignored, just leave a comment without TODO.

[Stebalien]

I didn't see the introduction of send_generalised, but when this comes to master I will strongly argue for collapsing to a single send method (possibly with a trait helper with default impl that fills in the flags).

That's how it's currently implemented:

builtin-actors/runtime/src/runtime/mod.rs

Lines 186 to 200 in 610f399

	/// Sends a message to another actor, returning the exit code and return value envelope.
	/// If the invoked method does not return successfully, its state changes
	/// (and that of any messages it sent in turn) will be rolled back.
	/// Note that the current return type cannot distinguish between a successful invocation
	/// that returns an error code, and an error originating from the syscall prior to
	/// invoking the target actor/method.
	fn send(
	&self,
	to: &Address,
	method: MethodNum,
	params: Option<IpldBlock>,
	value: TokenAmount,
	) -> Result<Option<IpldBlock>, ActorError> {
	self.send_generalized(to, method, params, value, None, SendFlags::empty())
	}

We wrote it this way to reduce the diff between the next branch and master and to avoid any unnecessary refactors while diverged.

Once we merge, I'd be strongly in favor of removing the separate method and/or cleaning this up. However, I couldn't directly port these changes over to master because they rely on features that don't exist in FVM 2.0.

[Stebalien]

Suggested change

	exit_code: res.clone().map_or_else(|e| e.exit_code(), |_| ExitCode::OK),
	exit_code: res.as_ref().err().map(|e| e.exit_code()).unwrap_or(ExitCode::OK),

[Stebalien]

Suggested change

	return_data: res.map_or_else(|mut e| e.take_data(), |r| r),
	return_data: res.unwrap_or_else(|mut e| e.take_data()),