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

Brace for the new reward logic in nomination pools #7741

Closed
kianenigma opened this issue Jun 15, 2022 · 11 comments
Closed

Brace for the new reward logic in nomination pools #7741

kianenigma opened this issue Jun 15, 2022 · 11 comments

Comments

@kianenigma
Copy link

As described in paritytech/substrate#11669, the logic for how the rewards are computed is changing in a non-compatible way. This needs some catering for, until all chains are updated with the new logic.

@kianenigma
Copy link
Author

This is about to be baked into the 9.26 release pretty soon FYI https://polkadiff.parity.io/

@jacogr
Copy link
Member

jacogr commented Jul 25, 2022

Thank you. May have to disable it as detected until support is in.

@kianenigma
Copy link
Author

FYI, even though you said it won't be handy, I am adding this: paritytech/substrate#11831

The transaction to claim rewards is exactly the same as before. We could also add a endpoint that returns the pool's total pending reward.

You can re-implement the logic on your side, but RPC sounds much more sustainable to me.

@kianenigma
Copy link
Author

This will probably make it to 9.27

@kianenigma
Copy link
Author

If you say this will help you, I will see if we can add the RPC in 9.26 as well.

@kianenigma
Copy link
Author

A ping on this, as this will be soon enabled on Kusama and not all functions work (see Westend.).

Pools will also be on Polkadot hopefully in the next 4-8 weeks, which makes #7808 and #7902 pretty important too.

@kianenigma
Copy link
Author

kianenigma commented Sep 1, 2022

fwiw, here's an example of how to compute the pending rewards in JS (using overflow prone vanilla number type):

https://github.com/substrate-portfolio/polkadot-portfolio/pull/16/files#diff-17782fa0982182d57be83b90b00797e4ef1da85220c6780e23e310f272389aecR225

Once paritytech/substrate#12095 is merged (9.29), you can use this RPC.

@jacogr
Copy link
Member

jacogr commented Sep 4, 2022

The UI currently does retrieve rewards using the runtime call, but as suggested above (after this fix in API 9.3.1 which seems to have pointed to pre-merge PR status), however it is not quite working on Westend for me at least (everything is always 0 at this point, even with unclaimed pool rewards).

Usage here -

useEffect((): void => {
member && api.call.nominationPoolsApi &&
api.call.nominationPoolsApi
.pendingRewards(accountId)
.then((claimable) =>
isMountedRef.current && setState({ claimable, member })
)
.catch(console.error);
}, [accountId, member, api, isMountedRef]);

@kianenigma
Copy link
Author

If you want to support it now (which I encourage you do), you should use something along the lines of the link above: https://github.com/substrate-portfolio/polkadot-portfolio/pull/16/files#diff-17782fa0982182d57be83b90b00797e4ef1da85220c6780e23e310f272389aecR225

Once 9.29 is enacted, we will have the runtime API working again.

@jacogr
Copy link
Member

jacogr commented Sep 6, 2022

Thanks for confirming. my observations

I will close this then since the UI does support the correct logic to cater with rewards. Obviously because of an oversight this is not quite working as of yet (it happens) however it seems the runtime call bug has been addressed - once that is in, the UI will "just work, no adjustments" since the runtime is now properly treated as "the source of truth".

PS: Putting in code to work around bugs and then removing it is not quite maintainable with multiple runtimes. And even with the link, I do have my own implementation that was reverse-engineered, however maintenance-wise this is a dead-end and it it not going in. I would just suggest getting the fixed into the networks as a priority, starting with Westend.

(Other codebases may obviously have differing opinions on this and be willing to pay the long-term maintenance price of "either this or that since we cannot trust the runtime", I just believe the cause needed to be fixed instead of plastered over.)

Thanks for the pointers and especially the runtime interfaces which is "the right way to go(tm)", this would be great when fully enabled and runtime, no-bug functional on all relay chains.

@jacogr jacogr closed this as completed Sep 6, 2022
@polkadot-js-bot
Copy link

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue if you think you have a related problem or query.

@polkadot-js polkadot-js locked as resolved and limited conversation to collaborators Sep 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants