-
Notifications
You must be signed in to change notification settings - Fork 562
Conversation
63fe945
to
78ad2c0
Compare
Unit tests are failing @yihuang 🙏 |
Codecov Report
@@ 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
|
There was a problem hiding this 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
- 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
- Do we need to update the Cosmos ante handler to enable prioritization?
The validator is still free to choose the
The cosmos ante handler has a default mechanism to extract prioritization from fees directly, in another PR we'll customize that one with feemarket. |
There was a problem hiding this 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
added a priority integration test, which checked that a higher priority tx gets included first even if sent later.
added sth in feemarket's spec in the ante handlers section. |
4c136eb
to
5d929bd
Compare
99545ab
to
9b30b65
Compare
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
There was a problem hiding this 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?
Description
Set the tx priority to the lowest priority in the messages.
For contributor use:
docs/
) or specification (x/<module>/spec/
)godoc
comments.Unreleased
section inCHANGELOG.md
Files changed
in the Github PR explorerFor admin use:
WIP
,R4R
,docs
, etc)