-
Notifications
You must be signed in to change notification settings - Fork 17
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
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
anxolin
force-pushed
the
dont-fail-if-simulation-fails
branch
from
August 18, 2023 09:37
6f0b6f2
to
0579d48
Compare
anxolin
changed the base branch from
refactor-actions
to
deploy-to-dev-and-prod
August 18, 2023 09:37
anxolin
changed the title
Don't fail if simulation fails
Improve error handling on getTradeableOrderWithSignature CALL simulation
Aug 18, 2023
mfw78
previously approved these changes
Aug 18, 2023
There was a problem hiding this 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
).
anxolin
force-pushed
the
deploy-to-dev-and-prod
branch
from
August 19, 2023 20:30
8829428
to
cd62a45
Compare
anxolin
force-pushed
the
dont-fail-if-simulation-fails
branch
from
August 19, 2023 20:32
a70c8f9
to
1d3ccc4
Compare
anxolin
force-pushed
the
dont-fail-if-simulation-fails
branch
from
August 19, 2023 21:01
bc3d451
to
c2901f9
Compare
Addressed the comments and merging. Pls let me know if you need to re-iterate again, i will follow up in another PR. |
17 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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