refactor: don't abuse serialization for type erasure #5813
Conversation
1ca6c1b to
6db8389
Compare
| } | ||
| } | ||
|
|
||
| trait AnyEq: Any + Send + Sync { |
There was a problem hiding this comment.
Does this truly need to be using Any? Could std::error::Error be utilized? It also supports downcasting and is generally probably more accurately conveys the intent of what's happening here.
There was a problem hiding this comment.
Good question!
On the one hand, yes, this could use std::error::Error, and that does seem like a better fit (it's an error!). On the other hand, the intention here is subtly different -- we are not going to use this as a dynamic error, the sole purpose here is to downcast this to a concrete repr down the line. In this sense, Any is a better, because it does strictly what we need, and nothing more.
Perhaps more important thing here is practicality -- if I use Error here, than I'd have to write Display for ExternalError, but that would effectively be dead code.
So, I'd say let's keep this as Any. But thanks for the sugestions -- using Error didn't occure to me!
|
|
||
| #[derive(Debug, Clone, PartialEq, Eq)] | ||
| #[derive(Debug, PartialEq, Eq)] |
There was a problem hiding this comment.
The PartialEq and Eq impls here don't appear to be used at all. Is there any reason why we need to keep these implementations in place?
Comparing errors for equality (as opposed to matching their structure) in general seems like a pretty weird thing to need.
There was a problem hiding this comment.
If these are dropped, then the whole AnyEq thing could be replaced with just Box<dyn std::error::Error + Send + Sync> fields. Which is pretty common way to do this kind of type erasure in Rust.
EDIT: or Box<dyn Any + Send + Sync> I guess.
There was a problem hiding this comment.
These are used in our tests 😭 And yeah, this is also problematic, as it, eg, prevents us from using io::Error. I do want to fix that (by rewriting our test), but not in this PR.
vm-logic is "generic" over External implementation. We don't want however to literally add a type parameter to it, so we use `dyn External`. This creates a problem that, when a particular external fails, we need to report the error, but we don't know error's concrete types. So let's use `Box<dyn Any>` for that, and a bit of down-casting. The previous impl did roughly equivalent thing, but via serialize/deserialize route, which is more complicated than what we need here. As a result, we eliminate a couple of BorshSerializes and clones.
…ror can take through runtime (#5837) This started as pure refactor to remove duplicate logic, but it uncovered a surprising behavior, at least in theory. It seems today runtime can upgrade `TreNodeMissing` error into `InconsistentStateError`, is this expected? Builds on top of #5813 and can be viewed per commit. The actuall change is the last commit, the stuff before are pure refactors which make the change more obvious. Previously, there were two different code paths an StorageError could take: Path 1: * RuntimeExt emits StorageError * RuntimeExt calls wrap_storage_error to wrap it into VMLogicError::ExternalError * near_vm_runner translates that into VMError::ExternalError * actions.rs matches on AnyError and emits a StorageError Path 2 * RuntimeExt emits StorageError * RuntimeExt wraps that into VMLogicError::InconsistentStateError. Note that this state is especially weird: `InconsistentStateError` is a *subset* of `StorageError`. But here RuntimeExt treats *any* storage error as InconsistentStateError. * near_vm_runner translates that into VMError::InconsistentStateError * actions.rs translates that into `StorageError::InconsistentStateError` Note in particular how on the path 2 we can start with `StorageError::TrieNodeMissing` and end with `StorageError::StorageInconsistentState("TrieNodeMissing")`, which feels like a bug maybe? Turns out, that the second path is actually dead code, so we can safely remove it, what we actually do in this PR.
vm-logic is "generic" over External implementation. We don't want
however to literally add a type parameter to it, so we use
dyn External. This creates a problem that, when a particular externalfails, we need to report the error, but we don't know error's concrete
types. So let's use
Box<dyn Any>for that, and a bit of down-casting.The previous impl did roughly equivalent thing, but via
serialize/deserialize route, which is more complicated than what we need
here.
As a result, we eliminate a couple of BorshSerializes and clones.