Add EVM helper functions for testing environment#8391
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds emulator-only EVM state read/write and a RunTxAs execution path exposed to Cadence, injects Cadence test helpers when chainID is Emulator, updates stdlib.ContractCode to accept chainID, and updates genesis/bootstrap state commitment constants and related test expectations. Changes
Sequence Diagram(s)sequenceDiagram
participant Cadence as Cadence Test
participant InternalEVM as InternalEVM Contract
participant Handler as ContractHandler
participant StateDB as Emulator StateDB
Cadence->>InternalEVM: store(target, slot, value)
InternalEVM->>Handler: SetState(address, slot, value)
Handler->>StateDB: New StateDB (emulator)
StateDB->>StateDB: Write state at address/slot
StateDB->>StateDB: Commit
StateDB-->>Handler: previous value
Handler-->>InternalEVM: return previous value
InternalEVM-->>Cadence: complete
sequenceDiagram
participant Cadence as Cadence Test
participant InternalEVM as InternalEVM Contract
participant Handler as ContractHandler
participant StateDB as Emulator StateDB
Cadence->>InternalEVM: load(target, slot)
InternalEVM->>Handler: GetState(address, slot)
Handler->>StateDB: New StateDB (emulator)
StateDB->>StateDB: Read state at address/slot
StateDB-->>Handler: value
Handler-->>InternalEVM: return bytes
InternalEVM-->>Cadence: [UInt8]
sequenceDiagram
participant Cadence as Cadence Test
participant InternalEVM as InternalEVM Contract
participant Handler as ContractHandler
participant VM as EVM Account/VM
Cadence->>InternalEVM: runTxAs(from,to,data,gas,value)
InternalEVM->>Handler: RunTxAs(from,to,txData,gasLimit,balance)
Handler->>VM: Account.Call / execute tx as `from`
VM-->>Handler: ResultSummary
Handler-->>InternalEVM: ResultSummary
InternalEVM-->>Cadence: Result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
m-Peter
force-pushed
the
mpeter/evm-testing-storage-modification
branch
2 times, most recently
from
f9493f3 to
d4e575f
Compare
m-Peter
changed the title
EVM helper function to modify contract storageEVM helper functions to inspect and modify contract storage
m-Peter
force-pushed
the
mpeter/evm-testing-storage-modification
branch
2 times, most recently
from
9fb215c to
a119eb6
Compare
m-Peter
force-pushed
the
mpeter/evm-testing-storage-modification
branch
from
a119eb6 to
a93d2d0
Compare
m-Peter
changed the title
EVM helper functions to inspect and modify contract storageEVM helper functions for testing environment
m-Peter
requested review from
RZhang05,
fxamacker,
jribbink,
turbolent and
zhangchiqing
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
fvm/evm/impl/impl.go (1)
966-968: Consider usingAddressBytesArrayValueToEVMAddressfor consistent address validation.The Load function (line 929-932) and Store function both use
interpreter.ByteArrayValueToByteSlicefollowed bytypes.NewAddressFromBytes. However, other functions in this file (e.g.,newInternalEVMTypeCodeFunctionat line 387-390) useAddressBytesArrayValueToEVMAddresswhich validates the address length is exactly 20 bytes.Using
types.NewAddressFromBytesdirectly with an unchecked byte slice could silently produce incorrect addresses if the input length is wrong, sincegethCommon.BytesToAddresspads or truncates without error.Proposed fix for Store function
func newInternalEVMTypeStoreFunction( gauge common.MemoryGauge, handler types.ContractHandler, ) *interpreter.HostFunctionValue { return interpreter.NewStaticHostFunctionValue( gauge, stdlib.InternalEVMTypeStoreFunctionType, func(invocation interpreter.Invocation) interpreter.Value { - context := invocation.InvocationContext + context := invocation.InvocationContext // Get target argument targetValue, ok := invocation.Arguments[0].(*interpreter.ArrayValue) if !ok { panic(errors.NewUnreachableError()) } - target, err := interpreter.ByteArrayValueToByteSlice(context, targetValue) + addr, err := AddressBytesArrayValueToEVMAddress(context, targetValue) if err != nil { panic(err) } // Get slot argument ... - addr := types.NewAddressFromBytes(target) - handler.SetState(addr.ToCommon(), slot, value) return interpreter.Void }, ) }Similar fix for Load function (lines 929-942)
// Get target argument targetValue, ok := invocation.Arguments[0].(*interpreter.ArrayValue) if !ok { panic(errors.NewUnreachableError()) } - target, err := interpreter.ByteArrayValueToByteSlice(context, targetValue) + addr, err := AddressBytesArrayValueToEVMAddress(context, targetValue) if err != nil { panic(err) } // Get slot argument slotValue, ok := invocation.Arguments[1].(*interpreter.StringValue) if !ok { panic(errors.NewUnreachableError()) } slot := gethCommon.HexToHash(slotValue.Str) - addr := types.NewAddressFromBytes(target) - value := handler.GetState(addr.ToCommon(), slot)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fvm/evm/impl/impl.go` around lines 966 - 968, The Load and Store code paths use interpreter.ByteArrayValueToByteSlice followed by types.NewAddressFromBytes which can silently accept wrong-length slices; replace that sequence with AddressBytesArrayValueToEVMAddress to enforce 20-byte address validation. Concretely, in the Load function (and the Store function where targetValue/target are converted) stop calling interpreter.ByteArrayValueToByteSlice + types.NewAddressFromBytes and instead call AddressBytesArrayValueToEVMAddress(context, targetValue) (or the equivalent helper used elsewhere, e.g., in newInternalEVMTypeCodeFunction) and propagate any returned error so invalid-length addresses return an error instead of being padded/truncated. Ensure you remove the old unchecked conversion and handle the error from AddressBytesArrayValueToEVMAddress consistently with surrounding error handling.fvm/evm/stdlib/contract_test.go (2)
332-337: Avoid hardcodingflow.Emulatorinside shareddeployContracts.This helper is reused widely; pinning chain ID here makes non-emulator helper-injection checks harder to write. Passing
chainIDintodeployContractskeeps behavior explicit and testable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fvm/evm/stdlib/contract_test.go` around lines 332 - 337, The deployContracts helper currently hardcodes flow.Emulator when building stdlib.ContractCode; update deployContracts to accept a chainID parameter (e.g., chainID flow.ChainID) and pass that into stdlib.ContractCode instead of flow.Emulator so the helper is chain-agnostic and testable; update all call sites of deployContracts to provide the appropriate chain id (emulator or other) and adjust the stdlib.ContractCode invocation in deployContracts to use the new chainID parameter.
147-160: Expose callback hooks forSetState/GetStateintestContractHandler.These new methods always panic, so this harness can’t exercise positive
store/loadbehavior without further structural edits. Mirror the existing callback pattern (run,dryRun, etc.) to make storage-helper tests straightforward.♻️ Suggested refactor
type testContractHandler struct { flowTokenAddress common.Address evmContractAddress common.Address deployCOA func(uint64) types.Address accountByAddress func(types.Address, bool) types.Account lastExecutedBlock func() *types.Block run func(tx []byte, coinbase types.Address) *types.ResultSummary batchRun func(txs [][]byte, coinbase types.Address) []*types.ResultSummary generateResourceUUID func() uint64 dryRun func(tx []byte, from types.Address) *types.ResultSummary dryRunWithTxData func(txData gethTypes.TxData, from types.Address) *types.ResultSummary commitBlockProposal func() + setState func(address gethCommon.Address, slot gethCommon.Hash, value gethCommon.Hash) gethCommon.Hash + getState func(address gethCommon.Address, slot gethCommon.Hash) gethCommon.Hash } func (t *testContractHandler) SetState( address gethCommon.Address, slot gethCommon.Hash, value gethCommon.Hash, ) gethCommon.Hash { - panic("unexpected SetState") + if t.setState == nil { + panic("unexpected SetState") + } + return t.setState(address, slot, value) } func (t *testContractHandler) GetState( address gethCommon.Address, slot gethCommon.Hash, ) gethCommon.Hash { - panic("unexpected GetState") + if t.getState == nil { + panic("unexpected GetState") + } + return t.getState(address, slot) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fvm/evm/stdlib/contract_test.go` around lines 147 - 160, The SetState and GetState methods on testContractHandler currently always panic; add callback hooks following the existing pattern used for run/dryRun (e.g., add fields like setStateCb and getStateCb on testContractHandler) and update SetState/GetState to invoke those callbacks when present (returning their gethCommon.Hash result) and otherwise preserve the panic behavior or return a zero value as the existing pattern dictates; reference the testContractHandler type and its methods run/dryRun to mirror how callbacks are registered and invoked so storage-helper tests can provide positive store/load behavior.fvm/evm/stdlib/contract.go (1)
561-595: Hardenstore/loadboundary types (or enforce strict validation here).
slot/valueas free-formStringbroadens malformed-input cases for data that is semantically fixed-size EVM storage hashes. Prefer fixed-size bytes at the boundary, or strictly validate hex format/length before dispatch.As per coding guidelines "Treat all inputs as potentially byzantine and classify errors in a context-dependent manner; no code path is safe unless explicitly proven and documented."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fvm/evm/stdlib/contract.go` around lines 561 - 595, The load/store boundary currently accepts slot/value as sema.StringType which allows malformed inputs; update the function signatures named InternalEVMTypeStoreFunctionType and InternalEVMTypeLoadFunctionType (and references to InternalEVMTypeLoadFunctionName) to use a fixed-size byte representation (e.g., a 32-byte/byte-array type appropriate for EVM storage keys and values) or, if changing the public signature is not possible, add strict validation at the boundary: parse and validate hex format, enforce exact byte length (32 bytes for storage slots, and the correct size for values), and return explicit errors before dispatch; ensure the chosen fix is applied consistently for both the "store" and "load" parameter handling paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@fvm/evm/evm_test.go`:
- Around line 315-410: Tests only cover the happy-path for EVM.store/EVM.load so
add negative-path unit/integration tests that exercise malformed input and
handler/commit failures: add cases in the same test harness (using
RunWithNewEnvironment) that call EVM.store/EVM.load with invalid slot/value
formats (e.g., wrong-length hex strings, non-hex chars) and assert the script
transaction fails with the expected EVM error via vm.Run output.Err and
EVM.dryRun status/errorCode; also add a test that simulates a state-commit
failure by injecting a failing snapshot or mock into the environment (targeting
the storage commit path in fvm/evm/impl/impl.go and the handler path in
fvm/evm/handler/handler.go) and assert the handler error is propagated back to
vm.Run and surfaces in the Flow transaction result.
In `@fvm/evm/handler/handler.go`:
- Around line 76-105: SetState and GetState currently swallow errors from
state.NewStateDB and execState.Commit by returning a zero gethCommon.Hash,
making failures indistinguishable from legitimate zero values; update both
methods to explicitly surface failures using the existing panicOnError pattern:
call state.NewStateDB and pass any returned err into panicOnError (or panic with
context if panicOnError is not in scope) before continuing, and after
execState.Commit call panicOnError on its err as well, so SetState and GetState
never silently return gethCommon.Hash{} on internal errors.
In `@fvm/evm/impl/impl.go`:
- Line 958: In newInternalEVMTypeStoreFunction the local variable context
assigned from invocation.InvocationContext is unused; remove the assignment
entirely or rename it to _context to satisfy the linter (i.e., delete the line
"context := invocation.InvocationContext" or change it to "_context :=
invocation.InvocationContext" within newInternalEVMTypeStoreFunction).
In `@fvm/evm/stdlib/contract.go`:
- Around line 51-58: In the chainID == flow.Emulator branch where evmContract is
modified, replace the use of loadTestHelpersPattern.ReplaceAllString with
loadTestHelpersPattern.ReplaceAllLiteralString to avoid treating replacement
text as a template, then detect if the marker was missing by comparing the
returned string to the original evmContract (or using strings.Contains on the
marker) and panic with a clear message if the marker wasn't found; update the
code references evmContract, loadTestHelpersPattern, and contractTestHelpers
accordingly so the helper injection fails fast on a missing marker.
---
Nitpick comments:
In `@fvm/evm/impl/impl.go`:
- Around line 966-968: The Load and Store code paths use
interpreter.ByteArrayValueToByteSlice followed by types.NewAddressFromBytes
which can silently accept wrong-length slices; replace that sequence with
AddressBytesArrayValueToEVMAddress to enforce 20-byte address validation.
Concretely, in the Load function (and the Store function where
targetValue/target are converted) stop calling
interpreter.ByteArrayValueToByteSlice + types.NewAddressFromBytes and instead
call AddressBytesArrayValueToEVMAddress(context, targetValue) (or the equivalent
helper used elsewhere, e.g., in newInternalEVMTypeCodeFunction) and propagate
any returned error so invalid-length addresses return an error instead of being
padded/truncated. Ensure you remove the old unchecked conversion and handle the
error from AddressBytesArrayValueToEVMAddress consistently with surrounding
error handling.
In `@fvm/evm/stdlib/contract_test.go`:
- Around line 332-337: The deployContracts helper currently hardcodes
flow.Emulator when building stdlib.ContractCode; update deployContracts to
accept a chainID parameter (e.g., chainID flow.ChainID) and pass that into
stdlib.ContractCode instead of flow.Emulator so the helper is chain-agnostic and
testable; update all call sites of deployContracts to provide the appropriate
chain id (emulator or other) and adjust the stdlib.ContractCode invocation in
deployContracts to use the new chainID parameter.
- Around line 147-160: The SetState and GetState methods on testContractHandler
currently always panic; add callback hooks following the existing pattern used
for run/dryRun (e.g., add fields like setStateCb and getStateCb on
testContractHandler) and update SetState/GetState to invoke those callbacks when
present (returning their gethCommon.Hash result) and otherwise preserve the
panic behavior or return a zero value as the existing pattern dictates;
reference the testContractHandler type and its methods run/dryRun to mirror how
callbacks are registered and invoked so storage-helper tests can provide
positive store/load behavior.
In `@fvm/evm/stdlib/contract.go`:
- Around line 561-595: The load/store boundary currently accepts slot/value as
sema.StringType which allows malformed inputs; update the function signatures
named InternalEVMTypeStoreFunctionType and InternalEVMTypeLoadFunctionType (and
references to InternalEVMTypeLoadFunctionName) to use a fixed-size byte
representation (e.g., a 32-byte/byte-array type appropriate for EVM storage keys
and values) or, if changing the public signature is not possible, add strict
validation at the boundary: parse and validate hex format, enforce exact byte
length (32 bytes for storage slots, and the correct size for values), and return
explicit errors before dispatch; ensure the chosen fix is applied consistently
for both the "store" and "load" parameter handling paths.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
engine/execution/state/bootstrap/bootstrap_test.gofvm/bootstrap.gofvm/evm/evm_test.gofvm/evm/handler/handler.gofvm/evm/impl/impl.gofvm/evm/stdlib/contract.cdcfvm/evm/stdlib/contract.gofvm/evm/stdlib/contract_test.gofvm/evm/stdlib/contract_test_helpers.cdcfvm/evm/stdlib/type.gofvm/evm/types/handler.goutils/unittest/execution_state.go
| SetState(gethCommon.Address, gethCommon.Hash, gethCommon.Hash) gethCommon.Hash | ||
| GetState(gethCommon.Address, gethCommon.Hash) gethCommon.Hash |
There was a problem hiding this comment.
| SetState(gethCommon.Address, gethCommon.Hash, gethCommon.Hash) gethCommon.Hash | |
| GetState(gethCommon.Address, gethCommon.Hash) gethCommon.Hash | |
| // Testing only methods | |
| SetState(gethCommon.Address, gethCommon.Hash, gethCommon.Hash) gethCommon.Hash | |
| GetState(gethCommon.Address, gethCommon.Hash) gethCommon.Hash |
There was a problem hiding this comment.
Since these methods are testing (emulator) only, I'm afraid it's accidentally enabled by MN or TN, since nothing prevents that.
I wonder if make sense to add a check like if network != flow.Emulator { panic() } to be more defensive.
Or we wrap the handler with a EmulatorHandler, and add the two methods there.
Thoughts?
There was a problem hiding this comment.
Valid points indeed 👍 I will try your 2 suggestions, to see which one is more plausible.
There was a problem hiding this comment.
I wonder if make sense to add a check like if network != flow.Emulator { panic() } to be more defensive.
Updated in 1c88b01, as per your above suggestion.
| self.setupHeartbeat() | ||
| } | ||
|
|
||
| // Placeholder to load test helpers available only on Flow Emulator chain |
There was a problem hiding this comment.
Maybe we can just always have the methods here in the EVM contract, but they check the current chain or some FVM configuration option when invoked? This might be also extra protection in general, see Leo's comment below
Not sure if only allowing the Emulator chain is sufficient / too restrictive? Will this also work when running the Emulator in forked mode?
There was a problem hiding this comment.
Maybe we can just always have the methods here in the EVM contract, but they check the current chain or some FVM configuration option when invoked?
Although that would simplify things a lot, the main issue is in optics. Having those methods available on TN/MN, it means that anyone can call those from a tx, trying to exploit those. They should fail/panic for unsupported networks, but it can still generate unnecessary tx error noise.
Not sure if only allowing the Emulator chain is sufficient / too restrictive? Will this also work when running the Emulator in forked mode?
I believe that @jribbink and @RZhang05 are already using these test-only EVM APIs in combination with the forked mode. However, I think they do indeed have to re-deploy the EVM contract with the new functions. Maybe we can special-case the EVM contract for the forked mode, and always update it to the version deployed in Emulator?
There was a problem hiding this comment.
Yeah, we're currently deplyoing a contract update.
We can probably add any setup steps (i.e. contract deployment/update) as part of the emulator startup for fork mode. The emulator already commits an initial block on top of the forked state anyways.
Also - regarding tooling integration, if these APIs are not part of the mainnet EVM contract, they will show as errors in our local environment. @turbolent's suggestion of always including these methods in the API would address this, however I understand the optics concern.
I guess an alternative would be to separate it into another, like EVMUtils, that we deploy to mainnet (with noop/panics). This way there would be no issues with static type checking & installing as a dependency in the Flow CLI, but it would keep these functions outside of the core EVM contract. Although not sure how much complexity this adds on your side.
There was a problem hiding this comment.
If the current solution works, please feel free to go ahead. I just wanted to make sure we would be able to use it as expected, i.e. use the testing framework, incl. forking mode
There was a problem hiding this comment.
🧹 Nitpick comments (1)
fvm/evm/handler/handler.go (1)
76-109: Please add targeted tests for these new emulator-only state paths.
SetState/GetStatenow carry critical guard + persistence behavior, but this changed area is currently untested in patch coverage. Add tests for: (1) non-emulator rejection, (2) SetState round-trip via GetState, and (3) commit error propagation path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fvm/evm/handler/handler.go` around lines 76 - 109, Add unit tests targeting ContractHandler.SetState and GetState: (1) non-emulator rejection — create a ContractHandler with flowChainID != flow.Emulator and assert the call to SetState and GetState returns/causes the ErrUnsupportedNetworkOperation path (via panicOnError behavior); (2) SetState/GetState round-trip — use a ContractHandler with flowChainID == flow.Emulator and a real or in-memory backend so that calling SetState(address, slot, value) followed by GetState(address, slot) returns the previously set value; (3) commit error propagation — inject or mock state.NewStateDB (or the returned state DB) so that Commit(true) returns an error and assert SetState surfaces that error (matches panicOnError behavior). Reference ContractHandler.SetState, ContractHandler.GetState, flow.Emulator, state.NewStateDB, evm.StorageAccountAddress and panicOnError when locating code to exercise and when arranging mocks or test backends.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@fvm/evm/handler/handler.go`:
- Around line 76-109: Add unit tests targeting ContractHandler.SetState and
GetState: (1) non-emulator rejection — create a ContractHandler with flowChainID
!= flow.Emulator and assert the call to SetState and GetState returns/causes the
ErrUnsupportedNetworkOperation path (via panicOnError behavior); (2)
SetState/GetState round-trip — use a ContractHandler with flowChainID ==
flow.Emulator and a real or in-memory backend so that calling SetState(address,
slot, value) followed by GetState(address, slot) returns the previously set
value; (3) commit error propagation — inject or mock state.NewStateDB (or the
returned state DB) so that Commit(true) returns an error and assert SetState
surfaces that error (matches panicOnError behavior). Reference
ContractHandler.SetState, ContractHandler.GetState, flow.Emulator,
state.NewStateDB, evm.StorageAccountAddress and panicOnError when locating code
to exercise and when arranging mocks or test backends.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
fvm/evm/stdlib/contract.go (2)
587-599: Consider addingPurity: sema.FunctionPurityViewto the load function.Other read-only EVM functions like
balance,nonce,code, andcodeHash(lines 411-465) specifyPurity: sema.FunctionPurityView. Sinceloadonly reads from EVM storage without modifying state, it may benefit from the same annotation for consistency and semantic correctness.♻️ Optional: Add purity annotation
var InternalEVMTypeLoadFunctionType = &sema.FunctionType{ + Purity: sema.FunctionPurityView, Parameters: []sema.Parameter{ { Label: "target",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fvm/evm/stdlib/contract.go` around lines 587 - 599, The function type definition InternalEVMTypeLoadFunctionType is missing a purity annotation—add Purity: sema.FunctionPurityView to the sema.FunctionType literal so the load function is marked read-only like balance/nonce/code/codeHash; locate InternalEVMTypeLoadFunctionType and insert Purity: sema.FunctionPurityView into its sema.FunctionType fields.
738-749: Handler-level validation forstoreandloadis not enforced, relying solely on Cadence-level access control.While the
storeandloadfunctions are registered unconditionally inInternalEVMContractTypeon all chains, they are protected by Cadence—the test helpers that expose these functions are only injected on the Emulator chain (if chainID == flow.Emulator). This single-layer access control works, but adding handler-level chain ID validation would provide defense-in-depth hardening consistent with the project's principle of treating inputs as potentially byzantine.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fvm/evm/stdlib/contract.go` around lines 738 - 749, Add handler-level chain ID validation for the store/load handlers so they don't rely solely on Cadence access control: inside the handler implementations registered on InternalEVMContractType for InternalEVMTypeStoreFunctionName and InternalEVMTypeLoadFunctionName, fetch the executing chain ID (e.g., via the runtime/execution context or runtimeInterface.ChainID()) and return an authorization/error if it is not the Emulator chain ID (or the permitted chain constant used elsewhere). Ensure the check runs before performing any store/load logic and returns a clear unauthorized error to harden against byzantine callers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@fvm/evm/stdlib/contract.go`:
- Around line 587-599: The function type definition
InternalEVMTypeLoadFunctionType is missing a purity annotation—add Purity:
sema.FunctionPurityView to the sema.FunctionType literal so the load function is
marked read-only like balance/nonce/code/codeHash; locate
InternalEVMTypeLoadFunctionType and insert Purity: sema.FunctionPurityView into
its sema.FunctionType fields.
- Around line 738-749: Add handler-level chain ID validation for the store/load
handlers so they don't rely solely on Cadence access control: inside the handler
implementations registered on InternalEVMContractType for
InternalEVMTypeStoreFunctionName and InternalEVMTypeLoadFunctionName, fetch the
executing chain ID (e.g., via the runtime/execution context or
runtimeInterface.ChainID()) and return an authorization/error if it is not the
Emulator chain ID (or the permitted chain constant used elsewhere). Ensure the
check runs before performing any store/load logic and returns a clear
unauthorized error to harden against byzantine callers.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
fvm/evm/stdlib/contract_test.go (1)
147-170: Optional: make new test-handler hooks injectable like existing ones.
SetState,GetState, andRunTxAscurrently always panic. Consider adding function fields (similar torun,dryRun, etc.) to enable direct tests against these new host-function paths in this harness.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fvm/evm/stdlib/contract_test.go` around lines 147 - 170, The testContractHandler currently panics in SetState, GetState, and RunTxAs, preventing tests from injecting behavior; add function fields to the testContractHandler struct (e.g., setState func(gethCommon.Address, gethCommon.Hash, gethCommon.Hash) gethCommon.Hash, getState func(gethCommon.Address, gethCommon.Hash) gethCommon.Hash, runTxAs func(types.Address, types.Address, types.Data, types.GasLimit, types.Balance) *types.ResultSummary) similar to existing run/dryRun fields, then implement SetState/GetState/RunTxAs to call the corresponding function field when non-nil and otherwise fall back to the current panic (or a safe default) so tests can inject behavior for these host-function paths.fvm/evm/handler/handler.go (1)
76-125: Add focused tests for the new emulator-only helper paths.These methods are a meaningful behavior surface (
SetState,GetState,RunTxAs), but current PR coverage indicates they’re lightly covered. Please add targeted tests for emulator gating and success paths (including persisted storage behavior).If helpful, I can draft a compact table-driven test skeleton for these three methods.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fvm/evm/handler/handler.go` around lines 76 - 125, Add unit tests that exercise ContractHandler.SetState, GetState, and RunTxAs for both emulator-gating (non-emulator should return ErrUnsupportedNetworkOperation or panic as currently enforced) and success paths on the Emulator: create a ContractHandler with flow.Emulator backend, call SetState(address, slot, value), assert returned prevValue and that storage is persisted by creating a new state.NewStateDB (evm.StorageAccountAddress flow.Emulator) and verifying GetState returns the new value, and test RunTxAs by constructing an account via AccountByAddress and invoking Call(to, txData, gasLimit, balance) to assert expected ResultSummary; also add negative tests where ContractHandler.flowChainID != flow.Emulator to confirm the panic/unsupported error for each of the three methods.fvm/evm/types/handler.go (1)
68-70: Document these new interface methods as emulator/testing-only.Please add short comments for
SetState,GetState, andRunTxAsto make their intended scope explicit and prevent accidental misuse by non-emulator implementations.As per coding guidelines: "Follow Go coding standards and conventions as documented in
@docs/agents/GoDocs.md".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fvm/evm/types/handler.go` around lines 68 - 70, Add Go-style doc comments above the three interface methods to mark them as emulator/testing-only: write a short sentence starting with the method name for each — "SetState sets a storage/state value for the emulator (testing-only)", "GetState retrieves a storage/state value from the emulator (testing-only)", and "RunTxAs runs a transaction as a specified sender in the emulator (testing-only)". Place these comments directly above the SetState, GetState, and RunTxAs declarations in handler.go, following Go comment conventions (start with the symbol name) so the intent and scope are explicit and discourages non-emulator implementations from using them.fvm/evm/stdlib/contract.go (1)
618-630: MarkInternalEVM.loadas view-pure.
loadis read-only; adding view purity keeps semantics explicit and allows cleaner usage from view-only call sites.Suggested refactor
var InternalEVMTypeLoadFunctionType = &sema.FunctionType{ + Purity: sema.FunctionPurityView, Parameters: []sema.Parameter{ { Label: "target", TypeAnnotation: sema.NewTypeAnnotation(EVMAddressBytesType), },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fvm/evm/stdlib/contract.go` around lines 618 - 630, The InternalEVMTypeLoadFunctionType function type is missing a view-purity annotation; update the sema.FunctionType literal for InternalEVMTypeLoadFunctionType to mark the function as read-only/view (e.g. set the appropriate purity flag on sema.FunctionType such as IsReadOnly = true or Purity = sema.FunctionPurityView according to the sema API used in this codebase) so that InternalEVM.load is explicitly view-pure; modify the FunctionType construction (InternalEVMTypeLoadFunctionType) to include that purity field alongside Parameters and ReturnTypeAnnotation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@fvm/evm/impl/impl.go`:
- Around line 461-494: Add focused table-driven tests in package fvm/evm/impl
that exercise the new RunTxAs/Load/Store host-function paths (covering
newInternalEVMTypeRunTxAsFunction and the corresponding Load/Store host
functions around lines 949-1030). For each case include: malformed slot/value
input (assert the host function returns an error or causes the expected panic
from parse/validation), malformed target bytes (assert the call fails with a
clear error/result), and a successful store→load round-trip (call the Store
host-function with valid slot/value, then Load and assert the retrieved value
equals what was stored). Use the interpreter invocation helpers
(parseCallArguments, handler.RunTxAs, NewResultValue) to construct invocations,
organize as table-driven tests with explicit assertions per case, and include
negative assertions for panics/errors as well as success assertions for the
round-trip.
- Line 467: The RunTxAs host binding is registered with the wrong type constant:
replace the use of InternalEVMTypeCallFunctionType with
InternalEVMTypeRunTxAsFunctionType where newInternalEVMTypeRunTxAsFunction is
bound so the binding uses the semantically correct type; update the registration
entry that references newInternalEVMTypeRunTxAsFunction to use
InternalEVMTypeRunTxAsFunctionType to avoid fragile signature coupling.
- Around line 965-981: newInternalEVMTypeLoadFunction and
newInternalEVMTypeStoreFunction currently call geth's HexToHash and
NewAddressFromBytes without validating inputs; update both functions to reject
malformed inputs before state access by (1) checking the slot string with
common.IsHexHash(slotValue.Str) and returning/raising an appropriate interpreter
error if false, and only then calling gethCommon.HexToHash; and (2) validating
the target byte slice length is exactly 20 bytes before calling
types.NewAddressFromBytes (return/raise an error if not), so no silent
truncation/padding occurs when creating addresses or hashes.
In `@fvm/evm/stdlib/contract_test_helpers.cdc`:
- Around line 5-8: The precondition currently on EVM.store checking only
slot.length and value.length must be tightened to also validate hex content and
optional "0x" prefix; update the checks for slot and value to accept either
exactly 64 hex chars or "0x" followed by 64 hex chars (and likewise for
66-length inputs) by adding a hex-shape test (e.g., regex-style check for
/^[0-9a-fA-F]{64}$/ or /^0x[0-9a-fA-F]{64}$/) before emitting the error message
so non-hex strings fail the precondition; apply the same change to the other
checks referenced for lines 17-18.
---
Nitpick comments:
In `@fvm/evm/handler/handler.go`:
- Around line 76-125: Add unit tests that exercise ContractHandler.SetState,
GetState, and RunTxAs for both emulator-gating (non-emulator should return
ErrUnsupportedNetworkOperation or panic as currently enforced) and success paths
on the Emulator: create a ContractHandler with flow.Emulator backend, call
SetState(address, slot, value), assert returned prevValue and that storage is
persisted by creating a new state.NewStateDB (evm.StorageAccountAddress
flow.Emulator) and verifying GetState returns the new value, and test RunTxAs by
constructing an account via AccountByAddress and invoking Call(to, txData,
gasLimit, balance) to assert expected ResultSummary; also add negative tests
where ContractHandler.flowChainID != flow.Emulator to confirm the
panic/unsupported error for each of the three methods.
In `@fvm/evm/stdlib/contract_test.go`:
- Around line 147-170: The testContractHandler currently panics in SetState,
GetState, and RunTxAs, preventing tests from injecting behavior; add function
fields to the testContractHandler struct (e.g., setState
func(gethCommon.Address, gethCommon.Hash, gethCommon.Hash) gethCommon.Hash,
getState func(gethCommon.Address, gethCommon.Hash) gethCommon.Hash, runTxAs
func(types.Address, types.Address, types.Data, types.GasLimit, types.Balance)
*types.ResultSummary) similar to existing run/dryRun fields, then implement
SetState/GetState/RunTxAs to call the corresponding function field when non-nil
and otherwise fall back to the current panic (or a safe default) so tests can
inject behavior for these host-function paths.
In `@fvm/evm/stdlib/contract.go`:
- Around line 618-630: The InternalEVMTypeLoadFunctionType function type is
missing a view-purity annotation; update the sema.FunctionType literal for
InternalEVMTypeLoadFunctionType to mark the function as read-only/view (e.g. set
the appropriate purity flag on sema.FunctionType such as IsReadOnly = true or
Purity = sema.FunctionPurityView according to the sema API used in this
codebase) so that InternalEVM.load is explicitly view-pure; modify the
FunctionType construction (InternalEVMTypeLoadFunctionType) to include that
purity field alongside Parameters and ReturnTypeAnnotation.
In `@fvm/evm/types/handler.go`:
- Around line 68-70: Add Go-style doc comments above the three interface methods
to mark them as emulator/testing-only: write a short sentence starting with the
method name for each — "SetState sets a storage/state value for the emulator
(testing-only)", "GetState retrieves a storage/state value from the emulator
(testing-only)", and "RunTxAs runs a transaction as a specified sender in the
emulator (testing-only)". Place these comments directly above the SetState,
GetState, and RunTxAs declarations in handler.go, following Go comment
conventions (start with the symbol name) so the intent and scope are explicit
and discourages non-emulator implementations from using them.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
fvm/evm/handler/handler.gofvm/evm/impl/impl.gofvm/evm/stdlib/contract.gofvm/evm/stdlib/contract_test.gofvm/evm/stdlib/contract_test_helpers.cdcfvm/evm/types/handler.goutils/unittest/execution_state.go
m-Peter
force-pushed
the
mpeter/evm-testing-storage-modification
branch
from
96fb0ca to
c5fa0d1
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (3)
fvm/evm/stdlib/contract_test_helpers.cdc (1)
5-8:⚠️ Potential issue | 🟡 MinorHex validation is still length-only in
store/loadpreconditions.Please also validate hex content (and optional
0xprefix shape), not just length, so malformed inputs fail at the contract boundary.As per coding guidelines "Treat all inputs as potentially byzantine and classify errors in a context-dependent manner; no code path is safe unless explicitly proven and documented."
Also applies to: 17-18
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fvm/evm/stdlib/contract_test_helpers.cdc` around lines 5 - 8, The preconditions in EVM.store() (and corresponding EVM.load()) currently only check hex string length for slot and value; add full shape validation to ensure strings are either exactly 64 hex chars or '0x' followed by 64 hex chars (i.e. match ^[0-9a-fA-F]{64}$ or ^0x[0-9a-fA-F]{64}$) and reject any other characters or malformed 0x prefixes, updating the error messages to mention invalid hex content as well as length and applying the same validation logic for both slot and value checks referenced in this file.fvm/evm/evm_test.go (1)
216-410:⚠️ Potential issue | 🟠 MajorAdd failure-path coverage for the new helper APIs (
runTxAs,store,load).These additions are still happy-path focused, while the PR’s uncovered lines are in the new handler/impl paths. Please add negative tests for malformed hex input, unsupported-network behavior, and error propagation from the internal handler path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fvm/evm/evm_test.go` around lines 216 - 410, Add negative tests alongside the existing EVM.runTxAs, EVM.store, and EVM.load tests that assert proper error propagation: (1) malformed-hex inputs — send invalid hex strings to EVM.addressFromString (for runTxAs) and invalid slot/value hex to EVM.store/EVM.load and assert the transaction fails and returns a non-zero errorCode via the tx event (use the same tx construction flow and inspect output.Events / types.DirectCallFromEncoded); (2) unsupported-network behavior — invoke the APIs with addresses/coinbase bytes that do not match the chain/network (e.g., wrong-length coinbase or an address with a mismatched chain id) and assert vm.Run returns an error or the event contains the expected unsupported-network error code; (3) internal handler error propagation — craft an inner tx or scenario that triggers the EVM handler to return an error (for example a bad calldata/unknown function for the target contract) and assert vm.Run/output.Err and the emitted event/errorCode reflect that handler failure (use existing helpers like getEVMAccountBalance, callEVMHeartBeat, and inspect output.Events to verify propagation).fvm/evm/impl/impl.go (1)
949-984:⚠️ Potential issue | 🟠 MajorUse
AddressBytesArrayValueToEVMAddressfor consistent address validation.The
newInternalEVMTypeLoadFunctionandnewInternalEVMTypeStoreFunctionuseinterpreter.ByteArrayValueToByteSlicefollowed bytypes.NewAddressFromBytes, which does not validate the address length. Other functions in this file (e.g.,newInternalEVMTypeCodeFunctionat line 388,newInternalEVMTypeNonceFunctionat line 417) useAddressBytesArrayValueToEVMAddresswhich enforces the 20-byte length requirement.Additionally,
gethCommon.HexToHashsilently pads short inputs and truncates long ones, which could lead to unexpected storage slot access.Suggested fix for address validation
func newInternalEVMTypeLoadFunction( gauge common.MemoryGauge, handler types.ContractHandler, ) *interpreter.HostFunctionValue { return interpreter.NewStaticHostFunctionValue( gauge, stdlib.InternalEVMTypeLoadFunctionType, func(invocation interpreter.Invocation) interpreter.Value { context := invocation.InvocationContext // Get target argument targetValue, ok := invocation.Arguments[0].(*interpreter.ArrayValue) if !ok { panic(errors.NewUnreachableError()) } - target, err := interpreter.ByteArrayValueToByteSlice(context, targetValue) + addr, err := AddressBytesArrayValueToEVMAddress(context, targetValue) if err != nil { panic(err) } // Get slot argument slotValue, ok := invocation.Arguments[1].(*interpreter.StringValue) if !ok { panic(errors.NewUnreachableError()) } slot := gethCommon.HexToHash(slotValue.Str) - addr := types.NewAddressFromBytes(target) - value := handler.GetState(addr, slot) return interpreter.ByteSliceToByteArrayValue(context, value.Bytes()) }, ) }Apply the same pattern to
newInternalEVMTypeStoreFunction.Also applies to: 986-1030
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fvm/evm/impl/impl.go` around lines 949 - 984, Replace the current address parsing in newInternalEVMTypeLoadFunction (and likewise in newInternalEVMTypeStoreFunction) that uses interpreter.ByteArrayValueToByteSlice + types.NewAddressFromBytes with interpreter.AddressBytesArrayValueToEVMAddress to enforce a 20-byte address; locate and remove the ByteArrayValueToByteSlice / types.NewAddressFromBytes sequence and call AddressBytesArrayValueToEVMAddress(context, targetValue) to get the validated address. Also avoid relying on gethCommon.HexToHash for slot parsing without validation—ensure the slot string is decoded/validated to a 32-byte hash before calling handler.GetState so inputs aren’t silently padded/truncated (replace or validate the result of gethCommon.HexToHash accordingly).
🧹 Nitpick comments (2)
fvm/evm/types/handler.go (1)
68-75: Document emulator-only scope on the new interface methods.Please annotate
SetState,GetState, andRunTxAsas emulator-only in the interface comments to make misuse on non-emulator chains less likely for future callers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fvm/evm/types/handler.go` around lines 68 - 75, The interface methods SetState, GetState and RunTxAs are emulator-only and need explicit documentation: update the comment lines above SetState, GetState and RunTxAs in the handler interface to state they are emulator-only (e.g., "Emulator-only: ...") and briefly describe expected behavior/constraints (that they should not be used on non-emulator chains and what semantics they provide like changing storage or simulating a call with a given msg.sender). Ensure each method comment includes the method name (SetState, GetState, RunTxAs) so future callers see the emulator-only restriction.utils/unittest/execution_state.go (1)
85-96: Make the fallback chain mapping explicit.The default return currently acts as an implicit emulator path; adding an explicit
flow.Emulatorcase (and documenting/handling any remaining default) would make accidental chain-ID drift easier to catch in tests.As per coding guidelines "Treat all inputs as potentially byzantine and classify errors in a context-dependent manner; no code path is safe unless explicitly proven and documented."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@utils/unittest/execution_state.go` around lines 85 - 96, The function genesisCommitHexByChainID currently returns a default value for any unknown chainID which hides accidental chain-ID drift; update genesisCommitHexByChainID to include an explicit branch for flow.Emulator returning the intended emulator commitment hex (instead of relying on the implicit final return), and change the remaining default to an explicit fallback that either returns an error/empty value or panics/logs with the unknown chainID to make unexpected inputs obvious in tests; reference the function name genesisCommitHexByChainID and the flow.Emulator constant when making these changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@fvm/evm/evm_test.go`:
- Around line 216-410: Add negative tests alongside the existing EVM.runTxAs,
EVM.store, and EVM.load tests that assert proper error propagation: (1)
malformed-hex inputs — send invalid hex strings to EVM.addressFromString (for
runTxAs) and invalid slot/value hex to EVM.store/EVM.load and assert the
transaction fails and returns a non-zero errorCode via the tx event (use the
same tx construction flow and inspect output.Events /
types.DirectCallFromEncoded); (2) unsupported-network behavior — invoke the APIs
with addresses/coinbase bytes that do not match the chain/network (e.g.,
wrong-length coinbase or an address with a mismatched chain id) and assert
vm.Run returns an error or the event contains the expected unsupported-network
error code; (3) internal handler error propagation — craft an inner tx or
scenario that triggers the EVM handler to return an error (for example a bad
calldata/unknown function for the target contract) and assert vm.Run/output.Err
and the emitted event/errorCode reflect that handler failure (use existing
helpers like getEVMAccountBalance, callEVMHeartBeat, and inspect output.Events
to verify propagation).
In `@fvm/evm/impl/impl.go`:
- Around line 949-984: Replace the current address parsing in
newInternalEVMTypeLoadFunction (and likewise in newInternalEVMTypeStoreFunction)
that uses interpreter.ByteArrayValueToByteSlice + types.NewAddressFromBytes with
interpreter.AddressBytesArrayValueToEVMAddress to enforce a 20-byte address;
locate and remove the ByteArrayValueToByteSlice / types.NewAddressFromBytes
sequence and call AddressBytesArrayValueToEVMAddress(context, targetValue) to
get the validated address. Also avoid relying on gethCommon.HexToHash for slot
parsing without validation—ensure the slot string is decoded/validated to a
32-byte hash before calling handler.GetState so inputs aren’t silently
padded/truncated (replace or validate the result of gethCommon.HexToHash
accordingly).
In `@fvm/evm/stdlib/contract_test_helpers.cdc`:
- Around line 5-8: The preconditions in EVM.store() (and corresponding
EVM.load()) currently only check hex string length for slot and value; add full
shape validation to ensure strings are either exactly 64 hex chars or '0x'
followed by 64 hex chars (i.e. match ^[0-9a-fA-F]{64}$ or ^0x[0-9a-fA-F]{64}$)
and reject any other characters or malformed 0x prefixes, updating the error
messages to mention invalid hex content as well as length and applying the same
validation logic for both slot and value checks referenced in this file.
---
Nitpick comments:
In `@fvm/evm/types/handler.go`:
- Around line 68-75: The interface methods SetState, GetState and RunTxAs are
emulator-only and need explicit documentation: update the comment lines above
SetState, GetState and RunTxAs in the handler interface to state they are
emulator-only (e.g., "Emulator-only: ...") and briefly describe expected
behavior/constraints (that they should not be used on non-emulator chains and
what semantics they provide like changing storage or simulating a call with a
given msg.sender). Ensure each method comment includes the method name
(SetState, GetState, RunTxAs) so future callers see the emulator-only
restriction.
In `@utils/unittest/execution_state.go`:
- Around line 85-96: The function genesisCommitHexByChainID currently returns a
default value for any unknown chainID which hides accidental chain-ID drift;
update genesisCommitHexByChainID to include an explicit branch for flow.Emulator
returning the intended emulator commitment hex (instead of relying on the
implicit final return), and change the remaining default to an explicit fallback
that either returns an error/empty value or panics/logs with the unknown chainID
to make unexpected inputs obvious in tests; reference the function name
genesisCommitHexByChainID and the flow.Emulator constant when making these
changes.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
engine/execution/state/bootstrap/bootstrap_test.gofvm/bootstrap.gofvm/evm/evm_test.gofvm/evm/handler/handler.gofvm/evm/impl/impl.gofvm/evm/stdlib/contract.cdcfvm/evm/stdlib/contract.gofvm/evm/stdlib/contract_test.gofvm/evm/stdlib/contract_test_helpers.cdcfvm/evm/stdlib/type.gofvm/evm/types/errors.gofvm/evm/types/handler.goutils/unittest/execution_state.go
🚧 Files skipped from review as they are similar to previous changes (3)
- fvm/evm/stdlib/contract_test.go
- fvm/evm/stdlib/contract.cdc
- engine/execution/state/bootstrap/bootstrap_test.go
fxamacker
left a comment
fxamacker
left a comment
There was a problem hiding this comment.
Nice! 👍 I only left suggestions to check for hex decoding problem because gethCommon.HexToHash() doesn't return error.
| panic(errors.NewUnreachableError()) | ||
| } | ||
|
|
||
| slot := gethCommon.HexToHash(slotValue.Str) |
There was a problem hiding this comment.
Maybe check if the returned slot is an empty [32]byte (all elements are zero values), because gethCommon.HexToHash() is silently ignoring hex decoding error.
There was a problem hiding this comment.
Good point 👍 Added some validation helper methods in 182957a .
| panic(errors.NewUnreachableError()) | ||
| } | ||
|
|
||
| slot := gethCommon.HexToHash(slotValue.Str) |
There was a problem hiding this comment.
See comment above. Maybe check if hash decoding is successful.
There was a problem hiding this comment.
Good point 👍 Added some validation helper methods in 182957a .
| panic(errors.NewUnreachableError()) | ||
| } | ||
|
|
||
| value := gethCommon.HexToHash(valueValue.Str) |
There was a problem hiding this comment.
See comment above. Maybe check if hash decoding is successful.
There was a problem hiding this comment.
Good point 👍 Added some validation helper methods in 182957a .
|
can we merge this now? |
6 participants
Adds 3 new
EVMfunctions for usage within the Cadence Testing Framework:EVM.store(target: EVM.EVMAddress, slot: String, value: String)EVM.load(target: EVM.EVMAddress, slot: String): [UInt8]EVM.runTxAs(from: EVM.EVMAddress, to: EVM.EVMAddress, data: [UInt8], gasLimit: UInt64, value: EVM.Balance)These new functions facilitate the testing of Solidity contracts, from withing the Cadence testing framework, and are only available on the Flow Emulator chain.
Summary by CodeRabbit
New Features
Tests
Bug Fixes