Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[evm] refactor evm functions parameters #3959

Merged
merged 5 commits into from
Nov 14, 2023
Merged

[evm] refactor evm functions parameters #3959

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
76dbbbf
refactor evm function parameters by HelperContext
envestcc Nov 1, 2023
9d63fb2
update tests
envestcc Nov 1, 2023
49e914f
address comments
envestcc Nov 9, 2023
fbf55a7
Revert "address comments"
envestcc Nov 13, 2023
8d71e84
add TODOs
envestcc Nov 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 33 additions & 0 deletions action/protocol/execution/evm/context.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
package evm

import (
"context"

"github.com/iotexproject/iotex-core/pkg/log"
)

type (
helperContextKey struct{}

// HelperContext is the context for EVM helper
HelperContext struct {
GetBlockHash GetBlockHash
DepositGasFunc DepositGasWithSGD
// TODO: sgd should be moved into depositGasFunc
Sgd SGDRegistry
}
)

// WithHelperCtx returns a new context with helper context
func WithHelperCtx(ctx context.Context, hctx HelperContext) context.Context {
return context.WithValue(ctx, helperContextKey{}, hctx)
}

// mustGetHelperCtx returns the helper context from the context
func mustGetHelperCtx(ctx context.Context) HelperContext {
hc, ok := ctx.Value(helperContextKey{}).(HelperContext)
if !ok {
log.S().Panic("Miss evm helper context")
}
return hc
}
29 changes: 17 additions & 12 deletions action/protocol/execution/evm/evm.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,7 @@ type (
genesis genesis.Blockchain
featureCtx protocol.FeatureCtx
actionCtx protocol.ActionCtx
helperCtx HelperContext
}
)

Expand All @@ -105,15 +106,16 @@ func newParams(
ctx context.Context,
execution *action.Execution,
stateDB *StateDBAdapter,
getBlockHash GetBlockHash,
) (*Params, error) {
var (
actionCtx = protocol.MustGetActionCtx(ctx)
blkCtx = protocol.MustGetBlockCtx(ctx)
featureCtx = protocol.MustGetFeatureCtx(ctx)
g = genesis.MustExtractGenesisContext(ctx)
helperCtx = mustGetHelperCtx(ctx)
evmNetworkID = protocol.MustGetBlockchainCtx(ctx).EvmNetworkID
executorAddr = common.BytesToAddress(actionCtx.Caller.Bytes())
getBlockHash = helperCtx.GetBlockHash

vmConfig vm.Config
contractAddrPointer *common.Address
Expand Down Expand Up @@ -198,6 +200,7 @@ func newParams(
g.Blockchain,
featureCtx,
actionCtx,
helperCtx,
}, nil
}

Expand All @@ -224,9 +227,6 @@ func ExecuteContract(
ctx context.Context,
sm protocol.StateManager,
execution *action.Execution,
getBlockHash GetBlockHash,
depositGasFunc DepositGasWithSGD,
sgd SGDRegistry,
) ([]byte, *action.Receipt, error) {
ctx, span := tracer.NewSpan(ctx, "evm.ExecuteContract")
defer span.End()
Expand All @@ -235,10 +235,11 @@ func ExecuteContract(
if err != nil {
return nil, nil, err
}
ps, err := newParams(ctx, execution, stateDB, getBlockHash)
ps, err := newParams(ctx, execution, stateDB)
if err != nil {
return nil, nil, err
}
sgd := ps.helperCtx.Sgd
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

retval, depositGas, remainingGas, contractAddress, statusCode, err := executeInEVM(ps, stateDB)
if err != nil {
return nil, nil, err
Expand Down Expand Up @@ -279,6 +280,7 @@ func ExecuteContract(
sharedGasFee, totalGasFee *big.Int
)
if ps.featureCtx.SharedGasWithDapp && sgd != nil {
// TODO: sgd is whether nil should be checked in processSGD
receiver, sharedGas, err = processSGD(ctx, sm, execution, consumedGas, sgd)
if err != nil {
return nil, nil, errors.Wrap(err, "failed to process Sharing of Gas-fee with DApps")
Expand All @@ -289,7 +291,7 @@ func ExecuteContract(
sharedGasFee.Mul(sharedGasFee, ps.txCtx.GasPrice)
}
totalGasFee = new(big.Int).Mul(new(big.Int).SetUint64(consumedGas), ps.txCtx.GasPrice)
depositLog, err = depositGasFunc(ctx, sm, receiver, totalGasFee, sharedGasFee)
depositLog, err = ps.helperCtx.DepositGasFunc(ctx, sm, receiver, totalGasFee, sharedGasFee)
if err != nil {
return nil, nil, err
}
Expand Down Expand Up @@ -610,7 +612,6 @@ func SimulateExecution(
sm protocol.StateManager,
caller address.Address,
ex *action.Execution,
getBlockHash GetBlockHash,
) ([]byte, *action.Receipt, error) {
ctx, span := tracer.NewSpan(ctx, "evm.SimulateExecution")
defer span.End()
Expand Down Expand Up @@ -638,14 +639,18 @@ func SimulateExecution(
)

ctx = protocol.WithFeatureCtx(ctx)
// TODO: move the logic out of SimulateExecution
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,
})
Comment on lines +643 to +650
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add TODO: move the logic out of SimulateExecution

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add TODO

return ExecuteContract(
ctx,
sm,
ex,
getBlockHash,
func(context.Context, protocol.StateManager, address.Address, *big.Int, *big.Int) (*action.TransactionLog, error) {
return nil, nil
},
nil,
Comment on lines -646 to -649
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

)
}
20 changes: 13 additions & 7 deletions action/protocol/execution/evm/evm_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,13 +60,16 @@ func TestExecuteContractFailure(t *testing.T) {
ChainID: 1,
EvmNetworkID: 100,
})
retval, receipt, err := ExecuteContract(ctx, sm, e,
func(uint64) (hash.Hash256, error) {
ctx = WithHelperCtx(ctx, HelperContext{
GetBlockHash: func(uint64) (hash.Hash256, error) {
return hash.ZeroHash256, nil
},
func(context.Context, protocol.StateManager, address.Address, *big.Int, *big.Int) (*action.TransactionLog, error) {
DepositGasFunc: func(context.Context, protocol.StateManager, address.Address, *big.Int, *big.Int) (*action.TransactionLog, error) {
return nil, nil
}, nil)
},
Sgd: nil,
})
retval, receipt, err := ExecuteContract(ctx, sm, e)
require.Nil(t, retval)
require.Nil(t, receipt)
require.Error(t, err)
Expand Down Expand Up @@ -255,11 +258,14 @@ func TestConstantinople(t *testing.T) {
GasLimit: testutil.TestGasLimit,
BlockHeight: e.height,
}))
fCtx = WithHelperCtx(fCtx, HelperContext{
GetBlockHash: func(uint64) (hash.Hash256, error) {
return hash.ZeroHash256, nil
},
})
stateDB, err := prepareStateDB(fCtx, sm)
require.NoError(err)
ps, err := newParams(fCtx, ex, stateDB, func(uint64) (hash.Hash256, error) {
return hash.ZeroHash256, nil
})
ps, err := newParams(fCtx, ex, stateDB)
require.NoError(err)

var evmConfig vm.Config
Expand Down
11 changes: 8 additions & 3 deletions action/protocol/execution/protocol.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,11 @@ package execution
import (
"context"

"github.com/iotexproject/go-pkgs/hash"
"github.com/iotexproject/iotex-address/address"
"github.com/pkg/errors"
"go.uber.org/zap"

"github.com/iotexproject/go-pkgs/hash"
"github.com/iotexproject/iotex-address/address"
"github.com/iotexproject/iotex-core/action"
"github.com/iotexproject/iotex-core/action/protocol"
"github.com/iotexproject/iotex-core/action/protocol/execution/evm"
Expand Down Expand Up @@ -66,7 +66,12 @@ func (p *Protocol) Handle(ctx context.Context, act action.Action, sm protocol.St
if !ok {
return nil, nil
}
_, receipt, err := evm.ExecuteContract(ctx, sm, exec, p.getBlockHash, p.depositGas, p.sgdRegistry)
ctx = evm.WithHelperCtx(ctx, evm.HelperContext{
GetBlockHash: p.getBlockHash,
DepositGasFunc: p.depositGas,
Sgd: p.sgdRegistry,
})
_, receipt, err := evm.ExecuteContract(ctx, sm, exec)

if err != nil {
return nil, errors.Wrap(err, "failed to execute contract")
Expand Down
6 changes: 4 additions & 2 deletions action/protocol/execution/protocol_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,8 +283,10 @@ func readExecution(
if err != nil {
return nil, nil, err
}

return sf.SimulateExecution(ctx, addr, exec, dao.GetBlockHash)
ctx = evm.WithHelperCtx(ctx, evm.HelperContext{
GetBlockHash: dao.GetBlockHash,
})
return sf.SimulateExecution(ctx, addr, exec)
}

func runExecutions(
Expand Down
29 changes: 18 additions & 11 deletions action/protocol/poll/consortium.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,16 +11,17 @@ import (
"github.com/ethereum/go-ethereum/crypto"
"github.com/iotexproject/go-pkgs/hash"
"github.com/iotexproject/iotex-address/address"
"github.com/iotexproject/iotex-proto/golang/iotextypes"
"github.com/pkg/errors"
"go.uber.org/zap"

"github.com/iotexproject/iotex-core/action"
"github.com/iotexproject/iotex-core/action/protocol"
"github.com/iotexproject/iotex-core/action/protocol/execution/evm"
"github.com/iotexproject/iotex-core/action/protocol/rolldpos"
"github.com/iotexproject/iotex-core/blockchain/genesis"
"github.com/iotexproject/iotex-core/pkg/log"
"github.com/iotexproject/iotex-core/state"
"github.com/iotexproject/iotex-proto/golang/iotextypes"
"github.com/pkg/errors"
"go.uber.org/zap"
)

var (
Expand Down Expand Up @@ -124,19 +125,21 @@ func (cc *consortiumCommittee) CreateGenesisStates(ctx context.Context, sm proto
}
ctx = protocol.WithActionCtx(ctx, actionCtx)
ctx = protocol.WithBlockCtx(ctx, blkCtx)
ctx = evm.WithHelperCtx(ctx, evm.HelperContext{
GetBlockHash: func(height uint64) (hash.Hash256, error) {
return hash.ZeroHash256, nil
},
DepositGasFunc: func(context.Context, protocol.StateManager, address.Address, *big.Int, *big.Int) (*action.TransactionLog, error) {
return nil, nil
},
Sgd: nil,
})

// deploy consortiumCommittee contract
_, receipt, err := evm.ExecuteContract(
ctx,
sm,
execution,
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,
)
if err != nil {
return err
Expand Down Expand Up @@ -291,7 +294,11 @@ func getContractReaderForGenesisStates(ctx context.Context, sm protocol.StateMan
return nil, err
}

res, _, err := evm.SimulateExecution(ctx, sm, addr, ex, getBlockHash)
// TODO: move to the caller, then getBlockHash param can be removed
ctx = evm.WithHelperCtx(ctx, evm.HelperContext{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added a TODO

GetBlockHash: getBlockHash,
})
res, _, err := evm.SimulateExecution(ctx, sm, addr, ex)

return res, err
}
Expand Down
16 changes: 9 additions & 7 deletions action/protocol/poll/staking_committee.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,18 +135,20 @@ func (sc *stakingCommittee) CreateGenesisStates(ctx context.Context, sm protocol
}
ctx = protocol.WithActionCtx(ctx, actionCtx)
ctx = protocol.WithBlockCtx(ctx, blkCtx)
ctx = evm.WithHelperCtx(ctx, evm.HelperContext{
GetBlockHash: func(height uint64) (hash.Hash256, error) {
return hash.ZeroHash256, nil
},
DepositGasFunc: func(context.Context, protocol.StateManager, address.Address, *big.Int, *big.Int) (*action.TransactionLog, error) {
return nil, nil
},
Sgd: nil,
})
// deploy native staking contract
_, receipt, err := evm.ExecuteContract(
ctx,
sm,
execution,
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,
Comment on lines -143 to -149
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a TODO

)
if err != nil {
return err
Expand Down
17 changes: 13 additions & 4 deletions api/coreservice.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -517,7 +517,7 @@ func (core *coreService) ReadContract(ctx context.Context, callerAddr address.Ad
}
sc.SetGasPrice(big.NewInt(0)) // ReadContract() is read-only, use 0 to prevent insufficient gas

retval, receipt, err := core.sf.SimulateExecution(ctx, callerAddr, sc, core.dao.GetBlockHash)
retval, receipt, err := core.simulateExecution(ctx, callerAddr, sc, core.dao.GetBlockHash)
if err != nil {
return "", nil, status.Error(codes.Internal, err.Error())
}
Expand Down Expand Up @@ -1479,7 +1479,8 @@ func (core *coreService) isGasLimitEnough(
if err != nil {
return false, nil, err
}
_, receipt, err := core.sf.SimulateExecution(ctx, caller, sc, core.dao.GetBlockHash)

_, receipt, err := core.simulateExecution(ctx, caller, sc, core.dao.GetBlockHash)
if err != nil {
return false, nil, err
}
Expand Down Expand Up @@ -1628,7 +1629,7 @@ func (core *coreService) SimulateExecution(ctx context.Context, addr address.Add
return nil, nil, err
}
exec.SetGasLimit(core.bc.Genesis().BlockGasLimit)
return core.sf.SimulateExecution(ctx, addr, exec, core.dao.GetBlockHash)
return core.simulateExecution(ctx, addr, exec, core.dao.GetBlockHash)
}

// SyncingProgress returns the syncing status of node
Expand Down Expand Up @@ -1715,7 +1716,7 @@ func (core *coreService) TraceCall(ctx context.Context,
getblockHash := func(height uint64) (hash.Hash256, error) {
return blkHash, nil
}
retval, receipt, err := core.sf.SimulateExecution(ctx, callerAddr, exec, getblockHash)
retval, receipt, err := core.simulateExecution(ctx, callerAddr, exec, getblockHash)
return retval, receipt, traces, err
}

Expand All @@ -1731,3 +1732,11 @@ func (core *coreService) Track(ctx context.Context, start time.Time, method stri
Success: success,
}, size)
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add a TODO to add depositGas

return core.sf.SimulateExecution(ctx, addr, exec)
}
7 changes: 6 additions & 1 deletion chainservice/builder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"github.com/iotexproject/iotex-core/action/protocol/account"
accountutil "github.com/iotexproject/iotex-core/action/protocol/account/util"
"github.com/iotexproject/iotex-core/action/protocol/execution"
"github.com/iotexproject/iotex-core/action/protocol/execution/evm"
"github.com/iotexproject/iotex-core/action/protocol/poll"
"github.com/iotexproject/iotex-core/action/protocol/rewarding"
"github.com/iotexproject/iotex-core/action/protocol/rolldpos"
Expand Down Expand Up @@ -617,7 +618,11 @@ func (builder *Builder) registerRollDPoSProtocol() error {
return nil, err
}

data, _, err := factory.SimulateExecution(ctx, addr, ex, dao.GetBlockHash)
// TODO: add depositGas
ctx = evm.WithHelperCtx(ctx, evm.HelperContext{
GetBlockHash: dao.GetBlockHash,
})
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

data, _, err := factory.SimulateExecution(ctx, addr, ex)

return data, err
},
Expand Down
Loading