pulsse fixes

Author: jayantk

target_chains/ethereum/contracts/contracts/pulse/Pulse.sol

@@ -56,7 +56,7 @@ abstract contract Pulse is IPulse, PulseState {
     // TODO: there can be a separate wrapper function that defaults the provider (or uses the cheapest or something).
     function requestPriceUpdatesWithCallback(
         address provider,
-        uint256 publishTime,
+        uint64 publishTime,
         bytes32[] calldata priceIds,
         uint256 callbackGasLimit
     ) external payable override returns (uint64 requestSequenceNumber) {
@@ -65,7 +65,7 @@ abstract contract Pulse is IPulse, PulseState {
             "Provider not registered"
         );
 
-        // FIXME: this comment is wrong.
+        // FIXME: this comment is wrong. (we're not using tx.gasprice)
         // NOTE: The 60-second future limit on publishTime prevents a DoS vector where
         //      attackers could submit many low-fee requests for far-future updates when gas prices
         //      are low, forcing executors to fulfill them later when gas prices might be much higher.
@@ -93,7 +93,6 @@ abstract contract Pulse is IPulse, PulseState {
         for (uint8 i = 0; i < priceIds.length; i++) {
             req.priceIds[i] = priceIds[i];
         }
-
         _state.accruedFeesInWei += _state.pythFeeInWei;
 
         emit PriceUpdateRequested(req, priceIds);
@@ -129,7 +128,6 @@ abstract contract Pulse is IPulse, PulseState {
             }
         }
 
-        // Parse price feeds first to measure gas usage
         // TODO: should this use parsePriceFeedUpdatesUnique? also, do we need to add 1 to maxPublishTime?
         IPyth pyth = IPyth(_state.pyth);
         uint256 pythFee = pyth.getUpdateFee(updateData);
@@ -142,7 +140,6 @@ abstract contract Pulse is IPulse, PulseState {
             SafeCast.toUint64(req.publishTime)
         );
 
-        clearRequest(sequenceNumber);
         // TODO: if this effect occurs here, we need to guarantee that executeCallback can never revert.
         // If executeCallback can revert, then funds can be permanently locked in the contract.
         // TODO: there also needs to be some penalty mechanism in case the expected provider doesn't execute the callback.
@@ -153,6 +150,18 @@ abstract contract Pulse is IPulse, PulseState {
         _state.providers[providerToCredit].accruedFeesInWei += SafeCast
             .toUint128(msg.value - pythFee);
 
+        clearRequest(sequenceNumber);
+
+        // TODO: I'm pretty sure this is going to use a lot of gas because it's doing a storage lookup for each sequence number.
+        // a better solution would be a doubly-linked list of active requests.
+        // After successful callback, update firstUnfulfilledSeq if needed
+        while (
+            _state.firstUnfulfilledSeq < _state.currentSequenceNumber &&
+            !isActive(findRequest(_state.firstUnfulfilledSeq))
+        ) {
+            _state.firstUnfulfilledSeq++;
+        }
+
         try
             IPulseConsumer(req.requester)._pulseCallback{
                 gas: req.callbackGasLimit
@@ -164,7 +173,7 @@ abstract contract Pulse is IPulse, PulseState {
             // Explicit revert/require
             emit PriceUpdateCallbackFailed(
                 sequenceNumber,
-                msg.sender,
+                providerToCredit,
                 priceIds,
                 req.requester,
                 reason
@@ -173,21 +182,12 @@ abstract contract Pulse is IPulse, PulseState {
             // Out of gas or other low-level errors
             emit PriceUpdateCallbackFailed(
                 sequenceNumber,
-                msg.sender,
+                providerToCredit,
                 priceIds,
                 req.requester,
                 "low-level error (possibly out of gas)"
             );
         }
-
-        // TODO: I'm pretty sure this is going to use a lot of gas because it's doing a storage lookup for each sequence number.
-        // After successful callback, update firstUnfulfilledSeq if needed
-        while (
-            _state.firstUnfulfilledSeq < _state.currentSequenceNumber &&
-            !isActive(findRequest(_state.firstUnfulfilledSeq))
-        ) {
-            _state.firstUnfulfilledSeq++;
-        }
     }
 
     function emitPriceUpdate(
@@ -198,7 +198,7 @@ abstract contract Pulse is IPulse, PulseState {
         int64[] memory prices = new int64[](priceFeeds.length);
         uint64[] memory conf = new uint64[](priceFeeds.length);
         int32[] memory expos = new int32[](priceFeeds.length);
-        uint256[] memory publishTimes = new uint256[](priceFeeds.length);
+        uint64[] memory publishTimes = new uint256[](priceFeeds.length);
 
         for (uint i = 0; i < priceFeeds.length; i++) {
             prices[i] = priceFeeds[i].price.price;

target_chains/ethereum/contracts/contracts/pulse/PulseErrors.sol

@@ -4,6 +4,7 @@ pragma solidity ^0.8.0;
 
 error NoSuchProvider();
 error NoSuchRequest();
+// TODO: add expected / provided values
 error InsufficientFee();
 error Unauthorized();
 error InvalidCallbackGas();

target_chains/ethereum/contracts/contracts/pulse/PulseEvents.sol

@@ -13,7 +13,7 @@ interface PulseEvents {
         int64[] prices,
         uint64[] conf,
         int32[] expos,
-        uint256[] publishTimes
+        uint64[] publishTimes
     );
 
     event FeesWithdrawn(address indexed recipient, uint128 amount);

target_chains/ethereum/contracts/contracts/pulse/PulseState.sol

@@ -11,8 +11,10 @@ contract PulseState {
 
     struct Request {
         uint64 sequenceNumber;
-        uint256 publishTime;
+        uint64 publishTime;
         // TODO: this is going to absolutely explode gas costs. Need to do something smarter here.
+        // possible solution is to hash the price ids and store the hash instead.
+        // The ids themselves can be retrieved from the event.
         bytes32[MAX_PRICE_IDS] priceIds;
         uint8 numPriceIds; // Actual number of price IDs used
         uint256 callbackGasLimit;

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

@@ -12,13 +12,22 @@ import "../contracts/pulse/PulseEvents.sol";
 import "../contracts/pulse/PulseErrors.sol";
 
 contract MockPulseConsumer is IPulseConsumer {
+    address private _pulse;
     uint64 public lastSequenceNumber;
     PythStructs.PriceFeed[] private _lastPriceFeeds;
 
+    constructor(address pulse) {
+        _pulse = pulse;
+    }
+
+    function getPulse() internal view override returns (address) {
+        return _pulse;
+    }
+
     function pulseCallback(
         uint64 sequenceNumber,
         PythStructs.PriceFeed[] memory priceFeeds
-    ) external override {
+    ) internal override {
         lastSequenceNumber = sequenceNumber;
         for (uint i = 0; i < priceFeeds.length; i++) {
             _lastPriceFeeds.push(priceFeeds[i]);
@@ -35,25 +44,46 @@ contract MockPulseConsumer is IPulseConsumer {
 }
 
 contract FailingPulseConsumer is IPulseConsumer {
+    address private _pulse;
+
+    constructor(address pulse) {
+        _pulse = pulse;
+    }
+
+    function getPulse() internal view override returns (address) {
+        return _pulse;
+    }
+
     function pulseCallback(
         uint64,
         PythStructs.PriceFeed[] memory
-    ) external pure override {
+    ) internal pure override {
         revert("callback failed");
     }
 }
 
 contract CustomErrorPulseConsumer is IPulseConsumer {
     error CustomError(string message);
 
+    address private _pulse;
+
+    constructor(address pulse) {
+        _pulse = pulse;
+    }
+
+    function getPulse() internal view override returns (address) {
+        return _pulse;
+    }
+
     function pulseCallback(
         uint64,
         PythStructs.PriceFeed[] memory
-    ) external pure override {
+    ) internal pure override {
         revert CustomError("callback failed");
     }
 }
 
+// FIXME: this shouldn't be IPulseConsumer.
 contract PulseTest is Test, PulseEvents, IPulseConsumer {
     ERC1967Proxy public proxy;
     PulseUpgradeable public pulse;
@@ -1187,11 +1217,15 @@ contract PulseTest is Test, PulseEvents, IPulseConsumer {
         );
     }
 
+    function getPulse() internal view override returns (address) {
+        return address(pulse);
+    }
+
     // Mock implementation of pulseCallback
     function pulseCallback(
         uint64 sequenceNumber,
         PythStructs.PriceFeed[] memory priceFeeds
-    ) external override {
+    ) internal override {
         // Just accept the callback, no need to do anything with the data
         // This prevents the revert we're seeing
     }