core: add chain Error → TxStatusError conversion

[mina86]

Having conversion from near_chain_primitves::Error to TxStatusError
eliminates a handful of trivial map_error calls.

[matklad]

I think I am -0 here: this changes makes it much harder to answer the "where does TxStatusError::ChainError originates", as I can't grep for this specific error variant, and rather need to comment-out the from impl and rebuild the code.

If we are doing this, consider maybe just thiserroring it then?

[matklad]

Oh dear, is this a messed-up Display?

[mina86]

I think I am -0 here: this changes makes it much harder to answer the "where does TxStatusError::ChainError originates", as I can't grep for this specific error variant, and rather need to comment-out the from impl and rebuild the code.

Why is TxStatusError different from any other error which has the conversion implemented?

If we are doing this, consider maybe just thiserroring it then?

I’d do that in separate PR since there are already many existing implementations which should all be converted.

[matklad]

Why is TxStatusError different from any other error which has the conversion implemented?

It's not: I'd say we in general overuse From for errors, and that we could benefit from removing some of the Froms in favor of explicitly tagging the variants. I did spend some time hunting for hidden call-sites of particular From impls to understand where a particular error originates (aka, guideline 2 from https://kazlauskas.me/entries/errors)

But it's not like we have explictliy decided that we in general should avoid froms, so yeah, I can see this as an improvement to consistency at the expense of readability.

Tangentially, but InvalidTx(InvalidTxError) seems to be a dead variant?

[mina86]

aka, guideline 2 from https://kazlauskas.me/entries/errors

I don’t find the arguments for the guideline presented there convincing at all. Having explicit map_err does not help in keeping the first guideline. I can just as easily violate it regardless of how I convert objects between error types. And if an error does need additional context, adding fields which cannot be derived from than that makes implementation of From impossible in the first place so the second guideline is moot.

For finding where the error originates, I get that but at the same time, this is Rust where all the types are inferred. What is the criteria here to decide when we use From and when we don’t? Should we also annotate all types in let statements so usage of types is easier to find?

[matklad]

What is the criteria here to decide when we use From and when we don’t?

I don't have hard and fast criteria here, but in this particular case two rules of thumb stand out for me:

• This is "using generics in non-generic context". At the call-site, we don't take advantage of genericness, of some Foo: From<Bar>, we know concrete types of Foo and Bar
• There's typically a lot of usages of any particular type, but the most important usages are those that create new instances, constructors. If you know just all the sites where constructors are called, you get a rough feeling of how the type flows from the system.

For From in applications, this gets specialized to "don't use From". I almost always prefer explicit name constructors over From, and only reach for impl From<Foo> from Bar when I need to pass Foo into a generic T: Into<Bar> function.

The principle here is not "any kind of inference is bad", it's more specific:

• obscuring construction site
• global inference: unlike type inference in function bodies, trait resolution is fundamentally build from a global search of impls.

As usual, don't have strong feelings here :)