Parameterise `StakePools` API over apiPool type #1738

Anviking · 2020-06-10T09:03:29Z

Issue Number

ADP-311, #1718

Overview

I parameterised StakePools API over apiPool
I renamed ApiStakePool to ApiJormungandrStakePool, such that we can later introduce a haskell ApiStakePool
I made the default Api type use ApiJormungandrStakePool. We should later switch to ApiStakePool, and update the swagger scheme such that it targets haskell, and not jormungandr.
I moved StakePoolLayer to the jormungandr package.

Comments

I am keeping the Pool DB in core, since I think we still want it. But I have not thought about how it would work with shelley.
changelog: this is pure refactoring, but in combination with follow-up PRs this is a new feature.

Anviking · 2020-06-10T09:08:02Z

bors try

Anviking · 2020-06-10T09:10:48Z

lib/core/src/Cardano/Pool.hs

-
-- | @StakePoolLayer@ is a thin layer ontop of the DB. It is /one/ value that
-- can easily be passed to the API-server, where it can be used in a simple way.
-data StakePoolLayer e m = StakePoolLayer


Moved to Cardano.Pool.Jormungandr.Metrics

Anviking · 2020-06-10T09:11:41Z

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

-    -> StakePoolLayer e IO
+    -> IO [PoolId]
+       -- ^ Known pools
+       -- We could maybe replace this with a @IO (PoolId -> Bool)@


No need to have a common StakePoolLayer at this point. No need to have a common StakePool type in core either.

We just need to make sure the target packages can provide a Handler (ListStakePool apiPool), for some apiPool, and can provide this IO [PoolId].

iohk-bors · 2020-06-10T09:16:23Z

try

Build failed

buildkite/cardano-wallet

lib/jormungandr/src/Cardano/Pool/Jormungandr/Metrics.hs

KtorZ · 2020-06-10T10:51:46Z

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

@@ -183,13 +183,16 @@ import Servant.API.Verbs

 type ApiV2 n = "v2" :> Api n

+-- | The full cardano-wallet API.
+--
+-- The API used in cardano-wallet-jormungandr may differ from this one.
 type Api n =


I assume this one can be fully moved to each target package, alongside the server instantiation.

No! Sadly not easily, because we have a swagger spec in core!

My plan is to keep this type, but let jormungandr use a custom Api type.

Hmmm. Well no, we can also declare an "Api" type in the relevant test.

Well no, we can also declare an "Api" type in the relevant test

That might make our test meaningless since if it is no longer testing the Api type that is actually used.

I say we keep the haskell API in core as the most important api, and test it, but not the jormungandr one.

…or, we define and test both APIs in core 🤔

…or, maybe one could do something with a ApiStakePool | ApiJormungandrStakePool sum type 🤔

Can sort out later.

…or, maybe one could do something with a ApiStakePool | ApiJormungandrStakePool sum type thinking

Yeah, I thought of it. Since the stake pools are only something the server produces, it might just be okay to go down that path and fairly transparent for clients.

paweljakubas · 2020-06-10T11:14:26Z

lib/jormungandr/src/Cardano/Pool/Jormungandr/Metrics.hs

+
+-- | @StakePoolLayer@ is a thin layer ontop of the DB. It is /one/ value that
+-- can easily be passed to the API-server, where it can be used in a simple way.
+data StakePoolLayer e m = StakePoolLayer


don't we want to have StakePoolLayer interface in core? And in jormunagdr and shelley implementations of it?

I believe we/you currently only need the knownStakePools :: IO [PoolId] field to check that the pool we're joining actually exists. So I see no point in keeping the entire StakePoolLayer then, since it would just be more work trying to unify the different pool-types under one common interface.

a ok, got it . so now we will sneak IO [PoolId] instead of StakePoolLayer on wallet level. and just call it (with liftIO). and this will be knownPools type argument...

Yeah (see diff at #1738 (comment))

I was curious about using a IO (PoolId -> Bool) also, but didn't do it.

Anviking · 2020-06-10T12:04:15Z

bors r+

1738: Parameterise `StakePools` API over apiPool type r=Anviking a=Anviking # Issue Number ADP-311, #1718 # Overview  - [x] I parameterised `StakePools` API over `apiPool` - [x] I renamed `ApiStakePool` to `ApiJormungandrStakePool`, such that we can later introduce a haskell `ApiStakePool` - [x] I made the default `Api` type use `ApiJormungandrStakePool`. We should later switch to `ApiStakePool`, and update the swagger scheme such that it targets haskell, and not jormungandr. - [x] I moved `StakePoolLayer` to the jormungandr package. # Comments - I am keeping the Pool DB in core, since I think we still want it. But I have not thought about how it would work with shelley. - changelog: this is pure refactoring, but in combination with follow-up PRs this is a new feature.   Co-authored-by: Johannes Lund <[email protected]> Co-authored-by: KtorZ <[email protected]>

iohk-bors · 2020-06-10T14:21:59Z

Build failed

ci/hydra-build:required

Anviking · 2020-06-10T14:23:13Z

bors r+

1738: Parameterise `StakePools` API over apiPool type r=Anviking a=Anviking # Issue Number ADP-311, #1718 # Overview  - [x] I parameterised `StakePools` API over `apiPool` - [x] I renamed `ApiStakePool` to `ApiJormungandrStakePool`, such that we can later introduce a haskell `ApiStakePool` - [x] I made the default `Api` type use `ApiJormungandrStakePool`. We should later switch to `ApiStakePool`, and update the swagger scheme such that it targets haskell, and not jormungandr. - [x] I moved `StakePoolLayer` to the jormungandr package. # Comments - I am keeping the Pool DB in core, since I think we still want it. But I have not thought about how it would work with shelley. - changelog: this is pure refactoring, but in combination with follow-up PRs this is a new feature.   Co-authored-by: Johannes Lund <[email protected]> Co-authored-by: KtorZ <[email protected]>

KtorZ · 2020-06-10T15:25:21Z

bors r-

iohk-bors · 2020-06-10T15:25:23Z

Canceled

KtorZ · 2020-06-10T15:25:34Z

Hydra will cache the previous build and it'll fail all-the-same.

KtorZ · 2020-06-10T15:26:16Z

bors try

iohk-bors · 2020-06-10T17:14:58Z

try

Build failed

ci/hydra-build:required

KtorZ · 2020-06-10T17:16:27Z

Twice in a row, the unit tests on Windows aren't passing 🤔

cardano-wallet-core-test-unit-2020.6.5-x86_64-w64-mingw32-check-x86_64-w64-mingw32.drv

KtorZ · 2020-06-10T17:22:17Z

I don't see anything obviously wrong on Windows in the changes from this PR though...

Anviking · 2020-06-11T13:19:57Z

bors r+

1738: Parameterise `StakePools` API over apiPool type r=Anviking a=Anviking # Issue Number ADP-311, #1718 # Overview  - [x] I parameterised `StakePools` API over `apiPool` - [x] I renamed `ApiStakePool` to `ApiJormungandrStakePool`, such that we can later introduce a haskell `ApiStakePool` - [x] I made the default `Api` type use `ApiJormungandrStakePool`. We should later switch to `ApiStakePool`, and update the swagger scheme such that it targets haskell, and not jormungandr. - [x] I moved `StakePoolLayer` to the jormungandr package. # Comments - I am keeping the Pool DB in core, since I think we still want it. But I have not thought about how it would work with shelley. - changelog: this is pure refactoring, but in combination with follow-up PRs this is a new feature.   Co-authored-by: Johannes Lund <[email protected]> Co-authored-by: KtorZ <[email protected]>

KtorZ · 2020-06-11T14:20:07Z

bors r-

iohk-bors · 2020-06-11T14:20:10Z

Canceled

KtorZ · 2020-06-11T15:59:09Z

bors r+

1738: Parameterise `StakePools` API over apiPool type r=KtorZ a=Anviking # Issue Number ADP-311, #1718 # Overview  - [x] I parameterised `StakePools` API over `apiPool` - [x] I renamed `ApiStakePool` to `ApiJormungandrStakePool`, such that we can later introduce a haskell `ApiStakePool` - [x] I made the default `Api` type use `ApiJormungandrStakePool`. We should later switch to `ApiStakePool`, and update the swagger scheme such that it targets haskell, and not jormungandr. - [x] I moved `StakePoolLayer` to the jormungandr package. # Comments - I am keeping the Pool DB in core, since I think we still want it. But I have not thought about how it would work with shelley. - changelog: this is pure refactoring, but in combination with follow-up PRs this is a new feature.   Co-authored-by: Johannes Lund <[email protected]> Co-authored-by: KtorZ <[email protected]>

iohk-bors · 2020-06-11T16:58:02Z

Build failed

buildkite/cardano-wallet

Anviking · 2020-06-12T12:01:46Z

bors r+

1738: Parameterise `StakePools` API over apiPool type r=Anviking a=Anviking # Issue Number ADP-311, #1718 # Overview  - [x] I parameterised `StakePools` API over `apiPool` - [x] I renamed `ApiStakePool` to `ApiJormungandrStakePool`, such that we can later introduce a haskell `ApiStakePool` - [x] I made the default `Api` type use `ApiJormungandrStakePool`. We should later switch to `ApiStakePool`, and update the swagger scheme such that it targets haskell, and not jormungandr. - [x] I moved `StakePoolLayer` to the jormungandr package. # Comments - I am keeping the Pool DB in core, since I think we still want it. But I have not thought about how it would work with shelley. - changelog: this is pure refactoring, but in combination with follow-up PRs this is a new feature.   Co-authored-by: Johannes Lund <[email protected]> Co-authored-by: KtorZ <[email protected]>

iohk-bors · 2020-06-12T12:20:49Z

Build failed

ci/hydra-build:required

Anviking · 2020-06-12T12:39:57Z

This doesn't make sense, because this is not the actual code:

src/Cardano/Wallet/Shelley/Api/Server.hs:23:1: error:
    Could not find module ‘Cardano.Pool’
    Perhaps you meant
      Cardano.Pool.DB (from cardano-wallet-core-2020.6.5)
    Use -v to see a list of the files searched for.
   |
23 | import Cardano.Pool
   | ^^^^^^^^^^^^^^^^^^^...
[2 of 6] Compiling Cardano.Wallet.Shelley.Compatibility ( src/Cardano/Wallet/Shelley/Compatibility.hs, dist/build/Cardano/Wallet/Shelley/Compatibility.o )
[3 of 6] Compiling Cardano.Wallet.Shelley.Network ( src/Cardano/Wallet/Shelley/Network.hs, dist/build/Cardano/Wallet/Shelley/Network.o )
[4 of 6] Compiling Cardano.Wallet.Shelley.Transaction ( src/Cardano/Wallet/Shelley/Transaction.hs, dist/build/Cardano/Wallet/Shelley/Transaction.o )
builder for '/nix/store/q9jxhhslcw0j4f4aqdkn3xpvyyx0y9z6-cardano-wa

The build failure can't be cached, since master has changed?

Anviking · 2020-06-12T12:41:30Z

Ok, it is because of the merge 🤔

…askell - ApiStakePool -> ApiJormungandrStakePool - Move StakePoolLayer to the jormungandr package - add missing JSON golden file 'ApiJormungandrStakePool.json' - Move jorm listPools handler to server module - re-generate JSON golden file, in hope (commit from Matthias) Hoping to see the windows unit test timeout disappear

Anviking · 2020-06-12T13:08:07Z

bors r+

iohk-bors · 2020-06-12T14:30:57Z

Build succeeded

iohk-bors bot added a commit that referenced this pull request Jun 10, 2020

Try #1738:

3451913

Anviking commented Jun 10, 2020

View reviewed changes

Anviking force-pushed the anviking/ADP-311/liberate-stakepool-api branch from d5e4b84 to 11d2408 Compare June 10, 2020 09:39

Anviking requested review from paweljakubas and KtorZ June 10, 2020 09:40

Anviking commented Jun 10, 2020

View reviewed changes

lib/jormungandr/src/Cardano/Pool/Jormungandr/Metrics.hs Outdated Show resolved Hide resolved

Anviking self-assigned this Jun 10, 2020

Anviking force-pushed the anviking/ADP-311/liberate-stakepool-api branch 2 times, most recently from 9fe0e15 to 2d547bd Compare June 10, 2020 10:26

KtorZ reviewed Jun 10, 2020

View reviewed changes

KtorZ approved these changes Jun 10, 2020

View reviewed changes

paweljakubas reviewed Jun 10, 2020

View reviewed changes

paweljakubas approved these changes Jun 10, 2020

View reviewed changes

Anviking added the ADDING FEATURE Mark a PR as adding a new feature, for auto-generated CHANGELOG label Jun 10, 2020

KtorZ force-pushed the anviking/ADP-311/liberate-stakepool-api branch from e240a99 to 08d6c86 Compare June 10, 2020 15:26

iohk-bors bot added a commit that referenced this pull request Jun 10, 2020

Try #1738:

a0dac25

KtorZ force-pushed the anviking/ADP-311/liberate-stakepool-api branch from 08d6c86 to 1e27e7e Compare June 11, 2020 14:47

Anviking force-pushed the anviking/ADP-311/liberate-stakepool-api branch 2 times, most recently from 8ac875f to 9c249fb Compare June 12, 2020 13:06

Anviking force-pushed the anviking/ADP-311/liberate-stakepool-api branch from 9c249fb to a42e9ae Compare June 12, 2020 13:08

iohk-bors bot merged commit 2787bbd into master Jun 12, 2020

iohk-bors bot deleted the anviking/ADP-311/liberate-stakepool-api branch June 12, 2020 14:31

Parameterise StakePools API over apiPool type #1738

Parameterise StakePools API over apiPool type #1738

Conversation

Anviking commented Jun 10, 2020 • edited Loading

Issue Number

Overview

Comments

Anviking commented Jun 10, 2020

Anviking Jun 10, 2020 • edited Loading

Choose a reason for hiding this comment

Anviking Jun 10, 2020 • edited Loading

Choose a reason for hiding this comment

iohk-bors bot commented Jun 10, 2020

try

Build failed

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

paweljakubas Jun 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Anviking commented Jun 10, 2020

iohk-bors bot commented Jun 10, 2020

Build failed

Anviking commented Jun 10, 2020

KtorZ commented Jun 10, 2020

iohk-bors bot commented Jun 10, 2020

Canceled

KtorZ commented Jun 10, 2020

KtorZ commented Jun 10, 2020

iohk-bors bot commented Jun 10, 2020

try

Build failed

KtorZ commented Jun 10, 2020

KtorZ commented Jun 10, 2020

Anviking commented Jun 11, 2020

KtorZ commented Jun 11, 2020

iohk-bors bot commented Jun 11, 2020

Canceled

KtorZ commented Jun 11, 2020

iohk-bors bot commented Jun 11, 2020

Build failed

Anviking commented Jun 12, 2020

iohk-bors bot commented Jun 12, 2020

Build failed

Anviking commented Jun 12, 2020

Anviking commented Jun 12, 2020

Anviking commented Jun 12, 2020

iohk-bors bot commented Jun 12, 2020

Build succeeded

Parameterise `StakePools` API over apiPool type #1738

Parameterise `StakePools` API over apiPool type #1738

Anviking commented Jun 10, 2020 •

edited

Loading

Anviking Jun 10, 2020 •

edited

Loading

Anviking Jun 10, 2020 •

edited

Loading

paweljakubas Jun 10, 2020 •

edited

Loading