cleaning up

Author: jayantk

target_chains/ethereum/contracts/contracts/entropy/Entropy.sol

@@ -515,17 +515,20 @@ abstract contract Entropy is IEntropy, EntropyState {
         // 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.gasLimit10k != 0 && req.callbackStatus == EntropyStatusConstants.CALLBACK_NOT_STARTED) {
+        if (
+            req.gasLimit10k != 0 &&
+            req.callbackStatus == EntropyStatusConstants.CALLBACK_NOT_STARTED
+        ) {
             req.callbackStatus = EntropyStatusConstants.CALLBACK_IN_PROGRESS;
             bool success;
             bytes memory ret;
             uint256 startingGas = gasleft();
             (success, ret) = callAddress.excessivelySafeCall(
                 // Warning: the provided gas limit below is only an *upper bound* on the gas provided to the call.
-                // If the current context has less gas the desired value, 63/64ths of the current context's gas will
-                // be provided instead. (See CALL opcode docs here https://www.evm.codes/?fork=cancun#f1)
+                // At most 63/64ths of the current context's gas will be provided to a call, which may be less
+                // than the indicated gas limit. (See CALL opcode docs here https://www.evm.codes/?fork=cancun#f1)
                 // Consequently, out-of-gas reverts need to be handled carefully to ensure that the callback
-                // was truly provided with a sufficient amount of gas.                
+                // was truly provided with a sufficient amount of gas.
                 uint256(req.gasLimit10k) * 10000,
                 256, // copy at most 256 bytes of the return value into ret.
                 abi.encodeWithSelector(
@@ -535,7 +538,6 @@ abstract contract Entropy is IEntropy, EntropyState {
                     randomNumber
                 )
             );
-            uint256 endingGas = gasleft();
             // Reset status to not started here in case the transaction reverts.
             req.callbackStatus = EntropyStatusConstants.CALLBACK_NOT_STARTED;
 
@@ -547,10 +549,15 @@ abstract contract Entropy is IEntropy, EntropyState {
                     randomNumber
                 );
                 clearRequest(provider, sequenceNumber);
-            } else if (ret.length > 0 || startingGas - endingGas >= uint256(req.gasLimit10k) * 10000) {
+            } else if (
+                ret.length > 0 ||
+                (startingGas * 31) / 32 > uint256(req.gasLimit10k) * 10000
+            ) {
                 // The callback reverted for some reason.
-                // If ret.length == 0, then the callback ran out of gas. In this case, ensure that the
-                // callback was provided with sufficient gas.
+                // If ret.length > 0, then we know the callback manually triggered a revert, so it's safe to mark it as failed.
+                // If ret.length == 0, then the callback might have run out of gas (though there are other ways to trigger a revert with ret.length == 0).
+                // In this case, ensure that the callback was provided with sufficient gas. Technically, 63/64ths of the startingGas is forwarded,
+                // but we're using 31/32 to introduce a margin of safety.
                 emit CallbackFailed(
                     provider,
                     req.requester,
@@ -562,11 +569,11 @@ abstract contract Entropy is IEntropy, EntropyState {
                 );
                 req.callbackStatus = EntropyStatusConstants.CALLBACK_FAILED;
             } else {
-                // Callback reverted by running out of gas, but the calling context did not have enough gas
+                // Callback reverted by (potentially) running out of gas, but the calling context did not have enough gas
                 // to run the callback. This is a corner case that can happen due to the nuances of gas passing
                 // in calls (see the comment on the call above).
-                // 
-                // (Note that reverting here plays nicely with the estimateGas RPC method, which binary searches for 
+                //
+                // (Note that reverting here plays nicely with the estimateGas RPC method, which binary searches for
                 // the smallest gas value that causes the transaction to *succeed*. See https://github.com/ethereum/go-ethereum/pull/3587 )
                 revert EntropyErrors.InsufficientGas();
             }
@@ -640,11 +647,15 @@ abstract contract Entropy is IEntropy, EntropyState {
         ];
 
         uint32 roundedGasLimit = uint32(roundGas(gasLimit)) * 10000;
-        if (roundedGasLimit > provider.defaultGasLimit) {
+        if (
+            provider.defaultGasLimit > 0 &&
+            roundedGasLimit > provider.defaultGasLimit
+        ) {
             // This calculation rounds down the fee, which means that users can get some gas in the callback for free.
             // However, the value of the free gas is < 1 wei, which is insignificant.
-            uint128 additionalFee = ((roundedGasLimit - provider.defaultGasLimit) *
-                provider.feeInWei) / provider.defaultGasLimit;
+            uint128 additionalFee = ((roundedGasLimit -
+                provider.defaultGasLimit) * provider.feeInWei) /
+                provider.defaultGasLimit;
             return provider.feeInWei + additionalFee;
         } else {
             return provider.feeInWei;
@@ -777,9 +788,7 @@ abstract contract Entropy is IEntropy, EntropyState {
 
     // Rounds the provided quantity of gas into units of 10k gas.
     // If gas is not evenly divisible by 10k, rounds up.
-    function roundGas(
-        uint32 gas
-    ) public pure returns (uint16) {
+    function roundGas(uint32 gas) internal pure returns (uint16) {
         uint32 gas10k = gas / 10000;
         if (gas10k * 10000 < gas) {
             gas10k += 1;

target_chains/ethereum/contracts/forge-test/Entropy.t.sol

@@ -1229,7 +1229,7 @@ contract EntropyTest is Test, EntropyTestUtils, EntropyEvents {
     // Test the corner case caused by the CALL opcode passing at most 63/64ths of the current gas
     // to the sub-call.
     function testRequestWithCallbackUsingTooMuchGas2() public {
-        // With a 64M gas limit, we will pass ~63M gas to the callback (which is insufficient), but still 
+        // With a 64M gas limit, we will pass ~63M gas to the callback (which is insufficient), but still
         // have ~1M gas to execute code within the revealWithCallback method, which should be enough to
         // run all of the logic subsequent to the excessivelySafeCall.
         uint32 defaultGasLimit = 64000000;
@@ -1246,10 +1246,6 @@ contract EntropyTest is Test, EntropyTestUtils, EntropyEvents {
         uint64 assignedSequenceNumber = consumer.requestEntropy{value: fee}(
             userRandomNumber
         );
-        EntropyStructs.Request memory req = random.getRequest(
-            provider1,
-            assignedSequenceNumber
-        );
 
         // The transaction reverts if the provider does not provide enough gas to forward
         // the gasLimit to the callback transaction.
@@ -1573,32 +1569,66 @@ contract EntropyTest is Test, EntropyTestUtils, EntropyEvents {
         assertEq(fee, random.getFee(provider1));
     }
 
-    function testRoundGas() public {
-        // TODO: test this with the full request/reveal flow and update the max value
+    function testGasLimitsAndFeeRounding() public {
+        vm.prank(provider1);
+        random.setDefaultGasLimit(20000);
+        vm.prank(provider1);
+        random.setProviderFee(1);
 
         // Test exact multiples of 10,000
-        assertEq(random.roundGas(0), 0);
-        assertEq(random.roundGas(10000), 1);
-        assertEq(random.roundGas(20000), 2);
-        assertEq(random.roundGas(100000), 10);
+        assertGasLimit10k(0, 0, 1);
+        assertGasLimit10k(10000, 1, 1);
+        assertGasLimit10k(20000, 2, 1);
+        assertGasLimit10k(100000, 10, 5);
 
         // Test values just below multiples of 10,000
-        assertEq(random.roundGas(9999), 1);
-        assertEq(random.roundGas(19999), 2);
-        assertEq(random.roundGas(99999), 10);
+        assertGasLimit10k(9999, 1, 1);
+        assertGasLimit10k(19999, 2, 1);
+        assertGasLimit10k(39999, 4, 2);
+        assertGasLimit10k(99999, 10, 5);
 
         // Test values just above multiples of 10,000
-        assertEq(random.roundGas(10001), 2);
-        assertEq(random.roundGas(20001), 3);
-        assertEq(random.roundGas(100001), 11);
+        assertGasLimit10k(10001, 2, 1);
+        assertGasLimit10k(20001, 3, 1);
+        assertGasLimit10k(100001, 11, 5);
+        assertGasLimit10k(110001, 12, 6);
 
         // Test middle values
-        assertEq(random.roundGas(5000), 1);
-        assertEq(random.roundGas(15000), 2);
-        assertEq(random.roundGas(25000), 3);
+        assertGasLimit10k(5000, 1, 1);
+        assertGasLimit10k(15000, 2, 1);
+        assertGasLimit10k(25000, 3, 1);
 
-        // Test maximum uint32 value
-        assertEq(random.roundGas(type(uint32).max), 429497); // (2^32 - 1) / 10000 rounded up
+        // Test maximum value
+        assertGasLimit10k(
+            uint32(type(uint16).max) * 10000,
+            type(uint16).max,
+            uint128(type(uint16).max) / 2
+        );
+    }
+
+    // Helper method to create a request with a specific gas limit and check the gasLimit10k field
+    function assertGasLimit10k(
+        uint32 gasLimit,
+        uint16 expectedGasLimit10k,
+        uint128 expectedProviderFee
+    ) internal {
+        // Create a request with callback
+        bytes32 userRandomNumber = bytes32(uint(42));
+        uint fee = random.getFeeForGas(provider1, gasLimit);
+        assertEq(fee - random.getPythFee(), expectedProviderFee);
+
+        vm.deal(user1, fee);
+        vm.prank(user1);
+        uint64 sequenceNumber = random.requestWithCallbackAndGasLimit{
+            value: fee
+        }(provider1, userRandomNumber, gasLimit);
+
+        // Check the gasLimit10k field in the request
+        EntropyStructs.Request memory req = random.getRequest(
+            provider1,
+            sequenceNumber
+        );
+        assertEq(req.gasLimit10k, expectedGasLimit10k);
     }
 }