-
Notifications
You must be signed in to change notification settings - Fork 158
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: add new staking storage items #1432
Conversation
validators[validatorId] = exposure; | ||
|
||
exposure.others.forEach(({ who }, validatorIndex): void => { | ||
const nominatorId = who.toString(); | ||
if (exposure.others) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the same logic around nominatorId
is repeated here in both cases perhaps we can do something like
const individualExposure = exposure.others ? exposure.others : (exposure as unknown as Option<SpStakingExposurePage>).unwrap().others;
individualExposure.forEach(({ who }, validatorIndex): void => {
const nominatorId = who.toString();
nominators[nominatorId] = nominators[nominatorId] || [];
nominators[nominatorId].push({ validatorId, validatorIndex });
});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! Changed it! Thank you for the suggestion! 💯
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Accounting for Tarik's suggestion below this could look something like
const individualExposure = exposure.others ? exposure.others :
(exposure as unknown as Option<SpStakingExposurePage>).isSome ?
(exposure as unknown as Option<SpStakingExposurePage>).unwrap().others :
undefined;
if (individualExposure) {
individualExposure.forEach(({ who }, validatorIndex): void => {
const nominatorId = who.toString();
nominators[nominatorId] = nominators[nominatorId] || [];
nominators[nominatorId].push({ validatorId, validatorIndex });
});
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I already changed this.
I did it like this
const individualExposure = exposure.others
? exposure.others
: (exposure as unknown as Option<SpStakingExposurePage>).isSome
? (exposure as unknown as Option<SpStakingExposurePage>).unwrap().others
: [];
individualExposure.forEach(({ who }, validatorIndex): void => {
const nominatorId = who.toString();
nominators[nominatorId] = nominators[nominatorId] || [];
nominators[nominatorId].push({ validatorId, validatorIndex });
});
it looks the same as yours but instead of undefined
i set an empty array. Then forEach in an empty array will just not do anything.
Is it recommended to set undefined
and then check if (individualExposure)
or I can leave it like that ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nope, this is totally cool! great job
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR lgtm! Just had one minor nit/suggestion to consider
nominators[nominatorId].push({ validatorId, validatorIndex }); | ||
}); | ||
} else { | ||
(exposure as unknown as Option<SpStakingExposurePage>) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be safe to also check isSome
on the Option
before unwrapping it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added checks with isSome
in the code part mentioned here, as well as in 3 other parts of the code (in the extractExposure
function) where I am also unwrapping. The cases though in the extractExposure
is not adding much value since unwrap
is also called later on these variables (here and here) but I prefer not to refactor that part right now if that is ok.
Rel: #1411
Related Endpoint
/accounts/{accountId}/staking-payouts
The changes in this PR are affecting the responses returned by
/accounts/{accountId}/staking-payouts
endpoint.Description
After the runtime upgrade of Polkadot to v1.2.0, the new storage items
erasStakersPaged
anderasStakersOverview
are used instead oferasStakersClipped
.More information regarding this change can be found:
Hence, as mentioned here, to get the rewards:
we need to make use of the new storage items instead of relying on erasStakersClipped, as it currently returns empty results for eras > than the eras mentioned previously. We still use
erasStakersClipped
for eras before the runtime upgrade until it is removed completely which we would need to also remove.Implementation Notes
Since now the information is split into 2 storage items (link),
validatorsOverview
was added inIAdjustedDeriveEraExposure
so we can store and use the information from the additional call.Sidecar's response item
totalValidatorExposure
is retrieved fromquery.staking.erasStakersOverview
>total
IMPORTANT NOTE
There is also available
query.staking.erasStakersPaged
>pageTotal
but based on manual testing and quoting this part of the forum post :I believe the more complete information is found in
...Overview
-total
since there are cases when these 2 fields differ.Sidecar's response item
nominatorExposure
when the validator is also the nominator, is retrieved fromquery.staking.erasStakersOverview
>own
Sample Responses
Sidecar endpoint while connected to Polkadot chain :
returns
Cross checked results
totalValidatorRewardPoints
with subscantotalValidatorExposure
with pjs-apps (staking.erasStakersOverview
-total
for the specified account and erra)nominatorExposure
with pjs-apps (staking.erasStakersOverview
-own
for the specified account and erra)