Blockchain params endpoint - part 1 #1294

paweljakubas · 2020-01-21T15:17:15Z

Issue Number

#1291

Overview

I have added new endpoint scaffolding with needed types
I have extended swagger
I have updated unit tests

Comments

Anviking

Looks good! Left a few comments. Most notably I think ApiEpochNumber could be less stringly typed.

lib/core/src/Cardano/Wallet/Api.hs

Anviking · 2020-01-22T15:14:19Z

lib/core/src/Cardano/Wallet/Api/Types.hs

@@ -446,6 +466,33 @@ instance FromHttpApiData Iso8601Time where
 instance ToHttpApiData Iso8601Time where
    toUrlPiece = toText

+instance FromText ApiEpochNumber where
+    fromText txt = case txt of
+        "latest" -> Right (ApiEpochNumber "latest")


🤔 What about defining ApiEpochNumber as something like:

data ApiEpochNumber = ApiEpochNumberLatest | ApiEpochNumber EpochNumber

It seems like when actually using ApiEpochNumber, you'd have to re-parse the Text into either the notion of latest or an EpochNumber anyway.

I was persuaded and used the propose type. It is more precise and better reflect situation

Anviking · 2020-01-22T15:16:39Z

lib/core/test/unit/Cardano/Wallet/Api/TypesSpec.hs

+                            activeSlotCoefficient (x :: ApiNetworkParameters)
+                    }
+            in
+                x' === x .&&. show x' === show x


"pointless tests to trigger coverage for record accessors"

Hm, ok 👍

Unrelated to the PR, but do we still care about coverage to this extent? We have removed the badge from the readme at least.

yeah, pointless tests title says it all

Anviking · 2020-01-22T15:26:39Z

lib/core/src/Cardano/Wallet/Api/Server.hs

+    -> NetworkLayer IO t Block
+    -> ApiEpochNumber
+    -> Handler ApiNetworkParameters
+getNetworkParameters _ _ _ = error "not implemented"


I think best practice is to throwError err501 instead of erroring.

I would do this if we had not Handler but ExceptT . When Handler then I need to do :

liftHandler $ throwE err501

and then we have

No instance for (LiftHandler ServantErr)

So I would rather to stick to error and in replace it in the next PR with implementation

ok, but we can use here :

liftIO $ throwIO err501

I have a memory of err501 being used in the wallet, but I don't remember exactly how.

With liftIO $ throwIO err501 we actually get a 500:

http http://localhost:8090/v2/network/parameters/latest HTTP/1.1 500 Internal Server Error Content-Type: text/plain; charset=utf-8 Date: Thu, 23 Jan 2020 10:08:45 GMT Server: Warp/3.2.27 Transfer-Encoding: chunked Something went wrong

Which is the same result as error "not implemeted".

Whereas with Handler $ throwE err501 we at least get a 501 (but with generic code and message)

http http://localhost:8090/v2/network/parameters/latest HTTP/1.1 501 Not Implemented Content-Type: application/json;charset=utf-8 Date: Thu, 23 Jan 2020 10:10:33 GMT Server: Warp/3.2.27 Transfer-Encoding: chunked { "code": "unexpected_error", "message": "It looks like something unexpected went wrong. Unfortunately I don't yet know how to handle this type of situation. Here's some information about what happened: " }

agree. used Handler $ throwE err501

in the end used Servant's throwError err501

Anviking · 2020-01-22T15:31:18Z

lib/core/test/unit/Cardano/Wallet/Api/TypesSpec.hs

+instance Arbitrary (Quantity "second" NominalDiffTime) where
+    shrink (Quantity 0.0) = []
+    shrink _ = [Quantity 0.0]
+    arbitrary = Quantity . fromInteger <$> choose (0, 100)


Seems ok, but why so small?

ok, increased to 10000

Anviking · 2020-01-22T15:32:23Z

specifications/api/swagger.yaml

+      parameters:
+        - *parametersEpoch
+      description: |
+        <p align="right">status: <strong>under development</strong></p>


Anviking · 2020-01-22T15:47:55Z

CI failing with

Path parameter 'epoch' for 'get' operation in '/network/parameters/{epoch}' was not resolved

paweljakubas · 2020-01-22T22:07:19Z

added also ApiEpochNumber to Api.TypesSpec and ... found subtle error where T.decimal was cycling Word31 numbers. Adopted proper fix

Anviking

lgtm

Anviking · 2020-01-23T10:41:20Z

lib/core/src/Cardano/Wallet/Api/Types.hs

+    , slotLength :: !(Quantity "second" NominalDiffTime)
+    , epochLength :: !(Quantity "slot" Word32)
+    , epochStability :: !(Quantity "block" Word32)
+    , activeSlotCoefficient :: !(Quantity "percent" Percentage)


Percentage

Though I think you might find in the next PR that ActiveSlotCoeff is a Double whereas Percentage is a Word, [0, 100]

Ok, U/S says:

active_slot_coefficient: *percentage

And in x-percentage is a number. So it should be able to be a floating point.

I imagine some users would like to have the full 33.333333% instead of just 33%.

paweljakubas · 2020-01-23T11:35:46Z

bors r+

1294: Blockchain params endpoint - part 1 r=paweljakubas a=paweljakubas # Issue Number  #1291 # Overview  - [x] I have added new endpoint scaffolding with needed types - [x] I have extended swagger - [x] I have updated unit tests # Comments   Co-authored-by: Pawel Jakubas <pawel.jakubas@iohk.io>

iohk-bors · 2020-01-23T12:09:44Z

Build failed

buildkite/cardano-wallet

paweljakubas · 2020-01-23T12:34:05Z

bors r+

1294: Blockchain params endpoint - part 1 r=paweljakubas a=paweljakubas # Issue Number  #1291 # Overview  - [x] I have added new endpoint scaffolding with needed types - [x] I have extended swagger - [x] I have updated unit tests # Comments   Co-authored-by: Pawel Jakubas <pawel.jakubas@iohk.io>

clean arbitrary

fix attempt use better response for unimplemented API endpoint epoch -> epochId fix swagger use throwError from Servant to service err501 update Wallet.Api literal in endpoint

iohk-bors · 2020-01-23T13:07:07Z

Build succeeded

buildkite/cardano-wallet

paweljakubas force-pushed the paweljakubas/1291/blockchain-params-endpoint branch from be23675 to a481cd2 Compare January 22, 2020 12:09

paweljakubas marked this pull request as ready for review January 22, 2020 12:10

paweljakubas requested a review from Anviking January 22, 2020 12:10

paweljakubas self-assigned this Jan 22, 2020

paweljakubas added this to the (WB-47) Expose blockchain parameters via new endpoint milestone Jan 22, 2020

Anviking requested changes Jan 22, 2020

View reviewed changes

paweljakubas force-pushed the paweljakubas/1291/blockchain-params-endpoint branch from a481cd2 to 43df705 Compare January 22, 2020 22:09

paweljakubas requested a review from Anviking January 22, 2020 22:09

paweljakubas force-pushed the paweljakubas/1291/blockchain-params-endpoint branch 4 times, most recently from 347c232 to b6ea48b Compare January 23, 2020 10:30

Anviking approved these changes Jan 23, 2020

View reviewed changes

Anviking reviewed Jan 23, 2020

View reviewed changes

paweljakubas force-pushed the paweljakubas/1291/blockchain-params-endpoint branch 3 times, most recently from 1bf3e3a to 8858f6b Compare January 23, 2020 11:16

paweljakubas force-pushed the paweljakubas/1291/blockchain-params-endpoint branch from 8858f6b to 43e2f18 Compare January 23, 2020 12:33

paweljakubas added 5 commits January 23, 2020 13:38

introduce GetNetworkInformation

e565202

add GetNetworkParameters and propagate it through codebase

bd07065

add corresponding swagger part

77f7b30

update Api.TypesSpec and be more complient with requirement

4a37ec1

improve on ApiEpochNumber type and other fixes

7a909b0

clean arbitrary

change epoch -> epochId in swagger.yaml

43e2f18

fix attempt use better response for unimplemented API endpoint epoch -> epochId fix swagger use throwError from Servant to service err501 update Wallet.Api literal in endpoint

iohk-bors bot merged commit 43e2f18 into master Jan 23, 2020

Anviking deleted the paweljakubas/1291/blockchain-params-endpoint branch January 23, 2020 21:38

Anviking mentioned this pull request Jan 27, 2020

Blockchain params endpoint impl #1306

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Blockchain params endpoint - part 1 #1294

Blockchain params endpoint - part 1 #1294

paweljakubas commented Jan 21, 2020 •

edited

Loading

Anviking left a comment

Anviking Jan 22, 2020

Anviking Jan 22, 2020

paweljakubas Jan 22, 2020

Anviking Jan 22, 2020

paweljakubas Jan 22, 2020

Anviking Jan 22, 2020

paweljakubas Jan 22, 2020 •

edited

Loading

paweljakubas Jan 22, 2020

Anviking Jan 23, 2020

paweljakubas Jan 23, 2020

paweljakubas Jan 23, 2020

Anviking Jan 22, 2020

paweljakubas Jan 22, 2020

Anviking Jan 22, 2020

Anviking commented Jan 22, 2020

paweljakubas commented Jan 22, 2020

Anviking left a comment

Anviking Jan 23, 2020 •

edited

Loading

Anviking Jan 23, 2020

paweljakubas commented Jan 23, 2020

iohk-bors bot commented Jan 23, 2020

paweljakubas commented Jan 23, 2020

iohk-bors bot commented Jan 23, 2020

Blockchain params endpoint - part 1 #1294

Blockchain params endpoint - part 1 #1294

Conversation

paweljakubas commented Jan 21, 2020 • edited Loading

Issue Number

Overview

Comments

Anviking left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paweljakubas Jan 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Anviking commented Jan 22, 2020

paweljakubas commented Jan 22, 2020

Anviking left a comment

Choose a reason for hiding this comment

Anviking Jan 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paweljakubas commented Jan 23, 2020

iohk-bors bot commented Jan 23, 2020

Build failed

paweljakubas commented Jan 23, 2020

iohk-bors bot commented Jan 23, 2020

Build succeeded

paweljakubas commented Jan 21, 2020 •

edited

Loading

paweljakubas Jan 22, 2020 •

edited

Loading

Anviking Jan 23, 2020 •

edited

Loading