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.

Sign up for GitHub

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

Jump to bottom

Add cardano-client package #2109

Merged
merged 4 commits into from May 20, 2020
Merged

Add cardano-client package #2109

merged 4 commits into from May 20, 2020

Conversation

MarcFontaine
Copy link
Contributor

At the moment this only consists of the subscribe function.

@MarcFontaine MarcFontaine self-assigned this May 18, 2020
@MarcFontaine MarcFontaine added the api thing related exposed apis label May 18, 2020
@MarcFontaine MarcFontaine requested a review from coot May 18, 2020 12:50
coot
coot reviewed May 18, 2020
View reviewed changes
Copy link
Contributor

@coot coot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @MarcFontaine, I left some comments.

cardano-client/src/Cardano/Client/API.hs Outdated
{-# LANGUAGE ScopedTypeVariables #-}
{-# LANGUAGE TypeFamilies #-}

module Cardano.Client.API
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's worth to control what we export from this module. Otherwise it will be too easy to make changes to this module and not realize that they need to be propagated.

Then make sure that this module exports everything that the functions exported here needs..

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also a better name for this module would be Cardano.Client.Subscription

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now I only export the subscribe function
I want to figure out the exact exports later after this is tested in the applications.
This is just the first iteration.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you'd use cardano-haskell you could play with other repos before merging it.

cardano-client/src/Cardano/Client/API2.hs Outdated Show resolved Hide resolved
cardano-client/src/Cardano/Client/API2.hs Outdated Show resolved Hide resolved
cardano-client/src/Cardano/Client/API2.hs Outdated Show resolved Hide resolved
cardano-client/src/Cardano/Client/API2.hs Outdated
-- import Ouroboros.Network.NodeToClient
, ConnectionId (..)
, NetworkConnectTracers (..)
, NodeToClientVersion (..)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to think how to export it. It will clash with NodeToClientVersion in ouroboros-consensus which I think is more useful (but it would be good to check in various clients).

@mrBliss
Copy link
Contributor

mrBliss commented May 19, 2020

Maybe it would be useful to show how the new API improves the (lack of an) existing one? For example, show a small piece of code using it. Even better, translate one of the existing clients in cardano-node to use the new API (this is actually the approach I would follow to design the API).

@MarcFontaine
Copy link
Contributor Author

Maybe it would be useful to show how the new API improves the (lack of an) existing one? For example, show a small piece of code using it. Even better, translate one of the existing clients in cardano-node to use the new API (this is actually the approach I would follow to design the API).

This has been done two weeks ago for cardano-db-sync
Just look here:
https://github.com/input-output-hk/cardano-db-sync/pull/90/files#diff-6f865c57ae290973d1569e858871fb2d

@MarcFontaine MarcFontaine requested a review from coot May 19, 2020 11:44
coot
coot reviewed May 19, 2020
View reviewed changes
cardano-client/LICENSE
@@ -0,0 +1,177 @@

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You also need to copy the NOTICE file (ouroboros-network/NOTICE).

coot
coot reviewed May 19, 2020
View reviewed changes
cardano-client/cardano-client.cabal Outdated
synopsis: An API for ouroboros-network
-- description:
license: Apache-2.0
license-files: LICENSE
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also add NOTICE here.

coot
coot reviewed May 19, 2020
View reviewed changes
cardano-client/cardano-client.cabal Outdated
license: Apache-2.0
license-files: LICENSE
copyright: 2019 Input Output (Hong Kong) Ltd.
maintainer:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's good to put:

maintainer:          operations@iohk.io

For authors it might be good to put IOHK Engineering Team.

@MarcFontaine MarcFontaine requested a review from coot May 19, 2020 13:23
coot
coot approved these changes May 20, 2020
View reviewed changes
Copy link
Contributor

@coot coot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Please also open a PR which updates cardano-haskell's cabal.project file.

@MarcFontaine
Copy link
Contributor Author

bors r+

@iohk-bors
Copy link
Contributor

iohk-bors bot commented May 20, 2020

Build succeeded

@iohk-bors iohk-bors bot merged commit 9d1facc into master May 20, 2020
@coot
Copy link
Contributor

coot commented May 21, 2020

Related to #1013

@coot coot added this to the S13 2020-05-21 milestone May 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@coot coot coot approved these changes

Assignees

@MarcFontaine MarcFontaine

Labels
api thing related exposed apis
Projects
None yet
Milestone
S13 2020-05-21
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@MarcFontaine @mrBliss @coot