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

Set priority for eth transactions #1214

Merged
merged 5 commits into from
Aug 5, 2022
Merged

Conversation

yihuang
Copy link
Contributor

@yihuang yihuang commented Jul 29, 2022

Description

Set the tx priority to the lowest priority in the messages.


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 marked this pull request as ready for review July 29, 2022 03:56
CHANGELOG.md Outdated Show resolved Hide resolved
@yihuang yihuang force-pushed the eth-priority branch 4 times, most recently from 63fe945 to 78ad2c0 Compare July 29, 2022 05:45
@facs95
Copy link
Contributor

facs95 commented Jul 29, 2022

Unit tests are failing @yihuang 🙏

@codecov
Copy link

codecov bot commented Jul 29, 2022

Codecov Report

Merging #1214 (4c760a3) into main (8d3a2b1) will increase coverage by 0.01%.
The diff coverage is 71.11%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1214      +/-   ##
==========================================
+ Coverage   57.19%   57.21%   +0.01%     
==========================================
  Files          94       94              
  Lines        8399     8419      +20     
==========================================
+ Hits         4804     4817      +13     
- Misses       3351     3356       +5     
- Partials      244      246       +2     
Impacted Files Coverage Δ
x/evm/types/access_list_tx.go 88.96% <0.00%> (-1.18%) ⬇️
x/evm/types/dynamic_fee_tx.go 89.94% <0.00%> (ø)
x/evm/types/legacy_tx.go 90.83% <0.00%> (-1.41%) ⬇️
x/evm/types/tx_data.go 97.91% <ø> (ø)
x/evm/keeper/utils.go 66.23% <69.56%> (-1.38%) ⬇️
app/ante/eth.go 80.05% <100.00%> (+0.43%) ⬆️
app/ante/handler_options.go 79.45% <100.00%> (ø)
x/evm/keeper/keeper.go 81.25% <100.00%> (+0.23%) ⬆️

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.

LGTM, would be great to add more integration tests and update the specification. Some remarks

  1. I see the case for some applications that might not want to enable the prioritized mempool so in that scenario, we should add a parameter to the EVM to disable tx prioritization
  2. Do we need to update the Cosmos ante handler to enable prioritization?

x/evm/keeper/utils.go Outdated Show resolved Hide resolved
x/evm/keeper/utils.go Show resolved Hide resolved
@yihuang
Copy link
Contributor Author

yihuang commented Jul 29, 2022

  • I see the case for some applications that might not want to enable the prioritized mempool so in that scenario, we should add a parameter to the EVM to disable tx prioritization

The validator is still free to choose the v0 mempool which is FIFO, ignore the priority field.

  • Do we need to update the Cosmos ante handler to enable prioritization?

The cosmos ante handler has a default mechanism to extract prioritization from fees directly, in another PR we'll customize that one with feemarket.

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.

We should add specific integration tests for tx prioritization before we can merge this:

  • legacy and access list txs
  • dynamic fee tx with 0 and > 0 priority tip

We also need to update the specification for the EVM with more details about tx prioritization for Ethereum msgs

x/evm/keeper/utils.go Show resolved Hide resolved
@yihuang
Copy link
Contributor Author

yihuang commented Aug 1, 2022

We should add specific integration tests for tx prioritization before we can merge this:

  • legacy and access list txs
  • dynamic fee tx with 0 and > 0 priority tip

added a priority integration test, which checked that a higher priority tx gets included first even if sent later.

We also need to update the specification for the EVM with more details about tx prioritization for Ethereum msgs

added sth in feemarket's spec in the ante handlers section.

@yihuang yihuang force-pushed the eth-priority branch 2 times, most recently from 4c136eb to 5d929bd Compare August 1, 2022 11:21
@yihuang yihuang mentioned this pull request Aug 1, 2022
11 tasks
go.mod Outdated Show resolved Hide resolved
@yihuang yihuang force-pushed the eth-priority branch 2 times, most recently from 99545ab to 9b30b65 Compare August 2, 2022 06:20
@yihuang yihuang requested a review from fedekunze August 2, 2022 06:22
Set the tx priority to the lowest priority in the messages.

fix unit tests

code cleanup and spec

update spec

fix go lint

add priority integration test

add python linter job

add access list tx type

fix gas limit

remove ledger tag, so no need to replace hid dependency

fix earlier check

ibc-go v5.0.0-beta1
@yihuang yihuang requested a review from fedekunze August 4, 2022 01:24
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. @facs95 @alexanderbez can you review?

@fedekunze fedekunze merged commit e156084 into evmos:main Aug 5, 2022
@yihuang yihuang deleted the eth-priority branch August 5, 2022 13:59
@danburck danburck mentioned this pull request Nov 30, 2022
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.

4 participants