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

Update to v14 #84

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

Conversation

kryzasada
Copy link

Update validator list and pool nominations

@youPickItUp youPickItUp self-requested a review November 4, 2024 11:01
src/contexts/Staking/index.tsx Outdated Show resolved Hide resolved
sessionUnsub.current = unsub;
});
const sessionValidatorsRaw: AnyApi =
await api.query.staking.validators.entries();

Choose a reason for hiding this comment

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

I'm not sure if this should be really session validators or eras validators. Either way staking.validators is not the right storage 😅

This returns not only era's validators, but also as stated here "wannabe". I think we should use erasStakersOverview to get era's validators or session.validators to get session's validators. I'll try to figure out which one are we interested in 😅

Choose a reason for hiding this comment

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

I'm confused. On production session.validators is used, but I think staking.erasStakersOverview makes more sense. @Marcin-Radecki WDYT?

Copy link

@youPickItUp youPickItUp left a comment

Choose a reason for hiding this comment

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

Nice! I have a few questions and there a few issues and edge cases to work out. @Marcin-Radecki will take a look tomorrow and then we can figure out what is the plan 🚀


const results = await Promise.all(
unclaimedRewardsEntries.map(([era, v]) =>
api.query.staking.claimedRewards<AnyApi>(era, v)

Choose a reason for hiding this comment

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

this supports only new storage key, but it would be nice to support ledger.legacyClaimedRewards as well 😅

@@ -67,6 +68,9 @@ const processEraForExposure = (data: AnyJson) => {
const inOthers = others.find((o: AnyJson) => o.who === who);

if (inOthers) {
const index = others.findIndex((o: any) => o.who === who);
const exposedPage = Math.floor(index / Number(1024));

Choose a reason for hiding this comment

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

It would be better to use maxExposurePageSize if possible

Copy link
Author

Choose a reason for hiding this comment

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

Ofc! Fixed on a8d13dced

@@ -58,6 +58,7 @@ const processEraForExposure = (data: AnyJson) => {
total,
share,
isValidator,
exposedPage: 0,

Choose a reason for hiding this comment

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

It's a bit tricky here. On one hand validator stake based payout is made on page 0. On the other hand his commission is spread across all pages. I'd leave it as it is. @Marcin-Radecki let us know what you think 🙏

Copy link
Author

Choose a reason for hiding this comment

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

On Polkadot:
(but they probably have a bug because they start from page 1)
https://github.com/polkadot-cloud/polkadot-staking-dashboard/blob/main/packages/app/src/workers/stakers.ts#L70-L71

src/locale/en/tips.json Outdated Show resolved Hide resolved
src/locale/en/modals.json Outdated Show resolved Hide resolved
if (!unclaimedPayout.isZero()) {
unclaimed[era] = {
...unclaimed[era],
[validator]: [exposedPage, unclaimedPayout.toString()],

Choose a reason for hiding this comment

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

I'm not sure if exposedPage makes sense here. I think it should be an array of unclaimed pages. Now, if I'm not mistaken it's just the 0 🤔

Copy link
Author

Choose a reason for hiding this comment

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

No, it’s not always 0. The value of exposedPage depends on the validator pages.

https://github.com/kryzasada/aleph-zero-dashboard/blob/a8d13dced1626caad8d0bb846257ad9af0296154/src/workers/stakers.ts#L79-L91

Comment on lines +339 to +353
await api.query.staking.erasStakersOverview.entries(era);

const validators = overview.reduce(
(prev: Record<string, Exposure>, [keys, value]: AnyApi) => {
const validator = keys.toHuman()[1];
const { own, total } = value.toHuman();
return { ...prev, [validator]: { own, total } };
},
{}
);
const validatorKeys = Object.keys(validators);

const pagedResults = await Promise.all(
validatorKeys.map((v) =>
api.query.staking.erasStakersPaged.entries(era, v)

Choose a reason for hiding this comment

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

There is an edge case of an era in which migration happens. In that era the erasStakersOverview and erasStakersPaged will be empty - not populated with validators. In this case we should fallback to staking.erasStakers. I'm not sure if it's imperative we fix this, but until there is a decision I'd opt into fixing it @Marcin-Radecki

Comment on lines +29 to +44
api.query.staking.erasStakersOverview.entries(era).then(
(stakers) =>
stakers?.map(([args, data]) => {
// @ts-expect-error TS2339: Contrary to the TS error "own" and "total" do exist
const { total, own } = data.toHuman();
const validatorId =
(args?.toHuman() as string[] | undefined)?.[1] || '';

return {
validatorId,
ownStake: new u128(registry, rmCommas(own)),
total: new u128(registry, rmCommas(total)),
};
})
),
api.query.staking.erasStakersPaged.entries(era).then((stakers) =>

Choose a reason for hiding this comment

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

Same as above, there is an edge case of an era in which migration happens. In that era the erasStakersOverview and erasStakersPaged will be empty - not populated with validators. In this case we should fallback to staking.erasStakers. I'm not sure if it's imperative we fix this, but until there is a decision I'd opt into fixing it @Marcin-Radecki

Comment on lines +57 to +62
payouts?.forEach(({ era, paginatedValidators }) => {
if (!paginatedValidators) {
return [];
}
return paginatedValidators.forEach(([page, v]) =>
calls.push(api.tx.staking.payoutStakersByPage(v, era, page))

Choose a reason for hiding this comment

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

I'm not sure, but doesn't this mean we are paying out a lot of pages in one transaction? We shouldn't do it, because there is no guarantee that the batched call will fit into block.

It seems that the previous logic was paying out multiple validators as well, but when I've tested it does only one payout. Am I missing something and it's the same with this code?

Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure about the behavior. I agree that there possibility of exceeding the block. However, it's the same on polkadot:
https://github.com/polkadot-cloud/polkadot-staking-dashboard/blob/main/packages/app/src/modals/ClaimPayouts/Forms.tsx#L72

Comment on lines +70 to +75
totalPayout.isGreaterThan(0) && totalPayoutValidators > 0
);

// Ensure payouts value is valid.
useEffect(
() => setValid(totalPayout.isGreaterThan(0) && getCalls().length > 0),
() => setValid(totalPayout.isGreaterThan(0) && totalPayoutValidators > 0),

Choose a reason for hiding this comment

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

What was wrong with previous condition?

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.

2 participants