8000 Add common EstimateGas by 2dvorak · Pull Request #1878 · klaytn/klaytn · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content
This repository was archived by the owner on Aug 19, 2024. It is now read-only.

Add common EstimateGas #1878

Merged
merged 2 commits into from
Jul 25, 2023
Merged

Add common EstimateGas #1878

merged 2 commits into from
Jul 25, 2023

Conversation

2dvorak
Copy link
Collaborator
@2dvorak 2dvorak commented Jul 12, 2023 •
edited by blukat29
Loading

Proposed changes

Summary

Additional changes

  • Fix the OutOfGas handling in api.EthDoEstimateGas to follow geth.
    • Now returns the "gas required exceeds allowance" error upon ReceiptStatusErrOutOfGas.
  • Add GasPriceCap logic to api.DoEstimateGas. Clears a TODO.
    • Now returns the "insufficient funds..." error.
  • Replace duplicate revertError structs with blockchain.RevertError
  • Delete unused function isReverted()
  • New unit tests for eth_estimateGas and klay_estimateGas

Types of changes

Please put an x in the boxes related to your change.

  • Bugfix
  • New feature or enhancement
  • Others

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.

  • I have read the CONTRIBUTING GUIDELINES doc
  • I have signed the CLA
  • Lint and unit tests pass locally with my changes ($ make test)
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • Any dependent changes have been merged and published in downstream modules

Related issues

None

Further comments

The actions that each implementations perform can be listed into:

  • GasLimitCap: If user specified gas limit, spend gas up to it
  • GasPriceCap: If user specified gas price, spend gas up to (user balance - transferred value) / gas price
  • RpcGasCap: If node has rpc.gascap setting, spend gas up to it
  • BinarySearch: Binary search the required amount of gas
  • FindReason: If it fails to find the required amount of gas, find out why
Impl Before After
Simulated GasLimitCap
GasPriceCap
BinarySearch
FindReason		 CommonEstimateGas



EthEstimateGas GasLimitCap
GasPriceCap
RpcGasCap
BinarySearch
FindReason		 CommonEstimateGas




KlayEstimateGas GasLimitCap
BinarySearch
FindReason		 CommonEstimateGas


CommonEstimateGas GasLimitCap
GasPriceCap
RpcGasCap
BinarySearch
FindReason

@2dvorak 2dvorak self-assigned this Jul 12, 2023
@2dvorak 2dvorak force-pushed the common-estimategas branch 4 times, most recently from a75158c to 1b930df Compare July 14, 2023 16:08
@2dvorak 2dvorak marked this pull request as ready for review July 14, 2023 17:00
@blukat29 blukat29 marked this pull request as draft July 18, 2023 05:33
hyunsooda
hyunsooda reviewed Jul 18, 2023
View reviewed changes
Copy link
Contributor
@hyunsooda hyunsooda left a 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.

@blukat29 blukat29 force-pushed the common-estimategas branch from 1b930df to eba7485 Compare July 18, 2023 08:33
@2dvorak @blukat29
Implement common DoEstimateGas
e64287e 
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.
@blukat29 blukat29 force-pushed the common-estimategas branch from eba7485 to e64287e Compare July 18, 2023 09:03
@blukat29 blukat29 marked this pull request as ready for review July 18, 2023 09:10
@blukat29
Copy link
Contributor

Update: rewrite unit tests using MockBackend

hyunsooda
hyunsooda reviewed Jul 20, 2023
View reviewed changes
@blukat29 blukat29 force-pushed the common-estimategas branch from e2e523d to e7ce4fd Compare July 20, 2023 09:34
ian0371
ian0371 approved these changes Jul 24, 2023
View reviewed changes
Copy link
Contributor
@ian0371 ian0371 left a 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

@aidan-kwon aidan-kwon added the need to merge Need to merge for the next time label Jul 24, 2023
@2dvorak
Copy link
Collaborator Author
2dvorak commented Jul 24, 2023

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 DoEstimateGas in dev's api_public_blockchain.go lacks "GasPriceCap" (setting gas cap with user's balance and gas price) and leave it as TODO. This PR effectively adds GasPriceCap to DoEstimateGas in api_public_blockchain.go and that's why they behave differently.
Thanks for clarifying this point.

@JayChoi1736 JayChoi1736 mentioned this pull request Jul 24, 2023
Closed
20 tasks
@aidan-kwon
Copy link
Member

@kjeom @nohkwak PTAL

nohkwak
nohkwak approved these changes Jul 25, 2023
View reviewed changes
Copy link
Member
@nohkwak nohkwak left a 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 8000 changes in the SDK, is that correct?

@aidan-kwon aidan-kwon merged commit b464ee9 into klaytn:dev Jul 25, 2023
@2dvorak
Copy link
Collaborator Author
2dvorak commented Jul 25, 2023 •
edited
Loading

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 klay_estimateGas so the API result may differ. See above comment by @ian0371 as a reference. If there are test cases in SDK that uses the return value of klay_estimateGas, that test would probably need update too.

However, I could not find any test related to the return value of klay_estimateGas in caver-js. The most relevant one I could find was this, and I confirmed that the test passes with this PR's change.

> caver-js@1.10.2 myTest /home/2dvorak/repo/caver-js
> ./node_modules/mocha/bin/_mocha test/methodErrorHandling.js



  Error handling in Method package
    ✔ CAVERJS-UNIT-TX-729: should reject correct errors when fail to deploy contract (2051ms)
    ✔ CAVERJS-UNIT-TX-730: should throw expected errors when fail to execute contract (6076ms)

Since I am not familiar with caver-js and its tests, could you check if there is any other tests related to klay_estimateGas? If there's any, please see if the test passes with this PR's change.

@2dvorak 2dvorak removed the need to merge Need to merge for the next time label Jul 26, 2023
@2dvorak 2dvorak added this to the v1.11.0 milestone Jul 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@nohkwak nohkwak nohkwak approved these changes

@ian0371 ian0371 ian0371 approved these changes

@hyunsooda hyunsooda hyunsooda approved these changes

@aidan-kwon aidan-kwon aidan-kwon approved these changes

@blukat29 blukat29 Awaiting requested review from blukat29 blukat29 is a code owner

@kjeom kjeom Awaiting requested review from kjeom

Assignees

@2dvorak 2dvorak

Labels
None yet
Projects
None yet
Milestone
v1.11.0
Development

Successfully merging this pull request may close these issues.
6 participants
@2dvorak @blukat29 @aidan-kwon @nohkwak @ian0371 @hyunsooda
0