Refactor handler errors to try on next block watch tower pattern

[yvesfracari]

Description

Refactor Stop Loss, Trade Above Threshold, and Good After Time errors to watch tower try again pattern.

Changes

• Add revertPollAtNextBlockmethod on ConditionalOrdersUtilsLib
• Stop Loss Refactoring
• Trade Above Threshold Refactoring
• Good After Time Refactoring

How to test

1. Run modified test files
2. Stop Loss Deployment on sepolia: 0x410c267baf50b36475B30051Da2aE734f6726F01

Related Issues

• Fix chore: Interface with errors supported by the watchtower #85

[github-actions]

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

[yvesfracari]

I have read the CLA Document and I hereby sign the CLA

[mfw78]

In general, I'd suggest to include all the watch-tower errors that are possible. I'd already done some work in develop on this. Refer:

composable-cow/src/interfaces/IConditionalOrder.sol

Lines 17 to 25 in 24ce6fb

	// --- errors specific for polling
	// Signal to a watch tower that polling should be attempted again.
	error PollTryNextBlock(string reason);
	// Signal to a watch tower that polling should be attempted again at a specific block number.
	error PollTryAtBlock(uint256 blockNumber, string reason);
	// Signal to a watch tower that polling should be attempted again at a specific epoch (unix timestamp).
	error PollTryAtEpoch(uint256 timestamp, string reason);
	// Signal to a watch tower that the conditional order should not be polled again (delete).
	error PollNever(string reason);

Instead of introducing this within IConditionalOrder as I did, having a separate interface, as you have done, for the watch-tower custom errors is fine.

[mfw78]

In general looks OK. I've left some comments about IwatchtowerCustomErrors in the main PR comments. FYI, there is a error PollTryNextBlock(string reason) that is supported by the watch-tower. This allows for simpler code as it was identified that trying at the very next block is a common pattern and alleviates a lot of block.number + 1.

[mfw78]

I'd generally discourage the use of revert utilities, such as this, to reduce the layers of indirection required when reviewing the logic within the smart contract to know where I can expect a revert (sure, it's in the name, but it's another function that I then need to drill into to triple check that it indeed does revert). Depending on the optimiser as well and how this gets compiled, this may also result in additional gas consumption.

[yvesfracari]

Thanks for review @mfw78 . I refactor the contracts to address them. Let me know if we need more on the dev side. Also, once you think it's ready for deployment, let me know what should be the process (if we should do it or not).

[mfw78]

LGTM

[ribeirojose]

rebased onto #88 @mfw78

[yvesfracari]

@mfw78 since we're planning to launch the stop-loss app soon, I think that we should have this PR merged. Let me know if you need more things on our side. All the changed handlers contracts will need to be redeployed, I was assuming that you guys will do it since you have a wallet to do this process, right?

[fedgiac]

The changes look good to me with the exception of the comment below. Once clarified it's good for merging.

[fedgiac]

Nice!