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

feat: Burning ProtoRev Profit #7509

Merged
merged 6 commits into from
Feb 21, 2024
Merged

feat: Burning ProtoRev Profit #7509

merged 6 commits into from
Feb 21, 2024

Conversation

davidterpay
Copy link
Contributor

@davidterpay davidterpay commented Feb 16, 2024

Closes #7320

What is the purpose of the change

This PR updates the profit distribution mechanism of protorev such that if the profit is denominated in uosmo, send the profit to the zero address (osmo1qqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqmcn030) after redistributing the developer profits.

Testing and Verifying

Added test coverage to the updated distribution logic.

Documentation and Release Note

  • Does this pull request introduce a new feature or user-facing behavior changes?
  • Changelog entry added to Unreleased section of CHANGELOG.md?

Where is the change documented?

  • Specification (x/{module}/README.md)
  • Osmosis documentation site
  • Code comments?
  • N/A

@davidterpay davidterpay requested a review from p0mvn as a code owner February 16, 2024 20:34
@ValarDragon ValarDragon added the V:state/breaking State machine breaking PR label Feb 16, 2024
Copy link
Member

@ValarDragon ValarDragon left a comment

Choose a reason for hiding this comment

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

This LGTM! Left comments in Slack requesting that we make this not happen on every single trade though

@ValarDragon
Copy link
Member

Can you add a changelog, and then I think it's good to merge?

I think we need to make this function get called periodically pre release though. I'll try to make an issue for that today

@davidterpay
Copy link
Contributor Author

Will add the change log! I'll also update this to migrate non-osmo denom profits to the community pool.

@github-actions github-actions bot added the C:app-wiring Changes to the app folder label Feb 20, 2024
expectedErr: false,
expectedDevProfit: sdk.NewCoin(types.OsmosisDenomination, osmomath.NewInt(20)),
expectedDevProfit: sdk.NewCoin(types.OsmosisDenomination, osmomath.NewInt(200)),
Copy link
Member

Choose a reason for hiding this comment

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

ah numbers 10x'd because below we changed distribute profit call to be hard coded at 1000, just switching which denom,

@ValarDragon ValarDragon merged commit 905a21c into main Feb 21, 2024
1 check passed
@ValarDragon ValarDragon deleted the terpay/protorev-burn-update branch February 21, 2024 18:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:app-wiring Changes to the app folder V:state/breaking State machine breaking PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ProtoRev adjustments [Proposal 709/710]
2 participants