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

feat!: Apply feemarket to native cosmos tx #1194

Merged
facs95 merged 12 commits into from
Aug 10, 2022
Merged

feat!: Apply feemarket to native cosmos tx #1194

facs95 merged 12 commits into from
Aug 10, 2022

Conversation

@yihuang
Copy link

@yihuang yihuang commented Jul 20, 2022 •
edited
Loading

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)

@yihuang yihuang force-pushed the native-feemarket branch from d1e2c98 to 18a6c70 Compare July 20, 2022 03:16
fedekunze
fedekunze reviewed Jul 20, 2022
View reviewed changes
@fedekunze fedekunze mentioned this pull request Jul 28, 2022
Merged
11 tasks
@fedekunze
Copy link
Collaborator

fedekunze commented Jul 28, 2022

@yihuang can you fix the conflicts

@yihuang yihuang force-pushed the native-feemarket branch from 0a02832 to 0a7ab8b Compare July 29, 2022 03:57
@yihuang
Copy link
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
yihuang
yihuang commented Aug 5, 2022
View reviewed changes
@yihuang yihuang marked this pull request as ready for review August 5, 2022 14:08
@yihuang yihuang requested review from danburck and facs95 as code owners 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 •
edited
Loading

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%> (ø)

yihuang
yihuang commented Aug 8, 2022
View reviewed changes
@yihuang yihuang changed the title Apply feemarket to native cosmos tx feat!: Apply feemarket to native cosmos tx Aug 8, 2022
facs95
facs95 approved these changes Aug 8, 2022
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.

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.

fedekunze
fedekunze suggested changes Aug 9, 2022
View reviewed changes
Copy link
Collaborator

@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

HuangYi added 4 commits August 10, 2022 12:20
Problem: feemarket's query cli has redundant height parameter
e000bb2 
Soluton:
- remove the positional height parameter, since there's a flag already.

Update CHANGELOG.md
Apply feemarket to native cosmos tx
6ae8e85 
- 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 <31522760+fedekunze@users.noreply.github.com>

address review suggestions
fix unit tests
8d4510d
fix integration test
f37399c
@yihuang yihuang mentioned this pull request Aug 10, 2022
Closed
@yihuang yihuang requested review from facs95 and fedekunze August 10, 2022 06:45
@yihuang
Copy link
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

fedekunze
fedekunze approved these changes Aug 10, 2022
View reviewed changes
Copy link
Collaborator

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

@facs95 facs95 merged commit b1cd16e into evmos:main Aug 10, 2022
@yihuang yihuang deleted the native-feemarket branch August 10, 2022 22:40
This was referenced Aug 16, 2022
Closed
Closed
@danburck danburck mentioned this pull request Nov 30, 2022
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@danburck danburck Awaiting requested review from danburck

@facs95 facs95 Awaiting requested review from facs95

1 more reviewer

@fedekunze fedekunze fedekunze approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@yihuang @fedekunze @facs95