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

fix: claimed in staking info endpoint #1445

Open
wants to merge 22 commits into
base: master
Choose a base branch
from
Open

Conversation

Imod7
Copy link
Contributor

@Imod7 Imod7 commented Jun 4, 2024

Description

Closes #1433

This PR restores the field claimed (under staking) in the staking-info endpoint's response which was "broken" and the root cause of this is explained here.

Suggested Fix

The suggested solution includes :

  • Adding an IStakingLedger interface to have more control on the response fields under staking.
  • Retrieving the claimed eras from :
      1. stakingLedger.legacyClaimedRewards OR stakingLedger.claimedRewards (depending on what call is available every time)
      1. AND staking.claimedRewards (if available)

Todos

  • Implement new logic based on comment
  • Update the description of this PR
  • Add a test that includes the era when the migration to the new logic happened
  • Add a test that includes also partially claimed eras & erasStakersOverview = null
  • Update docs again based on the new implementation
  • Should return a total amount of eras (and their status) that are equal or less than depth.
  • Merge master branch & update all historical tests (with new response type)
  • Handle the case in early blocks/eras where there is lastReward instead of claimedRewards under stakingLedger.
  • 20240805 : Update logic again of claimed field for validator account
  • 20240805 : Update logic again of claimed field for nominator account
  • Update docs
  • Update the description of this PR to reflect latest changes
  • Add the case of era < 518 and chain = isKusama -> the era is considered as claimed "automatically".
  • Add case of status = undefined -> which is when we do not have enough info to determine the status of the era.
  • Add the nominator logic mentioned in this comment
  • Add a separate document that outlines the implementation details related to this PR.

Nice to have

  • A test for the case staking.erasStakersOverview = null & erasStakers = 0 which can be implemented by mocking this request http://127.0.0.1:8080/accounts/CmjHFdR59QAZMuyjDF5Sn4mwTgGbKMH2cErUFuf6UT51UwS/staking-info?at=22869643

Changelog

This PR introduces breaking changes to the staking-info endpoint response by :

  1. removing the returned field lastReward for early eras and adding claimedRewards updated accordingly.
  2. changing the type of the returned claimedRewards field (under staking)
    Hence, it should be explicitly mentioned in the corresponding Changelog and Release Notes of the next release.

@Imod7 Imod7 requested a review from a team as a code owner June 4, 2024 12:05
@IkerAlus IkerAlus requested a review from bee344 June 4, 2024 15:21
@IkerAlus
Copy link
Contributor

IkerAlus commented Jun 7, 2024

The change is reasonable. Sidecar user does not care of internal renaming changes since the retrieved info has the same meaning.

Is this PR ready for review or are we waiting for the Todos list to be checked?

@Imod7
Copy link
Contributor Author

Imod7 commented Jun 7, 2024

The change is reasonable. Sidecar user does not care of internal renaming changes since the retrieved info has the same meaning.

Is this PR ready for review or are we waiting for the Todos list to be checked?

Yes, ideally the todos needs to be completed before a review. However, if any review will speed things up then please feel free to add them.

@Imod7
Copy link
Contributor Author

Imod7 commented Jun 13, 2024

Update
After getting feedback from @Ank4n, for the 2nd check mentioned in the logic section above, I concluded that the logic and thus the implementation in this PR should change to the following:

  • If staking.erasStakersOverview.pageCount == query.staking.claimedRewards -> era claimed
  • If staking.erasStakersOverview.pageCount != query.staking.claimedRewards. length -> era partially claimed
  • If overview == null && erasStakers > 0 -> pageCount = 1 : so then it depends on query.staking.claimedRewards value to see if era claimed or unclaimed

The response will also needs to be changed from a Vec of eras to a Vec of objects of type {era: eraNumber, status : claimed | unclaimed | partially claimed}

Related Resource
polkadot-js/api#5859 (comment)

Copy link
Collaborator

@marshacb marshacb left a comment

Choose a reason for hiding this comment

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

LGTM! Just had a question about unwraps

for _validators._ This array is populated with values from `stakingLedger.legacyClaimedRewards`
or `stakingLedger.claimedRewards`, as well as the `query.staking.claimedRewards` call, depending
on whether the queried block is before or after the migration. For more details on the implementation
and the migration, refer to the related PR and linked issue.
Copy link
Contributor

@IkerAlus IkerAlus Jun 21, 2024

Choose a reason for hiding this comment

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

Where/how can the user find the relevant linked info?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added specific links of the PR and linked issue. Let me know if this covers your question. I also updated the schema description since I no longer return lastReward. Changes included in this commit.

@IkerAlus
Copy link
Contributor

The response time of the endpoint is significantly slower now

Do we have specific data for this statement? How long does it take to return the staking info in the any worst case scenario (for example, 84 eras in different pages and/or with different logic)?

@Imod7
Copy link
Contributor Author

Imod7 commented Jun 24, 2024

The response time of the endpoint is significantly slower now

Do we have specific data for this statement? How long does it take to return the staking info in the any worst case scenario (for example, 84 eras in different pages and/or with different logic)?

You are right, I didn't have any formal data when I wrote that. It was based on my observations from the tests I did and also on my knowledge of the calculations of each implementation.
After adding some simple code to measure the execution time of fetchAccountStakingInfo, it looks like there are cases with similar runtimes (old code vs new code) and others have significant differences. However, these results can be influenced by factors such as the connection and the machine used. I am including below some results but to back this statement I need to run proper benchmarks.

  • http://127.0.0.1:8080/accounts/GpyTMuLmG3ADWRxhZpHQh5rqMgNpFoNUyxA1DJAXfvsQ2Ly/staking-info?at=16869643 - kusama

    • old code : 608 ms
    • new code : 1.612 ms
  • http://127.0.0.1:8080/accounts/CmjHFdR59QAZMuyjDF5Sn4mwTgGbKMH2cErUFuf6UT51UwS/staking-info?at=22869643 - kusama

    • old code : 372 ms
    • new code : 5.312 ms [this is an edge case- goes super slow]
  • http://127.0.0.1:8080/accounts/11VR4pF6c7kfBhfmuwwjWY3FodeYBKWx7ix2rsRCU2q6hqJ/staking-info?at=18157800 - polkadot

    • old code : 433 ms
    • new code : 408 ms

@Imod7
Copy link
Contributor Author

Imod7 commented Jun 26, 2024

@IkerAlus After our last convo on whether the status partially claimed is valid or not and some more feedback from the staking master @Ank4n :

For a validator to get all commission, they should claim reward for all pages of an era.

Here is the next round of conclusions about claimed:

  • When a user queries the staking-info endpoint with a validator address and at a specific era&block
    • The possible status is still : claimed, partially claimed or unclaimed
  • When a user queries a nominator address and at a specific era&block
    • @IkerAlus you were right, the possible status can only be : claimed or unclaimed

Currently, I am also considering that in both scenarios, it is useful to add the undefined status which is the case when we only have lastReward available (early eras). In such cases, we cannot determine if the queried era was claimed unless it matches the era noted in lastReward. If the queried era does not correspond to the lastReward era then claimed should be set as undefined.

- removed `partially claimed` value
- added `undefined` value
- updated tests
- bringing back `partially claimed` for validator
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants