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

v1.x backport: add burn tax split logic #103

Merged
merged 2 commits into from
Feb 12, 2023
Merged

Conversation

nghuyenthevinh2000
Copy link
Contributor

@nghuyenthevinh2000 nghuyenthevinh2000 commented Feb 8, 2023

Summary of changes

  • Need to have an unit test to check burn tax split logic

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

@nghuyenthevinh2000 nghuyenthevinh2000 changed the title add burn tax split logic v1.x backport: add burn tax split logic Feb 8, 2023
@nghuyenthevinh2000 nghuyenthevinh2000 self-assigned this Feb 8, 2023
@ZaradarBH
Copy link
Contributor

LGTM. Great work buddy :)

@ZaradarBH ZaradarBH added enhancement New feature or request in scope Work approved by the community labels Feb 8, 2023
@ZaradarBH ZaradarBH added this to the v1.0.6 milestone Feb 8, 2023
@nghuyenthevinh2000
Copy link
Contributor Author

LGTM. Great work buddy :)

Great comment buddy :))

@nghuyenthevinh2000
Copy link
Contributor Author

Added an unit test to check if:

  1. before tax split logic upgrade height, all taxes go to burn module
  2. after tax split logic upgrade height, 50% goes to burn module, 50% goes to community pool (50% is split percentage)

@inon-man @ZaradarBH @edk208 can you confirm that it is the correct unit test. I am not sure if I understand the split logic correctly and fully.

@inon-man
Copy link
Collaborator

I can find param query for burn_tax_split in the commits. Is it intended?

@nghuyenthevinh2000
Copy link
Contributor Author

I can find param query for burn_tax_split in the commits. Is it intended?

I think it is normal to be able to query all params

@inon-man
Copy link
Collaborator

I can find param query for burn_tax_split in the commits. Is it intended?

I think it is normal to be able to query all params

I meant I could not find cli and rest query for the parameter unlike the all other treasury parameters.

@nghuyenthevinh2000
Copy link
Contributor Author

nghuyenthevinh2000 commented Feb 11, 2023

I can find param query for burn_tax_split in the commits. Is it intended?

I think it is normal to be able to query all params

I meant I could not find cli and rest query for the parameter unlike the all other treasury parameters.

you can find it here for burn_tax_split. This is after upgrade test.

Screenshot 2023-02-11 at 15 11 05

@inon-man
Copy link
Collaborator

inon-man commented Feb 12, 2023

I can find param query for burn_tax_split in the commits. Is it intended?

I think it is normal to be able to query all params

I meant I could not find cli and rest query for the parameter unlike the all other treasury parameters.

you can find it here for burn_tax_split. This is after upgrade test.

Screenshot 2023-02-11 at 15 11 05

I am aware of it but my question was there is no separate query for burn tax split like othe params for keeping the consistency. However, I am okay with not having it because there is no such consistent pattern between modules in Cosmos SDK.

@inon-man inon-man merged commit f1c1b2a into release/v1.x Feb 12, 2023
@inon-man inon-man deleted the v1.x/burn-tax-split branch February 13, 2023 15:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request 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