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 5 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
43 changes: 38 additions & 5 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 @@ -224,7 +226,9 @@ export class AccountsStakingPayoutsService extends AbstractService {

return {
at,
erasPayouts: allEraData.map((eraData) => this.deriveEraPayouts(address, unclaimedOnly, eraData, isKusama)),
erasPayouts: allEraData.map((eraData) =>
this.deriveEraPayouts(address, unclaimedOnly, eraData, isKusama, Number(stakingVersion)),
),
};
}

Expand Down Expand Up @@ -327,6 +331,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 +346,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 All @@ -362,6 +374,7 @@ export class AccountsStakingPayoutsService extends AbstractService {
unclaimedOnly: boolean,
{ deriveEraExposure, eraRewardPoints, erasValidatorRewardOption, exposuresWithCommission, eraIndex }: IEraData,
isKusama: boolean,
stakingVersion: number,
): IEraPayouts | { message: string } {
if (!exposuresWithCommission) {
return {
Expand Down Expand Up @@ -409,8 +422,10 @@ export class AccountsStakingPayoutsService extends AbstractService {
* any reward data.
*/
let indexOfEra: number;
if (validatorLedger.legacyClaimedRewards) {
if (validatorLedger.legacyClaimedRewards && stakingVersion < 14) {
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

indexOfEra = 0;
} else if ((validatorLedger as unknown as StakingLedger).claimedRewards) {
indexOfEra = (validatorLedger as unknown as StakingLedger).claimedRewards.indexOf(eraIndex);
} else if ((validatorLedger as unknown as StakingLedgerTo240).lastReward) {
Expand Down Expand Up @@ -471,6 +486,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 +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.

validatorLedger = validatorLedgerOption.unwrap();
} else {
const claimed = await historicApi.query.staking.claimedRewards(era, validatorControllerOption.unwrap());
if (claimed.length > 0) {
const validatorLedgerOpts = validatorLedgerOption.unwrap();
validatorLedger = {
stash: validatorLedgerOpts.stash,
total: validatorLedgerOpts.total,
active: validatorLedgerOpts.active,
unlocking: validatorLedgerOpts.unlocking,
legacyClaimedRewards: claimed,
} as PalletStakingStakingLedger;
} else {
return {
commission,
};
}
}

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