List stake pools #1753

Anviking · 2020-06-12T20:06:47Z

Issue Number

#1718, #1720

Overview

Allow heterogeneous queues of LSQ queries such that we can query for stake, rewards, and pparams using the same queue.
Retrieve pool id, stake and saturation and make the API return it
Stubbed values for non-myopic member rewards, margin, cost, produced blocks

Comments

Actually fetching non-myopic member rewards depends on a consensus change reaching cardano-node.
Working chain following may be a non-goal for this PR, but trying to make room for it.

Anviking · 2020-06-13T19:49:05Z

bors try

lib/shelley/src/Cardano/Wallet/Shelley/Network.hs

iohk-bors · 2020-06-13T20:08:17Z

try

Build failed

buildkite/cardano-wallet

rvl

I'm not sure if you would like a review yet, but I see that you are using the Cursor fields to access the lsqQ. Perhaps we would just need another NetworkLayer method for this. Because for Jormungandr, we needed to track pools, but for cardano-node, we can simply query.

Anviking · 2020-06-15T07:34:18Z

Perhaps we would just need another NetworkLayer method for this.

The NetworkLayer is shared between jormungandr and haskell, and I don't see the point in trying to generalise over the two where they are different. We'd need to move a bunch of Shelley-specific types to core, and we'd need to add error "not implemented" on the jormungandr side.

we needed to track pools

We are still going to track pool registration certificates. It could perhaps be useful to allow the LSQ to fetch metrics from the chain-follower tip, instead of the NetworkLayer's currentNodeTip, to guarantee consistency — but just a guess.

KtorZ · 2020-06-15T08:25:16Z

to allow the LSQ to fetch metrics from the chain-follower tip,

I don't think this is generally possible. The LSQ has only a certain history of the past. It'd be nice if we could move the chain sync cursor forward, fetch some ledger state data, move forward, and so forth.
Yet in practice, they are separate protocols and they don't really share state here. Thus, unless your chain sync tip is within 2k from the node's tip, you won't be able to fetch corresponding ledger data.

To allow both PParams, and StakeDistribution checks in the same queue.

- Return dummy rewards, cost, margin, productions for now - Embed LSQ queue in cursor - Remove mock knownPools

lib/shelley/src/Cardano/Wallet/Shelley/Pools.hs

KtorZ · 2020-06-15T12:42:50Z

lib/shelley/src/Cardano/Wallet/Shelley/Pools.hs

+        Map.merge stakeButNoRew rewardsButNoStake bothPresent
+      where
+        -- calculate the saturation from the relative stake
+        sat s = fromRational $ (getPercentage s) / (1 / fromIntegral nOpt)


KtorZ · 2020-06-15T12:45:44Z

lib/shelley/src/Cardano/Wallet/Shelley/Pools.hs

+        -> Map PoolId (Quantity "lovelace" Word64)
+        -> Map PoolId PoolLsqMetrics
+    combine nOpt =
+        Map.merge stakeButNoRew rewardsButNoStake bothPresent


How is it actually possible with the LSQ to have cases where a poolId wouldn't be in both maps? From the excerpt above, I get that these are two separate requests, but the local state query allows for "locking" a state and making several queries on that locked state if I recall correctly? So in principle, that's sort of a guarantee that the list of pools fetched from the stake distribution is exactly the same as the list of pools get from the member rewards.

The poolId may not be in the rewards map (clarified comment in cacff55)

As for the rewardsButNoStake case: I suspect that is impossible. (Maybe @rvl knows for sure though?)

I guess we should have some InconcistentMetrics similar to what we had for jormungandr, instead of the current silent dropMissing.

We will have more problems like this when we start folding over pool registration certificates on the chain.

Yes, I suspect that is impossible

But I guess regardless of locking or not, we are making the queries with the same specific Point, so they should all be consistent if they succeed 🤔

I guess we should have some InconcistentMetrics similar to what we had for jormungandr, instead of the current silent dropMissing.

Not really though. In the case of Jörmungandr, we knew we could end up with inconsistent metrics because of the concurrent nature of our logic + the stateless API from Jörmungandr. So it was kind of expected. With cardano-node here, this is really something we do not expect, and therefore,not something we're willing to push onto the caller of this function.

…for failure) Also, now passing around spl instead of a separate server for the list stake pool endpoint.

…lets

specifications/api/swagger.yaml

Using a type family to allow jormungandr and haskell implementations to be different.

KtorZ · 2020-06-16T11:14:20Z

lib/shelley/src/Cardano/Wallet/Shelley/Api/Server.hs

+            stake <- withWorkerCtx shelley wid liftE liftE $ \wrk -> do
+                (w, _, pending) <- liftHandler $ W.readWallet wrk wid
+                return $ Coin $ fromIntegral $ totalBalance pending w
+            liftHandler $ listStakePools spl stake


This should eventually include the reward balance as well.

KtorZ · 2020-06-16T11:16:11Z

lib/shelley/src/Cardano/Wallet/Shelley/Network.hs

+        rewardsPerAccount <- fromNonMyopicMemberRewards <$> handleQueryFailure
+            (queue `send` CmdQueryLocalState pt (OC.GetNonMyopicMemberRewards toStake))
+        pparams <- handleQueryFailure
+            (queue `send` CmdQueryLocalState pt OC.GetCurrentPParams)


This one feels quite redundant / awkward. It sure is needed by the caller to compute few things on the pool data, but the caller should simply call getProtocolParameters, don't you think ?

This code guarantees that the pparams are valid at the same point pt.

If we were to instead use

getProtocolParameters :: m ProtocolParameters

We lose that control, and introduce potential race conditions. Even if unlikely, that feels incredibly wrong to me.

KtorZ · 2020-06-16T11:16:32Z

lib/shelley/src/Cardano/Wallet/Shelley/Network.hs

-- Temporary
-
-notImplemented :: HasCallStack => String -> a
-notImplemented what = error ("Not implemented: " <> what)


KtorZ · 2020-06-16T11:17:22Z

lib/shelley/src/Cardano/Wallet/Shelley/Pools.hs

+        { nonMyopicMemberRewards = Quantity 0
+        , relativeStake = s
+        , saturation = (sat s)
+        }


Doesn't the spec suggests something about this? Should we order pool randomly in that case?

I thought that was when we don't have enough data.

I would think that ranking according to the rewards if the user had money, would be a better thing to do.

A user can't delegate without any money anyway.

We could include ROI in % and return null for the non-myopic member rewards.

KtorZ · 2020-06-16T11:18:07Z

lib/shelley/src/Cardano/Wallet/Shelley/Pools.hs

+    _knownPools = do
+        pt <- getTip
+        res <- runExceptT $ map fst . Map.toList
+            . combineLsqData <$> stakeDistribution nl pt dummyCoin


This shouldn't use the network layer but use the database. Yet, you'll need the work on my branch for that. I'll modify this function in there.

👍 Though why?

I know knownPools are used to validate the users's choice of delegation.

The pool worker is less likely to be in sync with the network (more downstream) than the node.

Wouldn't implementing it in terms of the LSQ make more sense?

KtorZ · 2020-06-16T11:18:57Z

bors try

KtorZ · 2020-06-16T11:20:24Z

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

@@ -163,7 +169,8 @@ data StakePoolLayer e m = StakePoolLayer
 -- The pool productions and stake distrubtions in the db can /never/ be from
 -- different forks such that it's safe for readers to access it.
 monitorStakePools
-    :: Tracer IO StakePoolLog
+    :: GetStakeDistribution t IO ~ GetStakeDistribution Jormungandr IO


Any reason for not writing:

:: GetStakeDistribution Jormungandr IO

?

Yes - allowing other targets than Jormungandr in the tests :S https://github.com/input-output-hk/cardano-wallet/blob/1fab8e511360af8e567a6f2fd4e9e7b77c3df917/lib/jormungandr/test/unit/Cardano/Pool/Jormungandr/MetricsSpec.hs#L278-L281

I am thinking we should get rid of the NetworkLayer (or break it up) later and the need for the type families (and this weirdness)

Okay, this is required for module Cardano.Pool.Jormungandr.MetricsSpec and the polymorphism on the target.

I am thinking we should get rid of the NetworkLayer (or break it up) later and the need for the type families (and this weirdness)

Although the source of the problem isn't much the NetworkLayer here, but the double-support Jormungandr / cardano-node 🙃
Another abstraction could work better, but it'd still have to deal with backend discrepancies.

iohk-bors · 2020-06-16T13:33:37Z

try

Timed out

Anviking self-assigned this Jun 12, 2020

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

This was linked to issues Jun 12, 2020

List id and stake of pools #1718

Closed

Order stake pools by non-myopic member rewards #1720

Closed

Anviking force-pushed the anviking/ADP-311/shelley-stake-pool-api branch 2 times, most recently from d9ebdfe to a63516f Compare June 12, 2020 20:32

Anviking force-pushed the anviking/ADP-311/list-pools branch from 9b25d56 to a9064eb Compare June 12, 2020 20:49

Anviking force-pushed the anviking/ADP-311/shelley-stake-pool-api branch from a63516f to cd63108 Compare June 13, 2020 08:08

Anviking force-pushed the anviking/ADP-311/list-pools branch from a9064eb to 356e55a Compare June 13, 2020 09:23

Anviking force-pushed the anviking/ADP-311/shelley-stake-pool-api branch from cd63108 to 5022e3f Compare June 13, 2020 09:27

Anviking force-pushed the anviking/ADP-311/list-pools branch from 356e55a to 8b900d8 Compare June 13, 2020 10:48

Base automatically changed from anviking/ADP-311/shelley-stake-pool-api to master June 13, 2020 11:25

Anviking force-pushed the anviking/ADP-311/list-pools branch 2 times, most recently from 5259ebf to 85e2c05 Compare June 13, 2020 14:12

Anviking marked this pull request as ready for review June 13, 2020 14:18

Anviking force-pushed the anviking/ADP-311/list-pools branch from 85e2c05 to ebca9b7 Compare June 13, 2020 19:15

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

Try #1753:

6097914

Anviking commented Jun 13, 2020

View reviewed changes

lib/shelley/src/Cardano/Wallet/Shelley/Network.hs Outdated Show resolved Hide resolved

rvl reviewed Jun 15, 2020

View reviewed changes

Allow localStateQ to have multiple queries in the same queue

4536083

To allow both PParams, and StakeDistribution checks in the same queue.

Anviking force-pushed the anviking/ADP-311/list-pools branch from ebca9b7 to b776da2 Compare June 15, 2020 11:25

Anviking changed the base branch from master to anviking/ADP-311/bump-node June 15, 2020 11:26

Anviking added 3 commits June 15, 2020 13:46

List stake pools

2c00aa8

- Return dummy rewards, cost, margin, productions for now - Embed LSQ queue in cursor - Remove mock knownPools

Remove mock pool ids from integration tests

c9050cd

Add integration test for listing stake pools

31c7cb5

Anviking force-pushed the anviking/ADP-311/list-pools branch from b776da2 to 31c7cb5 Compare June 15, 2020 11:46

KtorZ reviewed Jun 15, 2020

View reviewed changes

lib/shelley/src/Cardano/Wallet/Shelley/Pools.hs Outdated Show resolved Hide resolved

KtorZ reviewed Jun 15, 2020

View reviewed changes

lib/shelley/src/Cardano/Wallet/Shelley/Pools.hs Outdated Show resolved Hide resolved

KtorZ reviewed Jun 15, 2020

View reviewed changes

lib/shelley/src/Cardano/Wallet/Shelley/Pools.hs Outdated Show resolved Hide resolved

KtorZ reviewed Jun 15, 2020

View reviewed changes

Anviking added 8 commits June 15, 2020 16:03

Actually read wallet balance when listing pools (w/ test and swagger …

cdc4ad7

…for failure) Also, now passing around spl instead of a separate server for the list stake pool endpoint.

Generalize fromRewards, rename to fromNonMyopicMemberRewards

b36cfe6

Fix mislabeled safeConvert

5f93fcd

Rename stake -> relativeStake

05256b7

Rename stakeButNoRew -> stakeButNoRewards

c6ddbea

askNode -> fetchLsqPoolMetrics, improve docs

b65293b

Adjust comment on stakeButNoRewards merge-case

cacff55

Add: STAKE_POOLS_LIST_06 - NonMyopicMemberRewards are 0 for empty wal…

0ec2e2b

…lets

KtorZ reviewed Jun 15, 2020

View reviewed changes

specifications/api/swagger.yaml Outdated Show resolved Hide resolved

Anviking added 2 commits June 15, 2020 20:01

fixup: embed responsesErr404 in responsesListStakePools itself

da27707

Move shelley LSQ code to Network module

4ba37cf

Using a type family to allow jormungandr and haskell implementations to be different.

Anviking force-pushed the anviking/ADP-311/list-pools branch from 95a8af8 to 4ba37cf Compare June 16, 2020 10:44

KtorZ reviewed Jun 16, 2020

View reviewed changes

KtorZ approved these changes Jun 16, 2020

View reviewed changes

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

Try #1753:

98e307b

KtorZ reviewed Jun 16, 2020

View reviewed changes

KtorZ merged commit c04336f into anviking/ADP-311/bump-node Jun 16, 2020

KtorZ deleted the anviking/ADP-311/list-pools branch June 16, 2020 13:00

Anviking mentioned this pull request Jun 22, 2020

Use and combine chain data with LSQ data for stake pools #1769

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

List stake pools #1753

List stake pools #1753

Anviking commented Jun 12, 2020 •

edited

Loading

Anviking commented Jun 13, 2020

iohk-bors bot commented Jun 13, 2020

rvl left a comment

Anviking commented Jun 15, 2020

KtorZ commented Jun 15, 2020

KtorZ Jun 15, 2020

KtorZ Jun 15, 2020

Anviking Jun 15, 2020 •

edited

Loading

Anviking Jun 15, 2020 •

edited

Loading

KtorZ Jun 15, 2020

KtorZ Jun 16, 2020

KtorZ Jun 16, 2020

Anviking Jun 16, 2020

KtorZ Jun 16, 2020

KtorZ Jun 16, 2020

Anviking Jun 16, 2020

KtorZ Jun 16, 2020

Anviking Jun 16, 2020 •

edited

Loading

KtorZ commented Jun 16, 2020

KtorZ Jun 16, 2020

Anviking Jun 16, 2020

Anviking Jun 16, 2020

KtorZ Jun 16, 2020

KtorZ Jun 16, 2020

iohk-bors bot commented Jun 16, 2020

List stake pools #1753

List stake pools #1753

Conversation

Anviking commented Jun 12, 2020 • edited Loading

Issue Number

Overview

Comments

Anviking commented Jun 13, 2020

iohk-bors bot commented Jun 13, 2020

try

Build failed

rvl left a comment

Choose a reason for hiding this comment

Anviking commented Jun 15, 2020

KtorZ commented Jun 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Anviking Jun 15, 2020 • edited Loading

Choose a reason for hiding this comment

Anviking Jun 15, 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

Choose a reason for hiding this comment

Anviking Jun 16, 2020 • edited Loading

Choose a reason for hiding this comment

KtorZ commented Jun 16, 2020

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

iohk-bors bot commented Jun 16, 2020

try

Timed out

Anviking commented Jun 12, 2020 •

edited

Loading

Anviking Jun 15, 2020 •

edited

Loading

Anviking Jun 15, 2020 •

edited

Loading

Anviking Jun 16, 2020 •

edited

Loading