feat(entropy): Add callback failure states to the contract #2546
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
4 Skipped Deployments
|
| // any reverts will be reported as an event. Any failing requests move to a failure state | ||
| // at which point they can be recovered. The recovery flow invokes the callback directly | ||
| // (no catching errors) which allows callers to easily see the revert reason. | ||
| if (req.status == EntropyConstants.STATUS_CALLBACK_NOT_STARTED) { |
There was a problem hiding this comment.
Note that this is a behavior change at the moment. This means that we won't be able to upgrade the contracts without upgrading the keeper.
However, in the next PR I will additionally condition this on the request having a gas limit, which means that the contract will be 100% backward compatible. (i.e., you don't get the failure flow until the provider explicitly sets a gas limit for callbacks)
| @@ -938,12 +939,13 @@ contract EntropyTest is Test, EntropyTestUtils, EntropyEvents { | |||
| ); | |||
| } | |||
|
|
|||
| // TODO: restore this test once providers have custom gas limits, which allow us to toggle between | |||
| // the old and new failure behavior. | |||
There was a problem hiding this comment.
(restoring this test will ensure backward compatibility of the flow, per the comment above)
| bool isRequestWithCallback; | ||
| // There are 2 remaining bytes of free space in this slot. | ||
| // Status flag for requests with callbacks. See EntropyConstants for the possible values of this flag. | ||
| uint8 status; |
There was a problem hiding this comment.
(bools are internally represented as uint8 so this is a compatible change. I'm reusing this flag because I want the remaining 2 bytes for the gas limit)
| library EntropyConstants { | ||
| // Status values for Request.status // | ||
| // not a request with callback | ||
| uint8 public constant STATUS_NO_CALLBACK = 0; |
There was a problem hiding this comment.
how about naming the library EntropyStatusConstants and remove the STATUS_ prefix here?
| library EntropyConstants { | ||
| // Status values for Request.status // | ||
| // not a request with callback | ||
| uint8 public constant STATUS_NO_CALLBACK = 0; |
There was a problem hiding this comment.
the name of this variant is a bit confusing. Maybe something like uninitialized?
| bool isRequestWithCallback; | ||
| // There are 2 remaining bytes of free space in this slot. | ||
| // Status flag for requests with callbacks. See EntropyConstants for the possible values of this flag. | ||
| uint8 status; |
There was a problem hiding this comment.
maybe name this callback_status?
| bool success; | ||
| bytes memory ret; | ||
| (success, ret) = callAddress.excessivelySafeCall( | ||
| gasleft(), // TODO: providers need to be able to configure this in the future. |
There was a problem hiding this comment.
I think this actually needs to be exactly what the requester asked for and not what provider specifyes
| ); | ||
| req.status = EntropyConstants.STATUS_CALLBACK_FAILED; | ||
| } else { | ||
| // The callback ran out of gas |
There was a problem hiding this comment.
I think there are other failure scenarios where ret.length is 0.
Summary
Currently, the Entropy keeper cannot invoke a callback if the underlying callback fails. This creates confusion for users, who can't distinguish between "the keeper is offline" and "my callback code reverts".
This change creates a failure flow for callbacks that allows the keeper's transaction to succeed while explicitly reporting a failure of the underlying callback. Requests with failing callbacks remain on the blockchain and can be re-invoked by users. This feature allows users to gracefully recover in cases where the callback fails for transient reasons. (In the future, it will also allow recovery in the case where the callback uses too much gas.)
Note that this PR doesn't cover the case where the callback uses too much gas. I will send a follow-up PR for that.
How has this been tested?