fix: claimed in staking info endpoint #1445

Imod7 · 2024-06-04T12:05:58Z

Description

This PR restores the field claimed (under staking) in the staking-info endpoint's response which was "broken". 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)

For the Reviewer

Please refer to this hackMD doc for more information and a step-by-step on how to check, test and review this PR.

Todos

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 :

Removing the returned field lastReward for early eras and adding claimedRewards updated accordingly.
Restoring the claimedRewards field name after the breaking change of renaming it to legacyClaimedRewards
Changing the structure/type of the returned claimedRewards field (under staking) : before we had an array of eras that were claimed, now we have an object with the list of eras and their corresponding status
Refactoring the whole logic that returns the claimedRewards
Adding the possibility to query nominator's claimed Rewards apart the validator's.

These changes should be explicitly mentioned in the corresponding Changelog and Release Notes of the release in which this PR is included.

IkerAlus · 2024-06-07T08:56:47Z

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 · 2024-06-07T10:07:00Z

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 · 2024-06-13T15:41:57Z

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)

src/services/accounts/AccountsStakingInfoService.ts

marshacb

LGTM! Just had a question about unwraps

IkerAlus · 2024-06-21T15:55:44Z

docs/src/openapi-v1.yaml

+            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.


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

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 · 2024-06-21T16:02:58Z

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 · 2024-06-24T07:52:07Z

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 · 2024-06-26T10:58:57Z

@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

- Added validator and nominator status types to allow specific status values for each - Updated existing test

fix: claimed in staking info endpoint

c65010a

Imod7 requested a review from a team as a code owner June 4, 2024 12:05

fix existing tests

50943e1

IkerAlus requested a review from bee344 June 4, 2024 15:21

Imod7 added 2 commits June 7, 2024 21:09

add test for kusama (includes era 6508)

82457aa

updated docs

9012e24

Imod7 mentioned this pull request Jun 13, 2024

Use pending_rewards for claimed info in related endpoints #1452

Open

Imod7 added 6 commits June 14, 2024 20:06

implement new logic & update test responses

f320c08

add test for partially claimed & overview = null

2aa5996

updated docs

de06c6b

output up until 84 eras & update tests

8c47eda

merge master, update historical tests & code for lastReward

56e3738

Remove yarn.lock files from PR

9e838e8

marshacb reviewed Jun 20, 2024

View reviewed changes

src/services/accounts/AccountsStakingInfoService.ts Outdated Show resolved Hide resolved

marshacb reviewed Jun 20, 2024

View reviewed changes

src/services/accounts/AccountsStakingInfoService.ts Outdated Show resolved Hide resolved

marshacb approved these changes Jun 20, 2024

View reviewed changes

Imod7 added 2 commits June 21, 2024 10:34

check before unwrapping & fix types

753f3ee

run linter

7ac0236

IkerAlus reviewed Jun 21, 2024

View reviewed changes

docs: added links & updated schema description

171e10a

Imod7 mentioned this pull request Jun 24, 2024

fix: staking payouts claimed with new logic & statuses #1457

Open

1 task

Imod7 added 3 commits August 5, 2024 18:41

updated logic of claimed field for validator account

88be07e

- removed `partially claimed` value - added `undefined` value - updated tests

fix for claimed in case of validator account

cd66735

- bringing back `partially claimed` for validator

moved validator logic in separate function

00a54a0

Imod7 added 6 commits September 16, 2024 16:21

adding first draft of nominator logic

df175e8

nominator logic when validator era partially claimed

0829f44

- Added validator and nominator status types to allow specific status values for each - Updated existing test

added test for nominator

22ad9f7

Merged master, resolve conflicts & rebuilt docs

b04c28f

run linter

4a2cba0

abstracting logic into separate functions

343f2cb

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: claimed in staking info endpoint #1445

fix: claimed in staking info endpoint #1445

Imod7 commented Jun 4, 2024 •

edited

Loading

IkerAlus commented Jun 7, 2024

Imod7 commented Jun 7, 2024

Imod7 commented Jun 13, 2024 •

edited

Loading

marshacb left a comment

IkerAlus Jun 21, 2024 •

edited

Loading

Imod7 Jun 24, 2024

IkerAlus commented Jun 21, 2024

Imod7 commented Jun 24, 2024

Imod7 commented Jun 26, 2024

fix: claimed in staking info endpoint #1445

Are you sure you want to change the base?

fix: claimed in staking info endpoint #1445

Conversation

Imod7 commented Jun 4, 2024 • edited Loading

Description

Suggested Fix

For the Reviewer

Todos

Nice to have

Changelog

IkerAlus commented Jun 7, 2024

Imod7 commented Jun 7, 2024

Imod7 commented Jun 13, 2024 • edited Loading

marshacb left a comment

Choose a reason for hiding this comment

IkerAlus Jun 21, 2024 • edited Loading

Choose a reason for hiding this comment

Imod7 Jun 24, 2024

Choose a reason for hiding this comment

IkerAlus commented Jun 21, 2024

Imod7 commented Jun 24, 2024

Imod7 commented Jun 26, 2024

Imod7 commented Jun 4, 2024 •

edited

Loading

Imod7 commented Jun 13, 2024 •

edited

Loading

IkerAlus Jun 21, 2024 •

edited

Loading