Conversation
fernandofcampos
force-pushed
the
ENGN-3511
branch
from
0811d17 to
011373f
Compare
guilherme-brandao
suggested changes
Mar 18, 2025
Contributor
guilherme-brandao
left a comment
guilherme-brandao
left a comment
There was a problem hiding this comment.
Looks good! Just need to add tests that check the topic weight recalculation
fernandofcampos
force-pushed
the
ENGN-3511
branch
from
011373f to
b8286a9
Compare
Contributor
Author
Done. |
xmariachi
reviewed
Mar 19, 2025
xmariachi
reviewed
Mar 19, 2025
fernandofcampos
force-pushed
the
ENGN-3511
branch
4 times, most recently
from
76c8251 to
60ec66f
Compare
guilherme-brandao
previously approved these changes
Mar 20, 2025
amimart
suggested changes
Mar 21, 2025
Contributor
There was a problem hiding this comment.
I noticed that there's an end blocker updating topic weights, AFAIU topic weights depends on stake only, so since we now update topic weights when stake is added or removed we should removed this end blocker cc @xmariachi @guilherme-brandao
fernandofcampos
force-pushed
the
ENGN-3511
branch
from
60ec66f to
f84b926
Compare
guilherme-brandao
previously approved these changes
Mar 24, 2025
xmariachi
requested changes
Mar 24, 2025
Contributor
xmariachi
left a comment
xmariachi
left a comment
There was a problem hiding this comment.
Please do not merge this just yet; there is a quirk I want to check with research re. cadence of updates. Setting review as "Request Changes" to block it explicitly
fernandofcampos
dismissed stale reviews from guilherme-brandao and amimart
via
f1ecf0e
fernandofcampos
force-pushed
the
ENGN-3511
branch
4 times, most recently
from
53a330b to
0307085
Compare
Contributor
Hey @amimart , topic weights depend on reputers' stake and topicFeeRevenue. In the |
Contributor
@xmariachi Yeah you're right we need to keep this |
fernandofcampos
force-pushed
the
ENGN-3511
branch
2 times, most recently
from
13d24fb to
53f7ee1
Compare
xmariachi
previously approved these changes
May 5, 2025
Contributor
xmariachi
left a comment
xmariachi
left a comment
There was a problem hiding this comment.
I think it is consistent with the other topic operations (which happen at that time too).
LGTM, please rebase.
amimart
previously approved these changes
May 6, 2025
guilherme-brandao
previously approved these changes
May 6, 2025
fernandofcampos
dismissed stale reviews from guilherme-brandao, amimart, and xmariachi
via
dbaec55
amimart
approved these changes
May 7, 2025
zale144
pushed a commit
that referenced
this pull request
May 13, 2025
<!-- < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < ☺ v ✰ Thanks for creating a PR! You're awesome! ✰ v Please note that maintainers will only review those PRs with a completed PR template. ☺ > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > --> The issue: - The RemoveStakes and RemoveDelegateStakes functions were reducing a topic's staking amount - However, they were not updating the topic's weight or the totalSumPreviousTopicWeights - This was causing unfair distribution of rewards, as inactive topics or topics with reduced stake were still receiving a share of rewards they shouldn't The fix: - When stake is removed from a topic, the topic's weight is immediately recalculated - The totalSumPreviousTopicWeights value is updated accordingly - This ensures that reward distribution is fair and accurate - Topics that should receive a larger share now do https://linear.app/alloralabs/issue/ENGN-3511/136-the-removestakes-function-does-not-update-the-topics-state-or - [x] If tested, please describe how. If not, why tests are not needed. - [ ] If documented, please describe where. If not, describe why docs are not needed. - [x] Added to `Unreleased` section of `CHANGELOG.md`? *Fill this out if this is a Draft PR so others can help.*
zale144
pushed a commit
that referenced
this pull request
Jun 2, 2025
<!-- < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < ☺ v ✰ Thanks for creating a PR! You're awesome! ✰ v Please note that maintainers will only review those PRs with a completed PR template. ☺ > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > --> The issue: - The RemoveStakes and RemoveDelegateStakes functions were reducing a topic's staking amount - However, they were not updating the topic's weight or the totalSumPreviousTopicWeights - This was causing unfair distribution of rewards, as inactive topics or topics with reduced stake were still receiving a share of rewards they shouldn't The fix: - When stake is removed from a topic, the topic's weight is immediately recalculated - The totalSumPreviousTopicWeights value is updated accordingly - This ensures that reward distribution is fair and accurate - Topics that should receive a larger share now do https://linear.app/alloralabs/issue/ENGN-3511/136-the-removestakes-function-does-not-update-the-topics-state-or - [x] If tested, please describe how. If not, why tests are not needed. - [ ] If documented, please describe where. If not, describe why docs are not needed. - [x] Added to `Unreleased` section of `CHANGELOG.md`? *Fill this out if this is a Draft PR so others can help.*
zale144
pushed a commit
that referenced
this pull request
Jun 2, 2025
<!-- < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < ☺ v ✰ Thanks for creating a PR! You're awesome! ✰ v Please note that maintainers will only review those PRs with a completed PR template. ☺ > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > --> The issue: - The RemoveStakes and RemoveDelegateStakes functions were reducing a topic's staking amount - However, they were not updating the topic's weight or the totalSumPreviousTopicWeights - This was causing unfair distribution of rewards, as inactive topics or topics with reduced stake were still receiving a share of rewards they shouldn't The fix: - When stake is removed from a topic, the topic's weight is immediately recalculated - The totalSumPreviousTopicWeights value is updated accordingly - This ensures that reward distribution is fair and accurate - Topics that should receive a larger share now do https://linear.app/alloralabs/issue/ENGN-3511/136-the-removestakes-function-does-not-update-the-topics-state-or - [x] If tested, please describe how. If not, why tests are not needed. - [ ] If documented, please describe where. If not, describe why docs are not needed. - [x] Added to `Unreleased` section of `CHANGELOG.md`? *Fill this out if this is a Draft PR so others can help.*
zale144
pushed a commit
that referenced
this pull request
Jun 2, 2025
<!-- < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < ☺ v ✰ Thanks for creating a PR! You're awesome! ✰ v Please note that maintainers will only review those PRs with a completed PR template. ☺ > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > --> The issue: - The RemoveStakes and RemoveDelegateStakes functions were reducing a topic's staking amount - However, they were not updating the topic's weight or the totalSumPreviousTopicWeights - This was causing unfair distribution of rewards, as inactive topics or topics with reduced stake were still receiving a share of rewards they shouldn't The fix: - When stake is removed from a topic, the topic's weight is immediately recalculated - The totalSumPreviousTopicWeights value is updated accordingly - This ensures that reward distribution is fair and accurate - Topics that should receive a larger share now do https://linear.app/alloralabs/issue/ENGN-3511/136-the-removestakes-function-does-not-update-the-topics-state-or - [x] If tested, please describe how. If not, why tests are not needed. - [ ] If documented, please describe where. If not, describe why docs are not needed. - [x] Added to `Unreleased` section of `CHANGELOG.md`? *Fill this out if this is a Draft PR so others can help.* # Conflicts: # x/emissions/keeper/keeper_test.go # x/emissions/module/rewards/nonce_management_test.go # x/emissions/module/rewards/rewards_test.go # x/emissions/module/rewards/scores_test.go # x/emissions/module/rewards/topic_rewards_test.go
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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
The issue:
The fix:
Link(s) to Ticket(s) or Issue(s) resolved by this PR
https://linear.app/alloralabs/issue/ENGN-3511/136-the-removestakes-function-does-not-update-the-topics-state-or
Are these changes tested and documented?
Unreleasedsection ofCHANGELOG.md?Still Left Todo
Fill this out if this is a Draft PR so others can help.