Handling `null` cases on methods like getTransactionReceipt

[XertroV]

One thing I noticed the other day is that currently getTransactionReceipt will return a TransactionReceipt, but this isn't entirely consistent with the spec.

Particularly, either a TransactionReceipt or null can be returned. I presume there will be some other methods like this too (such as getBlock with a block number that doesn't exist yet).

The immediate solution that occurs to me is using a NullOrUndefined wrapper around methods like this. But that will likely include many of the methods available.

Web3 is built on Aff which can handle exceptions.

Do we want exceptions to be thrown when things like null are returned? (Note: not sure of current behaviour); it feels like it'd be better to me to handle these events through NullOrUndefined instead - lots of catch functions isn't particularly idiomatic (we have a great type system, how do we use it best?)

One thing I've ended up doing is catching errors from JS to detect whether the ETH node is available or not. It's not super pretty but works okay.

Interested on any thoughts on how we should handle this sort of stuff before I make a PR, or whether anything needs to be done at all.

[XertroV]

@blinky3713 @kejace any thoughts on the above?

Currently the error thrown when querying a nonexistent txid is (NonEmptyList (NonEmpty (ErrorAtIndex 0 (TypeMismatch "object" "object")) Nil)) which I definitely think can be improved upon.

As a side note should runWeb3 include exception :: EXCEPTION in the row of effects?

[martyall]

@XertroV Our error handling is somewhere between non-existent and terrible.

we don't need EXCEPTION in the row of effects because Aff has it already. that being said, a parse error is not even really an exception, even though we're treating it like that. What you're describing is effectively something like a null pointer exception, and probably should throw a more helpful message.

we welcome any improvements to the error handling stuff, big or small.

Part of the problem is also that we implemented the rpc layer as fast as possible, ideally one would be using an rpc library that knows about the error codes, etc. i think it would be nice to ultimately do something like that to separate the layers that the error can be coming from.

[XertroV]

Hmm. Going to have a think re error handling then.

Naively it feels like NullOrUndefined would be nice there. It depends on whether you're looking for finer grained errors or not.

Another philosophy might be that everything should be wrapped in a Maybe (or Either) since we're getting it from a type-unsafe source, even for those RPC commands that should always return something (like eth_version). The reality is if we don't catch the errors in the library then any error will cause an exception in someone's code, though, and I think that's worse, particularly because the error messages aren't readable at all.

Theoretically we could just change runWeb3 to something like runWeb3 :: Web3 _ a -> Aff _ (Maybe a) and only have to add one catchException/catchError instance.

[cmditch]

Indeed, I ran into a similar issue. Not to keep bringing up elm-web3, but it seems quite relevant considering the goals of each library are the same: interact with Ethereum using a rich and strict type system. The Maybe route was the safest and made the most sense to me. Using FP, the tradeoff is often more explicitness (more code, or at least more function parameters) for more safety (less bugs). Coding is much more fun than debugging, so I don't mind this tradeoff.

I ended up wrapping everything that might return null with Maybe https://github.com/cmditch/elm-web3/blob/master/src/Web3/Eth.elm#L287

I'm only now just beginning to use the library in "production", so I haven't the best feel for the developer experience around this. Especially since I have to use Elm's equivalent to Aff which is Task. This has the unfortunate effect of wrapping all the Maybe's in an Either (or Result in Elm world) https://github.com/cmditch/elm-web3/blob/master/test/src/Pages/Eth.elm#L138 This is kind of ghastly and might very well change.

[martyall]

I'm going to say a few things about what i consider the ideal solution to be. Here are the list of errors that I can consider recovering from, i.e. being able to do something intelligent with where the baseline for intelligence is defined as a helpful message.

1. RPC error with error code (see here)
2. Null pointer exception (which we're talking about now) from ethereum.

Currently we have

newtype Web3 p e a = Web3 (Aff (eth : ETH | e) a)

Now we're in an interesting position because of the generality of the call and sendTx methods from Contracts.purs. We could change to something like this without any problems at all

data Web3Error =
    RpcError {errorCode :: Int,  message :: String, data :: Maybe Foreign} 
  | NullException {functionName :: String, data :: Maybe Foreign}

newtype Web3 p e a = Web3 (EitherT Web3Error (Aff (eth : ETH | e)) a)

This would allow us to display either the RPC error info and also give a lot of info about the null pointer exception. What do other people think?

[XertroV]

@blinky3713 Prelim comments:

• EitherT doesn't exist in purescript yet (looked into this the other day by chance); but in Haskell it's ExceptT under the hood anyway; probably better to use ExceptT unless you're interested in maintaining a separate lib
• Should EitherT be outside Aff or inside? Initially was thinking inside is better; but since we have to use runWeb3 anyway the only time this might matter is when using Web3 in another transformer (not that we have Web3T in any case).
• My intuition is that runWeb3 should do something like this:

runWeb3 :: Provider -> Web3 p e a -> Aff (eth :: ETH | e) (Either Web3Error a)

someWeb3Req :: HexString -> Web3 p e a

But on reflection I'm not convinced that's best because it means handling Eithers in a non-monadic way (probably within do notation anyway)

If we use newtype Web3 p e a = Web3 (EitherT Web3Error (Aff (eth : ETH | e)) a) then I think the following would apply:

someFunc :: Aff _ a
someFunc = do 
  result :: Either Web3Error a <- runWeb3 http eth_version
  case result of 
    Left err = throw $ error $ show err
    Right a = pure a

What I mean to demonstrate here is that unless we're working within an EitherT err (Aff _) we end up with some boilerplate anyway - though not sure how we'd really get around that - and there is no EitherT yet

Anyway, not exactly sure what the best path forward is - this is mostly thinking aloud