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 improvements #213

Merged
merged 4 commits into from May 28, 2019

Conversation

Projects
None yet
4 participants
@slpopejoy
Copy link
Contributor

commented May 25, 2019

Ready once Pact PR goes in

slpopejoy added some commits May 25, 2019

@slpopejoy slpopejoy marked this pull request as ready for review May 26, 2019

@emilypi
Copy link
Contributor

left a comment

LGTM. Happy to see the orphans gone!

@@ -140,7 +140,7 @@ data PactDbStatePersist = PactDbStatePersist
, _pdbspPactDbState :: PactDbState
}

newtype GasSupply = GasSupply { _gasSupply :: Decimal }
newtype GasSupply = GasSupply { _gasSupply :: GasPrice }

This comment has been minimized.

Copy link
@emilypi

emilypi May 26, 2019

Contributor

GasSupply is our current working total. Why was Decimal replaced with GasPrice?

This comment has been minimized.

Copy link
@slpopejoy

slpopejoy May 27, 2019

Author Contributor

Maybe not the best idea, it was in hopes of minimizing coercion. I'll look at this

This comment has been minimized.

Copy link
@slpopejoy

slpopejoy May 27, 2019

Author Contributor

Ah the reason is it needs a JSON representation to go into the message body, ie in mkBuyGasCmd and GasPrice has the right instances.

The larger issue is using dedicated newtypes for gas prices, when in reality these are simply money values which don't have a first class representation in the API. I propose leaving this for now and addressing in a Pact PR, ie a Price or Money API type.

This comment has been minimized.

Copy link
@slpopejoy

slpopejoy May 27, 2019

Author Contributor

OK figured out a better solution, and one we should have done for GasPrice: newtype ParsedDecimal to grab its instances. Likewise, GasLimit should newtype ParsedInteger.

This comment has been minimized.

Copy link
@emilypi

emilypi May 27, 2019

Contributor

Works for me

Show resolved Hide resolved stack.yaml Outdated
Show resolved Hide resolved cabal.project Outdated
Show resolved Hide resolved default.nix Outdated
@gregorycollins
Copy link
Contributor

left a comment

👍

@fosskers fosskers merged commit 270f64f into master May 28, 2019

1 check passed

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

@fosskers fosskers deleted the feat/pact-api-improvements branch May 28, 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.