fix: drop incompatible orders

[mfw78]

Description

Currently incompatible orders (where there is a low level reversion that's not expected) are NOT deleted. This corrects that.

Changes

• Drop orders that return non-compatible interface revert messages.

How to test

1. Sync goerli from genesis and confirm no unexpected errors.

Related Issues

Fixes #91

[anxolin]

I don't think this is a good idea. catching ANY error thrown in the block above and just stop indexing the order might be a source of bugs.

Can't we handle it in a different way?

Maybe could be usefult to extract the error handling to a testable function and create some tests. This way, we can make sure we capture the cases you want to capture and there's no future regressions.

Also, by making the error handling independent, it will make checkForAndPlaceOrders thinner, which would be a good thing

Alternativelly, we could move the whole legacy polling and do the same. This in fact is something that is good is independent, since it should be moved to the SDK. Watch Tower logic should be simpler

[mfw78]

The only code path here (yes, it should be tested) that should throw and result in invalidating the order, is customErrorDecode that solely depends on only ABI decoding. All other errors (ethers-js / RPC provider etc), should be routed to the subsequent PollResultCode.TRY_NEXT_BLOCK).

We need to kill these orders from the registry as they will always fail, and they haven't complied with the interface. I'm all for refactoring / moving things after we have observability and stability in prod - including not spamming with error logs in some chains because of incompatible interfaces that are from the initial development versions of ComposableCoW on Goerli.

So, our risks are primarily around killing orders from monitoring that shouldn't be killed.

Therefore I propose we offset this with:

1. Comprehensive logging. We have ELK in place, and much work has been done with logging to ensure great visibility.
2. Comprehensive metrics. In chore: metrics todo and naming #95 the order tuple (chainId, handler, owner, id) is labelled against the watch_tower_polling_onchain_invalid_interface_total metric. Therefore we can see when orders are getting dropped due to this, and investigate to ensure compliance.
3. We do some unit testing - test: unit coverage for poll legacy #97 - and validate while in prod.

[anxolin]

The only code path here (yes, it should be tested) that should throw and result in invalidating the order, is customErrorDecode that solely depends on only ABI decoding

I would hope we structure it so typescript already can check this assumption

Comprehensive logging. We have ELK in place, and much work has been done with logging to ensure great visibility.
Comprehensive metrics. In #95 the order tuple (chainId, handler, owner, id) is labelled against the watch_tower_polling_onchain_invalid_interface_total metric. Therefore we can see when orders are getting dropped due to this, and investigate to ensure compliance.

I think these don't solve the problem. I think it should be impossible to remove an order if there's some other error that is not the decoding. Typescript can probably help with these at compile time

We do some unit testing - #97 - and validate while in prod.

This will help indeed

[mfw78]

I've gone through and implemented everything with very strong typing now to enforce the assumptions. Some tests have been added as well.

[anxolin]

Looks better, but you are still not exhaustive. You can't be. Because you use parsedCustomError.selector in the switch, you can't know all selectors. You need to handle an unknown selector, but your code now assumes its impossible that will happen

[mfw78]

The revert data is parsed in parseCustomError to ensure that it complies with the ComposableCoW interface. If it does not, it will throw. Therefore the switch is exhaustive as only valid selectors reach that point in the execution path.

Invalid selectors (ie. that do not comply with the interface that we have prescribed), are caught in handleOnChainCustomError from the parseCustomError throw, dropped, and metrics are in place to monitor this.

[alfetopito]

🤯

[anxolin]

Agree, this feels too complicated. What is this type needed for?

Why not the function just returns a PollResultErrors so you also simplify the othe switch.

[mfw78]

This type covers the exact interface for all ComposableCoW-compatible orders (as returned from ABI on-chain). We control this interface, and therefore we can assert that only these revert types are valid. If an order fails to adhere to this interface, it is to be dropped.

Typechain does not do generation of typings for solidity custom error reverts (despite them being in the ABI), and therefore, we've basically had to implement this ourselves, which is essentially what this type is.

[anxolin]

do we really need this hard casting of types?

[mfw78]

I moved the explicit hard casting to be on the same line as the decode. The hard casting is required because when ABI decoding in ethers v5, the return type is Result, whose interface is:

export interface Result extends ReadonlyArray<any> {
    readonly [key: string]: any;
}

IMO we should always hard cast from a decode if the decode is returning Result, as this increases the readability of the code, and enforces that Typescript catches more potential issues.