This repository has been archived by the owner on Apr 4, 2024. It is now read-only.
Problem: state transition code is duplicated #674
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
yihuang
requested review from
fedekunze,
jolube and
khoslaventures
as code owners
yihuang
force-pushed
the
ctxstack
branch
2 times, most recently
from
ea35bc4
to
d7bd298
Compare
yihuang
commented
Oct 14, 2021
Codecov Report
@@ Coverage Diff @@
## main #674 +/- ##
==========================================
+ Coverage 57.20% 57.36% +0.16%
==========================================
Files 63 63
Lines 5545 5505 -40
==========================================
- Hits 3172 3158 -14
+ Misses 2209 2180 -29
- Partials 164 167 +3
|
fedekunze
suggested changes
Oct 14, 2021
x/evm/keeper/state_transition.go
Outdated
| @@ -314,72 +338,40 @@ func (k *Keeper) ApplyMessage(evm *vm.EVM, msg core.Message, cfg *params.ChainCo | |||
| ret, leftoverGas, vmErr = evm.Call(sender, *msg.To(), msg.Data(), leftoverGas, msg.Value()) | |||
| } | |||
|
|
|||
| refundQuotient := params.RefundQuotient | |||
| refundQuotient := ethparams.RefundQuotient | |||
There was a problem hiding this comment.
The module name is in conflict with the variable name, thus the rename.
There was a problem hiding this comment.
nit, I think you can undo this with the latest change
yihuang
commented
Oct 15, 2021
yihuang
force-pushed
the
ctxstack
branch
2 times, most recently
from
5881c0e
to
388e413
Compare
fedekunze
approved these changes
Oct 21, 2021
x/evm/keeper/state_transition.go
Outdated
| @@ -314,72 +338,40 @@ func (k *Keeper) ApplyMessage(evm *vm.EVM, msg core.Message, cfg *params.ChainCo | |||
| ret, leftoverGas, vmErr = evm.Call(sender, *msg.To(), msg.Data(), leftoverGas, msg.Value()) | |||
| } | |||
|
|
|||
| refundQuotient := params.RefundQuotient | |||
| refundQuotient := ethparams.RefundQuotient | |||
There was a problem hiding this comment.
nit, I think you can undo this with the latest change
fixed, changelog added. |
|
The test-solidity test case failure seems to also happens on the main branch, I've opened an issue for that: #690 |
|
@yihuang there seems to be a significant performance regression, can you check that? |
Closes: evmos#672 Solution: - move gas refund out from ApplyMessage - move check into ApplyMessage - move evm construction into ApplyMessage - ensure context stack is clean after ApplyMessage return fix unit tests undo rename add underflow check
- don't duplicate params loading - passing EVMConfig around as pointer
it seems fixed. |
yihuang
added a commit
to yihuang/ethermint
that referenced
this pull request
Nov 15, 2021
* Problem: state transition code is duplicated Closes: evmos#672 Solution: - move gas refund out from ApplyMessage - move check into ApplyMessage - move evm construction into ApplyMessage - ensure context stack is clean after ApplyMessage return fix unit tests undo rename add underflow check * improve performance - don't duplicate params loading - passing EVMConfig around as pointer
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes: #672
Description
The refactor is separated into three commits:
For contributor use:
docs/) or specification (x/<module>/spec/)godoccomments.Unreleasedsection inCHANGELOG.mdFiles changedin the Github PR explorerFor admin use:
WIP,R4R,docs, etc)