Revert error should not be retried - #254
Conversation
mcmire
left a comment
mcmire
left a comment
There was a problem hiding this comment.
This change makes sense. I also checked the other middleware to make sure there wasn't any other place we needed to do this. The fetch middleware also retries requests, but has an allowlist of retriable errors, and this revert error isn't one of them, so we're good on that (these retriable errors, by the way, are what I think the issue you found is referring to).
Also — and this is just a sidebar — I don't really know why retryOnEmpty retries errors; based on its name it should only retry empty requests. Unfortunately I don't have enough context on this to know what the right thing to do here is, so it's probably best to leave it alone.
In conclusion, your change makes sense, I just had some tweaks.
|
Hey @mcmire : I updated the PR and also added unit test coverage. |
mcmire
left a comment
mcmire
left a comment
There was a problem hiding this comment.
Just a few suggestions. Solution looks good aside from this though!
Co-authored-by: Elliot Winkler <elliot.winkler@gmail.com>
Co-authored-by: Elliot Winkler <elliot.winkler@gmail.com>
Co-authored-by: Elliot Winkler <elliot.winkler@gmail.com>
|
Hey @mcmire : thanks a lot for review feedbacks, I addressed all of those. |
2 participants
Fixes: https://github.com/MetaMask/MetaMask-planning/issues/1461
This is an issue brought up by blockaid team, in case of revert we should not retry and send back original error.