[evm] refactor evm functions parameters #3959

envestcc · 2023-11-01T16:41:38Z

Description

Introducing HelperContext in EVM simplifies the parameter passing in public methods, mitigates the issue of passing parameters through multiple layers, and minimize subsequent modifications when adding parameters.

Type of change

Please delete options that are not relevant.

[] Bug fix (non-breaking change which fixes an issue)
[] New feature (non-breaking change which adds functionality)
Code refactor or improvement
[] Breaking change (fix or feature that would cause a new or changed behavior of existing functionality)
[] This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

make test
[] fullsync
[] Other test (please specify)

Test Configuration:

Firmware version:
Hardware:
Toolchain:
SDK:

Checklist:

[] My code follows the style guidelines of this project
[] I have performed a self-review of my code
[] I have commented my code, particularly in hard-to-understand areas
[] I have made corresponding changes to the documentation
[] My changes generate no new warnings
[] I have added tests that prove my fix is effective or that my feature works
[] New and existing unit tests pass locally with my changes
[] Any dependent changes have been merged and published in downstream modules

codecov · 2023-11-01T17:00:47Z

Codecov Report

Merging #3959 (4ed4158) into master (e1f0636) will increase coverage by 0.74%.
Report is 100 commits behind head on master.
The diff coverage is 77.22%.

❗ Current head 4ed4158 differs from pull request most recent head 8d71e84. Consider uploading reports for the commit 8d71e84 to get more accurate results

@@            Coverage Diff             @@
##           master    #3959      +/-   ##
==========================================
+ Coverage   75.38%   76.12%   +0.74%     
==========================================
  Files         303      330      +27     
  Lines       25923    28076    +2153     
==========================================
+ Hits        19541    21372    +1831     
- Misses       5360     5606     +246     
- Partials     1022     1098      +76

Files	Coverage Δ
action/candidate_register.go	`90.00% <100.00%> (ø)`
action/candidate_update.go	`89.01% <100.00%> (+0.12%)`	⬆️
action/claimreward.go	`90.62% <100.00%> (-0.42%)`	⬇️
action/depositreward.go	`87.50% <100.00%> (-0.56%)`	⬇️
action/protocol/context.go	`67.66% <100.00%> (+0.49%)`	⬆️
action/protocol/execution/protocol.go	`48.83% <100.00%> (+6.73%)`	⬆️
action/protocol/rewarding/protocol.go	`80.08% <ø> (+0.67%)`	⬆️
action/protocol/rewarding/reward.go	`90.07% <100.00%> (+0.60%)`	⬆️
...tion/protocol/staking/candidate_buckets_indexer.go	`89.31% <100.00%> (-0.47%)`	⬇️
...col/staking/ethabi/stake_composite_bucketscount.go	`100.00% <100.00%> (ø)`
... and 71 more

... and 4 files with indirect coverage changes

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

CoderZhi · 2023-11-07T00:30:17Z

action/protocol/execution/evm/evm.go

+	helperCtx := mustGetHelperCtx(ctx)
+	ctx = WithHelperCtx(ctx, HelperContext{
+		GetBlockHash: helperCtx.GetBlockHash,
+		DepositGasFunc: func(context.Context, protocol.StateManager, address.Address, *big.Int, *big.Int) (*action.TransactionLog, error) {
+			return nil, nil
+		},
+		Sgd: nil,
+	})


add TODO: move the logic out of SimulateExecution

It is preferred to be done in this pr since putting mustGetHelperCtx aside WithHelperCtx is strange

will open another PR to address it, to avoid causing too many code conflicts in the upcoming PRs.

CoderZhi · 2023-11-07T00:31:29Z

action/protocol/execution/evm/evm.go

-		func(context.Context, protocol.StateManager, address.Address, *big.Int, *big.Int) (*action.TransactionLog, error) {
-			return nil, nil
-		},
-		nil,


wrong implementation. SimulateExecution is supposed to have the same behavior as ExecuteContract, including gas

CoderZhi · 2023-11-07T05:41:59Z

action/protocol/poll/staking_committee.go

-		func(height uint64) (hash.Hash256, error) {
-			return hash.ZeroHash256, nil
-		},
-		func(context.Context, protocol.StateManager, address.Address, *big.Int, *big.Int) (*action.TransactionLog, error) {
-			return nil, nil
-		},
-		nil,


CoderZhi · 2023-11-07T05:42:34Z

api/coreservice.go

+func (core *coreService) simulateExecution(ctx context.Context, addr address.Address, exec *action.Execution, getBlockHash evm.GetBlockHash) ([]byte, *action.Receipt, error) {
+	ctx = evm.WithHelperCtx(ctx, evm.HelperContext{
+		GetBlockHash: getBlockHash,
+	})


add a TODO to add depositGas

CoderZhi · 2023-11-07T05:42:46Z

chainservice/builder.go

-			data, _, err := factory.SimulateExecution(ctx, addr, ex, dao.GetBlockHash)
+			ctx = evm.WithHelperCtx(ctx, evm.HelperContext{
+				GetBlockHash: dao.GetBlockHash,
+			})


CoderZhi · 2023-11-08T01:19:18Z

action/protocol/execution/evm/evm.go

 	if err != nil {
 		return nil, nil, err
 	}
+	sgd := ps.helperCtx.Sgd


Change L277-287:

receiver, sharedGas, err = processSGD(ctx, sm, execution, consumedGas, helperCtx) if err != nil { return nil, nil, errors.Wrap(err, "failed to process Sharing of Gas-fee with DApps") }

after modifying func processSGD(ctx, sm, execution, consumeGas, helperCtx)

CoderZhi · 2023-11-10T15:53:29Z

action/protocol/execution/evm/evm.go

+func depositeGas(ctx context.Context, sm protocol.StateManager, execution *action.Execution, consumedGas uint64, gasPrice *big.Int, hCtx HelperContext, fCtx protocol.FeatureCtx) (*action.TransactionLog, error) {
+	var (
+		receiver                  address.Address
+		sharedGas                 uint64
+		sharedGasFee, totalGasFee *big.Int
+		err                       error
+	)
+	if fCtx.SharedGasWithDapp && hCtx.Sgd != nil {
+		receiver, sharedGas, err = processSGD(ctx, sm, execution, consumedGas, hCtx.Sgd)
+		if err != nil {
+			return nil, errors.Wrap(err, "failed to process Sharing of Gas-fee with DApps")
+		}
+	}
+	if sharedGas > 0 {
+		sharedGasFee = big.NewInt(int64(sharedGas))
+		sharedGasFee.Mul(sharedGasFee, gasPrice)
+	}
+	totalGasFee = new(big.Int).Mul(new(big.Int).SetUint64(consumedGas), gasPrice)
+	return hCtx.DepositGasFunc(ctx, sm, receiver, totalGasFee, sharedGasFee)
+}


this is not what I mean.

func processSGD(ctx context.Context, sm protocol.StateManager, execution *action.Execution, consumedGas uint64, sgd SGDRegistry, ) (address.Address, uint64, error) { if sgd == nil { return nil, 0, nil } ....

thus, outside, we only need to checkif ps.featureCtx.SharedGasWithDapp

reverted, and added TODO

Liuhaai · 2023-11-10T17:02:57Z

action/protocol/poll/consortium.go

@@ -291,7 +294,10 @@ func getContractReaderForGenesisStates(ctx context.Context, sm protocol.StateMan
 			return nil, err
 		}

-		res, _, err := evm.SimulateExecution(ctx, sm, addr, ex, getBlockHash)
+		ctx = evm.WithHelperCtx(ctx, evm.HelperContext{


WithHelperCtx could be called by the caller, then getBlockHash argument can be removed

added a TODO

Liuhaai · 2023-11-10T17:09:02Z

action/protocol/execution/evm/evm.go

@@ -316,6 +300,27 @@ func ExecuteContract(
 	return retval, receipt, nil
 }

+func depositeGas(ctx context.Context, sm protocol.StateManager, execution *action.Execution, consumedGas uint64, gasPrice *big.Int, hCtx HelperContext, fCtx protocol.FeatureCtx) (*action.TransactionLog, error) {


this change isn't related to the intro of WithHelperCtx, which could be done in another pr.

This reverts commit d7bf0a4.

sonarqubecloud · 2023-11-13T15:01:15Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
6 Code Smells

No Coverage information
7.8% Duplication

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

envestcc force-pushed the pr_evmfunction branch 2 times, most recently from b8b6ed6 to 59c3d73 Compare November 7, 2023 01:24

envestcc marked this pull request as ready for review November 7, 2023 01:25

envestcc requested review from CoderZhi, dustinxie, Liuhaai, millken and a team as code owners November 7, 2023 01:25

CoderZhi approved these changes Nov 7, 2023

View reviewed changes

envestcc mentioned this pull request Nov 7, 2023

[evm] introduce GetBlockTime function #3961

Merged

1 task

CoderZhi reviewed Nov 8, 2023

View reviewed changes

envestcc force-pushed the pr_evmfunction branch from 08595a8 to d7bf0a4 Compare November 9, 2023 15:17

CoderZhi reviewed Nov 10, 2023

View reviewed changes

Liuhaai reviewed Nov 10, 2023

View reviewed changes

Liuhaai approved these changes Nov 10, 2023

View reviewed changes

envestcc added 5 commits November 13, 2023 22:59

refactor evm function parameters by HelperContext

76dbbbf

update tests

9d63fb2

address comments

49e914f

Revert "address comments"

fbf55a7

This reverts commit d7bf0a4.

add TODOs

8d71e84

envestcc force-pushed the pr_evmfunction branch from d7bf0a4 to 8d71e84 Compare November 13, 2023 15:00

envestcc merged commit be3a09a into iotexproject:master Nov 14, 2023
2 of 3 checks passed

envestcc deleted the pr_evmfunction branch November 14, 2023 09:27

envestcc linked an issue Nov 17, 2023 that may be closed by this pull request

Upgrade evm to Shanghai #3973

Closed

6 tasks

envestcc mentioned this pull request Nov 24, 2023

Upgrade evm to Shanghai #3973

Closed

6 tasks

envestcc removed a link to an issue Nov 24, 2023

Upgrade evm to Shanghai #3973

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[evm] refactor evm functions parameters #3959

[evm] refactor evm functions parameters #3959

envestcc commented Nov 1, 2023 •

edited

Loading

codecov bot commented Nov 1, 2023 •

edited

Loading

CoderZhi Nov 7, 2023

Liuhaai Nov 8, 2023

envestcc Nov 9, 2023

Liuhaai Nov 10, 2023

CoderZhi Nov 7, 2023

CoderZhi Nov 7, 2023

CoderZhi Nov 7, 2023

CoderZhi Nov 7, 2023

CoderZhi Nov 8, 2023

CoderZhi Nov 10, 2023

envestcc Nov 13, 2023 •

edited

Loading

Liuhaai Nov 10, 2023

envestcc Nov 13, 2023

Liuhaai Nov 10, 2023

envestcc Nov 13, 2023

sonarqubecloud bot commented Nov 13, 2023

[evm] refactor evm functions parameters #3959

[evm] refactor evm functions parameters #3959

Conversation

envestcc commented Nov 1, 2023 • edited Loading

Description

Type of change

How Has This Been Tested?

Checklist:

codecov bot commented Nov 1, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

envestcc Nov 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Nov 13, 2023

envestcc commented Nov 1, 2023 •

edited

Loading

codecov bot commented Nov 1, 2023 •

edited

Loading

envestcc Nov 13, 2023 •

edited

Loading