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

imp(evm): update move gas functions to a new file #1299

Closed
wants to merge 15 commits into from

Conversation

fedekunze
Copy link
Contributor

@fedekunze fedekunze commented Aug 24, 2022

Closes: #XXX

Description


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)

@fedekunze fedekunze changed the title fedekunze/minor gas refactor imp(evm): update move gas functions to a new file Aug 24, 2022
res.Logs = types.NewLogsFromEth(receipt.Logs)
res.GasUsed = receipt.GasUsed
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@facs95 @yihuang if the gas used from the receipt is updated in the hook, the gas used was still the old one defined by the initial result

Copy link
Contributor

@yihuang yihuang Aug 25, 2022

Choose a reason for hiding this comment

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

I see, so now hooks can increase the gas used, but it needs to be careful to not increase beyond the gas limit.
Then should we update the leftoverGas?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the main issue is that the gas used in the hooks is not considered in the transaction. Gas estimation would also need to be updated to reflect this change.

Not sure if it's worth it...

Copy link
Contributor

@yihuang yihuang Aug 25, 2022

Choose a reason for hiding this comment

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

yeah, precompiles has an RequiredGas method though, which is better in this regard.

Copy link
Contributor

Choose a reason for hiding this comment

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

@fedekunze increasing the gasUsed by how much gas was used during Post-Processing is out of scope of this PR and should be treated as an extra ticket.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup, agree

@fedekunze fedekunze marked this pull request as ready for review August 24, 2022 15:41
@codecov
Copy link

codecov bot commented Aug 24, 2022

Codecov Report

Merging #1299 (314d80b) into main (5b13f6d) will increase coverage by 0.00%.
The diff coverage is 72.97%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1299   +/-   ##
=======================================
  Coverage   55.86%   55.86%           
=======================================
  Files         108      109    +1     
  Lines       10017    10022    +5     
=======================================
+ Hits         5596     5599    +3     
- Misses       4140     4142    +2     
  Partials      281      281           
Impacted Files Coverage Δ
x/evm/keeper/state_transition.go 76.72% <50.00%> (-0.31%) ⬇️
x/evm/keeper/gas.go 77.41% <77.41%> (ø)

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.

Is this something that we still think is needed? Based on the conversation https://github.com/evmos/ethermint/pull/1299/files#r953968127

temporaryGasUsed := msg.Gas() - leftoverGas

// gasUsed = max(gasUsed, gasLimit * minGasMultiplier)
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove comment

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.

@fedekunze moving the gas functions LGTM

It sounds like there are more changes required to account for gas used during post processing. Can you add a ticket for this please?

@danburck
Copy link
Contributor

@fedekunze the integration tests are breaking. Can you add a description to the PR? Are there any expected changes in the amount of gasUsed?

@danburck danburck requested a review from a team as a code owner October 18, 2022 13:59
@fedekunze fedekunze marked this pull request as draft October 19, 2022 07:11
@github-actions
Copy link

github-actions bot commented Dec 4, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days-before-close if no further activity occurs.

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

Successfully merging this pull request may close these issues.

4 participants