Electric-Coin-Company / zcash-swift-wallet-sdk

iOS light client Framework proof-of-concept
MIT License
37 stars 33 forks source link

[#1035] Create ZcashError.unknown #1036

Closed LukasKorba closed 1 year ago

LukasKorba commented 1 year ago

Closes #1035

This code review checklist is intended to serve as a starting point for the author and reviewer, although it may not be appropriate for all types of changes (e.g. fixing a spelling typo in documentation). For more in-depth discussion of how we think about code review, please see Code Review Guidelines.

Author

Reviewer

Chlup commented 1 year ago

My main objection to this is that it defeats the purpose of errors having locality and being pin pointed to there they occurred.

Even a more local but yet generic error has some value of providing clients some code location.

Being reported an unknown error would be undesirable to us, because: 1- our app throws unknown errors. 2- we don't get any information beforehand that the user can capture to fix it.

I agree. Problem is that our SDK emits and throws Error type. Not ZcashError. It may be possible to change error types in streams. But it's impossible to specify error type when throwing error from method. So we need some easy way how to convert Error type to ZcashError. Because in fact all the errors generated by the SDK are ZcashError. Without unknown error there will be some Optional handling when doing Error -> ZcashError conversion. unknown is there to not need to handle Optionals when doing that conversion. And it should never be used directly. Like no function should explicitly throw ZcashError.unknown. It will be used only in case that we made a mistake and somewhere is thrown something else than ZcashError. And it will be used only in case that client uses toZcashError() function.

I checked the PR and toZcashError() is wrongly implemented due to misunderstanding. Implementation should looks like this:

func toZcashError() -> ZcashError {
        if let zcashError = self as? ZcashError {
            return zcashError
        } else {
            return ZcashError.unknown(self)
        }
    }
LukasKorba commented 1 year ago

Main reason we added this is to use TCA actions

case someFailure(ZcashError)

so we need to ensure ZcashError is possible to pass from a throwing function. If it's not a ZcashError, we don't have a tool to wrap it from the app side.

pacu commented 1 year ago

I guess we don't have much wiggle room here and will have to live with it.