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

refactor: optimize AnteHandler gas consumption #1388

Merged
merged 16 commits into from
Oct 19, 2022

Conversation

Vvaradinov
Copy link
Contributor

@Vvaradinov Vvaradinov commented Oct 18, 2022

Description

This PR aims to optimize gas consumption by reordering AnteHandlers by executing stateless ones first followed by stateful ones. Additionally it provides specific getters for each individual parameter in the keeper and only calls ones needed by the particular AnteHandler.

Closes: ENG-905 ENG-444


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)

app/ante/eth.go Outdated Show resolved Hide resolved
app/ante/eth.go Outdated Show resolved Hide resolved
x/evm/keeper/params.go Outdated Show resolved Hide resolved
x/evm/keeper/params.go Outdated Show resolved Hide resolved
x/evm/keeper/params.go Outdated Show resolved Hide resolved
Co-authored-by: Federico Kunze Küllmer <31522760+fedekunze@users.noreply.github.com>
@github-actions github-actions bot added the Type: Tests issues and PR related to tests label Oct 19, 2022
@Vvaradinov Vvaradinov marked this pull request as ready for review October 19, 2022 09:37
@Vvaradinov Vvaradinov requested a review from a team as a code owner October 19, 2022 09:37
@Vvaradinov Vvaradinov requested review from GAtom22, 4rgon4ut and danburck and removed request for a team October 19, 2022 09:37
@codecov
Copy link

codecov bot commented Oct 19, 2022

Codecov Report

Merging #1388 (1ba4ac8) into main (5879750) will increase coverage by 0.12%.
The diff coverage is 98.18%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1388      +/-   ##
==========================================
+ Coverage   55.88%   56.00%   +0.12%     
==========================================
  Files         108      108              
  Lines       10021    10049      +28     
==========================================
+ Hits         5600     5628      +28     
  Misses       4140     4140              
  Partials      281      281              
Impacted Files Coverage Δ
app/ante/eth.go 80.16% <94.73%> (+0.10%) ⬆️
app/ante/fee_market.go 76.00% <100.00%> (ø)
app/ante/fees.go 96.84% <100.00%> (ø)
app/ante/handler_options.go 79.45% <100.00%> (ø)
x/evm/keeper/params.go 100.00% <100.00%> (ø)
x/feemarket/keeper/params.go 89.28% <100.00%> (+2.92%) ⬆️

Copy link
Contributor

@danburck danburck left a comment

Choose a reason for hiding this comment

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

@Vvaradinov nice work!

Can you add

  • unit tests for the getters and
  • an entry in the CHANGELOG.md?

x/feemarket/keeper/params.go Show resolved Hide resolved
x/evm/types/logs_test.go Show resolved Hide resolved
x/evm/keeper/params.go 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.

LGTM!

CHANGELOG.md Outdated Show resolved Hide resolved
app/ante/eth.go Show resolved Hide resolved
Vvaradinov and others added 3 commits October 19, 2022 14:38
x/evm/types/logs_test.go Show resolved Hide resolved
x/evm/keeper/params_test.go Show resolved Hide resolved
x/feemarket/keeper/params_test.go Show resolved Hide resolved
Copy link
Contributor

@danburck danburck left a comment

Choose a reason for hiding this comment

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

nice @Vvaradinov, I reviewed the changes and left a nit comment. Feel free to merge

x/evm/keeper/params_test.go Outdated Show resolved Hide resolved
@danburck danburck merged commit 83e509b into main Oct 19, 2022
@danburck danburck deleted the Vvaradinov/antehandler-gas-otpimization branch October 19, 2022 16:22
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.

The reason why we were calling the params as a set in the places I pointed out was not to make the query several time per property and per message. I liked the idea of making the code cleaner but maybe we can find a way in which is clear but keeps same - or better performance

app/ante/eth.go Show resolved Hide resolved
app/ante/eth.go Show resolved Hide resolved
app/ante/eth.go Show resolved Hide resolved
app/ante/eth.go Show resolved Hide resolved
x/evm/keeper/params.go Show resolved Hide resolved
x/evm/keeper/params.go Show resolved Hide resolved
x/evm/keeper/params.go Show resolved Hide resolved
x/evm/keeper/params.go Show resolved Hide resolved
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Type: Tests issues and PR related to tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants