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

Implement a Reverse Charge tax approach #525

Merged
merged 29 commits into from
Dec 3, 2024
Merged

Conversation

StrathCole
Copy link
Collaborator

Summary of changes

This is an implementation as an alternative to tax2gas.
It solves similar issues as tax2gas does, but additionally shifts taxation to the recipient side.
Also it removes double-taxation of contracts, i.e. contract calls are no longer taxed, but sending funds from inside contracts to the final wallet destination is.

Copilot Summary

This pull request introduces the integration of a new Tax module into the Terra blockchain application. The changes primarily involve adding the TaxKeeper to various parts of the application and ensuring it is properly initialized and utilized in transaction handling. Below is a summary of the most important changes:

Integration of Tax Module

  • Added TaxKeeper to the AppKeepers structure and initialized it in the NewAppKeepers function in app/keepers/keepers.go. [1] [2]
  • Included the Tax module in the module initialization and setup functions, such as appModules, simulationModules, orderBeginBlockers, orderEndBlockers, and orderInitGenesis in app/modules.go. [1] [2] [3] [4] [5]

Changes in AnteHandler

  • Updated the HandlerOptions and NewAnteHandler in custom/auth/ante/ante.go to include and check for TaxKeeper. [1] [2]
  • Modified the FeeDecorator to account for taxes and handle scenarios where fees are insufficient to cover both gas and taxes in custom/auth/ante/fee.go. [1] [2] [3]

Import Statements

  • Added necessary import statements for the Tax module in various files (app/keepers/keepers.go, app/modules.go, custom/auth/ante/ante.go, and custom/auth/ante/fee.go). [1] [2] [3] [4]

Initialization and Configuration

  • Added the Tax module's store key and subspace initialization in NewAppKeepers and initParamsKeeper functions in app/keepers/keepers.go. [1] [2]

These changes collectively ensure that the new Tax module is fully integrated into the Terra blockchain application, with proper initialization, configuration, and handling within the transaction processing pipeline.

custom/auth/ante/fee.go Outdated Show resolved Hide resolved
custom/auth/ante/fee.go Outdated Show resolved Hide resolved
@kien6034
Copy link
Contributor

@StrathCole Hats down. The PR looks good to me, well implemented and cover the requirements of the simplified tax handling with reverse charge.
Just leaving some small comments, and i think we just need to improve test coverage

@StrathCole StrathCole requested a review from Copilot November 16, 2024 08:05

Choose a reason for hiding this comment

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

Copilot reviewed 56 out of 56 changed files in this pull request and generated no suggestions.

@kien6034
Copy link
Contributor

@TropicalDog17 lets review the tests and add more test cases to this PR

- improve fee check handling
@StrathCole StrathCole requested a review from Copilot November 22, 2024 22:27

Choose a reason for hiding this comment

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

Copilot reviewed 41 out of 56 changed files in this pull request and generated no suggestions.

Files not reviewed (15)
  • client/docs/swagger-ui/swagger.yaml: Language not supported
  • contrib/updates/prepare_cosmovisor.sh: Language not supported
  • custom/auth/ante/fee_burntax.go: Evaluated as low risk
  • custom/auth/ante/expected_keeper.go: Evaluated as low risk
  • custom/auth/ante/fee_test.go: Evaluated as low risk
  • custom/wasm/keeper/handler_plugin.go: Evaluated as low risk
  • app/upgrades/v10/constants.go: Evaluated as low risk
  • custom/auth/tx/service.go: Evaluated as low risk
  • custom/auth/ante/ante_test.go: Evaluated as low risk
  • custom/auth/ante/integration_test.go: Evaluated as low risk
  • custom/auth/ante/ante.go: Evaluated as low risk
  • custom/auth/ante/fee_tax.go: Evaluated as low risk
  • custom/auth/client/utils/feeutils.go: Evaluated as low risk
  • custom/auth/post/post.go: Evaluated as low risk
  • app/keepers/keepers.go: Evaluated as low risk
Comments skipped due to low confidence (2)

custom/auth/ante/fee.go:63

  • [nitpick] The variable name reverseCharge is ambiguous. It should be renamed to isReverseCharge.
reverseCharge := false

custom/auth/ante/fee.go:64

  • [nitpick] The variable name refundNonTaxableTax is ambiguous. It should be renamed to shouldRefundNonTaxableTax.
refundNonTaxableTax := false
@StrathCole StrathCole merged commit bd3c50f into main Dec 3, 2024
24 checks passed
@StrathCole StrathCole deleted the strath/reverse-charge branch December 3, 2024 09:17
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.

3 participants