fix(S1): Inconsistent Error Handling Patterns Across Connectors

[m-Peter]

Closes: #111

---

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.

---

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.

[jordanschalm]

I understand why we want to revert in ERC4626SinkConnectors, to revert the approve that occurs beforehand. But why can we not no-op instead of panicking here?

[m-Peter]

I understand why we want to revert in ERC4626SinkConnectors, to revert the approve that occurs beforehand.

The allowance approval is easy to reset, with an extra EVM call. The main reason of the revert, is to avoid bridging back the already withdrawn amount:

let deposit <- from.withdraw(amount: amount)

But why can we not no-op instead of panicking here?

We can no-op here, but that would silently hide the fact that self.feeSource, which is responsible for paying bridging fees, has in fact run out of funds. I believe that this is something worth surfacing as a panic, to the holder of the EVMTokenConnectors.Sink struct. Overall, no strong opinion.

And to be honest, the audit suggestion isn't really actionable. It's unlikely to maintain a consistent error handling pattern across the different connectors, because they interact with different components. To top it up, Cadence doesn't have a try/catch construct, so the error handling patter is mostly on a per-case, depending on the functions' logic.

[jordanschalm]

silently hide the fact that self.feeSource, which is responsible for paying bridging fees, has in fact run out of funds

We have a trade-off to make. To steal some consensus terminology that rhymes, we can prioritize "liveness" and not panic, even when we encounter unexpected conditions, or we can prioritize "safety" and panic if we encounter unexpected conditions.

I believe that this is something worth surfacing as a panic, to the holder of the EVMTokenConnectors.Sink struct.

This is fair, but I think that warrants changing the documentation for Sink. Right now the Sink documentation says "always make the trade-off in favour of liveness". In this implementation we are explicitly not doing that. If we want to pick and choose the trade-off depending on the circumstances, or always make the other trade-off, we should change the Sink interface documentation to reflect that.

[zhangchiqing]

Q:But why can we not no-op instead of panicking here?

A:We can no-op here, but that would silently hide the fact that self.feeSource, which is responsible for paying bridging fees, has in fact run out of funds.

Would be great to include this to the comments.

[m-Peter]

Updated Sink documentation and included the above comment in 09e2114 .

[zhangchiqing]

Q:But why can we not no-op instead of panicking here?

A:We can no-op here, but that would silently hide the fact that self.feeSource, which is responsible for paying bridging fees, has in fact run out of funds.

Would be great to include this to the comments.

[jordanschalm]

From EVMTokenConnectors:

// We could return early here, but that would silently hide the fact
// that self.feeSource, which is responsible for paying bridging fees,
// has in fact run out of funds.

From the Sink interface doc:

favor graceful fallback on unmet conditions, such as zero capacity

These still seem inconsistent to me.

• The sink says " zero capacity is an example of a case where we should favour graceful fallback and not panic"
• EVMTokenConnectors says "we could gracefully fallback here, but that would hide the fact that we have zero capacity in the fee source"

Any errors that hinder liveness, can be surfaced for visibility.

I would frame it like this:

• A sink should prioritize liveness where possible, and not panic, for example if it has no funds or cannot access funds
• A sink should only panic if not panicking would cause the Sink to have an inconsistent internal state (unsafe or undefined for the Sink to continue in this state).

If something happens outside the Sink's control that would hinder liveness, then we have no choice but to panic. But in EVMTokenConnectors, we are choosing to panic, because we can't pay fees. I think an empty fee vault can be considered a valid state for the Sink, similar to any Sink which cannot accept funds, for any reason. In the context of how Sink's are used, where they are used abstractly without the user knowing about the implementation details, I think prioritizing non-reverting behaviour wherever possible makes sense.

[m-Peter]

Sounds fair, updated in b78efd6 .