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

feature: Smart Contract Tax Whitelist #301

Merged
merged 11 commits into from
Aug 8, 2023
Merged

Conversation

fragwuerdig
Copy link
Collaborator

@fragwuerdig fragwuerdig commented Jul 3, 2023

This is the Whitelist that passed governance. Description of acceptance criteria can be found here. This Changeset includes:

  • Add HasBurnTaxExemptionContract function to treasury keeper
  • Incject wasm keeper dependency into treasury (by reference)
  • Adjust test env mocking accordingly
  • Write Tests

@fragwuerdig fragwuerdig self-assigned this Jul 3, 2023
@fragwuerdig fragwuerdig added the state machine breaking Something that will impact the state machine and consensus label Jul 3, 2023
@LuncBurner LuncBurner requested a review from inon-man July 5, 2023 02:08
@fragwuerdig fragwuerdig linked an issue Jul 5, 2023 that may be closed by this pull request
3 tasks
@nghuyenthevinh2000
Copy link
Contributor

nghuyenthevinh2000 commented Jul 24, 2023

@alchemist-ti while waiting for USTC team response, can you work on this

@alchemist-ti alchemist-ti marked this pull request as ready for review July 31, 2023 07:09
@nghuyenthevinh2000
Copy link
Contributor

nghuyenthevinh2000 commented Aug 2, 2023

@alchemist-ti I think the unit test is broken somehow, when I try with feeAmt * 2 for "MsgExecuteContract(exemption), MsgExecuteContract(normal)", it also passes as well. So this is false positive scenario.

Screenshot 2023-08-02 at 22 08 45 Screenshot 2023-08-02 at 22 07 48

@alchemist-ti
Copy link
Contributor

@alchemist-ti I think the unit test is broken somehow, when I try with feeAmt * 2 for "MsgExecuteContract(exemption), MsgExecuteContract(normal)", it also passes as well. So this is false positive scenario.

Screenshot 2023-08-02 at 22 08 45 Screenshot 2023-08-02 at 22 07 48

@nghuyenthevinh2000 This test case makes sense. ExpectedFeeAmount means the minimum FeeAmount, if gas fee is greater than it, the transaction will pass.

@nghuyenthevinh2000
Copy link
Contributor

nghuyenthevinh2000 commented Aug 7, 2023

@alchemist-ti I think the unit test is broken somehow, when I try with feeAmt * 2 for "MsgExecuteContract(exemption), MsgExecuteContract(normal)", it also passes as well. So this is false positive scenario.
Screenshot 2023-08-02 at 22 08 45 Screenshot 2023-08-02 at 22 07 48

@nghuyenthevinh2000 This test case makes sense. ExpectedFeeAmount means the minimum FeeAmount, if gas fee is greater than it, the transaction will pass.

ExpectedFeeAmount means the correct amount. I built this test case to ensure that fee calculation will be skipped only on whitelisted <-> whitelisted. Please have a look again.

@alchemist-ti
Copy link
Contributor

alchemist-ti commented Aug 8, 2023

ExpectedFeeAmount means the correct amount. I built this test case to ensure that fee calculation will be skipped only on whitelisted <-> whitelisted. Please have a look again.

For Cosmos, unused gas fees are not refunded, so if fee amount > min fee amount, fee amount will be deducted. If we want to check that taxes are calculated correctly, we should check treasury.TaxProceeds.

Copy link
Contributor

@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.

Confirmed working, thanks you @alchemist-ti

custom/auth/ante/expected_keeper.go Outdated Show resolved Hide resolved
@nghuyenthevinh2000 nghuyenthevinh2000 merged commit b0bc33f into main Aug 8, 2023
@nghuyenthevinh2000 nghuyenthevinh2000 deleted the whitelist-smart branch August 8, 2023 04:39
@nghuyenthevinh2000 nghuyenthevinh2000 added this to the v2.2.0 milestone Aug 8, 2023
@inon-man inon-man added the enhancement New feature or request label Sep 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request state machine breaking Something that will impact the state machine and consensus
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Whitelist dApp Contracts from Burn Taxes
4 participants