FUN-1009 - Remove redundant Functions Coordinator commitment & request id checks #10975
Conversation
justinkaseman
requested review from
a team,
se3000,
connorwstein and
RensR
as code owners
|
I see that you haven't updated any README files. Would it make sense to do so? |
…t id checks, favoring handling in Functions Router
justinkaseman
force-pushed
the
FUN-1009
branch
from
01a1412 to
ef9f852
Compare
| @@ -261,14 +261,6 @@ abstract contract FunctionsBilling is Routable, IFunctionsBilling { | |||
| ) internal returns (FunctionsResponse.FulfillResult) { | |||
| FunctionsResponse.Commitment memory commitment = abi.decode(onchainMetadata, (FunctionsResponse.Commitment)); | |||
|
|
|||
| if (s_requestCommitments[requestId] == bytes32(0)) { | |||
There was a problem hiding this comment.
Can you explain in the PR description that these are repeated inside fulfill() and triggering them over there is better because it also emits events with error messages?
Also, why are tests such as "FunctionsRouter_Fulfill:test_Fulfill_RequestNotProcessedInvalidRequestId()" not affected gas-wise? They should be using slightly more now, no?
There was a problem hiding this comment.
Added.
The FunctionsRouter test doesn't go through the FunctionsCoordinator, it calls the Router directly, so it is unaffected by Coordinator changes
There was a problem hiding this comment.
I see. How hard would it be to add a simple test that goes through coordinator into router.fulfill()? I just want to be extra safe with coverage.
There was a problem hiding this comment.
I'll get a PR up that will add ^ before we merge this one, then we can see it change in this PR
There was a problem hiding this comment.
Actually you can see it changing on the Gas Minimum Request & Gas Maximum Request tests, those go through the end to end flow - we should probably just merge this, as I am adding more to the test PR that I mentioned
|
SonarQube Quality Gate
|
There are safety checks against incorrect request commitments and incorrect request ids in both the Functions Coordinator and the Functions Router. The issue with this is that the Functions Router contract is the one that emits an event. Since Functions Coordinator checks are hit before the Functions Router checks, the event is never given.
Favor the checks in Functions Router so that an event is emitted.