-
Notifications
You must be signed in to change notification settings - Fork 189
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added eth namespace apis Call, EstimateGas #1134
Merged
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
Added SetStorage method to StateDB. It is needed when overrides option is given.
There is no easy way to classify the error returned from TransitionDb in Klaytn is vmError or consensusError or just client side error(e.g. user specify gas limit which is lower than intrinsicGas). Ethereum uses ExecutionResult because they can specify that errors in their codebase, but this is not adaptable to current Klaytn codebase. The idea which classifies errors looks good, maybe we can apply it later.
Added SetStorage method to StateDB. It is needed when overrides option is given.
There is no easy way to classify the error returned from TransitionDb in Klaytn is vmError or consensusError or just client side error(e.g. user specify gas limit which is lower than intrinsicGas). Ethereum uses ExecutionResult because they can specify that errors in their codebase, but this is not adaptable to current Klaytn codebase. The idea which classifies errors looks good, maybe we can apply it later.
…sendtxargs-apis-0
GetEVM is used by both klay_call and eth_call but both works differently, I moved that logic out of GetEVM. * Ethereum's Call and EstimateGas both requires caller have enough balance to proceed.
aeharvlee
requested review from
aidan-kwon,
jeongkyun-oh,
jimni1222,
KimKyungup and
mckim19
as code owners
January 27, 2022 09:11
jimni1222
reviewed
Jan 28, 2022
7 tasks
# Conflicts: # api/api_ethereum.go
jimni1222
reviewed
Feb 9, 2022
jimni1222
reviewed
Feb 9, 2022
jimni1222
reviewed
Feb 9, 2022
jimni1222
reviewed
Feb 9, 2022
jimni1222
reviewed
Feb 9, 2022
jimni1222
reviewed
Feb 9, 2022
5 tasks
5 tasks
Please resolve conflicts |
# Conflicts: # api/api_ethereum.go
Ethereum can specify vmError and consensus error during state transition in their codebase. But it is hard to adapt that feature(specifying errors) into Klaytn code base as now. So I added some additional handling in EthDoEstimateGas to specify whether the error is vmError or not.
jimni1222
reviewed
Feb 10, 2022
jimni1222
reviewed
Feb 10, 2022
jimni1222
reviewed
Feb 10, 2022
jimni1222
reviewed
Feb 10, 2022
DataError interface is introduced by Ethereum's #22677 PR. To support Ethereum compatibility, we should support it's error message format too.
aidan-kwon
reviewed
Feb 10, 2022
aidan-kwon
reviewed
Feb 10, 2022
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.
I think it's good except for the comment I leaved
@jeongkyun-oh @yoomee1313 PTAL |
jimni1222
approved these changes
Feb 11, 2022
aidan-kwon
approved these changes
Feb 11, 2022
sirano11
reviewed
Feb 11, 2022
sirano11
approved these changes
Feb 11, 2022
9 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Proposed changes
This PR implemented Eth namespace APIs
Call
andEstimateGas
.Ethereum Source Code links
Review Points
1. State Override feature
Call
api.2. Differences from Klaytn
gas
andgasPrice
field.gas * gasPrice
.For this, I moved logic adding balance of caller from GetEVM to out of it because both
klay_call
andeth_call
are now using same API but works differently.Klaytn add gasFee by default before proceeding
call
andestimateGas
.3. Differences from Ethereum implementation
Removed ExecutionResult struct
We can apply this changes maybe later.
AccessList is not supported yet
How to test this PR
First, please clone my origin repo.
1. Build ken with source codes related with this PR.
make ken
2. Run ken
eth
namepsace to RPC_API and WS_API in kend.confPrepare the test
Click to expand
Bytecode
Storage.bin-runtime
:6080604052600436106049576000357c0100000000000000000000000000000000000000000000000000000000900463ffffffff1680632e64cec114604e5780636057361d146076575b600080fd5b348015605957600080fd5b50606060a0565b6040518082815260200191505060405180910390f35b348015608157600080fd5b50609e6004803603810190808035906020019092919050505060a9565b005b60008054905090565b80600081905550505600a165627a7a723058207783dba41884f73679e167576362b7277f88458815141651f48ca38c25b498f80029
I used
$ docker run -v /Users/denver.lee/playground/solidity/storage:/sol ethereum/solc:0.4.24 --asm --abi --bin --bin-runtime --metadata --overwrite -o /sol/output /sol/storage.sol
to compile below Storage contract code.ABI Set to test function call of KIP-7
Test Call
Click to expand
I tested this api in the following order.
totalSupply
andbalanceOf
to check whether call is working or not.Storage.sol
and callretrieve
function ofStorage.sol
.Here is the my test scenario info below.
0xbE3892d33620bE5aca8c75D39e7401871194d290
0xca7a99380131e6c76cfa622396347107aeedca2d
Call totalSupply()
Call balanceOf("0xca7a99380131e6c76cfa622396347107aeedca2d")
State Override with Storage.sol and Call retrieve
Call with gas and gasPrice but balance is not enough
Balance of caller
0xe2df3753d74bebd8c4ccdbd5320b36e5ef8ca08d
:0
.Call with gas and gasPrice but balance is not enough -> Using State Override to cover this
But if we use State Override we can use it. Let's override state with balance
0x99999
.Test EstimateGas
Click to expand
Estimate Gas with gas and gasPrice. Caller balance is enough to call it.
Balance of caller
0xca7a99380131e6c76cfa622396347107aeedca2d
:9.99999999999999999999999999999999097166e+49
Estimate Gas with gas and gasPrice but Caller balance is not enough.
Balance of caller
0xe2df3753d74bebd8c4ccdbd5320b36e5ef8ca08d
:0
Types of changes
Please put an x in the boxes related to your change.
Checklist
Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.
$ make test
)Related issues
Further comments
If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...