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

Order stake pools by non-myopic member rewards #1720

Closed
Anviking opened this issue Jun 3, 2020 · 14 comments · Fixed by #1753
Closed

Order stake pools by non-myopic member rewards #1720

Anviking opened this issue Jun 3, 2020 · 14 comments · Fixed by #1753
Assignees

Comments

@Anviking
Copy link
Member

Anviking commented Jun 3, 2020

Context

We want to implement this endpoint for (the cardano-node-) Shelley.
https://input-output-hk.github.io/cardano-wallet/api/edge/#operation/listStakePools

To help users chose between pools, the endpoint, which was added during the ITN, used to provide:

  • apparent_performance
  • desirability
  • (and saturation)

According to the delegation design spec pools should be sorted by non-myopic member rewards.

The node's has a local state query for non-myopic member rewards which we can use.

Decision

We should:

  • Order stake pools by non-myopic member rewards
  • Add non-myopic member rewards to the API response

And then:

  • Remove the fields apparent_performance, desirability, saturation?

Acceptance Criteria

  • GET /stake-pools must be ordered by non-myopic member rewards
  • GET /stake-pools should contain non-myopic member rewards in the response

Development

QA

@Anviking Anviking added this to the (ADP-311) List stake pools milestone Jun 3, 2020
@Anviking Anviking self-assigned this Jun 3, 2020
@Anviking Anviking changed the title Order stake pools in response Order stake pools by non-myopic member rewards Jun 3, 2020
@KtorZ
Copy link
Member

KtorZ commented Jun 3, 2020

Remove the fields apparent_performance, desirability, saturation?

@Anviking double-check this with Alex + Darko. I don't think this is desirable. So we probably need to push this upstream if these values aren't yet available from the local state query protocol. The saturation we can likely compute from knowing the total stake delegated to the pool. Others, it'd be preferable to get from the local state query (I don't have the formula in mind but can't we also compute the desirability from the non-myopic utilty?)

@Anviking Anviking mentioned this issue Jun 12, 2020
3 tasks
@Anviking Anviking linked a pull request Jun 12, 2020 that will close this issue
3 tasks
iohk-bors bot added a commit that referenced this issue Jun 17, 2020
1775: Allow listing pools based on stake instead of wallet id r=KtorZ a=Anviking

# Issue Number

#1720 


# Overview

<!-- Detail in a few bullet points the work accomplished in this PR -->

- [x] Make list stake pool endpoint take `Coin` instead of `WalletId`.
- [x] Adjust corresponding types and tests (A few more I think)
- [x] Fail with `err400` when query param is not present on Shelley.
- [x] Adjust swagger



# Comments

```
Originally:
GET /stake-pools

What was recently introduced:
GET /wallets/:wid/stake-pools

Now:
GET /stake-pools?stake=1000

For jormungandr the query parameter is not required, and unused. This
minimises breaking changes.
```

 - [Slack thread](https://input-output-rnd.slack.com/archives/C819S481Y/p1592322972378700)
- This PR gives greater flexibility to Daedalus and other API users

<!-- Additional comments or screenshots to attach if any -->

<!-- 
Don't forget to:

 ✓ Self-review your changes to make sure nothing unexpected slipped through
 ✓ Assign yourself to the PR
 ✓ Assign one or several reviewer(s)
 ✓ Once created, link this PR to its corresponding ticket
 ✓ Assign the PR to a corresponding milestone
 ✓ Acknowledge any changes required to the Wiki
-->


Co-authored-by: Johannes Lund <[email protected]>
iohk-bors bot added a commit that referenced this issue Jun 17, 2020
1775: Allow listing pools based on stake instead of wallet id r=Anviking a=Anviking

# Issue Number

#1720 


# Overview

<!-- Detail in a few bullet points the work accomplished in this PR -->

- [x] Make list stake pool endpoint take `Coin` instead of `WalletId`.
- [x] Adjust corresponding types and tests (A few more I think)
- [x] Fail with `err400` when query param is not present on Shelley.
- [x] Adjust swagger



# Comments

```
Originally:
GET /stake-pools

What was recently introduced:
GET /wallets/:wid/stake-pools

Now:
GET /stake-pools?stake=1000

For jormungandr the query parameter is not required, and unused. This
minimises breaking changes.
```

 - [Slack thread](https://input-output-rnd.slack.com/archives/C819S481Y/p1592322972378700)
- This PR gives greater flexibility to Daedalus and other API users

<!-- Additional comments or screenshots to attach if any -->

<!-- 
Don't forget to:

 ✓ Self-review your changes to make sure nothing unexpected slipped through
 ✓ Assign yourself to the PR
 ✓ Assign one or several reviewer(s)
 ✓ Once created, link this PR to its corresponding ticket
 ✓ Assign the PR to a corresponding milestone
 ✓ Acknowledge any changes required to the Wiki
-->


Co-authored-by: Johannes Lund <[email protected]>
iohk-bors bot added a commit that referenced this issue Jun 17, 2020
1775: Allow listing pools based on stake instead of wallet id r=Anviking a=Anviking

# Issue Number

#1720 


# Overview

<!-- Detail in a few bullet points the work accomplished in this PR -->

- [x] Make list stake pool endpoint take `Coin` instead of `WalletId`.
- [x] Adjust corresponding types and tests (A few more I think)
- [x] Fail with `err400` when query param is not present on Shelley.
- [x] Adjust swagger



# Comments

```
Originally:
GET /stake-pools

What was recently introduced:
GET /wallets/:wid/stake-pools

Now:
GET /stake-pools?stake=1000

For jormungandr the query parameter is not required, and unused. This
minimises breaking changes.
```

 - [Slack thread](https://input-output-rnd.slack.com/archives/C819S481Y/p1592322972378700)
- This PR gives greater flexibility to Daedalus and other API users

<!-- Additional comments or screenshots to attach if any -->

<!-- 
Don't forget to:

 ✓ Self-review your changes to make sure nothing unexpected slipped through
 ✓ Assign yourself to the PR
 ✓ Assign one or several reviewer(s)
 ✓ Once created, link this PR to its corresponding ticket
 ✓ Assign the PR to a corresponding milestone
 ✓ Acknowledge any changes required to the Wiki
-->


Co-authored-by: Johannes Lund <[email protected]>
@Fell-x27
Copy link

At present time this sorting can be "attacked" by fake pledge amount. Just tested my app on shelley-testnet and found this:

image

Its a non-myopic member rewards for 100K ada stake in the top-right corner and pledge of the pool 0c6fb7f6499477e3100bcc93624ab218abc58782fc79c2bcc27a91af below

So this is a vector for in-wallet ratings manipulations for now.
Is it possible to verify the pledge by somehow? Yes, I can just drop all pools with pledge value more than the total supply, but it is not the real solution of the problem.

@Anviking
Copy link
Member Author

@Fell-x27 There was a problem with the calculation of the non-myopic member rewards fixed by IntersectMBO/cardano-ledger#1601. Before that change makes it to cardano-node, the ranking will be… well, I think pretty useless.

It previously mistakingly used the pool owners' stake as the stake of the pool member about to delegate.

@Fell-x27
Copy link

Fell-x27 commented Jul 1, 2020

Well, everything I need is building master branch, right?

@Anviking
Copy link
Member Author

Anviking commented Jul 1, 2020

No, the cardano-ledger-specs dependency inside cardano-node needs to be bumped, which they do regularly

@piotr-iohk
Copy link
Contributor

I think let's keep this item in QA until the IntersectMBO/cardano-ledger#1601 goes into cardano-node. @Anviking it is not in the node 1.14.2, correct?

@Anviking
Copy link
Member Author

Anviking commented Jul 1, 2020

I think let's keep this item in QA

Yeah, I think that's sane to do.

it is not in the node 1.14.2, correct?

No, it's not even on (cardano-node-)master yet. It was updated 6 days ago, but the fix was merged yesterday.

@Fell-x27
Copy link

Fell-x27 commented Jul 1, 2020

Thanks! Will watch this.

@KtorZ
Copy link
Member

KtorZ commented Jul 1, 2020

Note: #1841

@Anviking
Copy link
Member Author

Anviking commented Jul 1, 2020

So cardano-node from IntersectMBO/cardano-node#1350 includes the fix

$ cardano-wallet-shelley stake-pool list --stake 1000000 | jq 'map(.metrics.non_myopic_member_rewards.quantity) | .[:5]'
Ok.
[
  144,
  131,
  129,
  128,
  128
]

$ cardano-wallet-shelley stake-pool list --stake 1000000000000 | jq 'map(.metrics.non_myopic_member_rewards.quantity) | .[:5]'
Ok.
[
  144460917,
  131335062,
  129802259,
  128592762,
  128223388
]

^ seems more promising

@Anviking
Copy link
Member Author

Anviking commented Jul 7, 2020

@piotr-iohk is this ok now?

@piotr-iohk
Copy link
Contributor

@Anviking I don't see difference on current wallet master and cardano-node 1.4.2:

# cardano-wallet-shelley stake-pool list --stake 100000000 | jq 'map(.metrics.non_myopic_member_rewards.quantity) | .[:5]'
Ok.
[
  7050526840142,
  3283649681,
  517920152,
  481603771,
  269570962
]
# cardano-wallet-shelley stake-pool list --stake 1 | jq 'map(.metrics.non_myopic_member_rewards.quantity) | .[:5]'
Ok.
[
  7050526840142,
  3283649681,
  517920152,
  481603771,
  269570962
]

@Anviking
Copy link
Member Author

Anviking commented Jul 7, 2020

It should be working with IntersectMBO/cardano-node@a2161ba which is what is specified on the wallet readme

@piotr-iohk
Copy link
Contributor

lgtm

iohk-bors bot added a commit that referenced this issue Jul 7, 2020
1874: test that rewards are smaller for smaller stake r=piotr-iohk a=piotr-iohk

# Issue Number

#1720

# Overview

- 3fbdeed
  test that rewards are smaller for smaller stake
  
- e941007
  compare sum of rewards from pools for small and big stake



# Comments

<!-- Additional comments or screenshots to attach if any -->

<!-- 
Don't forget to:

 ✓ Self-review your changes to make sure nothing unexpected slipped through
 ✓ Assign yourself to the PR
 ✓ Assign one or several reviewer(s)
 ✓ Once created, link this PR to its corresponding ticket
 ✓ Assign the PR to a corresponding milestone
 ✓ Acknowledge any changes required to the Wiki
-->


Co-authored-by: Piotr Stachyra <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants