From 241b651b20cd1b72ab81b00d96c8d24609178de6 Mon Sep 17 00:00:00 2001 From: Daniel Chew Date: Tue, 11 Feb 2025 15:53:01 -0800 Subject: [PATCH 1/4] feat(pulse): Add getLastActiveRequests function to retrieve active requests --- .../contracts/contracts/pulse/IPulse.sol | 20 ++ .../contracts/contracts/pulse/Pulse.sol | 34 ++- .../ethereum/contracts/forge-test/Pulse.t.sol | 205 +++++++++++++++++- 3 files changed, 257 insertions(+), 2 deletions(-) diff --git a/target_chains/ethereum/contracts/contracts/pulse/IPulse.sol b/target_chains/ethereum/contracts/contracts/pulse/IPulse.sol index 2dd8239381..f46f19b637 100644 --- a/target_chains/ethereum/contracts/contracts/pulse/IPulse.sol +++ b/target_chains/ethereum/contracts/contracts/pulse/IPulse.sol @@ -92,4 +92,24 @@ interface IPulse is PulseEvents { function setExclusivityPeriod(uint256 periodSeconds) external; function getExclusivityPeriod() external view returns (uint256); + + /** + * @notice Gets the last N active requests + * @param count Maximum number of active requests to return + * @return requests Array of active requests, ordered from newest to oldest + * @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 + * that need to be scanned to find active ones. Each iteration costs approximately: + * - 2300 gas for the storage read + * - Additional gas for array operations + * For example, if active requests are sparse (many completed requests between active ones), + * scanning for 100 active requests might need to iterate through 1000+ sequence numbers. + * Consider using smaller count values for gas-sensitive operations. + */ + function getLastActiveRequests( + 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 4e768c30ae..82cfb61b20 100644 --- a/target_chains/ethereum/contracts/contracts/pulse/Pulse.sol +++ b/target_chains/ethereum/contracts/contracts/pulse/Pulse.sol @@ -293,7 +293,7 @@ abstract contract Pulse is IPulse, PulseState { } } - function isActive(Request storage req) internal view returns (bool) { + function isActive(Request memory req) internal pure returns (bool) { return req.sequenceNumber != 0; } @@ -383,4 +383,36 @@ abstract contract Pulse is IPulse, PulseState { function getExclusivityPeriod() external view override returns (uint256) { return _state.exclusivityPeriodSeconds; } + + function getLastActiveRequests( + uint256 count + ) + external + view + override + returns (Request[] memory requests, uint256 actualCount) + { + requests = new Request[](count); + actualCount = 0; + + // Start from the most recent sequence number and work backwards + uint64 currentSeq = _state.currentSequenceNumber - 1; + + // Continue until we find enough active requests or run out of sequence numbers + while (actualCount < count && currentSeq > 0) { + 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/forge-test/Pulse.t.sol b/target_chains/ethereum/contracts/forge-test/Pulse.t.sol index b6a0e4f51e..0b9c07d205 100644 --- a/target_chains/ethereum/contracts/forge-test/Pulse.t.sol +++ b/target_chains/ethereum/contracts/forge-test/Pulse.t.sol @@ -54,7 +54,7 @@ contract CustomErrorPulseConsumer is IPulseConsumer { } } -contract PulseTest is Test, PulseEvents { +contract PulseTest is Test, PulseEvents, IPulseConsumer { ERC1967Proxy public proxy; PulseUpgradeable public pulse; MockPulseConsumer public consumer; @@ -876,4 +876,207 @@ contract PulseTest is Test, PulseEvents { vm.prank(secondProvider); pulse.executeCallback(sequenceNumber, updateData, priceIds); } + + function test_getLastActiveRequests() 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 { + uint256 publishTime = block.timestamp; + for (uint i = 0; i < 5; i++) { + vm.deal(address(this), 1 ether); + pulse.requestPriceUpdatesWithCallback{value: 1 ether}( + 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( + block.timestamp + ); + mockParsePriceFeedUpdates(priceFeeds); + updateData = createMockUpdateData(priceFeeds); + + vm.deal(defaultProvider, 2 ether); // Increase ETH allocation to prevent OutOfFunds + vm.startPrank(defaultProvider); + pulse.executeCallback{value: 1 ether}(2, updateData, priceIds); + pulse.executeCallback{value: 1 ether}(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 + .getLastActiveRequests(10); + assertEq(count, 3, "Should find 3 active requests"); + assertEq(requests.length, 3, "Array should be resized to 3"); + assertEq( + requests[0].sequenceNumber, + 5, + "First request should be newest" + ); + assertEq(requests[1].sequenceNumber, 3, "Second request should be #3"); + assertEq(requests[2].sequenceNumber, 1, "Third request should be #1"); + } + + function checkExactNumber() private { + (PulseState.Request[] memory requests, uint256 count) = pulse + .getLastActiveRequests(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 + .getLastActiveRequests(2); + assertEq(count, 2, "Should find 2 active requests"); + assertEq(requests.length, 2, "Array should match requested size"); + assertEq( + requests[0].sequenceNumber, + 5, + "First request should be newest" + ); + assertEq(requests[1].sequenceNumber, 3, "Second request should be #3"); + } + + function checkZeroRequest() private { + (PulseState.Request[] memory requests, uint256 count) = pulse + .getLastActiveRequests(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}(1, updateData, priceIds); + pulse.executeCallback{value: 1 ether}(3, updateData, priceIds); + pulse.executeCallback{value: 1 ether}(5, updateData, priceIds); + vm.stopPrank(); + } + + function checkEmptyState() private { + (PulseState.Request[] memory requests, uint256 count) = pulse + .getLastActiveRequests(10); + assertEq(count, 0, "Should find 0 active requests"); + assertEq(requests.length, 0, "Array should be empty"); + } + + function test_getLastActiveRequests_GasUsage() public { + // Setup test data + bytes32[] memory priceIds = new bytes32[](1); + priceIds[0] = bytes32(uint256(1)); + uint256 publishTime = block.timestamp; + uint256 callbackGasLimit = 1000000; + + // Create mock price feeds and setup Pyth response + PythStructs.PriceFeed[] memory priceFeeds = createMockPriceFeeds( + publishTime + ); + mockParsePriceFeedUpdates(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}( + 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}( + uint64(i + 1), + updateData, + priceIds + ); + } + } + + // Measure gas for different request counts + uint256 gas1 = gasleft(); + pulse.getLastActiveRequests(5); + uint256 gas1Used = gas1 - gasleft(); + + uint256 gas2 = gasleft(); + pulse.getLastActiveRequests(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" + ); + } + + // Mock implementation of pulseCallback + function pulseCallback( + uint64 sequenceNumber, + PythStructs.PriceFeed[] memory priceFeeds + ) external override { + // Just accept the callback, no need to do anything with the data + // This prevents the revert we're seeing + } } From 165d20ff6563b63e21bc24a478b2af501624d96b Mon Sep 17 00:00:00 2001 From: Daniel Chew Date: Tue, 11 Feb 2025 16:00:15 -0800 Subject: [PATCH 2/4] fix(pulse): Update gas usage comments for active requests function --- target_chains/ethereum/contracts/contracts/pulse/IPulse.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/target_chains/ethereum/contracts/contracts/pulse/IPulse.sol b/target_chains/ethereum/contracts/contracts/pulse/IPulse.sol index f46f19b637..df50a73a7c 100644 --- a/target_chains/ethereum/contracts/contracts/pulse/IPulse.sol +++ b/target_chains/ethereum/contracts/contracts/pulse/IPulse.sol @@ -100,7 +100,7 @@ interface IPulse is PulseEvents { * @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 * that need to be scanned to find active ones. Each iteration costs approximately: - * - 2300 gas for the storage read + * - 2100 gas for cold storage reads, 100 gas for warm storage reads (SLOAD) * - Additional gas for array operations * For example, if active requests are sparse (many completed requests between active ones), * scanning for 100 active requests might need to iterate through 1000+ sequence numbers. From 1c9bf3b5943e4c1ee2a334d9d258a362457d000e Mon Sep 17 00:00:00 2001 From: Daniel Chew Date: Tue, 11 Feb 2025 16:34:43 -0800 Subject: [PATCH 3/4] refactor(pulse): Rename getLastActiveRequests to getFirstActiveRequests and update related comments --- .../contracts/contracts/pulse/IPulse.sol | 13 ++++----- .../contracts/contracts/pulse/Pulse.sol | 22 +++++++++++---- .../contracts/contracts/pulse/PulseState.sol | 1 + .../ethereum/contracts/forge-test/Pulse.t.sol | 28 +++++++++---------- 4 files changed, 37 insertions(+), 27 deletions(-) diff --git a/target_chains/ethereum/contracts/contracts/pulse/IPulse.sol b/target_chains/ethereum/contracts/contracts/pulse/IPulse.sol index df50a73a7c..b22c5582b2 100644 --- a/target_chains/ethereum/contracts/contracts/pulse/IPulse.sol +++ b/target_chains/ethereum/contracts/contracts/pulse/IPulse.sol @@ -94,19 +94,18 @@ interface IPulse is PulseEvents { function getExclusivityPeriod() external view returns (uint256); /** - * @notice Gets the last N active requests + * @notice Gets the first N active requests * @param count Maximum number of active requests to return - * @return requests Array of active requests, ordered from newest to oldest + * @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 - * that need to be scanned to find active ones. Each iteration costs approximately: + * 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 - * For example, if active requests are sparse (many completed requests between active ones), - * scanning for 100 active requests might need to iterate through 1000+ sequence numbers. - * Consider using smaller count values for gas-sensitive 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 getLastActiveRequests( + function getFirstActiveRequests( uint256 count ) external diff --git a/target_chains/ethereum/contracts/contracts/pulse/Pulse.sol b/target_chains/ethereum/contracts/contracts/pulse/Pulse.sol index 82cfb61b20..91154a04e9 100644 --- a/target_chains/ethereum/contracts/contracts/pulse/Pulse.sol +++ b/target_chains/ethereum/contracts/contracts/pulse/Pulse.sol @@ -164,6 +164,14 @@ abstract contract Pulse is IPulse, PulseState { "low-level error (possibly out of gas)" ); } + + // After successful callback, update firstUnfulfilledSeq if needed + while ( + _state.firstUnfulfilledSeq < _state.currentSequenceNumber && + !isActive(findRequest(_state.firstUnfulfilledSeq)) + ) { + _state.firstUnfulfilledSeq++; + } } function emitPriceUpdate( @@ -384,7 +392,7 @@ abstract contract Pulse is IPulse, PulseState { return _state.exclusivityPeriodSeconds; } - function getLastActiveRequests( + function getFirstActiveRequests( uint256 count ) external @@ -395,17 +403,19 @@ abstract contract Pulse is IPulse, PulseState { requests = new Request[](count); actualCount = 0; - // Start from the most recent sequence number and work backwards - uint64 currentSeq = _state.currentSequenceNumber - 1; + // Start from the first unfulfilled sequence and work forwards + uint64 currentSeq = _state.firstUnfulfilledSeq; - // Continue until we find enough active requests or run out of sequence numbers - while (actualCount < count && currentSeq > 0) { + // 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--; + currentSeq++; } // If we found fewer requests than asked for, resize the array diff --git a/target_chains/ethereum/contracts/contracts/pulse/PulseState.sol b/target_chains/ethereum/contracts/contracts/pulse/PulseState.sol index 3d9fff9f76..a561b1544a 100644 --- a/target_chains/ethereum/contracts/contracts/pulse/PulseState.sol +++ b/target_chains/ethereum/contracts/contracts/pulse/PulseState.sol @@ -37,6 +37,7 @@ contract PulseState { 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 0b9c07d205..a39380d802 100644 --- a/target_chains/ethereum/contracts/forge-test/Pulse.t.sol +++ b/target_chains/ethereum/contracts/forge-test/Pulse.t.sol @@ -877,7 +877,7 @@ contract PulseTest is Test, PulseEvents, IPulseConsumer { pulse.executeCallback(sequenceNumber, updateData, priceIds); } - function test_getLastActiveRequests() public { + function test_getFirstActiveRequests() public { // Setup test data ( bytes32[] memory priceIds, @@ -955,41 +955,41 @@ contract PulseTest is Test, PulseEvents, IPulseConsumer { // Split test scenarios into separate functions function checkMoreThanAvailable() private { (PulseState.Request[] memory requests, uint256 count) = pulse - .getLastActiveRequests(10); + .getFirstActiveRequests(10); assertEq(count, 3, "Should find 3 active requests"); assertEq(requests.length, 3, "Array should be resized to 3"); assertEq( requests[0].sequenceNumber, - 5, - "First request should be newest" + 1, + "First request should be oldest" ); assertEq(requests[1].sequenceNumber, 3, "Second request should be #3"); - assertEq(requests[2].sequenceNumber, 1, "Third request should be #1"); + assertEq(requests[2].sequenceNumber, 5, "Third request should be #5"); } function checkExactNumber() private { (PulseState.Request[] memory requests, uint256 count) = pulse - .getLastActiveRequests(3); + .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 - .getLastActiveRequests(2); + .getFirstActiveRequests(2); assertEq(count, 2, "Should find 2 active requests"); assertEq(requests.length, 2, "Array should match requested size"); assertEq( requests[0].sequenceNumber, - 5, - "First request should be newest" + 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 - .getLastActiveRequests(0); + .getFirstActiveRequests(0); assertEq(count, 0, "Should find 0 active requests"); assertEq(requests.length, 0, "Array should be empty"); } @@ -1008,12 +1008,12 @@ contract PulseTest is Test, PulseEvents, IPulseConsumer { function checkEmptyState() private { (PulseState.Request[] memory requests, uint256 count) = pulse - .getLastActiveRequests(10); + .getFirstActiveRequests(10); assertEq(count, 0, "Should find 0 active requests"); assertEq(requests.length, 0, "Array should be empty"); } - function test_getLastActiveRequests_GasUsage() public { + function test_getFirstActiveRequests_GasUsage() public { // Setup test data bytes32[] memory priceIds = new bytes32[](1); priceIds[0] = bytes32(uint256(1)); @@ -1050,11 +1050,11 @@ contract PulseTest is Test, PulseEvents, IPulseConsumer { // Measure gas for different request counts uint256 gas1 = gasleft(); - pulse.getLastActiveRequests(5); + pulse.getFirstActiveRequests(5); uint256 gas1Used = gas1 - gasleft(); uint256 gas2 = gasleft(); - pulse.getLastActiveRequests(10); + pulse.getFirstActiveRequests(10); uint256 gas2Used = gas2 - gasleft(); // Log gas usage for analysis From ce15f14545e31fa924f98be6416837b1a3d780f7 Mon Sep 17 00:00:00 2001 From: Daniel Chew Date: Wed, 12 Feb 2025 10:57:37 -0800 Subject: [PATCH 4/4] refactor(tests): Rename test functions for consistency in naming convention --- target_chains/ethereum/contracts/forge-test/Pulse.t.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/target_chains/ethereum/contracts/forge-test/Pulse.t.sol b/target_chains/ethereum/contracts/forge-test/Pulse.t.sol index a39380d802..147ac4e0d7 100644 --- a/target_chains/ethereum/contracts/forge-test/Pulse.t.sol +++ b/target_chains/ethereum/contracts/forge-test/Pulse.t.sol @@ -877,7 +877,7 @@ contract PulseTest is Test, PulseEvents, IPulseConsumer { pulse.executeCallback(sequenceNumber, updateData, priceIds); } - function test_getFirstActiveRequests() public { + function testGetFirstActiveRequests() public { // Setup test data ( bytes32[] memory priceIds, @@ -1013,7 +1013,7 @@ contract PulseTest is Test, PulseEvents, IPulseConsumer { assertEq(requests.length, 0, "Array should be empty"); } - function test_getFirstActiveRequests_GasUsage() public { + function testGetFirstActiveRequestsGasUsage() public { // Setup test data bytes32[] memory priceIds = new bytes32[](1); priceIds[0] = bytes32(uint256(1));