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: tax2gas #500

Merged
merged 77 commits into from
Aug 13, 2024
Merged

feat: tax2gas #500

merged 77 commits into from
Aug 13, 2024

Conversation

phamminh0811
Copy link
Collaborator

Summary of changes

Close #481

Test scenario is listed in: https://github.com/classic-terra/core/blob/minh/tax2gas/x/tax2gas/README.md

x/tax2gas/post/post.go Outdated Show resolved Hide resolved
custom/auth/client/utils/feeutils.go Outdated Show resolved Hide resolved
x/tax2gas/post/post.go Outdated Show resolved Hide resolved
x/tax2gas/post/post.go Outdated Show resolved Hide resolved
@phamminh0811
Copy link
Collaborator Author

phamminh0811 commented Aug 1, 2024

@StrathCole @fragwuerdig thanks for all of your comments. After looking at the overall picture and considering all technical aspects, and discussing internally w/wo our consultants, we have come to the conclusion that this is the best version of tax2gas we have. We will put up a proposal for the community to vote on GenuineLabs' tax2gas implementation.

@StrathCole
Copy link
Collaborator

@StrathCole @fragwuerdig thanks for all of your comments. After looking at the overall picture and considering all technical aspects, and discussing internally w/wo our consultants, we have come to the conclusion that this is the best version of tax2gas we have. We will put up a proposal for the community to vote on GenuineLabs' tax2gas implementation.

That's your decision to do of course.

@fragwuerdig
Copy link
Collaborator

I am not going to approve this in the current state of implementation.

  • the users are charged for inter-contract calls that infer taxes. Depending on how many calls a contract makes or forwards an amount this can mean that the users will pay double, triple, quadruple,... amount of the original tax.

  • taxes can be paid in any denom... Even the ones that are worthless and have no liquidity on the markets (like KRTC, EUTC). On the one hand this infers a spamming risk where attackers can literally fill the blockchain with txs and data without even paying for it. On the other hand this reduces the potential overall burns and income denominated in LUNC or USTC.

Sorry to hear, that you have come to this decision :(

@StrathCole
Copy link
Collaborator

I am sorry, but I cannot approve the PR from my side as I disagree with the implementation in its current state and the effects it would have.

  • only consumed gas is deducted. This means that the gas fees on chain will reduce overall. This affects rewards as well as CP funding. Depending on the multiplier clients and dApps use, the effect is not predictable in height, but I consider it not insignificant
  • tax is paid by contract executor. This means if you call a contract, you don't necessarily pay 0.5% tax, but might pay double or a multiple. In turn the funds receiver pays nothing. This is both unfair and unnecessary and also not how it is currently working on chain.
  • taxes can be paid in any denom. This means that you can send your LUNC and USTC without actually paying tax in those coins, but use KRTC, EUTC etc. instead. This affects not only burns of LUNC and USTC, but also CP and OP funding as 20% of the tax goes there.

@phamminh0811
Copy link
Collaborator Author

As proposal 12120 has passed, this PR will be merged and be pushed on testnet

@phamminh0811 phamminh0811 merged commit d28ff8c into main Aug 13, 2024
24 checks passed
@phamminh0811 phamminh0811 deleted the minh/tax2gas branch August 13, 2024 03:41
StrathCole added a commit that referenced this pull request Sep 5, 2024
@StrathCole StrathCole mentioned this pull request Sep 5, 2024
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.

[FEATURE] Tax2Gas implementation
5 participants