Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve error handling on getTradeableOrderWithSignature CALL simulation #46

Merged
merged 5 commits into from
Aug 19, 2023

Conversation

anxolin
Copy link
Contributor

@anxolin anxolin commented Aug 18, 2023

Most of the erros happen during getTradeableOrderWithSignature CALL simulation.

There's many things that can go wrong here.

This error tries to make it more explicit the error handling, so it signals igiven the error, we should re-attempt later. Also, if this is expected or just an un-expeted errors.

Expected errors won't lead to the TENDERLY action to fail.
Un-expected will.

If the error handler decides that we shouldn't re-attempt later (its a final error), then it will signal that the order should be deleted.

Special mention

I changed slightly the implementation, @mfw78 pls double check if it makes sense now.

Before, we were deleting the order in case of unforeseen or un-recognised errors. This could be for example because of a temporal network failure or rate limiting or sth, so we should definitely re-attempt later.

see https://github.com/cowprotocol/composable-cow/pull/46/files#diff-8e2dc90f4a6ebdf01f8603707f37f7794972fdf1827102bfce89497804f72837R398

Edit

Analysing this case, i changed this PR to make less noise when

This order was created, and there was an AUTH, but it was revoked after doing a cancelation.

Its fine to delete the order, and I don't think we should make so much noise in these cases: 33e80fa

@anxolin anxolin force-pushed the dont-fail-if-simulation-fails branch from 6f0b6f2 to 0579d48 Compare August 18, 2023 09:37
@anxolin anxolin changed the base branch from refactor-actions to deploy-to-dev-and-prod August 18, 2023 09:37
@anxolin anxolin marked this pull request as ready for review August 18, 2023 11:27
@anxolin anxolin changed the title Don't fail if simulation fails Improve error handling on getTradeableOrderWithSignature CALL simulation Aug 18, 2023
@anxolin anxolin requested review from mfw78 and a team August 18, 2023 11:30
mfw78
mfw78 previously approved these changes Aug 18, 2023
Copy link
Contributor

@mfw78 mfw78 left a comment

Choose a reason for hiding this comment

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

Approve with a suggestion of how one can filter the contracts to remove those that don't comply (ie. don't implement getTradeableOrderWithSignature).

actions/checkForAndPlaceOrder.ts Outdated Show resolved Hide resolved
actions/checkForAndPlaceOrder.ts Outdated Show resolved Hide resolved
actions/checkForAndPlaceOrder.ts Outdated Show resolved Hide resolved
actions/checkForAndPlaceOrder.ts Outdated Show resolved Hide resolved
actions/checkForAndPlaceOrder.ts Show resolved Hide resolved
actions/checkForAndPlaceOrder.ts Outdated Show resolved Hide resolved
actions/checkForAndPlaceOrder.ts Outdated Show resolved Hide resolved
actions/checkForAndPlaceOrder.ts Outdated Show resolved Hide resolved
actions/checkForAndPlaceOrder.ts Outdated Show resolved Hide resolved
actions/checkForAndPlaceOrder.ts Show resolved Hide resolved
@anxolin anxolin force-pushed the dont-fail-if-simulation-fails branch from bc3d451 to c2901f9 Compare August 19, 2023 21:01
@anxolin anxolin changed the base branch from deploy-to-dev-and-prod to main August 19, 2023 21:01
@anxolin anxolin dismissed mfw78’s stale review August 19, 2023 21:01

The base branch was changed.

@anxolin
Copy link
Contributor Author

anxolin commented Aug 19, 2023

Addressed the comments and merging.

Pls let me know if you need to re-iterate again, i will follow up in another PR.

@anxolin anxolin merged commit eb922e0 into main Aug 19, 2023
2 checks passed
@alfetopito alfetopito deleted the dont-fail-if-simulation-fails branch August 21, 2023 09:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants