Add more functions to allow multiple data types in request-response… #466
Conversation
| } | ||
|
|
||
| function fulfillDataRequest(uint256 requestId, uint256 response) internal virtual; | ||
| function fulfillDataRequestUint256(uint256 requestId, uint256 response) internal virtual; |
There was a problem hiding this comment.
I wanted to suggest to keep function names, event names and others all same. It creates a unique function selector there is at least one parameter type different. However, it is not possible to access overloaded function through function selector in Solidity 😢 ethereum/solidity#3556
There was a problem hiding this comment.
Do you mean we can just keep different function names for different data types?
There was a problem hiding this comment.
We can and it would be nicer, however, we need to have an access to function selector from inside of smart contract and that is currently not supported in Solidity.
| req.initialize( | ||
| jobId, | ||
| address(COORDINATOR), | ||
| COORDINATOR.fulfillDataRequestUint256.selector |
There was a problem hiding this comment.
I think this should be changed. We do not always want to request for uint256.
|
|
||
| function fulfillDataRequestBytes(uint256 requestId, bytes memory response) internal virtual; | ||
|
|
||
| function rawFulfillDataRequestUint256(uint256 requestId, uint256 response) external { |
There was a problem hiding this comment.
Wouldn't it be better to have a single modifier that takes care of the response verification so we do not have to reimplement the same stuff again and again? 🤔
modifier verifyRawFulfillment {
address coordinatorAddress = address(COORDINATOR);
if (msg.sender != coordinatorAddress) {
revert OnlyCoordinatorCanFulfill(msg.sender, coordinatorAddress);
}
_;
}
function rawFulfillDataRequestUint256(uint256 requestId, uint256 response) external verifyRawFulfillment {
fulfillDataRequestUint256(requestId, response);
}| @@ -39,6 +39,8 @@ contract RequestResponseCoordinator is | |||
|
|
|||
| PrepaymentInterface s_prepayment; | |||
|
|
|||
| mapping(bytes4 => uint256) private sFulfillFunctionSelector; | |||
There was a problem hiding this comment.
I believe this is not used. Can you please remove it?
There was a problem hiding this comment.
yes, I'll remove this
| @@ -108,6 +105,43 @@ contract RequestResponseCoordinator is | |||
| event MinBalanceSet(uint256 minBalance); | |||
| event PrepaymentSet(address prepayment); | |||
|
|
|||
| event DataRequestFulfilledUint256( | |||
There was a problem hiding this comment.
We will have to think how to change the payment because each data type request will have theoretically different price. The easiest would be to make the price high enough so that it covers the most expensive data type. What do you think?
There was a problem hiding this comment.
yes, I think that makes sense, the prices for different data types would not be much for Klaytn though.
| requestId, | ||
| response | ||
| ); | ||
| bool success = fulfill(resp, rc); |
There was a problem hiding this comment.
Theoretically, we could merge the fulfill & pay functions together, but let's keep them separated for now. Later, we should measure the gas between those two approaches and ensure security.
| @@ -29,7 +35,7 @@ contract RequestResponseConsumerMock is RequestResponseConsumerBase { | |||
| uint64 accId, | |||
| uint32 callbackGasLimit | |||
| ) public onlyOwner returns (uint256 requestId) { | |||
| bytes32 jobId = keccak256(abi.encodePacked("any-api-int256")); | |||
| bytes32 jobId = keccak256(abi.encodePacked("Uint256")); | |||
There was a problem hiding this comment.
Let's keep the string with all lowercase as shown at https://orakl-network.gitbook.io/docs/developers-guide/request-response#request-and-response-post-processing
It applies to the request below as well.
There was a problem hiding this comment.
roger that! let me fix it
| const value = parseKlay(1) | ||
| await prepaymentContractConsumerSigner.deposit(accId, { value }) | ||
| const requestReceipt = await ( | ||
| await consumerContract.requestData(accId, maxGasLimit, { |
There was a problem hiding this comment.
In this branch requestData always encodes job id using keccak256(abi.encodePacked("Uint256")), so all tests should fail. Please fix the requestData call.
buildRequest inside of RequestResponseConsumerBase always uses COORDINATOR.fulfillDataRequestUint256.selector which is again wrong and the test should fail. Please fix it.
There was a problem hiding this comment.
I added a function to validate the JobID. Also, buildRequest is based on appropriate callbackfunc
function validateJobId(bytes32 jobId) internal pure returns (bool) {
return (jobId == keccak256("uint256") ||
jobId == keccak256("int256") ||
jobId == keccak256("bool") ||
jobId == keccak256("string") ||
jobId == keccak256("bytes32") ||
jobId == keccak256("bytes"));
}
function buildRequest(bytes32 jobId) internal view returns (Orakl.Request memory req) {
bytes4 callbackFunc = detectCallbackFunc(jobId);
return req.initialize(jobId, address(COORDINATOR), callbackFunc);
}
…r verifyRawFulfillment, removed mapping sFulfillFunctionSelector, check JobId valid
There was a problem hiding this comment.
@KelvinThai I commented on the new change. Please check it out :)
| } | ||
|
|
||
| function fulfillDataRequest(uint256 requestId, uint256 response) internal virtual; | ||
| function detectCallbackFunc(bytes32 jobId) internal view returns (bytes4 callbackFunc) { |
There was a problem hiding this comment.
I think we can make this much better. Currently for every request call we go through this convoluted if-else branch which does a lot of repeated computation. How about having a private mapping which stores the job IDs and its corresponding function selectors. It can be computed once inside of constructor.
mapping(bytes32 => byte4) private sJobIdToFunctionSelector;
constructor(address _requestResponseCoordinator) {
COORDINATOR = RequestResponseCoordinatorInterface(_requestResponseCoordinator);
sJobIdToFunctionSelector[keccak256("uint256")] = COORDINATOR.fulfillDataRequestUint256.selector;
sJobIdToFunctionSelector[keccak256("int256")] = COORDINATOR.fulfillDataRequestInt256.selector;
sJobIdToFunctionSelector[keccak256("string")] = COORDINATOR.fulfillDataRequestString.selector;
sJobIdToFunctionSelector[keccak256("bytes32")] = COORDINATOR.fulfillDataRequestBytes32.selector;
sJobIdToFunctionSelector[keccak256("bytes")] = COORDINATOR.fulfillDataRequestBytes.selector;
}and then
function buildRequest(bytes32 jobId) internal view returns (Orakl.Request memory req) {
return req.initialize(jobId, address(COORDINATOR), sJobIdToFunctionSelector[jobId]);
}| return payment; | ||
| } | ||
|
|
||
| function validateJobId(bytes32 jobId) internal pure returns (bool) { |
There was a problem hiding this comment.
How about we precompute these values and store them in mapping? It could be something similar to what I suggested at RequestResponseConsumerBase?
| s_response = response; | ||
| } | ||
|
|
||
| function fulfillDataRequestInt256(uint256 /*requestId*/, int256 response) internal override { |
There was a problem hiding this comment.
I am curious. Have you tried to remove some of these functions? Can it still compile?
There was a problem hiding this comment.
yes, I did, and it can not compile. All virtual functions need to be overrided
There was a problem hiding this comment.
In that case we have to figure out how to deal with it. We should not force people to override every possible fulfill* function when they most likely need just one. Can you please propose some solution?
There was a problem hiding this comment.
we can use an interface instead, but they have to import the interface
There was a problem hiding this comment.
I think importing interface is okay! 🚀
| expect(prepaymentEvent.name).to.be.equal('AccountBalanceDecreased') | ||
| expect(prepaymentEvent.args.accId).to.be.equal(accId) | ||
|
|
||
| // FIXME |
There was a problem hiding this comment.
Do we need to keep this here? It repeats in every test.
There was a problem hiding this comment.
Yes, we can just keep 1. I put it in all tests, just in case something weird happens with prepayment
There was a problem hiding this comment.
I think I am missing the point. Why do we need an unfinished commented test?
There was a problem hiding this comment.
aha! that's just code garbage. I'll clean it up.
| requestCommitment, | ||
| isDirectPayment, | ||
| { | ||
| gasLimit: maxGasLimit + 300_000 |
There was a problem hiding this comment.
Why do we need to increase the gasLimit?
There was a problem hiding this comment.
Did I put it there? LOL!!! I think it's a random piece of code when I merged master branch. Anyway in tests, we can just keep it high to make sure the tx is executed.
There was a problem hiding this comment.
I think we should have a reason for having it here to mitigate confusion. Please try to remove if possible. If not possible we have to explain why this is needed.
There was a problem hiding this comment.
got it! now it gets weird. We can not run if we remove it, if we remove it and increase the gas limit, it can not run either. Can only run with gasLimit: maxGasLimit + 300_000
Let me find the route course
There was a problem hiding this comment.
if we dont specify anything, it runs as well, maybe it's something with the callwithexcatchgas() function
…e comment(requestResponseCoordinaotr.test)
| @@ -113,6 +148,12 @@ contract RequestResponseCoordinator is | |||
| } | |||
|
|
|||
| constructor(address prepayment) { | |||
| sJobIdToFunctionSelector[keccak256("uint256")] = "uint256"; | |||
There was a problem hiding this comment.
I don't understand this mapping. It says it should include function selector, but actually you save string there instead of function selector, and eventually you don't even use that string. It is only checked for the length > 0.
Why don't we just do the following?
mapping(bytes32 => bool) sIsJobRegistered;
sJobIdToFunctionSelector[keccak256("uint256")] = true;
sJobIdToFunctionSelector[keccak256("int256")] = true;
sJobIdToFunctionSelector[keccak256("bool")] = true;
sJobIdToFunctionSelector[keccak256("string")] = true;
sJobIdToFunctionSelector[keccak256("bytes32")] = true;
sJobIdToFunctionSelector[keccak256("bytes")] = true;
Later in the requestData functions we can just test with
if (bytes(sJobIdToFunctionSelector[req.id])) {
revert InvalidJobId();
}| requestCommitment, | ||
| isDirectPayment, | ||
| { | ||
| gasLimit: maxGasLimit + 100000 |
There was a problem hiding this comment.
In all test the maxGasLimit has to be equal to the callbackGasLimit. Please adjust it accordingly.
Also, how did you come up with the 100k? Empirically?
There was a problem hiding this comment.
I was experimenting what if we just add a small amount, would the Tx executed, and it did
There was a problem hiding this comment.
Okay! Let's figure out later how can we compute the gas unit costs before executing request.
| consumer, | ||
| rrOracle0 | ||
| } = await loadFixture(deployFixture) | ||
| const prepaymentContractConsumerSigner = await ethers.getContractAt( |
There was a problem hiding this comment.
The test are very hard to read because there is a lot of repeated code that could be put to a single auxiliary function and called from each test for setup. Could you please make it? There are some issues in your tests that will be easier to spot when the tests are written more concisely. I am gonna point out some of them before the refactoring though.
| const value = parseKlay(1) | ||
| await prepaymentContractConsumerSigner.deposit(accId, { value }) | ||
| const requestReceipt = await ( | ||
| await consumerContract.requestData(accId, maxGasLimit, { |
There was a problem hiding this comment.
Consumer contract has single requestData function but many fulfill functions. In these tests you are recreating the situation when users tries to get the request different kind of data. If we want to have correct end-to-end scenario, we need to have multiple request functions.
There was a problem hiding this comment.
Yes, we can have better test cases. In the real world, consumers will mostly have 1 requests function and inherit 1 fulfill function from the abstract contracts. We can split that into multiple test cases for all data types
There was a problem hiding this comment.
Yes, that is actually what you have in these test, but the same request is always called.
| @@ -1,5 +1,5 @@ | |||
| export function requestResponseConfig() { | |||
| const maxGasLimit = 2_500_000 | |||
| const maxGasLimit = 2_800_000 | |||
There was a problem hiding this comment.
Why do we need to modify this value?
There was a problem hiding this comment.
Is some request so expensive?
There was a problem hiding this comment.
I modified that to experiment the gasLimit: maxGasLimit + 300_000
I modified it to 2.800.000. which is exactly the same as 2.500.000 + 300.000, but it didnot run. The error return even more confusing: "EVM reverts without a reason."
There was a problem hiding this comment.
@KelvinThai We should be careful about that because this initial maxGasLimit is then used everywhere. As far as I know the 300K extra gas was used in one of the tests. Now the increased value has impact on every test.
There was a problem hiding this comment.
OK. let's keep the config 2_500_000 consistently
There was a problem hiding this comment.
and let's remove the predefine gasLimit in the call, it would run with default
…n RequestResponseConsumerMock and RequestResponseConsumerContract.test
| @@ -223,7 +264,7 @@ contract RequestResponseCoordinator is | |||
| return (sDirectPaymentConfig.fulfillmentFee, sDirectPaymentConfig.baseFee); | |||
| } | |||
|
|
|||
| function getPrepaymentAddress() external view returns (address) { | |||
| function getPrepaymentAddress() public view returns (address) { | |||
There was a problem hiding this comment.
@KelvinThai Is there a reason to change it from external to public?
There was a problem hiding this comment.
It was a 10000 mistake. let me roll it back to external. I'll be more attention on self-code review
| uint32 callbackGasLimit, | ||
| bytes32 jobId | ||
| ) private returns (uint256 requestId) { | ||
| //bytes32 jobId = keccak256(abi.encodePacked("uint256")); |
There was a problem hiding this comment.
@KelvinThai Let's delete the commented code.
| @@ -25,12 +38,11 @@ contract RequestResponseConsumerMock is RequestResponseConsumerBase { | |||
| // Receive remaining payment from requestDataPayment | |||
| receive() external payable {} | |||
|
|
|||
| function requestData( | |||
| function requestDataInternal( | |||
There was a problem hiding this comment.
@KelvinThai I still think both the requestDataInternal and requestDataDirectPaymentInternal do not make sense. Each request asks for different data type, but use the same request definition (fetching KLAY-USD price from cryptocompare.com). Can we make it better?
There was a problem hiding this comment.
If we do not plan to create a separate request for each data type, we should at least explain it for future readers.
There was a problem hiding this comment.
Yes, I got what you mean! how about we separate the request function for each data type and comment on each funtions. The code would be longer but more readable for consumers.
There was a problem hiding this comment.
Yes, I think that would be nice! We could then later use this as a part of integration test as well.
Description
This PR allows multiple types for RR return data types
uint256int256boolstringbytes32bytesType of change
Checklist before requesting a review
Deployment