S1 Inconsistent Error Handling Patterns Across Connectors

[nialexsan]

File(s) affected:

• cadence/contracts/connectors/evm/ERC4626SinkConnectors.cdc
• cadence/contracts/connectors/evm/EVMTokenConnectors.cdc
• cadence/contracts/connectors/evm/UniswapV3SwapConnectors.cdc
• cadence/contracts/connectors/evm/ERC4626SwapConnectors.cdc

Description: The code-base uses inconsistent error handling strategies:

Panic on error:
ERC4626SinkConnectors.depositCapacity() panics when deposit fails

Silent early return:
EVMTokenConnectors.Sink.depositCapacity() returns early without error when fees are insufficient

Return nil/0.0:
Quote functions return zero values to indicate issues

This inconsistency makes it difficult for clients to:

• Predict behavior across different connectors
• Handle errors appropriately
• Debug issues when things go wrong.

Recommendation:
Establish and document consistent error handling patterns across connectors.

[m-Peter]

Regarding the inconsistent error handling between the two implementations of DeFiActions.Sink (that is ERC4626SinkConnectors & EVMTokenConnectors). The doc section on DeFiActions.Sink states:

/// Sink
///
/// A Sink Connector (or just “Sink”) is analogous to the Fungible Token Receiver interface that accepts deposits of
/// funds. It differs from the standard Receiver interface in that it is a struct interface (instead of resource
/// interface) and allows for the graceful handling of Sinks that have a limited capacity on the amount they can
/// accept for deposit. Implementations should therefore avoid the possibility of reversion with graceful fallback
/// on unexpected conditions, executing no-ops instead of reverting.
///

The documentation already describes the desired error handling: executing no-ops instead of reverting. The only reason that ERC4626SinkConnectors panics, is in order to undo the effect of a from.withdraw(amount: amount) FungibleToken Vault withdrawal and an approve(address,uint256) ERC-20 call. For the method's implementation, the panic is a good safety measure to ensure the logic is atomic. If the funds are withdrawn from the given vault, then they have to be deposited to the destination address, so both steps have to be successful, or both have to fail. It is good practice to also avoid the silent early return on EVMTokenConnectors.Sink.depositCapacity(), as it doesn't surface the underlying issue which causes the method to stop functioning properly, due to low fees.

[m-Peter]

Regarding the Return nil/0.0 pattern used on the two implementations of DeFiActions.Swapper (that is UniswapV3SwapConnectors & ERC4626SwapConnectors). Both quote functions:

• quoteIn(forDesired: UFix64, reverse: Bool): {Quote}
• quoteOut(forProvided: UFix64, reverse: Bool): {Quote}

have to return a struct that adheres to the Quote interface. The documentation already describes a convention there:

/// Quote
///
/// An interface for an estimate to be returned by a Swapper when asking for a swap estimate. This may be helpful
/// for passing additional parameters to a Swapper relevant to the use case. Implementations may choose to add
/// fields relevant to their Swapper implementation and downcast in swap() and/or swapBack() scope.
/// By convention, a Quote with inAmount==outAmount==0 indicates no estimated swap price is available.
///

The convention for using 0.0 as values in inAmount & outAmount is to denote that no estimated swap price is available. Given that Cadence doesn't have a try/catch construct, the usage of panic aborts the transaction, without giving the caller a chance to handle/recover. So it makes sense to return an optional value in some cases, such as UFix64?, which means that it can also be nil. This allows the caller to handle certain cases more gracefully.

[m-Peter]

Fixed in commit 2fb3c4c

Updated the Sink connector documentation, on favoring liveness and avoid panics, unless it would cause an inconsistent state.