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(fees): enable fees to be deducted from unclaimed staking rewards #1405

Merged
merged 83 commits into from
Mar 1, 2023
Merged

feat(fees): enable fees to be deducted from unclaimed staking rewards #1405

merged 83 commits into from
Mar 1, 2023

Conversation

MalteHerrmann
Copy link
Contributor

@MalteHerrmann MalteHerrmann commented Feb 17, 2023 •
edited

Description

This PR introduces the mechanics to achieve milestone 1 of the "fee subscription" initiative, which enables transaction fees to be deducted from unclaimed staking rewards.

  • Implement functionality
    • Cosmos Tx
    • EVM Tx
    • Method to withdraw a sufficient amount of staking rewards
  • Cover with tests (left for last because of ongoing refactors in the codebase)
    • Cosmos Tx
    • EVM Tx
    • Unit tests for reward withdrawal function

Closes ENG-1199

@linear
Copy link

linear bot commented Feb 17, 2023

ENG-1199 Milestone 1 - Only pay with rewards if user has no sufficient balance

Spec and Initiative:

https://www.notion.so/evmos/Evmos-Fee-Subscription-233036ff1d514573a2c72d8fd764eb5b

github-advanced-security[bot]
github-advanced-security bot found potential problems Feb 17, 2023
View reviewed changes
app/ante/cosmos/fees.go Fixed Show fixed Hide fixed
app/ante/cosmos/fees.go Fixed Show fixed Hide fixed
fedekunze
fedekunze reviewed Feb 17, 2023
View reviewed changes
app/ante/cosmos/fees.go Outdated Show resolved Hide resolved
app/ante/cosmos/fees.go Outdated Show resolved Hide resolved
app/ante/cosmos/fees.go Outdated Show resolved Hide resolved
app/ante/evm/eth.go Outdated Show resolved Hide resolved
fedekunze
fedekunze reviewed Feb 24, 2023
View reviewed changes
app/ante/cosmos/fees.go Outdated Show resolved Hide resolved
app/ante/cosmos/fees.go Outdated Show resolved Hide resolved
app/ante/cosmos/fees.go Outdated Show resolved Hide resolved
app/ante/cosmos/fees.go Outdated Show resolved Hide resolved
app/ante/cosmos/fees.go Outdated Show resolved Hide resolved
app/ante/cosmos/fees.go Outdated Show resolved Hide resolved
app/ante/cosmos/fees.go Outdated Show resolved Hide resolved
app/ante/utils/claim_rewards.go Outdated Show resolved Hide resolved
app/ante/utils/claim_rewards.go Outdated Show resolved Hide resolved
@Vvaradinov Vvaradinov mentioned this pull request Feb 27, 2023
Merged
facs95
facs95 approved these changes Feb 27, 2023
View reviewed changes
Copy link
Contributor

@facs95 facs95 left a comment

Choose a reason for hiding this comment

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

LGTM besides comments

app/ante/cosmos/fees.go Show resolved Hide resolved
app/ante/utils/claim_rewards.go Outdated Show resolved Hide resolved
fedekunze
fedekunze approved these changes Feb 28, 2023
View reviewed changes
Copy link
Contributor

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

ACK. Pending additional unit tests on app/ante/cosmos/fees.go

app/ante/cosmos/fees.go Show resolved Hide resolved
app/ante/utils/claim_rewards.go Outdated Show resolved Hide resolved
app/ante/utils/claim_rewards.go Outdated Show resolved Hide resolved
app/ante/utils/claim_rewards.go Outdated Show resolved Hide resolved
app/ante/utils/claim_rewards.go Show resolved Hide resolved
app/ante/utils/claim_rewards.go Outdated Show resolved Hide resolved
app/ante/cosmos/fees.go Show resolved Hide resolved
app/ante/cosmos/fees.go Show resolved Hide resolved
app/ante/cosmos/fees.go Show resolved Hide resolved
Vvaradinov and others added 4 commits February 28, 2023 16:07
@Vvaradinov
apply changes from code review
74d82d2
@MalteHerrmann
chore(tests): add integration tests for using staking rewards as fees (…
cdd604e 
…#1428)
@GAtom22
chore(tests) add more unit test for cosmos/fees.go (#1438)
7c9c137 
* chore(tests) add insufficient fees unit test

* chore(tests) add min gas price == 0 unit test

* chore(tests) add fee granter tests

* fix lint issues

* fix cosmos tests

* add fees unit test case

* add zero fees unit test case

* add unit test without freegrant keeper
@GAtom22
Merge branch 'main' into fee-subscription-milestone-1
fe872e7
GAtom22
GAtom22 approved these changes Feb 28, 2023
View reviewed changes
Copy link
Contributor

@GAtom22 GAtom22 left a comment

Choose a reason for hiding this comment

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

LGTM! Left a few comments and questions

app/ante/cosmos/fees.go Outdated Show resolved Hide resolved
app/ante/evm/fee_checker.go Show resolved Hide resolved
app/ante/setup_test.go Outdated Show resolved Hide resolved
app/ante/utils/claim_rewards.go Outdated Show resolved Hide resolved
app/ante/utils/setup_test.go Outdated Show resolved Hide resolved
app/ante/utils/setup_test.go Outdated Show resolved Hide resolved
@fedekunze fedekunze merged commit 4da4a04 into main Mar 1, 2023
@fedekunze fedekunze deleted the fee-subscription-milestone-1 branch March 1, 2023 02:54
@LampardNguyen234 LampardNguyen234 mentioned this pull request Aug 15, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fedekunze fedekunze fedekunze approved these changes

@danburck danburck danburck left review comments

@Vvaradinov Vvaradinov Vvaradinov approved these changes

@facs95 facs95 facs95 approved these changes

@GAtom22 GAtom22 GAtom22 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@MalteHerrmann @Vvaradinov @facs95 @fedekunze @danburck @GAtom22