Correct Discrepancies with Whitepaper modified reward fractions vs plain EMA #704
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
relyt29
requested review from
xmariachi,
jmdkastro,
fernandofcampos,
kpeluso,
spooktheducks,
amimart,
guilherme-brandao and
cal-hawk
cal-hawk
previously approved these changes
Dec 13, 2024
fernandofcampos
previously approved these changes
Dec 13, 2024
…o calculations should use modified EMAs
relyt29
dismissed stale reviews from fernandofcampos and cal-hawk
via
1998475
relyt29
force-pushed
the
tyler/proto-3039-swap-use-of-f-for-u-tilde-x2
branch
from
a7e061f to
1998475
Compare
xmariachi
approved these changes
Dec 14, 2024
kpeluso
approved these changes
Dec 15, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Purpose of Changes and their Description
Really all credit for this PR goes to @cal-hawk who did the hard work of finding the bug and tracing the actual lines of code related to it.
He found a discrepancy between the whitepaper and the allora-chain codebase where u-tilde (inferer, reputer, forecaster reward fractions) were being used for numberRatio calculations when it should be using f (the modifiedRewardFractions). And then also the reverse for setting the previous EMA score - we were using f instead of u-tilde in that case.
and in the other case it's:
f being used where it should be u-tilde
Link(s) to Ticket(s) or Issue(s) resolved by this PR
PROTO-3039, RES-579
Are these changes tested and documented?
To be quite honest I don't know of an easy way to test this in a unit test and show that the fixed code addresses issues other than just pointing to the whitepaper and saying read these formulas. The existing unit test suite at least will cover that directionally these changes do not break any major functionality of the chain.