Skip to content
This repository has been archived by the owner on Apr 4, 2024. It is now read-only.

feat!: Apply feemarket to native cosmos tx #1194

Merged
merged 12 commits into from
Aug 10, 2022
Merged

Conversation

yihuang
Copy link
Contributor

@yihuang yihuang commented Jul 20, 2022

Description

Apply feemarket to native cosmos tx

  • add tx extension option for user to input tip price
  • apply feemarket's base fee to native tx
  • fallback to default sdk logic when london hardfork not enabled

For contributor use:

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer

For admin use:

  • Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
  • Reviewers assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

x/evm/keeper/sdk_tx.go Outdated Show resolved Hide resolved
x/evm/keeper/sdk_tx.go Outdated Show resolved Hide resolved
x/evm/keeper/sdk_tx.go Outdated Show resolved Hide resolved
x/evm/keeper/sdk_tx.go Outdated Show resolved Hide resolved
x/evm/keeper/sdk_tx.go Outdated Show resolved Hide resolved
x/evm/keeper/sdk_tx.go Outdated Show resolved Hide resolved
x/evm/keeper/sdk_tx.go Outdated Show resolved Hide resolved
@fedekunze fedekunze mentioned this pull request Jul 28, 2022
11 tasks
@fedekunze
Copy link
Contributor

@yihuang can you fix the conflicts

@yihuang
Copy link
Contributor Author

yihuang commented Jul 29, 2022

@yihuang can you fix the conflicts

done, and extracted the evm tx priority part to separate PR, should do that one first: #1214

@yihuang yihuang force-pushed the native-feemarket branch 2 times, most recently from 6d7d6a5 to 5ff0552 Compare August 2, 2022 03:59
@yihuang yihuang changed the title Apply prioritized mempool in 0.46 Apply feemarket to native cosmos tx Aug 2, 2022
@yihuang yihuang force-pushed the native-feemarket branch 3 times, most recently from 43a89dd to f51f129 Compare August 2, 2022 09:22
CHANGELOG.md Outdated Show resolved Hide resolved
@yihuang yihuang marked this pull request as ready for review August 5, 2022 14:08
@yihuang yihuang requested a review from fedekunze August 5, 2022 14:08
@yihuang yihuang force-pushed the native-feemarket branch 2 times, most recently from 4a2539b to 89e1a16 Compare August 8, 2022 02:13
@codecov
Copy link

codecov bot commented Aug 8, 2022

Codecov Report

Merging #1194 (e209886) into main (42abb25) will increase coverage by 0.41%.
The diff coverage is 89.47%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1194      +/-   ##
==========================================
+ Coverage   51.92%   52.33%   +0.41%     
==========================================
  Files         102      104       +2     
  Lines        9284     9387     +103     
==========================================
+ Hits         4821     4913      +92     
- Misses       4215     4225      +10     
- Partials      248      249       +1     
Impacted Files Coverage Δ
app/ante/ante.go 52.45% <0.00%> (-2.72%) ⬇️
types/dynamic_fee.go 0.00% <0.00%> (ø)
x/evm/types/dynamic_fee_tx.go 89.94% <0.00%> (ø)
x/evm/types/utils.go 58.18% <0.00%> (-2.20%) ⬇️
app/ante/fee_checker.go 96.73% <96.73%> (ø)
app/app.go 84.86% <100.00%> (+0.06%) ⬆️
types/codec.go 100.00% <100.00%> (ø)
x/evm/keeper/utils.go 66.23% <100.00%> (ø)

app/simulation_test.go Outdated Show resolved Hide resolved
@yihuang yihuang changed the title Apply feemarket to native cosmos tx feat!: Apply feemarket to native cosmos tx Aug 8, 2022
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.

I think this looks good so I will approve it but I will open a discussion.

I would prefer sdk_tx.go logic to be in the Feemarket module instead of the EVM module. I understand that it needs the LondonHardFork param so it will be kind of tricky to have this in the FeeMarket so happy to leave it like this. I just think it will be the cleaner approach if there was a way to make it happen.

x/evm/keeper/sdk_tx.go Outdated Show resolved Hide resolved
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.

Thank for the PR. How do you see wallets implementing the extension? If we don't use it by default most wallets won't use this logic. I'd change the default to be the fee market to cosmos txs and the extension for the old logic.

Also, this PR is missing unit tests

proto/ethermint/evm/v1/tx.proto Outdated Show resolved Hide resolved
x/evm/types/extension_option.go Outdated Show resolved Hide resolved
app/ante/ante.go Outdated Show resolved Hide resolved
x/evm/keeper/sdk_tx.go Outdated Show resolved Hide resolved
x/feemarket/client/cli/query.go Outdated Show resolved Hide resolved
x/evm/keeper/sdk_tx.go Outdated Show resolved Hide resolved
x/evm/keeper/sdk_tx.go Outdated Show resolved Hide resolved
Soluton:
- remove the positional height parameter, since there's a flag already.

Update CHANGELOG.md
- add tx extension option for user to input tip price
- apply feemarket's base fee to native tx

comments and cleanup

fallback to default sdk logic when london hardfork not enabled

integration test

cleanup feemarket query cli commands

Update CHANGELOG.md

update unit tests

disable feemarket in simulation tests for now

fix lint

Update app/simulation_test.go

fix python lint

fix lint

Update x/evm/types/extension_option.go

Co-authored-by: Federico Kunze Küllmer <[email protected]>

address review suggestions
@yihuang
Copy link
Contributor Author

yihuang commented Aug 10, 2022

Thank for the PR. How do you see wallets implementing the extension? If we don't use it by default most wallets won't use this logic. I'd change the default to be the fee market to cosmos txs and the extension for the old logic.

  • I guess It'll take some time for keplr to support but we can try, hopefully creating some standard.
  • Apps that build tx by themselves are able to use it.
  • The default behavior when the extension option is missing should be intuitive.

Also, this PR is missing unit tests

added

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. I did a minor refactor to make the code more maintainable. Great work!

@fedekunze fedekunze enabled auto-merge (squash) August 10, 2022 09:03
auto-merge was automatically disabled August 10, 2022 09:51

Head branch was pushed to by a user without write access

@yihuang
Copy link
Contributor Author

yihuang commented Aug 10, 2022

ACK. I did a minor refactor to make the code more maintainable. Great work!

the gofmt warning can't be reproduced locally, any ideas @facs95 ?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants