refactor: add MarketError

[emizzle]

Add MarketError and convert all EthersErrors (ProviderError, SignerError) to MarketError

Note

Based on branch of PR #662.

[markspanbroek]

Very nice, thanks @emizzle!

[markspanbroek]

This one should probably also be wrapped in convertEthersError?

[dryajov]

Wouldn't it be better/safer to return a Result instead of converting to another exception?

[emizzle]

Wouldn't it be better/safer to return a Result instead of converting to another exception?

@dryajov Well, that's an interesting yet difficult question to answer, as we've discussed in the past. Would you prefer that Result is returned here?

Given that there's only one place that catches MarketError as of right now, I don't necessarily see the benefit yet, however, if there were to be more call sites that would indeed need to catch a MarketError in the future, then I agree it could be beneficial.

If we changed to return a Result now, every existing call site would need to be updated, and likely, in most instances, the error returned in the Result would simply be logged. Otherwise, the function containing the call site would also need to return a Result, spreading it further up the stack. I'm certainly not opposed to the spread, but it would be a fair amount of change, so maybe it's a good idea if we (and others) agree on it before the work is undertaken?

[dryajov]

If we changed to return a Result now, every existing call site would need to be updated, and likely, in most instances, the error returned in the Result would simply be logged. Otherwise, the function containing the call site would also need to return a Result, spreading it further up the stack. I'm certainly not opposed to the spread, but it would be a fair amount of change, so maybe it's a good idea if we (and others) agree on it before the work is undertaken?

Sure, if the change cascades too many other changes, it might not be worth it.

Overall I've since come around with the usage of result. I think the argument in are still valid (from both ends of the spectrum), but specifically to the ergonomics of result, things have improved (with ? and questionable). I believe that recent versions of chronos now also support ? in async code, which should make our async code a lot less verbose. All in all, results are easier to track and reason about.

[emizzle]

Sure, if the change cascades too many other changes, it might not be worth it.

@dryajov I had a look at this, and I think for now, the changes this would introduce would be too many for the benefit. Perhaps we should create an issue and handle this another time?