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

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
Open

fix: added staking.claimed() in replacement of staking.ledger().legacyClaimedRewards #1436

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
b62cb78
fix for legacyClaimedRewards after staking v14
bee344 May 3, 2024
16b5537
missed bug
bee344 May 3, 2024
c3426fd
fixed test
bee344 May 4, 2024
c04d281
linting
bee344 May 4, 2024
0dd2f54
remove redundant check
bee344 May 5, 2024
41799cc
bug fix
bee344 May 6, 2024
ead106a
Merge branch 'master' into anp-v14-claimed
bee344 May 6, 2024
f1c4da8
updated eras checks
bee344 May 14, 2024
3844db3
removed stakingVersion
bee344 May 14, 2024
c04adb1
recommit for removing stakingVersion
bee344 May 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/services/accounts/AccountsStakingPayoutsService.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import {
erasValidatorPrefsAt,
erasValidatorRewardAt,
ledgerAt,
palletVersionAt,
} from '../test-helpers/mock/accounts';
import { AccountsStakingPayoutsService } from './AccountsStakingPayoutsService';

Expand Down Expand Up @@ -59,6 +60,7 @@ const mockHistoricApi = {
erasStakersClipped: {
entries: erasStakersClippedAt,
},
palletVersion: palletVersionAt,
},
},
} as unknown as ApiDecoration<'promise'>;
Expand Down
24 changes: 21 additions & 3 deletions src/services/accounts/AccountsStakingPayoutsService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import type {
DeriveEraNominatorExposure,
DeriveEraValidatorExposure,
} from '@polkadot/api-derive/staking/types';
import { Compact, Option, StorageKey, u32, u128 } from '@polkadot/types';
import { Compact, Option, StorageKey, u16, u32, u128 } from '@polkadot/types';
import { Vec } from '@polkadot/types';
import type {
AccountId,
Expand Down Expand Up @@ -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


/**
* Given https://github.com/polkadot-js/api/issues/5232,
Expand Down Expand Up @@ -192,6 +193,7 @@ export class AccountsStakingPayoutsService extends AbstractService {
// Create an array of `DeriveEraExposure`
allErasGeneral.map((eraGeneral) => eraGeneral[0]),
isKusama,
stakingVersion.toNumber(),
).catch((err: Error) => {
throw this.createHttpErrorForAddr(address, err);
});
Expand Down Expand Up @@ -327,6 +329,7 @@ export class AccountsStakingPayoutsService extends AbstractService {
startEra: number,
deriveErasExposures: IAdjustedDeriveEraExposure[],
isKusama: boolean,
stakingVersion: number,
): Promise<ICommissionAndLedger[][]> {
// Cache StakingLedger to reduce redundant queries to node
const validatorLedgerCache: { [id: string]: PalletStakingStakingLedger } = {};
Expand All @@ -341,7 +344,14 @@ export class AccountsStakingPayoutsService extends AbstractService {
}

const singleEraCommissions = nominatedExposures.map(({ validatorId }) =>
this.fetchCommissionAndLedger(historicApi, validatorId, currEra, validatorLedgerCache, isKusama),
this.fetchCommissionAndLedger(
historicApi,
validatorId,
currEra,
validatorLedgerCache,
isKusama,
stakingVersion,
),
);

return Promise.all(singleEraCommissions);
Expand Down Expand Up @@ -471,6 +481,7 @@ export class AccountsStakingPayoutsService extends AbstractService {
era: number,
validatorLedgerCache: { [id: string]: PalletStakingStakingLedger },
isKusama: boolean,
stakingVersion: number,
): Promise<ICommissionAndLedger> {
let commission: Perbill;
let validatorLedger;
Expand Down Expand Up @@ -513,9 +524,16 @@ 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.

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.

}

validatorLedger = validatorLedgerOption.unwrap();
validatorLedgerCache[validatorId] = validatorLedger;
}

Expand Down
2 changes: 2 additions & 0 deletions src/services/test-helpers/mock/accounts/stakingPayouts.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,8 @@ export const erasRewardPointsAt = (_: EraIndex) =>
),
);

export const palletVersionAt = () => Promise.resolve().then(() => api.registry.createType('u16', '13'));

export const deriveEraExposureParam = {
era: api.registry.createType('EraIndex', ERA),
nominators: {
Expand Down
Loading