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

fix: burn tax split bug #113

Merged
merged 10 commits into from
Feb 13, 2023
Merged

fix: burn tax split bug #113

merged 10 commits into from
Feb 13, 2023

Conversation

inon-man
Copy link
Collaborator

@inon-man inon-man commented Feb 13, 2023

Summary of changes

This PR is a bug fix for the Burn Tax Split feature.

The current Burn Tax Split feature does not transfer the community pool amount from the fee collector to the treasury, so the total supply and remaining amount of the fee collector are not accurately calculated.

For example:

  1. Where
    total supply = 100
    fee collector = 100
    burn tax rate = 0.4
  2. In the AnteHandler
    split amount = fee collector * burn tax rate = 40
    community pool += split amount // fee collector sends 40 to community pool
    burn += fee collector - split amount // fee collector sends 60 to burn account
    fee collector = 0
    total supply = 40

This PR also included removal of TaxPowerSplitHeight as they are no longer needed in v2.x

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

@inon-man inon-man self-assigned this Feb 13, 2023
@inon-man inon-man added the bug Something isn't working label Feb 13, 2023
@nghuyenthevinh2000
Copy link
Contributor

panic_man

Copy link
Contributor

@ZaradarBH ZaradarBH left a comment

Choose a reason for hiding this comment

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

LGTM :)

Copy link
Contributor

@ZaradarBH ZaradarBH left a comment

Choose a reason for hiding this comment

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

FML your fast! Beat me by like 3 mins. LGTM buddy, I think this is the cleanest solution :)

@ZaradarBH
Copy link
Contributor

#proof-of-trying-to-code-faster-then-superman (at least I lived to tell the tale :D)

image

@inon-man inon-man mentioned this pull request Feb 13, 2023
8 tasks
@inon-man inon-man marked this pull request as ready for review February 13, 2023 15:29
Copy link
Contributor

@ZaradarBH ZaradarBH left a comment

Choose a reason for hiding this comment

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

LGTM :)

@ZaradarBH ZaradarBH added this to the v2.0.0 milestone Feb 13, 2023
@inon-man inon-man changed the title Fix Burn Tax Split bug fix: burn tax split bug Feb 13, 2023
@inon-man inon-man added the in scope Work approved by the community label Feb 13, 2023
@inon-man inon-man merged commit 1a46cdd into main Feb 13, 2023
@inon-man inon-man deleted the fix-burn-split branch February 13, 2023 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working in scope Work approved by the community
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants