Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pact API upgrade #208

Merged
merged 10 commits into from May 24, 2019

Conversation

Projects
None yet
6 participants
@slpopejoy
Copy link
Contributor

commented May 23, 2019

Fixes #139

slpopejoy added some commits May 23, 2019

@slpopejoy slpopejoy requested a review from kadena-io/chainweb-maintainers May 23, 2019

type LocalApi = "local" :> ReqBody '[JSON] (Command Text) :> Post '[JSON] (CommandSuccess Value)

type PactApi_ = "pact" :> ApiV1API
type PactApi_ = "pact" :> API.ApiV1API -- TODO unify with Pact versioning

This comment has been minimized.

Copy link
@slpopejoy

slpopejoy May 23, 2019

Author Contributor

We should really unify these. Pact api is api/v1/send etc. Chainweb is [chainweb stuff]/pact/send. Pact docs refer to the former. Do we fully conform, ie [chainweb stuff]/pact/api/v1/send? Or do we update the pact docs to start with v1/send and say the prefix can vary? Or, I guess we could modify pact -s to be pact/v1/send and then chainweb is strictly a prefix.

This comment has been minimized.

Copy link
@slpopejoy

slpopejoy May 23, 2019

Author Contributor

Based on discussion with @larskuhtz , going with fully specified url, since the "Pact API" is cross platform while chainweb might hang other services off of pact/ (ie SPV queries etc). Thus [chainweb stuff]/pact/api/v1/send.

@@ -3,34 +3,31 @@

module Chainweb.Pact.RestAPI.Orphans () where

This comment has been minimized.

Copy link
@slpopejoy

slpopejoy May 23, 2019

Author Contributor

Should we move these to Pact? Are they meaningful there?

This comment has been minimized.

Copy link
@marklnichols

marklnichols May 23, 2019

Contributor

I also have a couple of Aeson instances of GasPrice in Chainweb.Oprhans that I assume needs to be moved to Pact


runListen timedOut = go Nothing
where
go !prevCut = do
m <- waitForNewCut prevCut
case m of
Nothing -> return nullResponse -- timeout

This comment has been minimized.

Copy link
@slpopejoy

slpopejoy May 23, 2019

Author Contributor

@gregorycollins I think I have the semantics right here, please confirm

This comment has been minimized.

Copy link
@gregorycollins

gregorycollins May 23, 2019

Contributor

Timeout is an expected condition here, it shouldn't return 400. Instead the CommandResult should be populated with a timeout code if possible. I guess it doesn't matter as long as the web client code does the right thing with respect to retry. If you do use an HTTP error code, properly it should be 500-series because 400-series responses are meant to indicate some problem with the request.

This comment has been minimized.

Copy link
@slpopejoy

slpopejoy May 23, 2019

Author Contributor

What should it return then? 500 "Timeout"? cloudflare does a "522 Timeout" https://en.wikipedia.org/wiki/List_of_HTTP_status_codes#Cloudflare

This comment has been minimized.

Copy link
@larskuhtz

larskuhtz May 23, 2019

Contributor

If the timeout is expected. What about 204 No Content or 202 Accepted?

This comment has been minimized.

Copy link
@slpopejoy

slpopejoy May 24, 2019

Author Contributor

I like 204

This comment has been minimized.

Copy link
@slpopejoy

slpopejoy May 24, 2019

Author Contributor

OTOH, maybe we should just do 200 with { "status": "timeout" } body, just to avoid finicky header code docs. One thing I didn't do with this update is go for the "literate error responses", ie use errorResponse or whatever to make 402s etc have a parseable body with { "status": "failure", "code": "402", "message": ... } as discussed, so that JS frontends can simply parse a body.

@@ -422,7 +422,7 @@ poll version chainid = go
where
_ :<|> go :<|> _ :<|> _ = api version chainid

listen :: ChainwebVersion -> ChainId -> ListenerRequest -> ClientM ApiResult
listen :: ChainwebVersion -> ChainId -> ListenerRequest -> ClientM (CommandResult H.Hash)

This comment has been minimized.

Copy link
@slpopejoy

slpopejoy May 23, 2019

Author Contributor

Do we have any coverage of TXG? @emmanueldenloye @fosskers

This comment has been minimized.

Copy link
@fosskers

fosskers May 23, 2019

Contributor

Our tests don't test it, but perhaps they should be made to.

@emilypi
Copy link
Contributor

left a comment

Looks good, just a few comments

@@ -1,3 +1,4 @@
{-# LANGUAGE RecordWildCards #-}

This comment has been minimized.

Copy link
@emilypi

emilypi May 23, 2019

Contributor

No wildcards are used in this file

spvError' "associated pact transaction outputs have wrong format"
Just o -> do
logLog l "ERROR" $ show o
spvError' "associated pact transaction outputs have wrong format"

This comment has been minimized.

Copy link
@emilypi

emilypi May 23, 2019

Contributor

We should differentiate these two error cases so we know that one is a decode error vs. one being a 'wrong result format'

@@ -196,39 +195,42 @@ serviceRequests memPoolAccess reqQ = do
LocalMsg LocalReq{..} -> do
r <- try $ execLocal _localRequest
case r of
Left e -> liftIO $ putMVar _localResultVar $ Left e
Right r' -> liftIO $ putMVar _localResultVar r'
Left (SomeException e) -> liftIO $ putMVar _localResultVar $ toPactInternalError e

This comment has been minimized.

Copy link
@marklnichols

marklnichols May 23, 2019

Contributor

I assume Left can only be of type SomeExecption ?

This comment has been minimized.

Copy link
@slpopejoy

slpopejoy May 23, 2019

Author Contributor

This is an unusual one, where we don't think any exceptions will fire but "just in case". Also, the SomeException specializes try otherwise it wouldn't compile.

pure $! toHashCommandResult result

toHashCommandResult :: P.CommandResult [P.TxLog A.Value] -> HashCommandResult
toHashCommandResult = over (P.crLogs . _Just) (P.pactHash . encodeToByteString)

This comment has been minimized.

Copy link
@marklnichols

marklnichols May 23, 2019

Contributor

I doesn't seem that LamdaCase or TypeApplication are needed extension anymore

@@ -1,3 +1,4 @@
{-# LANGUAGE RecordWildCards #-}

This comment has been minimized.

Copy link
@marklnichols

marklnichols May 23, 2019

Contributor

This didn't need to be added

@@ -213,57 +217,56 @@ applyLocal
-> PublicData
-> SPVSupport
-> Command (Payload PublicMeta ParsedCode)
-> IO (Either SomeException (CommandSuccess (Term Name)))
-> IO (CommandResult [TxLog Value])
applyLocal logger dbEnv pd spv cmd@Command{..} = do

-- cmd env with permissive gas model
let pd' = set pdPublicMeta (publicMetaOf cmd) $ pd

This comment has been minimized.

Copy link
@marklnichols

marklnichols May 23, 2019

Contributor

can drop the $


fromCoinbaseOutput :: MonadThrow m => CoinbaseOutput -> m HashedLogTxOutput
fromCoinbaseOutput :: MonadThrow m => CoinbaseOutput -> m (HashCommandResult)

This comment has been minimized.

Copy link
@marklnichols

marklnichols May 23, 2019

Contributor

Can drop the paren here

@@ -29,13 +29,10 @@ import Chainweb.Payload
import Chainweb.Transaction
import Chainweb.Utils (codecDecode)

import Data.Aeson (Value)

This comment has been minimized.

Copy link
@marklnichols

marklnichols May 23, 2019

Contributor

It looks like BangPatterns is no longer needed

@@ -42,6 +41,8 @@ import Chainweb.Test.Pact.Utils
import Chainweb.Test.Utils
import Chainweb.Version (ChainwebVersion(..))

import Pact.Types.Command

This comment has been minimized.

Copy link
@marklnichols

marklnichols May 23, 2019

Contributor

It looks like LamdaCase and TupleSections are no longer needed

@@ -125,7 +125,7 @@ responseGolden networkIO rksIO = golden "command-0-resp" $ do
cwEnv <- _getClientEnv <$> networkIO

This comment has been minimized.

Copy link
@marklnichols

marklnichols May 23, 2019

Contributor

We don't need QuasiQuotes any more

slpopejoy added some commits May 23, 2019

@fosskers fosskers merged commit edb86f3 into master May 24, 2019

1 check passed

ci/gitlab/gitlab.com Pipeline passed on GitLab
Details

@fosskers fosskers deleted the feat/pact-api-upgrade branch May 24, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.