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: added staking.claimed() in replacement of staking.ledger().legacyClaimedRewards #1436

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

Conversation

bee344
Copy link
Contributor

@bee344 bee344 commented May 3, 2024

Description

After the staking pallet was upgraded to v14 in Polkadot, legacyClaimedRewards is no longer used, so any for any eras after 1418, the era is no longer stored there but in staking.claimed(Era, Account), so querying the former would make it look like the rewards weren't claimed when in turn they were, as you can see here:
imagen

With this PR it checks the staking pallet version, and if it's less than 14, it sticks to the old logic, otherwise staking.claimed takes the place of legacyClaimedRewards, as seen here.

With this fix, it returns the correct status:
imagen

NOTE staking.claimed() returns the page of the claimed reward, while legacyClaimedRewards instead contains the list of eras claimed. So we assume that if staking.claimed() returns anything other than an empty array, the era was claimed.

Edit: added image of response after fix
Edit 2: wrong era

@bee344 bee344 marked this pull request as ready for review May 5, 2024 01:06
@bee344 bee344 requested a review from a team as a code owner May 5, 2024 01:06
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!

@@ -152,6 +152,7 @@ export class AccountsStakingPayoutsService extends AbstractService {
const startEra = Math.max(0, sanitizedEra - (depth - 1));
const runtimeInfo = await this.api.rpc.state.getRuntimeVersion(at.hash);
const isKusama = runtimeInfo.specName.toString().toLowerCase() === 'kusama';
const stakingVersion = await historicApi.query.staking.palletVersion<u16>();
Copy link
Member

Choose a reason for hiding this comment

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

This is an awesome call. Has this been around for a bit? Is it safe to use historically?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, it has been around since v13. Before that it was api.query.staking.storageVersion() and it returned a type Releases first and then PalletStakingReleases, that went from V0 to V12_0_0

@@ -513,9 +529,26 @@ export class AccountsStakingPayoutsService extends AbstractService {
return {
commission,
};
} else if (stakingVersion < 14) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I have serious doubts about using staking.palletVersion.

By adding this check here for older eras even if legacyClaimedRewards has values it is skipping and checking the claimedRewards which does not have any values for that era. So then it does not return payouts.

Example
For account 1123RekaPHgWaPL5v9qfsikRemeZdYC4tvKXYuLXwhfT3NKy and era 1366 it should give payouts but it does not.

http://127.0.0.1:8080/accounts/1123RekaPHgWaPL5v9qfsikRemeZdYC4tvKXYuLXwhfT3NKy/staking-payouts?unclaimedOnly=false&era=1366

Payouts shown in Subscan

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made some changes and reduced the use of the stakingVersion. The bug you mention was a few lines below, but I changed it so that if the stakingVersion is below 14, it goes with the old logic, but if it's not, instead of replacing validatorLedger.legacyClaimedRewards with claimedRewards, the value of the era gets added to the array.

indexOfEra = validatorLedger.legacyClaimedRewards.indexOf(eraIndex);
} else if (stakingVersion >= 14) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If we add this check here, whenever the staking pallet version is >= 14, variable claimed will result to true correct ? That does not seem right but maybe I am missing something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Awesome catch! That's right, I changed it and should not happen now

@bee344 bee344 requested a review from Imod7 May 6, 2024 11:56
Comment on lines 527 to 534
} else if (stakingVersion < 14) {
validatorLedger = validatorLedgerOption.unwrap();
} else {
validatorLedger = validatorLedgerOption.unwrap();
const claimed = await historicApi.query.staking.claimedRewards(era, validatorControllerOption.unwrap());
if (claimed.length > 0) {
validatorLedger.legacyClaimedRewards.push(era as unknown as u32);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
} else if (stakingVersion < 14) {
validatorLedger = validatorLedgerOption.unwrap();
} else {
validatorLedger = validatorLedgerOption.unwrap();
const claimed = await historicApi.query.staking.claimedRewards(era, validatorControllerOption.unwrap());
if (claimed.length > 0) {
validatorLedger.legacyClaimedRewards.push(era as unknown as u32);
}
}
validatorLedger = validatorLedgerOption.unwrap();
if (historicApi.query.staking.claimedRewards) {
const claimed = await historicApi.query.staking.claimedRewards(era, validatorControllerOption.unwrap());
if (claimed.length > 0) {
const eraVal: u32 = historicApi.registry.createType('u32', era);
validatorLedger.legacyClaimedRewards.push(eraVal);
}
  1. Since in both cases we unwrap, we can put it outside the ifs and that way we call it once.

  2. We could avoid checking stakingVersion if we just check if claimedRewards exists. That way we don't need an extra call or to pass stakingVersion through the methods.

  3. I changed how you push the era because if we push era as unknown as u32, it messes up with the types of the Vec.

    • If you check in debug mode validatorLedger.legacyClaimedRewards you will see before the push it is
    • Type(8) [u32, u32, u32, u32, u32, u32, u32, u32
    • and after adding the additional era, it becomes Type(8) [u32, u32, u32, u32, u32, u32, u32, u32, 6585
    • and then calls like validatorLedger.legacyClaimedRewards.toHuman() also break

    Note: the use of createType is discouraged but in this case, I only found this way to make it work.

  4. Since we do not use the variable claimed we do not have to store the call. We could just do

    • if ((await historicApi.query.staking.claimedRewards(era, validatorControllerOption.unwrap())).length > 0) {
    • but this is up to you! By storing, the code is also more clear also so yeah.
  5. Question: Why when it retrieves the claimedRewards for the era the result is 0 ? is it something to be fixed in pjs-api ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. Done.
  2. Done.
  3. Nice one, done.
  4. Done.
  5. claimedRewards returns the index of the reward pages that were claimed. So if only the first page was claimed, it returns 0, if pages 1 and 2, 0,1, and so on.

Copy link
Contributor

@Imod7 Imod7 left a comment

Choose a reason for hiding this comment

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

There is another thing to take into account either in this PR or I will add an issue to make the change in the future.
legacyClaimed... will be removed (related PR) so when this happens, this line

validatorLedger.legacyClaimedRewards.push(eraVal);

will break. For now we can add a check if it exists

if (validatorLedger.legacyClaimedRewards) {
...
}

Copy link
Contributor

@Imod7 Imod7 left a comment

Choose a reason for hiding this comment

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

Due to the complexity and the number of changes, I checked out a new branch from your branch of this PR so you can review & test. All changes are mentioned here

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 this pull request may close these issues.

4 participants