f-o-a-m / purescript-web3

a purescript library for the web3 api
Apache License 2.0
127 stars 24 forks source link

Error handling #51

Closed martyall closed 6 years ago

martyall commented 6 years ago

This pr addresses two main concerns for error handling #44

In this case we chose to throw the exception, because it seems to me that its possible to recover from these with a try on a case by case basis. I don't want to have the entire rpc layer start returning Either types. Correct me if I'm wrong here, but it seems like this is an acceptable middle ground, and it least the error message that's thrown is more specific.

  1. When you are making a call on a storage which is not yet initialized, a contract address which doesn't exist, or something that plain just doesn't make sense. Here we distinguish between two types or parse errors, ones that occur from receiving a null data string (NullStorageException) and one that you just have an incorrect parser for (ParseError). In either case, we try to return as much information as possible about the call.
martyall commented 6 years ago

@kejace @XertroV If you agree with the changes, i will change the generator to work with this

XertroV commented 6 years ago

@blinky3713

In this case we chose to throw the exception, because it seems to me that its possible to recover from these with a try on a case by case basis. I don't want to have the entire rpc layer start returning Either types. Correct me if I'm wrong here, but it seems like this is an acceptable middle ground, and it least the error message that's thrown is more specific.

Cons I can see:

Pros:

In terms of a middle ground, I think that's fair and none of the cons I mention are too bad.

Parsing and NullStorage errors sound good.

Haven't done a full review but seems good in principle. My only hesitation is the explicit / implicit error thing, but not exactly sure how I fall on that. I like explicit errors but too much boilerplate code handling (or injecting lifts everywhere in something like a StateT (EitherT (Aff _))) can get annoying, so maybe using MonadError is actually a pretty good choice here.

Haven't done a full review yet.

martyall commented 6 years ago

@XertroV this is what it would look like using ExceptT. I have branches of the generator and the e2e tests that confirm that it all works.

XertroV commented 6 years ago

The main difference for users with ExceptT would be:

-- example request
eth_getBalance :: Address -> Web3 _ BigNumber

-- Web3 is something like:
type Web3 e a = ExceptT String (Aff e) a    -- String is error type

-- normal way of making your own Web3 computation
myLib_isTxConfirmed :: Txid -> Web3 _ Boolean
myLib_isTxConfirmed txid = do
    txR <- eth_getTransactionReceipt txid
    pure $ txR /= NullOrUndefined Nothing

-- the user can now write stuff that errors nicely
myLib_sendTaxToGovernment :: Web3 _ Txid
myLib_sendTaxToGovernment = do
    userAddress <- eth_coinbase
    userBalance <- eth_getBalance userAddress

    -- we need to `lift` Affs
    taxRequired <- lift $ (getTaxRequired :: Address -> Aff _ BigNumber) userAddress

    txid <- if userBalance < requiredTax
        -- returning a `Left` will terminate computation, like a chain of `Either`s
        then Left "User does not have enough balance"
        -- but we can still return normal Web3 stuff too
        else eth_sendTransaction userAddress requiredTax governmentAddress

    -- note `delay` produces an Aff and must be `lift`ed
    -- in this case `untilM_` is operating in Web3 monad, not Aff
    untilM_ (lift $ delay $ Milliseconds 100) (myLib_isTxConfirmed txid)
    pure txid

-- then they can do stuff like:
main :: Eff _ Unit
main = launchAff_ $ do
    doTax <- runWeb3 http myLib_sendTaxToGovernment
    case doTax of
        Left err -> log $ "Could not send tax to gov because: " <> err
        Right txid -> log $ "Thank you for paying your taxes. Taxes sent in " <> show txid

FYI I haven't run this and I'm not 100% certain that's how ExceptT works but I think that would roughly be how it works.

XertroV commented 6 years ago

Looking at https://github.com/f-o-a-m/purescript-web3/pull/51/files#diff-f38a192d15d07d00f868fae1a758f2e3R35 it seems like this is what's planned anyway 👍 - I like it.