Skip to content
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
100 changes: 79 additions & 21 deletions cadence/contracts/FlowYieldVaultsEVM.cdc
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,13 @@ import "FlowEVMBridgeConfig"
/// batch-update statuses (PENDING -> PROCESSING for valid, PENDING -> FAILED for invalid)
/// 2. Processing: For each PROCESSING request, executes Cadence-side operation
/// (create/deposit/withdraw/close YieldVault), then calls completeProcessing() to mark
/// as COMPLETED or FAILED (with refund to EVM contract on CREATE/DEPOSIT failure)
/// as COMPLETED or FAILED while reconciling any refund owed on the EVM side
/// PRECISION NOTE:
/// Native FLOW uses 18 decimals and ERC20 tokens use their own token decimals,
/// while Cadence UFix64 supports 8 decimal places.
/// CREATE/DEPOSIT requests therefore round the bridged amount down to the nearest
/// Cadence-representable quantity and refund any remainder back to EVM claimable refunds.

access(all) contract FlowYieldVaultsEVM {

// ============================================
Expand Down Expand Up @@ -573,7 +579,7 @@ access(all) contract FlowYieldVaultsEVM {
/// @dev This is the main dispatcher that:
/// 1. Validates request status - should be PROCESSING
/// 2. Dispatches to the appropriate process function based on request type
/// 3. Calls completeProcessing to update final status (with refund on failure for CREATE/DEPOSIT)
/// 3. Calls completeProcessing to update final status and return any refund due on EVM
/// @param request The EVM request to process
/// @return ProcessResult with success status, the yieldVaultId, and status message
access(all) fun processRequest(_ request: EVMRequest): ProcessResult {
Expand Down Expand Up @@ -620,14 +626,17 @@ access(all) contract FlowYieldVaultsEVM {
)
}

// Pass refund info - completeProcessing will determine if refund is needed
// based on success flag and request type
let refundAmount = FlowYieldVaultsEVM.refundAmountForCompletion(
request: request,
success: result!.success
)

if !self.completeProcessing(
requestId: request.id,
success: result!.success,
yieldVaultId: result!.yieldVaultId,
message: result!.message,
refundAmount: request.amount,
refundAmount: refundAmount,
tokenAddress: request.tokenAddress,
requestType: request.requestType
) {
Expand All @@ -651,7 +660,9 @@ access(all) contract FlowYieldVaultsEVM {
}

/// @notice Marks a request as FAILED
/// @dev Calls completeProcessing to mark the request as failed with the given message
/// @dev Calls completeProcessing to mark the request as failed with the given message.
/// Uses the same refund computation as the normal completion path to keep
/// failed CREATE/DEPOSIT and failed WITHDRAW/CLOSE behavior consistent.
/// @param request The EVM request to mark as failed
/// @param message The error message to include in the result
/// @return True if the request was marked as failed on EVM, false otherwise
Expand All @@ -662,12 +673,17 @@ access(all) contract FlowYieldVaultsEVM {

FlowYieldVaultsEVM.emitRequestFailed(request, message: message)

let refundAmount = FlowYieldVaultsEVM.refundAmountForCompletion(
request: request,
success: false,
)

return self.completeProcessing(
requestId: request.id,
success: false,
yieldVaultId: request.yieldVaultId,
message: message,
refundAmount: request.amount,
refundAmount: refundAmount,
tokenAddress: request.tokenAddress,
requestType: request.requestType,
)
Expand Down Expand Up @@ -1005,14 +1021,15 @@ access(all) contract FlowYieldVaultsEVM {
return nil // success
}

/// @notice Marks a request as COMPLETED or FAILED, returning escrowed funds on failure
/// @dev For failed CREATE/DEPOSIT: returns funds from COA to EVM contract via msg.value (native)
/// or approve+transferFrom (ERC20). For WITHDRAW/CLOSE or success: no refund sent.
/// @notice Marks a request as COMPLETED or FAILED, returning any refund owed on EVM
/// @dev Failed CREATE/DEPOSIT requests return the full escrowed amount.
/// Successful CREATE/DEPOSIT requests may return a precision residual that could not
/// be represented in Cadence. WITHDRAW/CLOSE requests never send a refund.
/// @param requestId The request ID to complete
/// @param success Whether the operation succeeded
/// @param yieldVaultId The associated YieldVault Id
/// @param message Status message or error reason
/// @param refundAmount Amount to refund (0 if no refund needed)
/// @param refundAmount Amount to refund to claimable refunds (0 if no refund needed)
/// @param tokenAddress Token address for refund (used to determine native vs ERC20)
/// @param requestType The type of request (needed to determine if refund applies)
/// @return True if the EVM call succeeded, false otherwise
Expand All @@ -1030,13 +1047,12 @@ access(all) contract FlowYieldVaultsEVM {
let evmYieldVaultId = yieldVaultId ?? UInt64.max

let calldata = EVM.encodeABIWithSignature(
"completeProcessing(uint256,bool,uint64,string)",
[requestId, success, evmYieldVaultId, message]
"completeProcessing(uint256,bool,uint64,string,uint256)",
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Bug (regression): markRequestAsFailed sends the wrong refundAmount for WITHDRAW requests

Switching from the 4-arg to the 5-arg ABI here is correct for processRequest, but markRequestAsFailed (line 679) still passes refundAmount: request.amount unconditionally and is not updated in this PR.

Before this PR, the 4-arg Solidity overload silently ignored refundAmount for non-escrowed types. Now the 5-arg Solidity overload calls _completeProcessing which strictly validates:

uint256 expectedFailedRefundAmount = isEscrowedRequest ? request.amount : 0;
if (refundAmount != expectedFailedRefundAmount) revert InvalidRefundAmount(...);

For a WITHDRAW_FROM_YIELDVAULT request with a non-zero amount:

  • isEscrowedRequest = false → expected = 0
  • Cadence passes request.amount → revert
  • markRequestAsFailed returns false → request permanently stuck in PROCESSING

This breaks both crash-recovery paths in FlowYieldVaultsEVMWorkerOps.cdc (lines 326 and 667).

The fix is to use the helper introduced in this PR in markRequestAsFailed:

refundAmount: FlowYieldVaultsEVM.refundAmountForCompletion(request: request, success: false),

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Breaking ABI change — needs coordinated testnet re-deployment

The Solidity function signature changed from completeProcessing(uint256,bool,uint64,string) to completeProcessing(uint256,bool,uint64,string,uint256). The Cadence calldata here is consistent with the new Solidity signature and the artifact JSON is updated, so the code is internally coherent.

However, any previously deployed testnet instance of FlowYieldVaultsRequests (address 0xF633C9... in CLAUDE.md) must be re-deployed before this Cadence contract is upgraded, otherwise every call to completeProcessing will revert because the 4-arg selector no longer exists. A phased or atomic upgrade is needed.

[requestId, success, evmYieldVaultId, message, refundAmount]
)

// Determine if refund is needed (failed CREATE or DEPOSIT)
let needsRefund = !success
&& refundAmount > 0
// Determine if refund is needed for CREATE/DEPOSIT lifecycle accounting
let needsRefund = refundAmount > 0
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Redundant requestType guard obscures invariant ownership

refundAmountForCompletion() already guarantees refundAmount == 0 for WITHDRAW/CLOSE requests, so the requestType check here is unreachable dead code. This is fine for defence-in-depth, but if the check is ever wrong it masks the real bug: Solidity would still reject the call via InvalidRefundAmount, so a caller cannot use this guard to bypass the accounting check.

Worth documenting explicitly:

// refundAmountForCompletion() already guarantees refundAmount == 0 for WITHDRAW/CLOSE,
// so the requestType guard below is a secondary safety net only.
let needsRefund = refundAmount > 0
    && (requestType == FlowYieldVaultsEVM.RequestType.CREATE_YIELDVAULT.rawValue
        || requestType == FlowYieldVaultsEVM.RequestType.DEPOSIT_TO_YIELDVAULT.rawValue)

Not a bug, but the comment should clarify that refundAmountForCompletion is the primary invariant so future readers don't assume the guard here is load-bearing.

&& (requestType == FlowYieldVaultsEVM.RequestType.CREATE_YIELDVAULT.rawValue
|| requestType == FlowYieldVaultsEVM.RequestType.DEPOSIT_TO_YIELDVAULT.rawValue)
Comment on lines +1059 to 1061
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Redundant requestType guard — refundAmountForCompletion already returns 0 for WITHDRAW/CLOSE

refundAmountForCompletion returns 0 for any non-escrowed request type (WITHDRAW/CLOSE), so refundAmount > 0 can only ever be true for CREATE/DEPOSIT requests. The requestType == CREATE_YIELDVAULT || ... check here is therefore dead code and can cause a reader to believe there is a code path where refundAmount > 0 for WITHDRAW/CLOSE.

Simplify to:

Suggested change
let needsRefund = refundAmount > 0
&& (requestType == FlowYieldVaultsEVM.RequestType.CREATE_YIELDVAULT.rawValue
|| requestType == FlowYieldVaultsEVM.RequestType.DEPOSIT_TO_YIELDVAULT.rawValue)
let needsRefund = refundAmount > 0


Expand All @@ -1047,10 +1063,9 @@ access(all) contract FlowYieldVaultsEVM {
let isNativeFlow = tokenAddress.toString() == FlowYieldVaultsEVM.nativeFlowEVMAddress.toString()

if isNativeFlow {
// Native FLOW: send with the call
// Convert UInt256 to UFix64 then to EVM.Balance
let refundUFix64 = FlowYieldVaultsEVM.ufix64FromUInt256(refundAmount, tokenAddress: tokenAddress)
refundValue = FlowYieldVaultsEVM.balanceFromUFix64(refundUFix64, tokenAddress: tokenAddress)
// Native FLOW: send the original attoflow amount back to EVM without
// round-tripping through UFix64, which would truncate sub-8-decimal precision.
refundValue = EVM.Balance(attoflow: UInt(refundAmount))
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The fix is correct. The old code round-tripped through UFix64 truncation:

// OLD — BROKEN for sub-8-decimal amounts
let refundUFix64 = ufix64FromUInt256(refundAmount, ...)  // truncates to 8 decimals
refundValue = balanceFromUFix64(refundUFix64, ...)        // encodes truncated amount

Because FlowYieldVaultsRequests.sol enforces msg.value == request.amount exactly:

if (msg.value != request.amount) revert MsgValueMustEqualAmount();

…any request.amount with sub-8-decimal precision would cause the EVM call to revert. completeProcessing returning false triggers a panic in processRequest, and the request would be permanently stuck in PROCESSING.

The new EVM.Balance(attoflow: UInt(refundAmount)) sends the original wei amount without precision loss, which is the right fix. UInt in Cadence is arbitrary-precision, so the cast is safe for any deposit size.

One gap: the Solidity tests added in this PR call completeProcessing directly as the COA and verify the Solidity-side accounting. They don't exercise the Cadence code path (completeProcessingEVM.Balance(attoflow: UInt(refundAmount))). Consider adding a Cadence error_handling_test scenario with a sub-8-decimal amount (e.g. 1_000_000_000_123_456_789 attoflow) that triggers a failure, and asserting the claimable refund matches the exact input amount.

} else {
// ERC20: approve contract to pull funds
let approveCalldata = EVM.encodeABIWithSignature(
Expand Down Expand Up @@ -1929,16 +1944,59 @@ access(all) contract FlowYieldVaultsEVM {
/// @notice Converts a UInt256 amount from EVM to UFix64 for Cadence
/// @dev For native FLOW: Uses 18 decimals (attoflow to FLOW conversion)
/// For ERC20: Uses FlowEVMBridgeUtils to look up token decimals
/// PRECISION WARNING: UFix64 has only 8 decimal places vs uint256's 18.
/// Precision beyond 8 decimals is truncated and must be reconciled on the EVM side.
/// @param value The amount in wei/smallest unit (UInt256)
/// @param tokenAddress The token address to determine decimal conversion
/// @return The converted amount in UFix64 format
/// @return The converted amount in UFix64 format (truncated to 8 decimals)
access(self) fun ufix64FromUInt256(_ value: UInt256, tokenAddress: EVM.EVMAddress): UFix64 {
if tokenAddress.toString() == FlowYieldVaultsEVM.nativeFlowEVMAddress.toString() {
return FlowEVMBridgeUtils.uint256ToUFix64(value: value, decimals: 18)
}
return FlowEVMBridgeUtils.convertERC20AmountToCadenceAmount(value, erc20Address: tokenAddress)
}

/// @notice Computes the exact EVM-side amount that survives a round trip through Cadence precision
/// @dev This floors the amount to the nearest quantity representable by Cadence UFix64.
/// @param value The requested amount in wei/smallest unit
/// @param tokenAddress The token address to determine decimal conversion
/// @return The EVM-side amount that can be processed in Cadence without losing precision
access(self) fun exactCadenceRepresentableAmount(_ value: UInt256, tokenAddress: EVM.EVMAddress): UInt256 {
let cadenceAmount = FlowYieldVaultsEVM.ufix64FromUInt256(value, tokenAddress: tokenAddress)
return FlowYieldVaultsEVM.uint256FromUFix64(cadenceAmount, tokenAddress: tokenAddress)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Risk: divergence between Cadence and Solidity precision residual computations

exactCadenceRepresentableAmount uses FlowEVMBridgeUtils (Cadence on-chain library) to round-trip the value. The Solidity mirror, _expectedPrecisionResidual, uses hardcoded arithmetic:

// native FLOW
return amount % 1e10;
// ERC20
uint8 decimals = IERC20Metadata(tokenAddress).decimals();
uint256 quantum = 10 ** (decimals - 8);
return amount % quantum;

If these two code paths ever return different values for the same input — e.g. because FlowEVMBridgeUtils.convertERC20AmountToCadenceAmount uses a different decimal source than IERC20Metadata.decimals() for some token — then completeProcessing will always revert with InvalidRefundAmount, permanently bricking every request for that token.

Recommendations:

  1. Add a test that cross-checks the two computations for at least one ERC20 with 18 decimals and one with 6 decimals.
  2. Consider adding a view function to the Solidity contract that exposes the expected residual so the Cadence worker can read it on-chain rather than recomputing it independently.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cross-chain residual invariant: implicit coupling between Cadence and Solidity arithmetic

exactCadenceRepresentableAmount computes the residual via a FlowEVMBridgeUtils round-trip, while Solidity's _expectedPrecisionResidual computes it independently with amount % (10 ** (decimals - 8)). completeProcessing() now rejects any call where these two values differ by even 1 wei with InvalidRefundAmount, permanently bricking the request in PROCESSING state.

For native FLOW both sides reduce to amount % 1e10, which is provably identical. For ERC20, the equivalence depends on FlowEVMBridgeUtils.convertERC20AmountToCadenceAmount / convertCadenceAmountToERC20Amount implementing exact floor division by 10^(decimals-8) — the same arithmetic Solidity uses. If those utils ever round instead of truncate, or use a different decimal lookup, the two computations diverge and all ERC20 requests with a non-zero residual are permanently stuck.

The integration test only exercises MOET (18 decimals, same math as native FLOW). Consider adding a test case with an ERC20 of non-18 decimals (e.g. 6 or 17) to confirm the round-trip produces the same residual as the Solidity formula, or add an explicit comment cross-referencing the bridge utils contract version this was validated against.

}

/// @notice Computes the refund amount that must be returned on EVM completion
/// @dev Failed CREATE/DEPOSIT requests refund the full amount. Successful CREATE/DEPOSIT
/// requests refund only the precision residual that could not be represented in Cadence.
/// @param request The request being completed
/// @param success Whether the Cadence operation succeeded
/// @return Amount to credit to claimable refunds on the Solidity side
access(self) fun refundAmountForCompletion(request: EVMRequest, success: Bool): UInt256 {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

No Cadence unit tests for refundAmountForCompletion

This function is the critical path that determines how much is sent to completeProcessing for every request completion. It has three distinct branches (non-escrowed, failed escrowed, successful escrowed with residual) and the ERC20 residual branch adds a round-trip through exactCadenceRepresentableAmount.

The new behaviour is covered by integration tests (run_worker_tests.sh) and Solidity unit tests, but there are no Cadence-level unit tests in cadence/tests/. Given that a wrong return value here either strands funds in the COA (residual not refunded) or bricks requests (wrong refund causes InvalidRefundAmount revert on the Solidity side), at minimum error_handling_test.cdc and evm_bridge_lifecycle_test.cdc should exercise the residual paths.

let isEscrowedRequest =
request.requestType == FlowYieldVaultsEVM.RequestType.CREATE_YIELDVAULT.rawValue
|| request.requestType == FlowYieldVaultsEVM.RequestType.DEPOSIT_TO_YIELDVAULT.rawValue

if !isEscrowedRequest {
return 0
}

if !success {
return request.amount
}

let exactProcessableAmount = FlowYieldVaultsEVM.exactCadenceRepresentableAmount(
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

ERC20 residual: round-trip through FlowEVMBridgeUtils must match Solidity's amount % 10^(decimals-8)

exactCadenceRepresentableAmount performs convertERC20AmountToCadenceAmountconvertCadenceAmountToERC20Amount, which is the Cadence-side equivalent of amount - (amount % 10^(decimals-8)). The Solidity side independently reimplements the same formula in _expectedPrecisionResidual using a live call to IERC20Metadata.decimals().

These two code paths must produce identical residuals or completeProcessing will revert with InvalidRefundAmount for every ERC20 CREATE/DEPOSIT success completion. The test suite verifies the Solidity half in isolation (test_CompleteProcessing_SuccessCreditsERC20ResidualRefund) but there is no Cadence-level or worker-level integration test for ERC20 dust amounts. An integration test (Cadence or shell worker test) that sends amount = N * 10^(decimals-8) + dust through a full lifecycle would pin this cross-system invariant.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

exactCadenceRepresentableAmount double-converts through FlowEVMBridgeUtils — decimal mismatch risk

The round-trip ufix64FromUInt256 → uint256FromUFix64 relies on FlowEVMBridgeUtils.convertERC20AmountToCadenceAmount / convertCadenceAmountToERC20Amount to determine how many decimal places the token has. The Solidity side uses IERC20Metadata(tokenAddress).decimals() for the same purpose.

These two sources of truth can diverge:

  • If the FlowEVMBridge registry stores a decimal count that differs from what the ERC20 token's decimals() returns (e.g., after a token upgrade or initial mis-registration), the residuals computed by refundAmountForCompletion (Cadence) and _expectedPrecisionResidual (Solidity) will differ, causing completeProcessing to revert on Solidity.

Consider adding an assertion here (or in tests) that the bridge-registry decimal count equals the ERC20 decimals() result for every configured token, and document that this invariant must hold for the precision accounting to work correctly.

request.amount,
tokenAddress: request.tokenAddress
)

if request.amount > exactProcessableAmount {
return request.amount - exactProcessableAmount
}

return 0
}

/// @notice Converts a UFix64 amount from Cadence to UInt256 for EVM
/// @dev For native FLOW: Uses 18 decimals (FLOW to attoflow conversion)
/// For ERC20: Uses FlowEVMBridgeUtils to look up token decimals
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

No Cadence unit tests exist for exactCadenceRepresentableAmount or refundAmountForCompletion. The integration path in run_worker_tests.sh covers the happy path with a 18-decimal native FLOW and a MOET ERC20 token, but the following cases are not tested at the unit level:

  • Token with ≤ 8 decimals (e.g. USDC at 6 decimal) — should return 0 residual.
  • Amount that is already exactly quantum-aligned — should return 0 residual, and a success-path completeProcessing with refundAmount = 0 must not revert.
  • Token with decimals > 85 (the precisionLossDecimals > 77 guard on the Solidity side, where the entire amount is residual).
  • WITHDRAW_FROM_YIELDVAULT / CLOSE_YIELDVAULT requests — refundAmountForCompletion must always return 0 regardless of success/failure.

Adding a Cadence test file (e.g. cadence/tests/precision_residual_test.cdc) that covers these cases would protect against silent regressions when FlowEVMBridgeUtils conversion behaviour changes.

Expand Down
20 changes: 12 additions & 8 deletions cadence/contracts/FlowYieldVaultsEVMWorkerOps.cdc
Original file line number Diff line number Diff line change
Expand Up @@ -146,10 +146,10 @@ access(all) contract FlowYieldVaultsEVMWorkerOps {
nextRunCapacity: UInt8?,
)

/// @notice Emitted when a WorkerHandler has panicked and SchedulerHandler has marked the request as FAILED
/// @notice Emitted when SchedulerHandler detects a failed WorkerHandler execution
/// @param status The status of the transaction (Unknown, Scheduled, Executed, Canceled)
/// @param markedAsFailed Whether the request was marked as FAILED
/// @param request The request that was marked as FAILED
/// @param markedAsFailed Whether the request was successfully marked as FAILED on EVM
/// @param request The tracked request being recovered
access(all) event WorkerHandlerPanicDetected(
status: UInt8?,
markedAsFailed: Bool,
Expand All @@ -171,6 +171,7 @@ access(all) contract FlowYieldVaultsEVMWorkerOps {
)

/// @notice Emitted when stopAll() cannot mark a cancelled request as FAILED
/// @dev The request remains tracked in scheduledRequests so a later recovery attempt can retry finalization.
/// @param requestId EVM request ID that could not be marked as FAILED
/// @param workerTransactionId Cancelled WorkerHandler transaction ID
access(all) event StopAllMarkFailedSkipped(
Expand Down Expand Up @@ -332,6 +333,7 @@ access(all) contract FlowYieldVaultsEVMWorkerOps {
requestId: scheduledRequestId,
workerTransactionId: request.workerTransactionId,
)
continue
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

continue with no retry bound — zombie entries accumulate in scheduledRequests

When markRequestAsFailed returns false (e.g. the EVM call reverts), the continue keeps the entry in scheduledRequests. _checkForFailedWorkerRequests will retry it on every subsequent scheduler run. If the underlying cause is persistent (e.g. a contract state invariant violated by the new InvalidRefundAmount check), the entry is never removed and the scheduler iterates over it forever.

The old behaviour silently dropped the entry, which was also wrong. The fix is directionally correct, but without a bounded retry count or a tombstone mechanism for definitively unrecoverable requests, these zombie entries impose growing per-run overhead. Consider emitting a more distinct event after N consecutive failures, or storing a retry counter alongside the tracked request.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Good fix — continue was genuinely missing here

Before this PR, the StopAllMarkFailedSkipped path fell through and unconditionally executed scheduledRequests.remove(key: scheduledRequestId) on line 339. That meant a request could be dropped from tracking even when markRequestAsFailed returned false — leaving it stuck in PROCESSING with no recovery. The continue correctly keeps the request in scheduledRequests so the next scheduler run can retry finalisation.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The continue fix is correct, but the failure path it protects is not regression-tested. Scenario 7 only exercises the path where markRequestAsFailed returns true (EVM completeProcessing succeeds). There is no test that forces completeProcessing to revert inside stopAll() and then verifies:

  1. The StopAllMarkFailedSkipped event is emitted.
  2. The request remains in scheduledRequests (not dropped).
  3. A subsequent scheduler run retries finalization and succeeds.

Without this test, a future refactor that accidentally removes the continue would not be caught. An integration-level test that stubs or temporarily breaks the EVM contract during stopAll() and then restores it would be the most direct regression coverage.

}

FlowYieldVaultsEVMWorkerOps.scheduledRequests.remove(key: scheduledRequestId)
Expand Down Expand Up @@ -583,7 +585,8 @@ access(all) contract FlowYieldVaultsEVMWorkerOps {
/// @notice Main scheduler logic
/// @dev Flow:
/// 1. Check for failed worker requests
/// - If a failure is identified, mark the request as failed and remove it from scheduledRequests
/// - If a failure is identified, attempt to mark the request as FAILED
/// - Remove it from scheduledRequests only after EVM finalization succeeds
/// 2. If fetchCount > 0, fetch pending requests from EVM
/// 3. Preprocess requests to drop invalid requests
/// 4. Start processing requests (PENDING -> PROCESSING)
Expand Down Expand Up @@ -631,7 +634,7 @@ access(all) contract FlowYieldVaultsEVMWorkerOps {
/// - Only acceptable transaction status is Scheduled (pending execution)
/// - No status is considered not acceptable because it means the manager cleaned up the request
/// 4. If the transaction status is invalid, mark the request as FAILED providing the transaction ID
/// 5. Remove the request from scheduledRequests
/// 5. Remove the request from scheduledRequests only after it has been marked as FAILED on EVM
/// @param manager The scheduler manager
/// @param worker The worker capability
access(self) fun _checkForFailedWorkerRequests(
Expand Down Expand Up @@ -669,9 +672,10 @@ access(all) contract FlowYieldVaultsEVMWorkerOps {
message: "Worker transaction did not execute successfully. Transaction ID: \(txId.toString())",
)

// Remove request from scheduledRequests
// Success is not checked because errors are not considered transient
FlowYieldVaultsEVMWorkerOps.scheduledRequests.remove(key: requestId)
// Keep the request tracked if EVM finalization fails so crash recovery can retry later.
if success {
FlowYieldVaultsEVMWorkerOps.scheduledRequests.remove(key: requestId)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

No retry limit — could leave requests stuck indefinitely

If markRequestAsFailed returns false (e.g., COA has insufficient native FLOW to send the refund, or the EVM call reverts for any persistent reason), the request stays in scheduledRequests and will be retried on every subsequent scheduler run with no back-off, counter, or escalation path beyond the WorkerHandlerPanicDetected event.

In practice the most plausible trigger is an underfunded COA: if the COA has zero balance it can never satisfy the msg.value == refundAmount check on the Solidity side for a native-FLOW failed CREATE/DEPOSIT, so the request is permanently unfinalisable.

Worth either: (a) documenting the invariant that the COA must maintain a reserve sufficient to cover refunds, or (b) adding a max-retry counter after which the request is dropped from scheduledRequests and an operator alert is emitted.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Unbounded retry burns scheduler capacity on every run

When markRequestFailed returns false (EVM call fails), the request stays in scheduledRequests indefinitely. On every subsequent scheduler run the same entry is re-checked, the EVM call fails again, and the slot is consumed but nothing progresses. With multiple such entries the effective scheduler capacity shrinks permanently.

The comment acknowledges this is unbounded. Consider adding a retry counter to ScheduledRequest and after N failures emitting a distinct MarkFailedExhausted event (or switching to a separate "needs-admin-review" set), so monitoring can surface the stuck entry without indefinitely degrading throughput.

}

emit WorkerHandlerPanicDetected(
status: txStatus?.rawValue,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
import "FlowTransactionScheduler"
import "FlowYieldVaultsEVMWorkerOps"

/// @title Run Scheduler Manually With Capacity
/// @notice Runs the scheduler manually with an explicit run capacity
/// @dev Test-only helper for forcing preprocessing/scheduling without waiting for recurrent runs
/// @param runCapacity The number of pending requests this manual run is allowed to preprocess
transaction(runCapacity: UInt8) {
let schedulerHandlerCap: Capability<auth(FlowTransactionScheduler.Execute) &{FlowTransactionScheduler.TransactionHandler}>

prepare(signer: auth(IssueStorageCapabilityController) &Account) {
self.schedulerHandlerCap = signer.capabilities.storage
.issue<auth(FlowTransactionScheduler.Execute) &{FlowTransactionScheduler.TransactionHandler}>(
FlowYieldVaultsEVMWorkerOps.SchedulerHandlerStoragePath
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

issue creates a new unrevoked capability controller on every invocation. Running this transaction repeatedly (as happens across test scenarios) accumulates orphaned controllers on the account.

Consider borrowing the existing controller if one exists, or revoking it before issuing a new one. A common pattern is:

let existing = signer.capabilities.storage.getControllers(forPath: FlowYieldVaultsEVMWorkerOps.SchedulerHandlerStoragePath)
if existing.length > 0 {
    self.schedulerHandlerCap = existing[0].capability as! Capability<auth(FlowTransactionScheduler.Execute) &{FlowTransactionScheduler.TransactionHandler}>
} else {
    self.schedulerHandlerCap = signer.capabilities.storage.issue<...>(...)
}

For a test-only helper this won't cause test failures, but it's an antipattern that will make debugging account state harder over time.

)
}

execute {
self.schedulerHandlerCap.borrow()!.executeTransaction(
id: 42,
data: runCapacity
)
}
}
21 changes: 21 additions & 0 deletions deployments/artifacts/FlowYieldVaultsRequests.json
Original file line number Diff line number Diff line change
Expand Up @@ -315,6 +315,11 @@
"name": "message",
"type": "string",
"internalType": "string"
},
{
"name": "refundAmount",
"type": "uint256",
"internalType": "uint256"
}
],
"outputs": [],
Expand Down Expand Up @@ -1998,6 +2003,22 @@
"name": "InvalidCOAAddress",
"inputs": []
},
{
"type": "error",
"name": "InvalidRefundAmount",
"inputs": [
{
"name": "expectedOrMaximum",
"type": "uint256",
"internalType": "uint256"
},
{
"name": "actual",
"type": "uint256",
"internalType": "uint256"
}
]
},
{
"type": "error",
"name": "InvalidRequestState",
Expand Down
Loading
Loading