[Flow EVM] Add new EVM functions that can be used to reduce computation cost of transactions#8418
Open
[Flow EVM] Add new EVM functions that can be used to reduce computation cost of transactions#8418
Conversation
fxamacker
requested review from
a team,
janezpodhostnik,
m-Peter,
turbolent and
zhangchiqing
Contributor
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Contributor
|
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 ABI signature+args call/dry-call flows with keccak256 hashing wiring, refactors ABI encode/decode into helpers, introduces a ResultDecoded type and parsing, and adds extensive tests covering gas, non-transactional behavior, decoding and error paths. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant InternalEVM
participant ABIEncoder
participant KeccakHash as Hash(Function)
participant EVMExecutor
participant ResultDecoder
Caller->>InternalEVM: callWithSigAndArgs(to, signature, args, gasLimit, value, resultTypes)
InternalEVM->>ABIEncoder: encodeABIWithSigAndArgs(signature, args)
ABIEncoder->>KeccakHash: hash(signature)
KeccakHash-->>ABIEncoder: signatureHash
ABIEncoder->>InternalEVM: encodedData
InternalEVM->>EVMExecutor: execute call (to, data, gasLimit, value)
EVMExecutor-->>InternalEVM: executionResult (status, gasUsed, data, errorCode, errorMsg)
InternalEVM->>ResultDecoder: decodeResultData(resultTypes, data)
ResultDecoder-->>Caller: ResultDecoded(status, errorCode, errorMessage, gasUsed, results?, deployedContract?)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
fxamacker
force-pushed
the
fxamacker/add-evm-wrapper-func
branch
from
89792d3 to
ed6ef7e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 545-548: The host function
newInternalEVMTypeDryCallWithArgsFunction is declared for dryCallWithSigAndArgs
but is constructed with the wrong type constant
(InternalEVMTypeDryCallFunctionType) causing a signature mismatch with its
implementation that calls parseCallArgumentsWithSigAndArgs; update the
constructor in newInternalEVMTypeDryCallWithArgsFunction to use
InternalEVMTypeDryCallWithArgsFunctionType so the declared host function type
matches the expected (signature, args) parameters and avoids runtime type
errors.
In `@fvm/evm/stdlib/contract_test.go`:
- Around line 4442-4882: The tests in TestEVMDryCallWithArgs embed hard-coded
EVM addresses and ABI-encoded payloads (see expectedData byte arrays, the
literal EVM address byte slices in script strings, and ABI packing in handlers
inside each t.Run block); replace those literals with shared fixtures from the
utils/unittest package (import the fixtures and use their exported EVM addresses
and encoded-payload helpers) so both the script strings and the
gethABI.Pack/encodedValues use the same centralized test data; update references
inside TestEVMDryCallWithArgs, the handler.dryRunWithTxData closures, and
expectedData variables to use the fixture constants/functions and add the
necessary import for utils/unittest.
In `@fvm/evm/testutils/result.go`:
- Around line 84-101: Validate the deployed contract address array before
converting: check deployedAddressField.Value is cadence.Struct (already done),
then ensure cadence.SearchFieldByName(evmAddress,
stdlib.EVMAddressTypeBytesFieldName) yields a cadence.Array and verify its
length equals types.AddressLength (20) before allocating convertedAddress; when
iterating bytes.Values, use safe type assertions (ok pattern) for cadence.UInt8
and return an error if any element is not cadence.UInt8 instead of panicking;
finally construct types.Address only after all validations pass and assign to
convertedDeployedAddress.
---
Duplicate comments:
In `@fvm/evm/stdlib/contract_test.go`:
- Around line 5275-5721: The test TestCadenceOwnedAccountCallWithArgs uses
hard-coded addresses, balances and returned ABI data instead of the shared
realistic fixtures; update the test to use the standard fixtures from
utils/unittest (e.g. the address, balance and encoded return-data fixtures)
rather than in-test literals. Concretely, replace the hardcoded expectedBalance,
the bytes used for contractsAddress and the types.Data/ReturnedData blobs in the
accountByAddress / testFlowAccount.call closures and encodedValues with the
corresponding helpers/fixtures from utils/unittest, updating imports as needed
and keeping the existing helpers executeScript, handler, testFlowAccount and
accountByAddress logic intact. Ensure any duplicated fixture setup elsewhere is
removed so this test reuses the unified unittest fixtures for realistic data.
- Around line 5850-6334: The test TestCadenceOwnedAccountDryCallWithArgs
contains duplicated inline test fixtures (raw addresses, expectedData, encoded
ABI outputs) across several subtests; replace those hard-coded byte arrays and
packed ABI values with reusable fixtures from utils/unittest (e.g.,
address/encoded data helpers) to ensure realistic, consistent test data. Update
references in this file to use the fixture helpers when constructing the
handler's evmContractAddress, the expectedData slice used in assertions, and the
gethABI-encoded ReturnedData in the dryRunWithTxData closures (look for
executeScript, testContractHandler.dryRunWithTxData, and the
expectedData/arguments.Pack usage) so all subtests share the common unittest
fixtures instead of duplicating literals.
Comment on lines
+84
to
+101
| if deployedAddressField.Value != nil { | ||
| evmAddress, ok := deployedAddressField.Value.(cadence.Struct) | ||
| if !ok { | ||
| return nil, fmt.Errorf("invalid input: unexpected type for deployed contract field") | ||
| } | ||
|
|
||
| bytes, ok := cadence.SearchFieldByName(evmAddress, stdlib.EVMAddressTypeBytesFieldName).(cadence.Array) | ||
| if !ok { | ||
| return nil, fmt.Errorf("invalid input: unexpected type for deployed contract field") | ||
| } | ||
|
|
||
| convertedAddress := make([]byte, len(bytes.Values)) | ||
| for i, value := range bytes.Values { | ||
| convertedAddress[i] = byte(value.(cadence.UInt8)) | ||
| } | ||
| addr := types.Address(convertedAddress) | ||
| convertedDeployedAddress = &addr | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the file and check its content around lines 84-101
cat -n fvm/evm/testutils/result.go | sed -n '80,105p'Repository: onflow/flow-go
Length of output: 1117
🏁 Script executed:
#!/bin/bash
# Verify the types.Address definition and AddressLength constant
rg -n "type Address|const AddressLength" fvm/evm/types/ -A 2Repository: onflow/flow-go
Length of output: 646
🏁 Script executed:
#!/bin/bash
# Check if types.Address file exists and examine it
fd "types\.go|address\.go" fvm/evm/typesRepository: onflow/flow-go
Length of output: 82
🏁 Script executed:
#!/bin/bash
# Check for AddressLength usage in the codebase to understand expected length
rg "AddressLength" fvm/evm/ -B 2 -A 2 | head -50Repository: onflow/flow-go
Length of output: 2593
🏁 Script executed:
#!/bin/bash
# Check the go-ethereum reference - gethCommon.AddressLength value
rg "gethCommon\." fvm/evm/types/address.go -A 3Repository: onflow/flow-go
Length of output: 721
Validate deployed contract address length and element types before conversion.
The code has two safety gaps in lines 95-99:
- Type assertion
value.(cadence.UInt8)at line 97 will panic if the element is notcadence.UInt8 - No length validation before converting the byte array to
types.Address
Per the coding guideline "Treat all inputs as potentially byzantine and validate accordingly", add length check against types.AddressLength (20 bytes) and safe type assertions:
Suggested fix
- convertedAddress := make([]byte, len(bytes.Values))
- for i, value := range bytes.Values {
- convertedAddress[i] = byte(value.(cadence.UInt8))
- }
- addr := types.Address(convertedAddress)
- convertedDeployedAddress = &addr
+ if len(bytes.Values) != types.AddressLength {
+ return nil, fmt.Errorf("invalid input: unexpected deployed contract address length")
+ }
+ var addr types.Address
+ for i, value := range bytes.Values {
+ b, ok := value.(cadence.UInt8)
+ if !ok {
+ return nil, fmt.Errorf("invalid input: unexpected type for deployed contract byte")
+ }
+ addr[i] = byte(b)
+ }
+ convertedDeployedAddress = &addr📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if deployedAddressField.Value != nil { | |
| evmAddress, ok := deployedAddressField.Value.(cadence.Struct) | |
| if !ok { | |
| return nil, fmt.Errorf("invalid input: unexpected type for deployed contract field") | |
| } | |
| bytes, ok := cadence.SearchFieldByName(evmAddress, stdlib.EVMAddressTypeBytesFieldName).(cadence.Array) | |
| if !ok { | |
| return nil, fmt.Errorf("invalid input: unexpected type for deployed contract field") | |
| } | |
| convertedAddress := make([]byte, len(bytes.Values)) | |
| for i, value := range bytes.Values { | |
| convertedAddress[i] = byte(value.(cadence.UInt8)) | |
| } | |
| addr := types.Address(convertedAddress) | |
| convertedDeployedAddress = &addr | |
| } | |
| if deployedAddressField.Value != nil { | |
| evmAddress, ok := deployedAddressField.Value.(cadence.Struct) | |
| if !ok { | |
| return nil, fmt.Errorf("invalid input: unexpected type for deployed contract field") | |
| } | |
| bytes, ok := cadence.SearchFieldByName(evmAddress, stdlib.EVMAddressTypeBytesFieldName).(cadence.Array) | |
| if !ok { | |
| return nil, fmt.Errorf("invalid input: unexpected type for deployed contract field") | |
| } | |
| if len(bytes.Values) != types.AddressLength { | |
| return nil, fmt.Errorf("invalid input: unexpected deployed contract address length") | |
| } | |
| var addr types.Address | |
| for i, value := range bytes.Values { | |
| b, ok := value.(cadence.UInt8) | |
| if !ok { | |
| return nil, fmt.Errorf("invalid input: unexpected type for deployed contract byte") | |
| } | |
| addr[i] = byte(b) | |
| } | |
| convertedDeployedAddress = &addr | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@fvm/evm/testutils/result.go` around lines 84 - 101, Validate the deployed
contract address array before converting: check deployedAddressField.Value is
cadence.Struct (already done), then ensure cadence.SearchFieldByName(evmAddress,
stdlib.EVMAddressTypeBytesFieldName) yields a cadence.Array and verify its
length equals types.AddressLength (20) before allocating convertedAddress; when
iterating bytes.Values, use safe type assertions (ok pattern) for cadence.UInt8
and return an error if any element is not cadence.UInt8 instead of panicking;
finally construct types.Address only after all validations pass and assign to
convertedDeployedAddress.
Internal functions that are frequently used together can be combined inside a larger function in order to reduce the conversion overhead of their input and output data. Specifically, contract function calls have data conversion overhead for both input and output data. Input data are converted from interpreter.Value to Go value, and the output data are converted from Go value to interpreter.Value. Currently, EVM contract has separate functions to: - encode signature and arguments for call/dryCall - invoke call/dryCall with encoded data - decode result data from call/dryCall These functions are commonly used together, which repeatedly introduces data conversion overhead. This commmit adds new EVM functions that combine several internal function calls to reduce conversion overhead: - CadenceOwnedAccount.callWithSigAndArgs - CadenceOwnedAccount.dryCallWithSigAndArgs - EVM.dryCallWithSigAndArgs The new functions have these parameters: - from: EVMAddress - to: EVMAddress - signature: String - args: [AnyStruct] - gasLimit: UInt64 - value: UInt - resultTypes: [Type]? The signature and args are used to create transaction data. The resultTypes is optional. When resultTypes is provided, call result data is decoded and it is included in the returned ResultDecoded. When resultTypes isn't provided, the result data is discarded. The value is a UInt instead of a Balance, to avoid the overhead of using a composite value, since it is typically 0 in most calls and dryCalls.
fxamacker
force-pushed
the
fxamacker/add-evm-wrapper-func
branch
from
ed6ef7e to
16120e2
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/abi.go`:
- Around line 1008-1011: The panic currently drops the original error from
arguments.Unpack; update the handling so you capture the returned err and
populate abiDecodingError.Message with its details before panicking (e.g., build
abiDecodingError using err.Error() or wrapping err) so the original ABI unpack
error is preserved when decodedValues, err := arguments.Unpack(data) fails;
ensure the panic still uses that abiDecodingError instance.
In `@fvm/evm/impl/impl.go`:
- Around line 1568-1584: In encodeABIWithSigAndArgs, guard against a short hash
from keccak256Hash before doing sig = sig[:4]: after calling keccak256Hash(check
signature) validate that the returned slice length is at least 4 and return a
descriptive error (wrap/format as appropriate for the project) if it is shorter;
this prevents a panic when slicing sig and keeps error classification/context
consistent with other error returns in this function.
In `@fvm/evm/testutils/result.go`:
- Around line 21-66: Replace all fmt.Errorf calls in
ResultDecodedFromEVMResultValue with the irrecoverable package constructors
(e.g., irrecoverable.New(...)) so validation failures become irrecoverable
errors; update the import list to include the irrecoverable package, keep the
original messages (e.g., "invalid input: ...") when constructing the
irrecoverable errors, and ensure each early return still returns nil,
<irrecoverable error> for the checks on cadence.Struct, field count, status
field typing, error code/message/gas/result field typings.
---
Duplicate comments:
In `@fvm/evm/stdlib/contract_test.go`:
- Around line 4442-6334: Replace the in-test hard-coded EVM addresses and ABI
payload byte arrays with the shared test fixtures from the utils/unittest
package: import the unittest fixtures and use them in TestEVMDryCallWithArgs,
TestCadenceOwnedAccountCallWithArgs, and TestCadenceOwnedAccountDryCallWithArgs
where addresses and payloads are currently inlined (e.g., the EVM.EVMAddress
byte literals in the scripts, the expectedData slices checked against
gethTx.Data() inside test handlers, the coinbase and evmTxs cadence arrays, and
the encoded ABI values created with gethABI.Arguments.Pack in the handler
callbacks); update the executeScript closures and testContractHandler callbacks
(dryRunWithTxData, accountByAddress.call, batchRun) to reference those fixtures
instead of literal arrays so the tests use centralized realistic data.
In `@fvm/evm/testutils/result.go`:
- Around line 84-100: The code that converts deployedAddressField into
types.Address lacks validation: ensure the field value is a cadence.Struct
(evmAddress), retrieve the bytes via cadence.SearchFieldByName(...,
stdlib.EVMAddressTypeBytesFieldName) and assert it is a cadence.Array, then
check len(bytes.Values) == types.AddressLength and return a validation error if
not; when iterating bytes.Values, use safe type assertions for each element
(e.g., check each value is cadence.UInt8 before converting) and return a clear
error on any type mismatch, finally construct types.Address only after all
checks pass and assign to convertedDeployedAddress.
Comment on lines
21
to
66
| func ResultDecodedFromEVMResultValue(val cadence.Value) (*ResultDecoded, error) { | ||
| str, ok := val.(cadence.Struct) | ||
| if !ok { | ||
| return nil, fmt.Errorf("invalid input: unexpected value type") | ||
| } | ||
|
|
||
| fields := cadence.FieldsMappedByName(str) | ||
|
|
||
| const expectedFieldCount = 6 | ||
| if len(fields) != expectedFieldCount { | ||
| return nil, fmt.Errorf( | ||
| "invalid input: field count mismatch: expected %d, got %d", | ||
| expectedFieldCount, | ||
| len(fields), | ||
| ) | ||
| } | ||
|
|
||
| statusEnum, ok := fields[stdlib.EVMResultTypeStatusFieldName].(cadence.Enum) | ||
| if !ok { | ||
| return nil, fmt.Errorf("invalid input: unexpected type for status field") | ||
| } | ||
|
|
||
| status, ok := cadence.FieldsMappedByName(statusEnum)[sema.EnumRawValueFieldName].(cadence.UInt8) | ||
| if !ok { | ||
| return nil, fmt.Errorf("invalid input: unexpected type for status field") | ||
| } | ||
|
|
||
| errorCode, ok := fields[stdlib.EVMResultTypeErrorCodeFieldName].(cadence.UInt64) | ||
| if !ok { | ||
| return nil, fmt.Errorf("invalid input: unexpected type for error code field") | ||
| } | ||
|
|
||
| errorMsg, ok := fields[stdlib.EVMResultTypeErrorMessageFieldName].(cadence.String) | ||
| if !ok { | ||
| return nil, fmt.Errorf("invalid input: unexpected type for error msg field") | ||
| } | ||
|
|
||
| gasUsed, ok := fields[stdlib.EVMResultTypeGasUsedFieldName].(cadence.UInt64) | ||
| if !ok { | ||
| return nil, fmt.Errorf("invalid input: unexpected type for gas field") | ||
| } | ||
|
|
||
| resultsField, ok := fields[stdlib.EVMResultTypeResultsFieldName].(cadence.Optional) | ||
| if !ok { | ||
| return nil, fmt.Errorf("invalid input: unexpected type for data field") | ||
| } |
Contributor
There was a problem hiding this comment.
Use irrecoverable error helpers instead of fmt.Errorf.
This helper returns validation errors via fmt.Errorf, which conflicts with the repo’s error-handling guidance. Please switch to the irrecoverable package (or its approved constructors) for these failures.
As per coding guidelines: "Use the irrecoverable package for exception handling instead of fmt.Errorf; NEVER log and continue on best effort basis, ALWAYS explicitly handle errors".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@fvm/evm/testutils/result.go` around lines 21 - 66, Replace all fmt.Errorf
calls in ResultDecodedFromEVMResultValue with the irrecoverable package
constructors (e.g., irrecoverable.New(...)) so validation failures become
irrecoverable errors; update the import list to include the irrecoverable
package, keep the original messages (e.g., "invalid input: ...") when
constructing the irrecoverable errors, and ensure each early return still
returns nil, <irrecoverable error> for the checks on cadence.Struct, field
count, status field typing, error code/message/gas/result field typings.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
fvm/evm/evm_test.go (2)
3851-3851: Remove commented-out debug code.This leftover line adds noise without serving any purpose.
♻️ Proposed fix
- //data := testContract.MakeCallData(t, "store", big.NewInt(updatedValue)) - signature := "store(uint256)"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fvm/evm/evm_test.go` at line 3851, Remove the leftover commented debug line calling testContract.MakeCallData (the line containing "//data := testContract.MakeCallData(t, \"store\", big.NewInt(updatedValue))") from the test; simply delete this commented-out statement so the test code is clean and free of dead debug artifacts.
2472-2472: Untyped empty array is inconsistent with the rest of the test file.
cadence.NewArray(nil)lacks explicit type metadata. The identical pattern elsewhere (e.g. Line 3807) explicitly types the array:♻️ Proposed fix
- args := json.MustEncode(cadence.NewArray(nil)) + args := json.MustEncode( + cadence.NewArray(nil).WithType(cadence.NewVariableSizedArrayType(cadence.AnyStructType)), + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fvm/evm/evm_test.go` at line 2472, The test uses an untyped empty Cadence array via cadence.NewArray(nil); replace it with an explicitly typed empty array to match the rest of the tests — e.g., change cadence.NewArray(nil) to cadence.NewArray([]cadence.Value{}) where args is created, so the empty array carries type metadata consistent with other usages.fvm/evm/stdlib/contract_test.go (4)
5404-5457: Sub-tests "tx result data is empty / doesn't match" lackcallCalledverificationThe
dryCallvariants of the same scenarios (e.g., lines 5982–6042) guard with adryCallCalledboolean andassert.True(t, dryCallCalled). ThecallWithSigAndArgssub-tests here omit this, so if the handler were never reached (e.g., encoding fails early), the test would still pass on the error-message check alone.♻️ Proposed change
t.Run("call includes result types, tx result data is empty", func(t *testing.T) { + callCalled := false handler := &testContractHandler{ ... accountByAddress: func(fromAddress types.Address, isAuthorized bool) types.Account { ... return &testFlowAccount{ address: fromAddress, call: func(...) *types.ResultSummary { + callCalled = true ... }, } }, } ... _, err := executeScript(handler, script) require.ErrorContains(t, err, "failed to ABI decode data") + assert.True(t, callCalled) })🤖 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 5404 - 5457, The test is missing verification that the handler's call path was actually invoked; add a boolean (e.g., callCalled) in the test and set it to true inside the testFlowAccount.call closure, then after executeScript assert.True(t, callCalled) so the test ensures callWithSigAndArgs reached the handler (similar to the dryCallCalled checks in the dryCall variants); update the sub-test that uses callWithSigAndArgs, referencing testContractHandler, testFlowAccount.call, and callWithSigAndArgs to locate where to set and assert the flag.
348-392:OnHashcallbacks in pre-existing tests are now dead code
newEVMTransactionEnvironmentandnewEVMScriptEnvironmentnow supplycryptoLibrary.Hashas the EVM-internal hash callback, bypassing theruntimeInterface.OnHashpath for Keccak operations. TheOnHashhandlers inTestEVMEncodeABIWithSignature(Line 3405),TestEVMDecodeABIWithSignature(Line 3533),TestEVMDecodeABIWithSignatureMismatch(Line 3649), andTestEVMEncodeDecodeABIRoundtripForUintIntTypes(Line 1936) are no longer invoked and can be removed.🤖 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 348 - 392, The tests became dead code because newEVMTransactionEnvironment and newEVMScriptEnvironment pass cryptoLibrary.Hash directly to impl.NewInternalEVMContractValue, bypassing the runtime's OnHash hook used by tests; change both factories to pass an OnHash wrapper that delegates to the runtime/environment OnHash handler (i.e., call the environment or handler's OnHash/runtimeInterface.OnHash path) instead of calling cryptoLibrary.Hash directly so the existing TestEVM* OnHash callbacks are invoked; locate impl.NewInternalEVMContractValue, stdlib.SetupEnvironment, and the cryptoLibrary.Hash usage in newEVMTransactionEnvironment and newEVMScriptEnvironment and replace the direct call with a function that forwards to the runtime's OnHash handler.
4447-4507:executeScripthelper is duplicated across three test functionsThe same closure (lines 4447–4507) appears again in
TestCadenceOwnedAccountCallWithArgs(lines 5283–5342) andTestCadenceOwnedAccountDryCallWithArgs(lines 5855–5913). Consider extracting a shared package-level helper:♻️ Proposed extraction
+// executeEVMScript is a shared test helper that deploys the EVM contracts and +// runs the given script against a fresh runtime using the provided handler. +func executeEVMScript( + t *testing.T, + handler types.ContractHandler, + contractsAddress flow.Address, + script []byte, +) (cadence.Value, error) { + rt := runtime.NewRuntime(runtime.Config{}) + accountCodes := map[common.Location][]byte{} + var events []cadence.Event + runtimeInterface := &TestRuntimeInterface{ + Storage: NewTestLedger(nil, nil), + OnGetSigningAccounts: func() ([]runtime.Address, error) { + return []runtime.Address{runtime.Address(contractsAddress)}, nil + }, + OnResolveLocation: newLocationResolver(contractsAddress), + OnUpdateAccountContractCode: func(location common.AddressLocation, code []byte) error { + accountCodes[location] = code + return nil + }, + OnGetAccountContractCode: func(location common.AddressLocation) (code []byte, err error) { + return accountCodes[location], nil + }, + OnEmitEvent: func(event cadence.Event) error { + events = append(events, event) + return nil + }, + OnDecodeArgument: func(b []byte, t cadence.Type) (cadence.Value, error) { + return json.Decode(nil, b) + }, + } + nextTransactionLocation := NewTransactionLocationGenerator() + nextScriptLocation := NewScriptLocationGenerator() + transactionEnvironment := newEVMTransactionEnvironment(handler, contractsAddress) + scriptEnvironment := newEVMScriptEnvironment(handler, contractsAddress) + deployContracts(t, rt, contractsAddress, runtimeInterface, transactionEnvironment, nextTransactionLocation) + return rt.ExecuteScript( + runtime.Script{Source: script}, + runtime.Context{ + Interface: runtimeInterface, + Environment: scriptEnvironment, + Location: nextScriptLocation(), + }, + ) +}Then each test replaces its local closure with
executeEVMScript(t, handler, contractsAddress, script).🤖 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 4447 - 4507, The executeScript closure is duplicated; extract it to a package-level helper (e.g., executeEVMScript) that takes t, handler, contractsAddress, and script and performs the same setup: create runtime.NewRuntime, build TestRuntimeInterface (including OnGetSigningAccounts, OnResolveLocation, OnUpdateAccountContractCode, OnGetAccountContractCode, OnEmitEvent, OnDecodeArgument), instantiate NewTransactionLocationGenerator and NewScriptLocationGenerator, build transactionEnvironment via newEVMTransactionEnvironment and scriptEnvironment via newEVMScriptEnvironment, call deployContracts, then call rt.ExecuteScript with the generated runtime.Context; replace the three local closures (executeScript) in the tests with calls to executeEVMScript(t, handler, contractsAddress, script).
4566-4566:assert.True(t, len(res.Results) == 0)yields poor failure diagnosticsThis pattern (repeated at lines 4566, 4812, 5401, 5650, 5979, 6256) only prints
"expected true but got false"when it fails. Preferassert.Emptyso the actual slice contents appear in the output.♻️ Proposed change (representative instance)
-assert.True(t, len(res.Results) == 0) +assert.Empty(t, res.Results)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fvm/evm/stdlib/contract_test.go` at line 4566, Replace the brittle length-check assertion with an empty-slice assertion to improve failure diagnostics: wherever tests call assert.True(t, len(res.Results) == 0) (checking the res.Results slice), change it to assert.Empty(t, res.Results) so the test framework will print the actual slice contents on failure; update each occurrence that uses res.Results (the instances at the noted locations) accordingly.
🤖 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 3683-3684: Update the stale comment that incorrectly references
EVM.dryCall: change the mention to EVM.dryCallWithSigAndArgs so the comment
matches the actual code under test (the block around the `limit =
gethParams.MaxTxGas + 1_000` assignment and the test invoking
EVM.dryCallWithSigAndArgs); ensure any surrounding wording consistently names
EVM.dryCallWithSigAndArgs instead of EVM.dryCall.
---
Duplicate comments:
In `@fvm/evm/testutils/result.go`:
- Around line 21-66: Replace all uses of fmt.Errorf in
ResultDecodedFromEVMResultValue with the repository's standard
error-construction helpers (do not use fmt.Errorf). Locate the function
ResultDecodedFromEVMResultValue and for each early-return that currently does
return nil, fmt.Errorf(...), switch to the project's error helper (the same
pattern used elsewhere in this package) to create typed/wrapped errors with the
same diagnostic messages; update imports accordingly and ensure each error
preserves the original message and any formatting arguments.
- Around line 85-102: The deployed address parsing block uses unsafe assertions
and doesn't validate length; update the logic around deployedAddressField ->
evmAddress -> bytes to (1) use safe type assertions (ok checks) for evmAddress
and the bytes array (evmAddress variable and the cadence.Array result from
stdlib.EVMAddressTypeBytesFieldName), (2) verify len(bytes.Values) equals the
expected address length (use the Address length constant from types, e.g.
types.AddressLength or equivalent), returning a clear error if it doesn't match,
and (3) when iterating bytes.Values, assert each element is a cadence.UInt8 with
an ok check before converting into convertedAddress and then constructing
types.Address; return descriptive errors on any bad types/lengths instead of
panicking.
---
Nitpick comments:
In `@fvm/evm/evm_test.go`:
- Line 3851: Remove the leftover commented debug line calling
testContract.MakeCallData (the line containing "//data :=
testContract.MakeCallData(t, \"store\", big.NewInt(updatedValue))") from the
test; simply delete this commented-out statement so the test code is clean and
free of dead debug artifacts.
- Line 2472: The test uses an untyped empty Cadence array via
cadence.NewArray(nil); replace it with an explicitly typed empty array to match
the rest of the tests — e.g., change cadence.NewArray(nil) to
cadence.NewArray([]cadence.Value{}) where args is created, so the empty array
carries type metadata consistent with other usages.
In `@fvm/evm/stdlib/contract_test.go`:
- Around line 5404-5457: The test is missing verification that the handler's
call path was actually invoked; add a boolean (e.g., callCalled) in the test and
set it to true inside the testFlowAccount.call closure, then after executeScript
assert.True(t, callCalled) so the test ensures callWithSigAndArgs reached the
handler (similar to the dryCallCalled checks in the dryCall variants); update
the sub-test that uses callWithSigAndArgs, referencing testContractHandler,
testFlowAccount.call, and callWithSigAndArgs to locate where to set and assert
the flag.
- Around line 348-392: The tests became dead code because
newEVMTransactionEnvironment and newEVMScriptEnvironment pass cryptoLibrary.Hash
directly to impl.NewInternalEVMContractValue, bypassing the runtime's OnHash
hook used by tests; change both factories to pass an OnHash wrapper that
delegates to the runtime/environment OnHash handler (i.e., call the environment
or handler's OnHash/runtimeInterface.OnHash path) instead of calling
cryptoLibrary.Hash directly so the existing TestEVM* OnHash callbacks are
invoked; locate impl.NewInternalEVMContractValue, stdlib.SetupEnvironment, and
the cryptoLibrary.Hash usage in newEVMTransactionEnvironment and
newEVMScriptEnvironment and replace the direct call with a function that
forwards to the runtime's OnHash handler.
- Around line 4447-4507: The executeScript closure is duplicated; extract it to
a package-level helper (e.g., executeEVMScript) that takes t, handler,
contractsAddress, and script and performs the same setup: create
runtime.NewRuntime, build TestRuntimeInterface (including OnGetSigningAccounts,
OnResolveLocation, OnUpdateAccountContractCode, OnGetAccountContractCode,
OnEmitEvent, OnDecodeArgument), instantiate NewTransactionLocationGenerator and
NewScriptLocationGenerator, build transactionEnvironment via
newEVMTransactionEnvironment and scriptEnvironment via newEVMScriptEnvironment,
call deployContracts, then call rt.ExecuteScript with the generated
runtime.Context; replace the three local closures (executeScript) in the tests
with calls to executeEVMScript(t, handler, contractsAddress, script).
- Line 4566: Replace the brittle length-check assertion with an empty-slice
assertion to improve failure diagnostics: wherever tests call assert.True(t,
len(res.Results) == 0) (checking the res.Results slice), change it to
assert.Empty(t, res.Results) so the test framework will print the actual slice
contents on failure; update each occurrence that uses res.Results (the instances
at the noted locations) accordingly.
Comment on lines
+3683
to
+3684
| // EVM.dryCall must not be limited to `gethParams.MaxTxGas` | ||
| limit = gethParams.MaxTxGas + 1_000 |
Contributor
There was a problem hiding this comment.
Stale comment names the wrong function.
This comment was copied from TestDryCall and still references EVM.dryCall instead of EVM.dryCallWithSigAndArgs.
📝 Proposed fix
- // EVM.dryCall must not be limited to `gethParams.MaxTxGas`
+ // EVM.dryCallWithSigAndArgs must not be limited to `gethParams.MaxTxGas`📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // EVM.dryCall must not be limited to `gethParams.MaxTxGas` | |
| limit = gethParams.MaxTxGas + 1_000 | |
| // EVM.dryCallWithSigAndArgs must not be limited to `gethParams.MaxTxGas` | |
| limit = gethParams.MaxTxGas + 1_000 |
🤖 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 3683 - 3684, Update the stale comment that
incorrectly references EVM.dryCall: change the mention to
EVM.dryCallWithSigAndArgs so the comment matches the actual code under test (the
block around the `limit = gethParams.MaxTxGas + 1_000` assignment and the test
invoking EVM.dryCallWithSigAndArgs); ensure any surrounding wording consistently
names EVM.dryCallWithSigAndArgs instead of EVM.dryCall.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
fvm/evm/impl/impl.go (1)
1282-1301:decodeResultDatanever populates itserrorreturn — dead code in both the signature and the caller's guard.
decodeABIsreturns a single value (no error), and both code paths indecodeResultDatareturnnilfor the error. As a result:
if err != nil { panic(err) }at line 1220–1222 is unreachable.- Future readers will check the error path for no reason.
Consider simplifying the signature to return only
interpreter.OptionalValue, with a TODO comment if error propagation fromdecodeABIsis planned.♻️ Suggested simplification
func decodeResultData( context interpreter.InvocationContext, location common.AddressLocation, evmSpecialTypeIDs *evmSpecialTypeIDs, resultTypes *interpreter.ArrayValue, result *types.ResultSummary, -) (interpreter.OptionalValue, error) { +) interpreter.OptionalValue { results := interpreter.NilOptionalValue if result.Status != types.StatusSuccessful || resultTypes == nil || resultTypes.Count() == 0 { - return results, nil + return results } resultValue := decodeABIs(context, location, evmSpecialTypeIDs, resultTypes, result.ReturnedData) results = interpreter.NewSomeValueNonCopying(context, resultValue) - return results, nil + return results }And update the call site:
- results, err := decodeResultData(context, location, evmSpecialTypeIDs, resultTypes, result) - if err != nil { - panic(err) - } + results := decodeResultData(context, location, evmSpecialTypeIDs, resultTypes, result)🤖 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 1282 - 1301, The function decodeResultData currently never returns a non-nil error (both branches return nil) because decodeABIs does not produce errors; change decodeResultData to return only interpreter.OptionalValue (remove the error return) and update its callers to drop error handling (e.g., the guard around err at the caller that currently panics is dead code). Locate decodeResultData, its signature and return sites, and replace returns of (results, nil) with just results; add a short TODO comment in decodeResultData mentioning that if decodeABIs later returns errors the signature should be revisited, and ensure references to types like interpreter.InvocationContext, interpreter.ArrayValue, types.ResultSummary, and interpreter.NewSomeValueNonCopying are preserved.
🤖 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/impl/impl.go`:
- Around line 1282-1301: The function decodeResultData currently never returns a
non-nil error (both branches return nil) because decodeABIs does not produce
errors; change decodeResultData to return only interpreter.OptionalValue (remove
the error return) and update its callers to drop error handling (e.g., the guard
around err at the caller that currently panics is dead code). Locate
decodeResultData, its signature and return sites, and replace returns of
(results, nil) with just results; add a short TODO comment in decodeResultData
mentioning that if decodeABIs later returns errors the signature should be
revisited, and ensure references to types like interpreter.InvocationContext,
interpreter.ArrayValue, types.ResultSummary, and
interpreter.NewSomeValueNonCopying are preserved.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
fvm/evm/testutils/result.go (1)
39-47: Ambiguous duplicate error message across two distinct failure paths.Both the enum type assertion (line 41) and the raw-value type assertion (line 46) return the same string
"invalid input: unexpected type for status field", making it impossible to distinguish the failure path during debugging.♻️ Suggested fix
statusEnum, ok := fields[stdlib.EVMResultTypeStatusFieldName].(cadence.Enum) if !ok { - return nil, fmt.Errorf("invalid input: unexpected type for status field") + return nil, fmt.Errorf("invalid input: status field is not a cadence.Enum") } status, ok := cadence.FieldsMappedByName(statusEnum)[sema.EnumRawValueFieldName].(cadence.UInt8) if !ok { - return nil, fmt.Errorf("invalid input: unexpected type for status field") + return nil, fmt.Errorf("invalid input: status enum raw value is not cadence.UInt8") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fvm/evm/testutils/result.go` around lines 39 - 47, The two type-assertion failures for the status field use the identical error string which obscures which check failed; update the error messages to be distinct and descriptive: when asserting fields[stdlib.EVMResultTypeStatusFieldName] to cadence.Enum (variable statusEnum) return an error like "invalid input: status field is not cadence.Enum" and when extracting the raw value via cadence.FieldsMappedByName(statusEnum)[sema.EnumRawValueFieldName] to cadence.UInt8 return an error like "invalid input: status enum raw value is not cadence.UInt8" so callers can tell whether the enum or its raw-value conversion failed.
🤖 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/testutils/result.go`:
- Around line 96-101: Guard the conversion from cadence bytes to types.Address
by using a checked type assertion for each element of bytes.Values (use the
comma-ok form when asserting cadence.UInt8) and handle non-UInt8 values
gracefully (return an error or skip with logging), then validate the resulting
convertedAddress length equals types.AddressLength before calling
types.Address(convertedAddress); if the length mismatches handle it explicitly
(return an error) instead of allowing panic or silent truncation. Ensure the
fixes are applied around the code that reads bytes.Values and assigns
convertedDeployedAddress so any invalid input is detected and surfaced rather
than causing a runtime panic.
- Around line 22-67: Replace all uses of fmt.Errorf in
ResultDecodedFromEVMResultValue with the irrecoverable package error
constructors: import the irrecoverable package and change each return like
`return nil, fmt.Errorf("...%d...", ...)` to `return nil,
irrecoverable.Newf("...%d...", ...)` (or `irrecoverable.New` for non-formatted
messages). Update every error return in ResultDecodedFromEVMResultValue (the
checks on `str`, `len(fields)`, `statusEnum`, `status`, `errorCode`, `errorMsg`,
`gasUsed`, and `resultsField`) to use irrecoverable so all failures originate
from the irrecoverable package.
---
Nitpick comments:
In `@fvm/evm/testutils/result.go`:
- Around line 39-47: The two type-assertion failures for the status field use
the identical error string which obscures which check failed; update the error
messages to be distinct and descriptive: when asserting
fields[stdlib.EVMResultTypeStatusFieldName] to cadence.Enum (variable
statusEnum) return an error like "invalid input: status field is not
cadence.Enum" and when extracting the raw value via
cadence.FieldsMappedByName(statusEnum)[sema.EnumRawValueFieldName] to
cadence.UInt8 return an error like "invalid input: status enum raw value is not
cadence.UInt8" so callers can tell whether the enum or its raw-value conversion
failed.
m-Peter
reviewed
Feb 18, 2026
m-Peter
reviewed
Feb 18, 2026
m-Peter
reviewed
Feb 18, 2026
m-Peter
reviewed
Feb 18, 2026
m-Peter
reviewed
Feb 18, 2026
m-Peter
reviewed
Feb 18, 2026
Co-Authored-By: Ardit Marku <markoupetr@gmail.com>
This commit updates the following EVM functions to return raw result data if EVM call fails or user doesn't provide resultTypes: - CadenceOwnedAccount.callWithSigAndArgs - CadenceOwnedAccount.dryCallWithSigAndArgs - EVM.dryCallWithSigAndArgs This commit updates CadenceOwnedAccount.callWithSigAndArgs, CadenceOwnedAccount.dryCallWithSigAndArgs, and EVM.dryCallWithSigAndArgs to return raw result data if evm call failes or user doesn't provide resultTypes.
Co-Authored-By: Ardit Marku <markoupetr@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #8405
EVM internal functions that are frequently used together can be combined inside a larger function in order to reduce the conversion overhead of their input and output data. For an example of impact, see "Estimated Impact" section below.
Specifically, contract function calls have data conversion overhead for both input and output data. Input data are converted from interpreter.Value to Go value, and the output data are converted from Go value to interpreter.Value.
Currently, EVM contract has separate functions to:
These functions are commonly used together, which repeatedly introduces data conversion overhead.
This PR adds new EVM functions that combine several internal function calls to reduce conversion overhead:
CadenceOwnedAccount.callWithSigAndArgsCadenceOwnedAccount.dryCallWithSigAndArgsEVM.dryCallWithSigAndArgsThe new functions have these parameters:
from: EVMAddressto: EVMAddresssignature: Stringargs: [AnyStruct]gasLimit: UInt64value: UIntresultTypes: [Type]?The
signatureandargsare used to create transaction data.The
resultTypesis optional. WhenresultTypesis provided, call result data is decoded and it is included in the returnedResultDecoded.results. WhenresultTypesisn't provided or empty, the result data is discarded.The value is a
UIntinstead of aBalance, to avoid the overhead of using a composite value, since it is typically 0 in most calls and dryCalls.Estimated Impact
This optimization focused on reducing conversion overhead in EVM function calls, together with several quick-hit PRs (#8397, #8398, #8399, #8400).
For the open yield vault transaction, using these optimized functions can reduce computation cost by at least 8% (ballpark estimate) when used together with the quick-hit optimizations.
Impact on other transactions have not yet been estimated.
Summary by CodeRabbit
New Features
Tests