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 api swagger spec for shelley stake pools #1744

Merged
merged 1 commit into from
Jun 13, 2020
Merged

Add api swagger spec for shelley stake pools #1744

merged 1 commit into from
Jun 13, 2020

Conversation

Anviking
Copy link
Member

@Anviking Anviking commented Jun 11, 2020 •
edited
Loading

Issue Number

#1718

Overview

  • Added swagger spec for shelley pools
  • The entire Api type is now parameterised over apiPool
  • ⚠️ Breaking: Make both the haskell and jormungadnr API require a wallet id to list stake pools

Comments

@Anviking Anviking self-assigned this Jun 11, 2020
KtorZ
KtorZ reviewed Jun 12, 2020
View reviewed changes
specifications/api/swagger.yaml Outdated
produced_blocks:
<<: *numberOfBlocks
description: Number of blocks produced by a given stake pool.
apparent_performance: *stakePoolApparentPerformance
Copy link
Member

Choose a reason for hiding this comment

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

Do we actually still have this one on cardano-node ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Are we going to display some form of produced_blocks? Then why not? Though it will be a different apparent performance from that used to calculate the non-myopic rewards.

Copy link
Member

Choose a reason for hiding this comment

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

We also need the pool's stake on the previous epoch, do we have that?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure. If LSQ worked for k blocks old, then we have stake distributions for the k latest blocks.

Should we remove?

Should we not also remove produced_blocks? (Or have say lifetime_blocks if we keep it?)

Copy link
Member

Choose a reason for hiding this comment

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

Ideally we would have both last production and total production. Though we do not depend on the LSQ for these, we have this information from the chain sync data.

I'm not sure. If LSQ worked for k blocks old, then we have stake distributions for the k latest blocks. Should we remove?

Well, without the stake distribution of past epochs, we are running into the same problem / awkwardness as during the ITN where we would have holes in our dataset. I'd rather not go through this again.

Copy link
Member Author

@Anviking Anviking Jun 12, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Well, without the stake distribution of past epochs, we are running into the same problem / awkwardness as during the ITN where we would have holes in our dataset. I'd rather not go through this again.

To be fair: We are guaranteed to have k blocks with corresponding stake distributions (except for at genesis). On the ITN we weren't guaranteed to have that.

Copy link
Member Author

Choose a reason for hiding this comment

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

What would the point of last_production be, and what would the window be? Wouldn't it just be to give users some notion of the apparent performance?

Then why not either remove both last_production and apparent_performance, or keep both? 🤔

@Anviking Anviking force-pushed the anviking/ADP-311/shelley-stake-pool-api branch 2 times, most recently from ab13993 to cedcba9 Compare June 12, 2020 09:47
Anviking
Anviking commented Jun 12, 2020
View reviewed changes
specifications/api/swagger.yaml Outdated
x-non-myopic-member-rewards: &nonMyopicMemberRewards
<<: *amount
description: |
Non-myopic member rewards is the recomended way to rank stake pools. It is the rewards the
Copy link
Member Author

Choose a reason for hiding this comment

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

Perhaps this is a weird way to describe it.

E.g. for what unit of time do the rewards correspond to? (I think it is an epoch.)

Copy link
Member

Choose a reason for hiding this comment

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

Non-myopic member rewards = rewards one can expect at the end of an epoch from a fully saturated pool.

Copy link
Member Author

Choose a reason for hiding this comment

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

I rephrased in 44e6ff8

epoch from a fully saturated pool

That is misleading though? It is assuming the top k pools are saturated, and the rest are empty (except the user's stake… and pledge?), or something like that. I didn't mention on those specifics in the commit.

@Anviking Anviking changed the title Swagger and Api types for shelley stake pools Add api swagger spec for shelley stake pools Jun 12, 2020
@Anviking Anviking force-pushed the anviking/ADP-311/liberate-stakepool-api branch from 1e27e7e to 8ac875f Compare June 12, 2020 13:03
@KtorZ KtorZ added the ADDING FEATURE Mark a PR as adding a new feature, for auto-generated CHANGELOG label Jun 12, 2020
@Anviking Anviking force-pushed the anviking/ADP-311/liberate-stakepool-api branch 2 times, most recently from 9c249fb to a42e9ae Compare June 12, 2020 13:08
@Anviking Anviking force-pushed the anviking/ADP-311/shelley-stake-pool-api branch from e73a779 to 78ab757 Compare June 12, 2020 13:59
Base automatically changed from anviking/ADP-311/liberate-stakepool-api to master June 12, 2020 14:31
@KtorZ KtorZ force-pushed the anviking/ADP-311/shelley-stake-pool-api branch from d42dd19 to 46c04c9 Compare June 12, 2020 15:28
KtorZ
KtorZ reviewed Jun 12, 2020
View reviewed changes
specifications/api/swagger.yaml Outdated
The rewards the wallet can expect to receive at the end of an epoch, in the long term, if delegating to
this pool.

For more details, see the "Design Specification for Delegation and Incentives in Cardano" document.
Copy link
Member

@KtorZ KtorZ Jun 12, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

We could link it here (the description is actually plain markdown)

KtorZ
KtorZ reviewed Jun 12, 2020
View reviewed changes
specifications/api/swagger.yaml Outdated

Some pools _may_ also have metadata attached to them.

The `non_myopic_member_rewards` — and thus the ordering — depends on the balance of the given wallet.
Copy link
Member

Choose a reason for hiding this comment

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

Small note, I'd actually swap these two sentences. So that it we give precision on the ordering after actually mentioning that pools are ordered by descending member rewards.

Copy link
Member

Choose a reason for hiding this comment

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

List all known stake pools ordered by descending non_myopic_member_rewards. The non_myopic_member_rewards — and thus the ordering — depends on the balance of the given wallet.

/!\ On the incentivized testnet, pools are instead ordered by descending desirability.

Some pools may also have metadata attached to them.

KtorZ
KtorZ reviewed Jun 12, 2020
View reviewed changes
specifications/api/swagger.yaml Outdated
produced_blocks:
<<: *numberOfBlocks
description: Number of blocks produced by a given stake pool
apparent_performance: *stakePoolApparentPerformance
Copy link
Member

Choose a reason for hiding this comment

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

Let's remove that one as discussed.

@Anviking
Copy link
Member Author

bors r+

@Anviking Anviking marked this pull request as ready for review June 12, 2020 17:50
iohk-bors bot added a commit that referenced this pull request Jun 12, 2020
@iohk-bors @Anviking
Merge #1744
fda8fe3 
1744: Add api swagger spec for shelley stake pools r=Anviking a=Anviking

# Issue Number

#1718 

# Overview

<!-- Detail in a few bullet points the work accomplished in this PR -->
- [x] Added swagger spec for shelley pools
- [x] ⚠️ Breaking: Make both the haskell and jormungadnr API require a wallet id to list stake pools

# 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: Johannes Lund <[email protected]>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Jun 12, 2020

Build failed

paweljakubas
paweljakubas approved these changes Jun 12, 2020
View reviewed changes
Copy link
Contributor

@paweljakubas paweljakubas left a comment

Choose a reason for hiding this comment

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

LGTM!

@Anviking Anviking force-pushed the anviking/ADP-311/shelley-stake-pool-api branch from 9529c4f to 282785f Compare June 12, 2020 19:55
Anviking
Anviking commented Jun 12, 2020
View reviewed changes
lib/core/test/unit/Cardano/Wallet/ApiSpec.hs Outdated
-- that differs, such that we validate both.
type CustomApi n =
Api n ApiStakePool
:<|> ("jormungandr" :> StakePools n ApiJormungandrStakePool)
Copy link
Member Author

Choose a reason for hiding this comment

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

@KtorZ I hope you don't mind this

@Anviking
Copy link
Member Author

bors try

iohk-bors bot added a commit that referenced this pull request Jun 12, 2020
@iohk-bors
Try #1744:
f518af8
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Jun 12, 2020

try

Build failed

@Anviking Anviking force-pushed the anviking/ADP-311/shelley-stake-pool-api branch 3 times, most recently from a63516f to cd63108 Compare June 13, 2020 08:08
@Anviking
Add swagger schema and types for shelley pool
5022e3f 
And make listing stake pools require a wallet:
Old: GET /stake-pools
New: GET /wallets/{id}/stake-pools

And parameterise the full API type over apiPool.

The current jormungandr specific types and schemas were renamed
appropriately, e.g:
ApiStakePoolMetrics -> ApiJormungandrStakePoolMetrics
@Anviking Anviking force-pushed the anviking/ADP-311/shelley-stake-pool-api branch from cd63108 to 5022e3f Compare June 13, 2020 09:27
@Anviking
Copy link
Member Author

bors r+

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Jun 13, 2020

Build succeeded

@iohk-bors iohk-bors bot merged commit 1fd73c2 into master Jun 13, 2020
@iohk-bors iohk-bors bot deleted the anviking/ADP-311/shelley-stake-pool-api branch June 13, 2020 11:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@paweljakubas paweljakubas paweljakubas approved these changes

@KtorZ KtorZ KtorZ approved these changes

Assignees

@Anviking Anviking

Labels
ADDING FEATURE Mark a PR as adding a new feature, for auto-generated CHANGELOG
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Anviking @KtorZ @paweljakubas