Skip to content

Replace COA.call with COA.callWithSigAndArgs for reduced computation cost#77

Open
m-Peter wants to merge 1 commit intomainfrom
mpeter/optimize-evm-call-usage
Open

Replace COA.call with COA.callWithSigAndArgs for reduced computation cost#77
m-Peter wants to merge 1 commit intomainfrom
mpeter/optimize-evm-call-usage

Conversation

@m-Peter
Copy link
Copy Markdown
Collaborator

@m-Peter m-Peter commented Mar 20, 2026

Related: onflow/flow-go#8401
Closes: #76

Description

Previously, to perform contract calls using a COA, it was necessary to produce the callData on Cadence side:

let calldata = EVM.encodeABIWithSignature(signature, args)

To be able to ABI-decode the returned data from the COA call, another step was needed:

let decoded = EVM.decodeABI(types: [Type<[UInt256]>()], data: res.data)
let amountsOut = decoded[0] as! [UInt256]

After doing some profiling on Cadence transactions, the computation cost of ABI encoding/decoding, amounted to about 15%-20% of the total computation, which is not trivial.

To optimize this case, we have a new wrapper function, callWithSigAndArgs, which does all this more efficiently:

coa.callWithSigAndArgs(
    to: to,
    signature: "approve(address,uint256)",
    args: [self.routerAddress, evmAmountIn],
    gasLimit: gasLimit,
    value: valueBalance.attoflow,
    resultTypes: [Type<[UInt256]>()]
)

We do the same for dryCall, by replacing it with dryCallWithSigAndArgs .

NOTE: CI is failing because we still need to deploy the EVM system contract on testnet/mainnet, for these new functions to be available.

@m-Peter m-Peter self-assigned this Mar 20, 2026
@m-Peter m-Peter force-pushed the mpeter/optimize-evm-call-usage branch from ae6fa70 to 12290b2 Compare March 20, 2026 11:54
@m-Peter m-Peter force-pushed the mpeter/optimize-evm-call-usage branch from 12290b2 to 85dc5f3 Compare March 20, 2026 12:09
@m-Peter m-Peter marked this pull request as ready for review March 23, 2026 10:48
@m-Peter m-Peter requested review from liobrasil and nvdtf March 23, 2026 10:48

if result.status != EVM.Status.successful {
let errorMsg = FlowYieldVaultsEVM.decodeEVMError(result.data)
let errorMsg = FlowYieldVaultsEVM.decodeEVMError(result.results as! [UInt8])
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Unverified type assumption in all 13 error paths

Every failure branch in this PR casts result.results as! [UInt8] (13 sites across the contract). The old code used result.data, which was a dedicated [UInt8] field guaranteed to hold raw revert bytes. The new callWithSigAndArgs/dryCallWithSigAndArgs API stores results in [AnyStruct] — a field shared by both success (decoded Cadence values) and failure (raw revert bytes).

The downcast result.results as! [UInt8] succeeds only if the dynamic type of results on failure is [UInt8] at the array level. If the implementation wraps the bytes as a single element (e.g. [[UInt8]]) or populates results element-by-element from [AnyStruct], the downcast panics at runtime with a Cadence type error instead of decoding and propagating the actual EVM error.

CI is explicitly broken because callWithSigAndArgs isn't deployed yet, so this assumption is currently untested. Before merging, please add at least one Cadence test that exercises a failing EVM call via the new API and confirms that result.results as! [UInt8] correctly contains the revert data.

let vaultIdentifiers = decoded[9] as! [String]
let strategyIdentifiers = decoded[10] as! [String]
if callResult.status != EVM.Status.successful {
let errorMsg = FlowYieldVaultsEVM.decodeEVMError(callResult.results as! [UInt8])
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Graceful nil return replaced with potential panic in critical scheduler path

In the old code decodeEVMError(callResult.data) was safe — data was always [UInt8]. Here callResult.results as! [UInt8] may panic if the API's failure representation isn't a bare [UInt8] (see discussion on line 994).

The consequence here is worse than in state-mutating paths: getPendingRequestsFromEVM is called by the SchedulerHandler to decide which requests to process. Old behaviour on EVM call failure: emit ErrorEncountered, return nil, SchedulerHandler skips gracefully. New behaviour if the cast panics: the entire SchedulerHandler transaction reverts, crash recovery must handle it, and the root cause (EVM call failure + reason) is lost.

Additionally, resultTypes here includes Type<[UInt8]>() (for requestTypes and statuses). If the EVM runtime tries to decode even on failure, results could be partially or incorrectly populated — making the as! [UInt8] cast semantically ambiguous on the failure path. Consider using a dedicated helper that checks callResult.status first and accesses raw error bytes via a well-typed accessor rather than a force-cast on the shared results field.

@claude
Copy link
Copy Markdown

claude bot commented Mar 23, 2026

Review: Replace COA.call with COA.callWithSigAndArgs

The optimization goal is sound. Eliminating the separate encodeABIWithSignature and decodeABI steps for a 15-20% computation saving is worthwhile. The refactor is mechanically consistent across all 11 files. Two concrete issues need resolution before merge.


Issue 1: Untested type assumption in every failure path (13 sites) -- High

Every if result.status != EVM.Status.successful block now does:

let errorMsg = FlowYieldVaultsEVM.decodeEVMError(result.results as! [UInt8])

The old API had a dedicated result.data: [UInt8] field -- always a flat byte array on success or failure. The new API stores everything in result.results: [AnyStruct] -- decoded Cadence values on success and (presumably) raw revert bytes on failure. The downcast result.results as! [UInt8] succeeds only if the dynamic type of results on failure is [UInt8] at the array level. If the Flow EVM implementation wraps the bytes differently (e.g. appends them element-by-element into [AnyStruct], or nests them as [[UInt8]]), every single error path panics with a Cadence runtime type error instead of decoding and reporting the actual EVM failure reason.

CI is explicitly broken because callWithSigAndArgs is not yet deployed, so this assumption is completely untested. The behavioural contract of the new API on failure must be confirmed -- ideally via a Cadence test that exercises a reverting EVM call and asserts that result.results as! [UInt8] yields the expected revert bytes -- before this lands.

Inline comment: #77 (comment)


Issue 2: getPendingRequestsFromEVM -- graceful nil replaced by potential panic in scheduler critical path -- Medium

In the old code, decodeEVMError(callResult.data) was unconditionally safe -- data was always [UInt8]. In the new code, if the results as! [UInt8] cast fails on an EVM call failure, the function panics instead of emitting ErrorEncountered and returning nil.

The consequence matters because this function drives SchedulerHandler batch selection. The old degraded behaviour (return nil, skip scheduling) is replaced by a transaction revert requiring crash recovery. The root cause (EVM call failure and reason) is also lost.

Additionally, the resultTypes list for this call includes Type<[UInt8]>() at indices 2 and 3 (for requestTypes/statuses). On failure, the entire results is cast to [UInt8]. These are semantically different uses of the same field -- correctness relies on the runtime choosing a completely different dynamic type for results in the failure case vs. the success case.

Inline comment: #77 (comment)


Other observations (no action needed)

  • value: refundValue.attoflow in completeProcessing: Correct -- .attoflow extracts UInt from EVM.Balance, matching the new API parameter type.
  • assert(callResult.results.length == N, ...): Slightly stricter than the old silent decode-and-crash; equivalent in practice given transaction atomicity.
  • Scripts (get_allowlist_status, get_evm_contract_config, etc.): Mechanically correct; failure paths in scripts do not call decodeEVMError, so the cast risk is isolated to the contract.
  • resultTypes: nil for void calls: Appropriate for state-mutating calls with no meaningful return value.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Optimization] Replace call & dryCall with their optimized versions

1 participant