diff --git a/target_chains/ethereum/contracts/contracts/pulse/IPulse.sol b/target_chains/ethereum/contracts/contracts/pulse/IPulse.sol index 6dff9b1283..8bc5c1887a 100644 --- a/target_chains/ethereum/contracts/contracts/pulse/IPulse.sol +++ b/target_chains/ethereum/contracts/contracts/pulse/IPulse.sol @@ -139,23 +139,4 @@ interface IPulse is PulseEvents { function setExclusivityPeriod(uint256 periodSeconds) external; function getExclusivityPeriod() external view returns (uint256); - - /** - * @notice Gets the first N active requests - * @param count Maximum number of active requests to return - * @return requests Array of active requests, ordered from oldest to newest - * @return actualCount Number of active requests found (may be less than count) - * @dev Gas Usage: This function's gas cost scales linearly with the number of requests - * between firstUnfulfilledSeq and currentSequenceNumber. Each iteration costs approximately: - * - 2100 gas for cold storage reads, 100 gas for warm storage reads (SLOAD) - * - Additional gas for array operations - * The function starts from firstUnfulfilledSeq (all requests before this are fulfilled) - * and scans forward until it finds enough active requests or reaches currentSequenceNumber. - */ - function getFirstActiveRequests( - uint256 count - ) - external - view - returns (PulseState.Request[] memory requests, uint256 actualCount); } diff --git a/target_chains/ethereum/contracts/contracts/pulse/Pulse.sol b/target_chains/ethereum/contracts/contracts/pulse/Pulse.sol index 9483bc015c..7a8fc7e5ac 100644 --- a/target_chains/ethereum/contracts/contracts/pulse/Pulse.sol +++ b/target_chains/ethereum/contracts/contracts/pulse/Pulse.sol @@ -44,11 +44,9 @@ abstract contract Pulse is IPulse, PulseState { req.publishTime = 1; req.callbackGasLimit = 1; req.requester = address(1); - req.numPriceIds = 0; - // Pre-warm the priceIds array storage - for (uint8 j = 0; j < MAX_PRICE_IDS; j++) { - req.priceIds[j] = bytes32(0); - } + req.priceIdsHash = bytes32(uint256(1)); + req.fee = 1; + req.provider = address(1); } } } @@ -72,9 +70,6 @@ abstract contract Pulse is IPulse, PulseState { // Since tx.gasprice is used to calculate fees, allowing far-future requests would make // the fee estimation unreliable. require(publishTime <= block.timestamp + 60, "Too far in future"); - if (priceIds.length > MAX_PRICE_IDS) { - revert TooManyPriceIds(priceIds.length, MAX_PRICE_IDS); - } requestSequenceNumber = _state.currentSequenceNumber++; uint128 requiredFee = getFee(provider, callbackGasLimit, priceIds); @@ -83,16 +78,12 @@ abstract contract Pulse is IPulse, PulseState { Request storage req = allocRequest(requestSequenceNumber); req.sequenceNumber = requestSequenceNumber; req.publishTime = publishTime; - req.callbackGasLimit = callbackGasLimit; + req.callbackGasLimit = SafeCast.toUint128(callbackGasLimit); req.requester = msg.sender; - req.numPriceIds = uint8(priceIds.length); req.provider = provider; req.fee = SafeCast.toUint128(msg.value - _state.pythFeeInWei); + req.priceIdsHash = keccak256(abi.encode(priceIds)); - // Copy price IDs to storage - for (uint8 i = 0; i < priceIds.length; i++) { - req.priceIds[i] = priceIds[i]; - } _state.accruedFeesInWei += _state.pythFeeInWei; emit PriceUpdateRequested(req, priceIds); @@ -118,14 +109,11 @@ abstract contract Pulse is IPulse, PulseState { } // Verify priceIds match - require( - priceIds.length == req.numPriceIds, - "Price IDs length mismatch" - ); - for (uint8 i = 0; i < req.numPriceIds; i++) { - if (priceIds[i] != req.priceIds[i]) { - revert InvalidPriceIds(priceIds[i], req.priceIds[i]); - } + if (req.priceIdsHash != keccak256(abi.encode(priceIds))) { + revert InvalidPriceIds( + keccak256(abi.encode(priceIds)), + req.priceIdsHash + ); } // TODO: should this use parsePriceFeedUpdatesUnique? also, do we need to add 1 to maxPublishTime? @@ -151,16 +139,6 @@ abstract contract Pulse is IPulse, PulseState { 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 @@ -450,38 +428,4 @@ abstract contract Pulse is IPulse, PulseState { function getExclusivityPeriod() external view override returns (uint256) { return _state.exclusivityPeriodSeconds; } - - function getFirstActiveRequests( - uint256 count - ) - external - view - override - returns (Request[] memory requests, uint256 actualCount) - { - requests = new Request[](count); - actualCount = 0; - - // Start from the first unfulfilled sequence and work forwards - uint64 currentSeq = _state.firstUnfulfilledSeq; - - // Continue until we find enough active requests or reach current sequence - while ( - actualCount < count && currentSeq < _state.currentSequenceNumber - ) { - Request memory req = findRequest(currentSeq); - if (isActive(req)) { - requests[actualCount] = req; - actualCount++; - } - currentSeq++; - } - - // If we found fewer requests than asked for, resize the array - if (actualCount < count) { - assembly { - mstore(requests, actualCount) - } - } - } } diff --git a/target_chains/ethereum/contracts/contracts/pulse/PulseErrors.sol b/target_chains/ethereum/contracts/contracts/pulse/PulseErrors.sol index e57719d4da..fe080fee4f 100644 --- a/target_chains/ethereum/contracts/contracts/pulse/PulseErrors.sol +++ b/target_chains/ethereum/contracts/contracts/pulse/PulseErrors.sol @@ -12,4 +12,3 @@ error CallbackFailed(); error InvalidPriceIds(bytes32 providedPriceIdsHash, bytes32 storedPriceIdsHash); error InvalidCallbackGasLimit(uint256 requested, uint256 stored); error ExceedsMaxPrices(uint32 requested, uint32 maxAllowed); -error TooManyPriceIds(uint256 provided, uint256 maximum); diff --git a/target_chains/ethereum/contracts/contracts/pulse/PulseState.sol b/target_chains/ethereum/contracts/contracts/pulse/PulseState.sol index 57560d276a..08fd4f0a0c 100644 --- a/target_chains/ethereum/contracts/contracts/pulse/PulseState.sol +++ b/target_chains/ethereum/contracts/contracts/pulse/PulseState.sol @@ -5,21 +5,22 @@ pragma solidity ^0.8.0; contract PulseState { uint8 public constant NUM_REQUESTS = 32; bytes1 public constant NUM_REQUESTS_MASK = 0x1f; - // Maximum number of price feeds per request. This limit keeps gas costs predictable and reasonable. 10 is a reasonable number for most use cases. - // Requests with more than 10 price feeds should be split into multiple requests - uint8 public constant MAX_PRICE_IDS = 10; struct Request { - uint64 sequenceNumber; - 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; + // Storage slot 1 // address requester; + uint64 sequenceNumber; + // 4 bytes unused + // Storage slot 2 // address provider; + uint64 publishTime; + // 4 bytes unused + // Storage slot 3 // + // Hash of the array of price ids that should be provided to fulfill this request. + // The hash is order-sensitive. + bytes32 priceIdsHash; + // Storage slot 4 // + uint128 callbackGasLimit; uint128 fee; } @@ -33,17 +34,24 @@ contract PulseState { } struct State { - address admin; + // Storage slot 1 // uint128 pythFeeInWei; uint128 accruedFeesInWei; + // Storage slot 2 // address pyth; uint64 currentSequenceNumber; + // 4 bytes unused + // Storage slot 3 // address defaultProvider; + // 12 bytes unused + // Storage slot 4 // uint256 exclusivityPeriodSeconds; + // Storage slot 5 // + address admin; + // 12 bytes unused Request[NUM_REQUESTS] requests; mapping(bytes32 => Request) requestsOverflow; mapping(address => ProviderInfo) providers; - uint64 firstUnfulfilledSeq; // All sequences before this are fulfilled } State internal _state; diff --git a/target_chains/ethereum/contracts/forge-test/Pulse.t.sol b/target_chains/ethereum/contracts/forge-test/Pulse.t.sol index 4041076bfa..b7f2a34250 100644 --- a/target_chains/ethereum/contracts/forge-test/Pulse.t.sol +++ b/target_chains/ethereum/contracts/forge-test/Pulse.t.sol @@ -148,19 +148,7 @@ contract PulseTest is Test, PulseEvents, IPulseConsumer, PulseTestUtils { PulseState.Request memory expectedRequest = PulseState.Request({ sequenceNumber: 1, publishTime: publishTime, - priceIds: [ - priceIds[0], - priceIds[1], - bytes32(0), // Fill remaining slots with zero - bytes32(0), - bytes32(0), - bytes32(0), - bytes32(0), - bytes32(0), - bytes32(0), - bytes32(0) - ], - numPriceIds: 2, + priceIdsHash: keccak256(abi.encode(priceIds)), callbackGasLimit: CALLBACK_GAS_LIMIT, requester: address(consumer), provider: defaultProvider, @@ -182,10 +170,7 @@ contract PulseTest is Test, PulseEvents, IPulseConsumer, PulseTestUtils { PulseState.Request memory lastRequest = pulse.getRequest(1); assertEq(lastRequest.sequenceNumber, expectedRequest.sequenceNumber); assertEq(lastRequest.publishTime, expectedRequest.publishTime); - assertEq(lastRequest.numPriceIds, expectedRequest.numPriceIds); - for (uint8 i = 0; i < lastRequest.numPriceIds; i++) { - assertEq(lastRequest.priceIds[i], expectedRequest.priceIds[i]); - } + assertEq(lastRequest.priceIdsHash, expectedRequest.priceIdsHash); assertEq( lastRequest.callbackGasLimit, expectedRequest.callbackGasLimit @@ -675,8 +660,8 @@ contract PulseTest is Test, PulseEvents, IPulseConsumer, PulseTestUtils { vm.expectRevert( abi.encodeWithSelector( InvalidPriceIds.selector, - wrongPriceIds[0], - priceIds[0] + keccak256(abi.encode(wrongPriceIds)), + keccak256(abi.encode(priceIds)) ) ); pulse.executeCallback( @@ -687,33 +672,6 @@ contract PulseTest is Test, PulseEvents, IPulseConsumer, PulseTestUtils { ); } - function testRevertOnTooManyPriceIds() public { - uint256 maxPriceIds = uint256(pulse.MAX_PRICE_IDS()); - // Create array with MAX_PRICE_IDS + 1 price IDs - bytes32[] memory priceIds = new bytes32[](maxPriceIds + 1); - for (uint i = 0; i < priceIds.length; i++) { - priceIds[i] = bytes32(uint256(i + 1)); - } - - vm.deal(address(consumer), 1 gwei); - uint128 totalFee = calculateTotalFee(); - - vm.prank(address(consumer)); - vm.expectRevert( - abi.encodeWithSelector( - TooManyPriceIds.selector, - maxPriceIds + 1, - maxPriceIds - ) - ); - pulse.requestPriceUpdatesWithCallback{value: totalFee}( - defaultProvider, - SafeCast.toUint64(block.timestamp), - priceIds, - CALLBACK_GAS_LIMIT - ); - } - function testProviderRegistration() public { address provider = address(0x123); uint128 providerFee = 1000; @@ -945,228 +903,6 @@ contract PulseTest is Test, PulseEvents, IPulseConsumer, PulseTestUtils { ); } - function testGetFirstActiveRequests() public { - // Setup test data - ( - bytes32[] memory priceIds, - bytes[] memory updateData - ) = setupTestData(); - createTestRequests(priceIds); - completeRequests(updateData, priceIds); - - testRequestScenarios(priceIds, updateData); - } - - function setupTestData() - private - pure - returns (bytes32[] memory, bytes[] memory) - { - bytes32[] memory priceIds = new bytes32[](1); - priceIds[0] = bytes32(uint256(1)); - - bytes[] memory updateData = new bytes[](1); - return (priceIds, updateData); - } - - function createTestRequests(bytes32[] memory priceIds) private { - uint64 publishTime = SafeCast.toUint64(block.timestamp); - for (uint i = 0; i < 5; i++) { - vm.deal(address(this), 1 ether); - pulse.requestPriceUpdatesWithCallback{value: 1 ether}( - defaultProvider, - publishTime, - priceIds, - 1000000 - ); - } - } - - function completeRequests( - bytes[] memory updateData, - bytes32[] memory priceIds - ) private { - // Create mock price feeds and setup Pyth response - PythStructs.PriceFeed[] memory priceFeeds = createMockPriceFeeds( - SafeCast.toUint64(block.timestamp) - ); - mockParsePriceFeedUpdates(pyth, priceFeeds); - updateData = createMockUpdateData(priceFeeds); - - vm.deal(defaultProvider, 2 ether); // Increase ETH allocation to prevent OutOfFunds - vm.startPrank(defaultProvider); - pulse.executeCallback{value: 1 ether}( - defaultProvider, - 2, - updateData, - priceIds - ); - pulse.executeCallback{value: 1 ether}( - defaultProvider, - 4, - updateData, - priceIds - ); - vm.stopPrank(); - } - - function testRequestScenarios( - bytes32[] memory priceIds, - bytes[] memory updateData - ) private { - // Test 1: Request more than available - checkMoreThanAvailable(); - - // Test 2: Request exact number - checkExactNumber(); - - // Test 3: Request fewer than available - checkFewerThanAvailable(); - - // Test 4: Request zero - checkZeroRequest(); - - // Test 5: Clear all and check empty - clearAllRequests(updateData, priceIds); - checkEmptyState(); - } - - // Split test scenarios into separate functions - function checkMoreThanAvailable() private { - (PulseState.Request[] memory requests, uint256 count) = pulse - .getFirstActiveRequests(10); - assertEq(count, 3, "Should find 3 active requests"); - assertEq(requests.length, 3, "Array should be resized to 3"); - assertEq( - requests[0].sequenceNumber, - 1, - "First request should be oldest" - ); - assertEq(requests[1].sequenceNumber, 3, "Second request should be #3"); - assertEq(requests[2].sequenceNumber, 5, "Third request should be #5"); - } - - function checkExactNumber() private { - (PulseState.Request[] memory requests, uint256 count) = pulse - .getFirstActiveRequests(3); - assertEq(count, 3, "Should find 3 active requests"); - assertEq(requests.length, 3, "Array should match requested size"); - } - - function checkFewerThanAvailable() private { - (PulseState.Request[] memory requests, uint256 count) = pulse - .getFirstActiveRequests(2); - assertEq(count, 2, "Should find 2 active requests"); - assertEq(requests.length, 2, "Array should match requested size"); - assertEq( - requests[0].sequenceNumber, - 1, - "First request should be oldest" - ); - assertEq(requests[1].sequenceNumber, 3, "Second request should be #3"); - } - - function checkZeroRequest() private { - (PulseState.Request[] memory requests, uint256 count) = pulse - .getFirstActiveRequests(0); - assertEq(count, 0, "Should find 0 active requests"); - assertEq(requests.length, 0, "Array should be empty"); - } - - function clearAllRequests( - bytes[] memory updateData, - bytes32[] memory priceIds - ) private { - vm.deal(defaultProvider, 3 ether); // Increase ETH allocation - vm.startPrank(defaultProvider); - pulse.executeCallback{value: 1 ether}( - defaultProvider, - 1, - updateData, - priceIds - ); - pulse.executeCallback{value: 1 ether}( - defaultProvider, - 3, - updateData, - priceIds - ); - pulse.executeCallback{value: 1 ether}( - defaultProvider, - 5, - updateData, - priceIds - ); - vm.stopPrank(); - } - - function checkEmptyState() private { - (PulseState.Request[] memory requests, uint256 count) = pulse - .getFirstActiveRequests(10); - assertEq(count, 0, "Should find 0 active requests"); - assertEq(requests.length, 0, "Array should be empty"); - } - - function testGetFirstActiveRequestsGasUsage() public { - // Setup test data - bytes32[] memory priceIds = new bytes32[](1); - priceIds[0] = bytes32(uint256(1)); - uint64 publishTime = SafeCast.toUint64(block.timestamp); - uint256 callbackGasLimit = 1000000; - - // Create mock price feeds and setup Pyth response - PythStructs.PriceFeed[] memory priceFeeds = createMockPriceFeeds( - publishTime - ); - mockParsePriceFeedUpdates(pyth, priceFeeds); - bytes[] memory updateData = createMockUpdateData(priceFeeds); - - // Create 20 requests with some gaps - for (uint i = 0; i < 20; i++) { - vm.deal(address(this), 1 ether); - pulse.requestPriceUpdatesWithCallback{value: 1 ether}( - defaultProvider, - publishTime, - priceIds, - callbackGasLimit - ); - - // Complete every third request to create gaps - if (i % 3 == 0) { - vm.deal(defaultProvider, 1 ether); - vm.prank(defaultProvider); - pulse.executeCallback{value: 1 ether}( - defaultProvider, - uint64(i + 1), - updateData, - priceIds - ); - } - } - - // Measure gas for different request counts - uint256 gas1 = gasleft(); - pulse.getFirstActiveRequests(5); - uint256 gas1Used = gas1 - gasleft(); - - uint256 gas2 = gasleft(); - pulse.getFirstActiveRequests(10); - uint256 gas2Used = gas2 - gasleft(); - - // Log gas usage for analysis - emit log_named_uint("Gas used for 5 requests", gas1Used); - emit log_named_uint("Gas used for 10 requests", gas2Used); - - // Verify gas usage scales roughly linearly - // Allow 10% margin for other factors - assertApproxEqRel( - gas2Used, - gas1Used * 2, - 0.1e18, // 10% tolerance - "Gas usage should scale roughly linearly" - ); - } - function getPulse() internal view override returns (address) { return address(pulse); }