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.

Sign up for GitHub

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

Jump to bottom

Fix buggy inflation calculation #1756

Merged
merged 5 commits into from
Jul 26, 2023
Merged

Fix buggy inflation calculation #1756

merged 5 commits into from
Jul 26, 2023

Conversation

bengtlofgren
Copy link
Collaborator

Pos inflation rewards were abnormally large. Now should be fixed

@bengtlofgren
Copy link
Collaborator Author

pls spawn devnet [devnet-0.20,3,heliaxdev@1be9883,ON]

2 similar comments
@bengtlofgren
Copy link
Collaborator Author

pls spawn devnet [devnet-0.20,3,heliaxdev@1be9883,ON]

@bengtlofgren
Copy link
Collaborator Author

pls spawn devnet [devnet-0.20,3,heliaxdev@1be9883,ON]

@github-actions
Copy link
Contributor

Devnet with chain id internal-devnet-4c0.c7c9918420 spawned succesfully!

@brentstone brentstone changed the title hopefully fixes inflation Fix buggy inflation calculation Jul 25, 2023
@brentstone
Copy link
Contributor

The bug is resolved for now by converting Dec values into Amount values as early as possible and then doing the comparison with these types (like < or std::cmp::min). However, the ordering for Dec seems to be buggy when large values are contained within.

Fraccaman added a commit that referenced this pull request Jul 26, 2023
@Fraccaman
Merge branch 'bengt/pos-inflation-fix' (#1756)
2ef92e3 
* origin/bengt/pos-inflation-fix:
  added changelog
  fix inflation calc
  hopefully fixes inflation
  ci: add masp params for integration tests
  make: run integration tests with unit test coverage
  ci/e2e: remove removed masp tests
  ci: put back accidentally removed changes
@Fraccaman Fraccaman mentioned this pull request Jul 26, 2023
Closed
tzemanovic
tzemanovic reviewed Jul 26, 2023
View reviewed changes
shared/src/ledger/inflation.rs Outdated
let inflation = if last_inflation_amount + control_val > max_inflation {
let control_val = token::Amount::from_uint(
control_val
.to_uint()
Copy link
Member

Choose a reason for hiding this comment

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

This panics when the locked ratio is > target as the control_val goes negative, which is valid value because it should be reducing the inflation from the last.

Copy link
Member

Choose a reason for hiding this comment

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

fixed

Copy link
Contributor

Choose a reason for hiding this comment

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

thx for realizing this and sorting it out

@brentstone brentstone marked this pull request as ready for review July 26, 2023 15:27
@brentstone brentstone self-requested a review July 26, 2023 16:46
brentstone
brentstone approved these changes Jul 26, 2023
View reviewed changes
Copy link
Contributor

@brentstone brentstone left a comment

Choose a reason for hiding this comment

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

LGTM

@Fraccaman Fraccaman merged commit c7fbc8c into main Jul 26, 2023
11 of 12 checks passed
@Fraccaman Fraccaman deleted the bengt/pos-inflation-fix branch July 26, 2023 19:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brentstone brentstone brentstone approved these changes

@tzemanovic tzemanovic Awaiting requested review from tzemanovic

Assignees
No one assigned
Labels
merged to draft
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@bengtlofgren @brentstone @tzemanovic @Fraccaman