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

feat(treasury): split the burn tax to the distribution module #272

Merged
merged 8 commits into from Jul 21, 2023
Merged

feat(treasury): split the burn tax to the distribution module #272

merged 8 commits into from Jul 21, 2023

Conversation

alchemist-ti
Copy link
Contributor

@alchemist-ti alchemist-ti commented Jun 19, 2023 •
edited by inon-man

Summary of changes

Proposal link: https://station.terrarebels.net/proposal/11513
Closes: #232

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

@alchemist-ti
Copy link
Contributor Author

Cosmos AllocateTokens from FeeCollector: https://github.com/cosmos/cosmos-sdk/blob/v0.45.13/x/distribution/keeper/allocation.go#L16.

Seems should split the Burn tax to the FeeCollector? @fragwuerdig @nghuyenthevinh2000 @inon-man

inon-man
inon-man requested changes Jun 19, 2023
View reviewed changes
Copy link
Collaborator

@inon-man inon-man left a comment

Choose a reason for hiding this comment

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

https://github.com/classic-terra/core/pull/272/files#diff-3098d720e9d0b5c40352856e6a134718bf7320094258cf690aadb2f6a475d10aR65-R70

I think we should refactor codes here. It's confusing after removing FundCommunityPool

@inon-man inon-man added enhancement New feature or request state machine breaking Something that will impact the state machine and consensus labels Jun 20, 2023
inon-man
inon-man approved these changes Jun 20, 2023
View reviewed changes
Copy link
Collaborator

@inon-man inon-man left a comment

Choose a reason for hiding this comment

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

LGTM, but please hold merging it because it's a consensus breaking change.

@inon-man inon-man self-requested a review June 20, 2023 22:16
inon-man
inon-man requested changes Jun 21, 2023
View reviewed changes
Copy link
Collaborator

@inon-man inon-man left a comment •
edited

Choose a reason for hiding this comment

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

Need a changelog entry. Add it under [Unreleased] section.

@inon-man inon-man changed the title Proposal 11513: split the Burn tax to the Distribution Module feat(treasury): split the burn tax to the distribution module Jun 21, 2023
@alchemist-ti
Copy link
Contributor Author

alchemist-ti commented Jun 21, 2023 •
edited

After a token transfer transaction of usdr, the burn tax is distributed to validators and community pool as expected.
test

@inon-man
Copy link
Collaborator

inon-man commented Jun 21, 2023 •
edited by alchemist-ti

After a token transfer transaction of usdr, the burn tax is distributed to validators and community pool.
test

Isn't it intended? Any issue?

@inon-man inon-man self-requested a review June 21, 2023 09:04
@alchemist-ti
Copy link
Contributor Author

alchemist-ti commented Jun 21, 2023 •
edited

Isn't it intended? Any issue?

@inon-man No, it's my test result, and that is the expected result.

@LuncBurner
Copy link
Collaborator

@nghuyenthevinh2000 @fragwuerdig Can you guys review?

nghuyenthevinh2000
nghuyenthevinh2000 approved these changes Jul 21, 2023
View reviewed changes
Copy link
Member

@nghuyenthevinh2000 nghuyenthevinh2000 left a comment

Choose a reason for hiding this comment

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

I have checked over, LGTM!

@nghuyenthevinh2000 nghuyenthevinh2000 merged commit 77547ce into main Jul 21, 2023
16 checks passed
@nghuyenthevinh2000 nghuyenthevinh2000 deleted the proposal-11513 branch July 21, 2023 04:14
@nghuyenthevinh2000 nghuyenthevinh2000 mentioned this pull request Jul 25, 2023
Closed
9 tasks
@nghuyenthevinh2000 nghuyenthevinh2000 added this to the v2.2.0 milestone Aug 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nghuyenthevinh2000 nghuyenthevinh2000 nghuyenthevinh2000 approved these changes

@inon-man inon-man inon-man approved these changes

@fragwuerdig fragwuerdig Awaiting requested review from fragwuerdig

Assignees

@alchemist-ti alchemist-ti

Labels
enhancement New feature or request state machine breaking Something that will impact the state machine and consensus
Projects
L1 Q3 Sprint
Status: ✅ Done
Milestone
v2.2.0
Development

Successfully merging this pull request may close these issues.

Split Burn Tax with Distribution Module
4 participants
@alchemist-ti @inon-man @LuncBurner @nghuyenthevinh2000