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: Burn Tax via utilization of existing Stability Tax code #784

Merged
merged 2 commits into from
Aug 9, 2022
Merged

feat: Burn Tax via utilization of existing Stability Tax code #784

merged 2 commits into from
Aug 9, 2022

Conversation

edk208
Copy link
Contributor

@edk208 edk208 commented Jul 26, 2022

Summary of changes

Implementation of a tax and burn on luna re-using the stability tax code
Creation of a new burn antehandler that immediately transfers the amount to the treasury burn module
Adjustments to allow luna to be taxed

Report of required housekeeping

  • Github issue OR spec proposal link
  • Wrote tests
  • Updated API documentation (client/lcd/swagger-ui/swagger.yaml)
  • Added a relevant changelog entry: clog add [section] [stanza] [message]

(FOR ADMIN) Before merging

  • Added appropriate labels to PR
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)
  • Confirm added tests are consistent with the intended behavior of changes
  • Ensure all tests pass

fbelugou
fbelugou previously approved these changes Jul 26, 2022
// At this point we have already run the DeductFees AnteHandler and taken the fees from the sending account
// Now we remove the taxes from the gas reward and immediately burn it

if !simulate {
Copy link

Choose a reason for hiding this comment

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

you don't seem to have any test with simulate=true, wonder what happens in this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if the tx is simulated, it does not perform the fee module to burn module transfer for the tax and just moves to the next AnteHandler. Do you believe there should be a simulate case? Thanks!

Copy link

Choose a reason for hiding this comment

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

OK didn't know that, no sense to add a test in this case then!

Choose a reason for hiding this comment

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

@simcheolhwan can you please merge this PR

joaopereira7717
joaopereira7717 previously approved these changes Aug 3, 2022
jhcpeixoto
jhcpeixoto previously approved these changes Aug 3, 2022
@ZaradarBH
Copy link
Contributor

Approve PR please 🙏

@aeuser999
Copy link

aeuser999 commented Aug 5, 2022

Note: This PR is submitted as part of Terra v1 governance proposal 4159 (https://station.terra.money/proposal/4159) which has passed. To read about the prior reviews and testing done, please see (Description of discussion):

Please comment back, or reach out to @edk208 for any further action items that would be required to meet internal build procedure requirements. Beyond @edk208 , please communicate any further concerns to @vegasmorph .

Thank you for your consideration, and I hope you have a wonderful day today :)

custom/auth/ante/burntax.go Show resolved Hide resolved
custom/auth/ante/tax.go Show resolved Hide resolved
x/treasury/keeper/keeper.go Show resolved Hide resolved
@yun-yeo yun-yeo self-requested a review August 9, 2022 10:20
@yun-yeo yun-yeo merged commit b602d3e into terra-money:main Aug 9, 2022
@aeuser999
Copy link

Thank you so much @YunSuk-Yeo for all you, and TFL, have done. I really appreciate it. It felt great to have a restored governance, and to be able to stake again :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.