-
Notifications
You must be signed in to change notification settings - Fork 182
Conversation
a75158c
to
1b930df
Compare
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 believe this PR is a significant refactoring and is now ready for the review stage.
1b930df
to
eba7485
Compare
Klaytn currently has 3 different but similar implementations for estimateGas, 1) `simulated.go`, 2) `api_ethereum.go`, and 3) `api_public_blockchain.go`. Using one from `api_ethereum.go`, implemented a common DoEstimateGas and use that function in 3 different files.
eba7485
to
e64287e
Compare
Update: rewrite unit tests using MockBackend |
e2e523d
to
e7ce4fd
Compare
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 ran TestKlaytnAPI_EstimateGas
with dev's api_public_blockchain.go
(with minor changes in order to compile), and got the results (which I think is an expected change):
api_ethereum_test.go:2575: tc[02] = 0 err: insufficient balance for transfer (supplied gas 500000010499)
api_ethereum_test.go:2578:
Error Trace: api_ethereum_test.go:2578
api_public_blockchain_test.go:44
Error: "err: insufficient balance for transfer (supplied gas 500000010499)" does not contain "insufficient funds for transfer"
Test: TestKlaytnAPI_EstimateGas
Messages: 2
api_ethereum_test.go:2575: tc[03] = 21000 <nil>
api_ethereum_test.go:2577:
Error Trace: api_ethereum_test.go:2577
api_public_blockchain_test.go:44
Error: Expected value not to be nil.
Test: TestKlaytnAPI_EstimateGas
@ian0371 The |
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.
If this PR is applied, it would be nice to eliminate duplicate code and add test cases.
@2dvorak
I don't think there will be any changes in the SDK, is that correct?
@nohkwak This PR changes the internal logic of However, I could not find any test related to the return value of
Since I am not familiar with |
Proposed changes
Summary
accounts/abi/bind/backends/simulated.go
- SimulatedBackend.EstimateGasapi/api_ethereum.go
- api.EthDoEstimateGas (eth_estimateGas)api/api_public_blockchain.go
- api.DoEstimateGas (klay_estimateGas)blockchain/evm.go
- blockchain.DoEstimateGasAdditional changes
ReceiptStatusErrOutOfGas
.revertError
structs withblockchain.RevertError
accounts/abi/bind/backends/simulated.go
api/tx_args.go
isReverted()
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
None
Further comments
The actions that each implementations perform can be listed into:
(user balance - transferred value) / gas price
GasPriceCap
BinarySearch
FindReason
GasPriceCap
RpcGasCap
BinarySearch
FindReason
BinarySearch
FindReason
GasPriceCap
RpcGasCap
BinarySearch
FindReason