From 86ae5c3f419088b4be6b9b2b5c6b13a0e9fe2e24 Mon Sep 17 00:00:00 2001 From: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Date: Mon, 12 May 2025 17:48:21 +0000 Subject: [PATCH 1/4] fix(pulse): reset priceLastUpdatedAt when price IDs change in updateSubscription Co-Authored-By: Tejas Badadare --- .../contracts/contracts/pulse/Scheduler.sol | 40 +++++- .../contracts/forge-test/PulseScheduler.t.sol | 114 ++++++++++++++++++ 2 files changed, 152 insertions(+), 2 deletions(-) diff --git a/target_chains/ethereum/contracts/contracts/pulse/Scheduler.sol b/target_chains/ethereum/contracts/contracts/pulse/Scheduler.sol index 4aa5e17f97..5e89c9561e 100644 --- a/target_chains/ethereum/contracts/contracts/pulse/Scheduler.sol +++ b/target_chains/ethereum/contracts/contracts/pulse/Scheduler.sol @@ -131,12 +131,17 @@ abstract contract Scheduler is IScheduler, SchedulerState { } // Clear price updates for removed price IDs before updating params - _clearRemovedPriceUpdates( + bool priceIdsChanged = _clearRemovedPriceUpdates( subscriptionId, currentParams.priceIds, newParams.priceIds ); + // Reset priceLastUpdatedAt to 0 if price IDs have changed + if (priceIdsChanged) { + _state.subscriptionStatuses[subscriptionId].priceLastUpdatedAt = 0; + } + // Update subscription parameters _state.subscriptionParams[subscriptionId] = newParams; @@ -211,12 +216,18 @@ abstract contract Scheduler is IScheduler, SchedulerState { * @param subscriptionId The ID of the subscription being updated. * @param currentPriceIds The array of price IDs currently associated with the subscription. * @param newPriceIds The new array of price IDs for the subscription. + * @return priceIdsChanged True if the price IDs list has changed (additions or removals), false otherwise. */ function _clearRemovedPriceUpdates( uint256 subscriptionId, bytes32[] storage currentPriceIds, bytes32[] memory newPriceIds - ) internal { + ) internal returns (bool priceIdsChanged) { + // Check if the arrays have different lengths, which means the price IDs have changed + if (currentPriceIds.length != newPriceIds.length) { + priceIdsChanged = true; + } + // Iterate through old price IDs for (uint i = 0; i < currentPriceIds.length; i++) { bytes32 oldPriceId = currentPriceIds[i]; @@ -233,8 +244,33 @@ abstract contract Scheduler is IScheduler, SchedulerState { // If not found in the new list, delete its stored update data if (!found) { delete _state.priceUpdates[subscriptionId][oldPriceId]; + priceIdsChanged = true; } } + + // Check if any new price IDs were added + if (!priceIdsChanged) { + for (uint i = 0; i < newPriceIds.length; i++) { + bytes32 newPriceId = newPriceIds[i]; + bool found = false; + + // Check if the new price ID exists in the current list + for (uint j = 0; j < currentPriceIds.length; j++) { + if (currentPriceIds[j] == newPriceId) { + found = true; + break; + } + } + + // If a new price ID was added, mark as changed + if (!found) { + priceIdsChanged = true; + break; + } + } + } + + return priceIdsChanged; } function updatePriceFeeds( diff --git a/target_chains/ethereum/contracts/forge-test/PulseScheduler.t.sol b/target_chains/ethereum/contracts/forge-test/PulseScheduler.t.sol index ee06ff171f..c80a161f5b 100644 --- a/target_chains/ethereum/contracts/forge-test/PulseScheduler.t.sol +++ b/target_chains/ethereum/contracts/forge-test/PulseScheduler.t.sol @@ -328,6 +328,120 @@ contract SchedulerTest is Test, SchedulerEvents, PulseSchedulerTestUtils { ); } + // Helper function to reduce stack depth in testUpdateSubscriptionResetsPriceLastUpdatedAt + function _setupSubscriptionAndFirstUpdate() + private + returns (uint256 subscriptionId, uint64 publishTime) + { + // Setup subscription with heartbeat criteria + uint32 heartbeatSeconds = 60; // 60 second heartbeat + SchedulerState.UpdateCriteria memory criteria = SchedulerState + .UpdateCriteria({ + updateOnHeartbeat: true, + heartbeatSeconds: heartbeatSeconds, + updateOnDeviation: false, + deviationThresholdBps: 0 + }); + + subscriptionId = addTestSubscriptionWithUpdateCriteria( + scheduler, + criteria, + address(reader) + ); + scheduler.addFunds{value: 1 ether}(subscriptionId); + + // Update prices to set priceLastUpdatedAt to a non-zero value + publishTime = SafeCast.toUint64(block.timestamp); + PythStructs.PriceFeed[] memory priceFeeds; + uint64[] memory slots; + (priceFeeds, slots) = createMockPriceFeedsWithSlots(publishTime, 2); + mockParsePriceFeedUpdatesWithSlots(pyth, priceFeeds, slots); + bytes[] memory updateData = createMockUpdateData(priceFeeds); + + vm.prank(pusher); + scheduler.updatePriceFeeds(subscriptionId, updateData); + + return (subscriptionId, publishTime); + } + + function testUpdateSubscriptionResetsPriceLastUpdatedAt() public { + // 1. Setup subscription and perform first update + ( + uint256 subscriptionId, + uint64 publishTime1 + ) = _setupSubscriptionAndFirstUpdate(); + + // Verify priceLastUpdatedAt is set + (, SchedulerState.SubscriptionStatus memory status) = scheduler + .getSubscription(subscriptionId); + assertEq( + status.priceLastUpdatedAt, + publishTime1, + "priceLastUpdatedAt should be set to the first update timestamp" + ); + + // 2. Update subscription to change price IDs + (SchedulerState.SubscriptionParams memory currentParams, ) = scheduler + .getSubscription(subscriptionId); + + // Create new price IDs (different from the original ones) + bytes32[] memory newPriceIds = createPriceIds(3); // Different number of price IDs + + SchedulerState.SubscriptionParams memory newParams = currentParams; + newParams.priceIds = newPriceIds; + + // Update the subscription + scheduler.updateSubscription(subscriptionId, newParams); + + // 3. Verify priceLastUpdatedAt is reset to 0 + (, status) = scheduler.getSubscription(subscriptionId); + assertEq( + status.priceLastUpdatedAt, + 0, + "priceLastUpdatedAt should be reset to 0 after changing price IDs" + ); + + // 4. Verify immediate update is possible + _verifyImmediateUpdatePossible(subscriptionId); + } + + function _verifyImmediateUpdatePossible(uint256 subscriptionId) private { + // Create new price feeds for the new price IDs + uint64 publishTime2 = SafeCast.toUint64(block.timestamp + 1); // Just 1 second later + PythStructs.PriceFeed[] memory priceFeeds; + uint64[] memory slots; + (priceFeeds, slots) = createMockPriceFeedsWithSlots(publishTime2, 3); // 3 feeds for new price IDs + mockParsePriceFeedUpdatesWithSlots(pyth, priceFeeds, slots); + bytes[] memory updateData = createMockUpdateData(priceFeeds); + + // This should succeed even though we haven't waited for heartbeatSeconds + // because priceLastUpdatedAt was reset to 0 + vm.prank(pusher); + scheduler.updatePriceFeeds(subscriptionId, updateData); + + // Verify the update was processed + (, SchedulerState.SubscriptionStatus memory status) = scheduler + .getSubscription(subscriptionId); + assertEq( + status.priceLastUpdatedAt, + publishTime2, + "Second update should be processed with new timestamp" + ); + + // Verify that normal heartbeat criteria apply again for subsequent updates + uint64 publishTime3 = SafeCast.toUint64(block.timestamp + 10); // Only 10 seconds later + (priceFeeds, slots) = createMockPriceFeedsWithSlots(publishTime3, 3); + mockParsePriceFeedUpdatesWithSlots(pyth, priceFeeds, slots); + updateData = createMockUpdateData(priceFeeds); + + // This should fail because we haven't waited for heartbeatSeconds since the last update + vm.expectRevert( + abi.encodeWithSelector(UpdateConditionsNotMet.selector) + ); + vm.prank(pusher); + scheduler.updatePriceFeeds(subscriptionId, updateData); + } + function testcreateSubscriptionWithInsufficientFundsReverts() public { uint8 numFeeds = 2; SchedulerState.SubscriptionParams From d9be0bdb3f42ed0073ce5d19d722c69d90c311f2 Mon Sep 17 00:00:00 2001 From: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Date: Tue, 27 May 2025 15:54:47 +0000 Subject: [PATCH 2/4] fix(pulse): reset priceLastUpdatedAt only when new price IDs are added Co-Authored-By: Tejas Badadare --- .../contracts/contracts/pulse/Scheduler.sol | 46 ++++++++----------- .../contracts/forge-test/PulseScheduler.t.sol | 2 +- 2 files changed, 20 insertions(+), 28 deletions(-) diff --git a/target_chains/ethereum/contracts/contracts/pulse/Scheduler.sol b/target_chains/ethereum/contracts/contracts/pulse/Scheduler.sol index 5e89c9561e..5eee39b8b4 100644 --- a/target_chains/ethereum/contracts/contracts/pulse/Scheduler.sol +++ b/target_chains/ethereum/contracts/contracts/pulse/Scheduler.sol @@ -131,14 +131,14 @@ abstract contract Scheduler is IScheduler, SchedulerState { } // Clear price updates for removed price IDs before updating params - bool priceIdsChanged = _clearRemovedPriceUpdates( + bool newPriceIdsAdded = _clearRemovedPriceUpdates( subscriptionId, currentParams.priceIds, newParams.priceIds ); - // Reset priceLastUpdatedAt to 0 if price IDs have changed - if (priceIdsChanged) { + // Reset priceLastUpdatedAt to 0 if new price IDs were added + if (newPriceIdsAdded) { _state.subscriptionStatuses[subscriptionId].priceLastUpdatedAt = 0; } @@ -216,18 +216,13 @@ abstract contract Scheduler is IScheduler, SchedulerState { * @param subscriptionId The ID of the subscription being updated. * @param currentPriceIds The array of price IDs currently associated with the subscription. * @param newPriceIds The new array of price IDs for the subscription. - * @return priceIdsChanged True if the price IDs list has changed (additions or removals), false otherwise. + * @return newPriceIdsAdded True if any new price IDs were added, false otherwise. */ function _clearRemovedPriceUpdates( uint256 subscriptionId, bytes32[] storage currentPriceIds, bytes32[] memory newPriceIds - ) internal returns (bool priceIdsChanged) { - // Check if the arrays have different lengths, which means the price IDs have changed - if (currentPriceIds.length != newPriceIds.length) { - priceIdsChanged = true; - } - + ) internal returns (bool newPriceIdsAdded) { // Iterate through old price IDs for (uint i = 0; i < currentPriceIds.length; i++) { bytes32 oldPriceId = currentPriceIds[i]; @@ -244,33 +239,30 @@ abstract contract Scheduler is IScheduler, SchedulerState { // If not found in the new list, delete its stored update data if (!found) { delete _state.priceUpdates[subscriptionId][oldPriceId]; - priceIdsChanged = true; } } // Check if any new price IDs were added - if (!priceIdsChanged) { - for (uint i = 0; i < newPriceIds.length; i++) { - bytes32 newPriceId = newPriceIds[i]; - bool found = false; - - // Check if the new price ID exists in the current list - for (uint j = 0; j < currentPriceIds.length; j++) { - if (currentPriceIds[j] == newPriceId) { - found = true; - break; - } - } + for (uint i = 0; i < newPriceIds.length; i++) { + bytes32 newPriceId = newPriceIds[i]; + bool found = false; - // If a new price ID was added, mark as changed - if (!found) { - priceIdsChanged = true; + // Check if the new price ID exists in the current list + for (uint j = 0; j < currentPriceIds.length; j++) { + if (currentPriceIds[j] == newPriceId) { + found = true; break; } } + + // If a new price ID was added, mark as changed + if (!found) { + newPriceIdsAdded = true; + break; + } } - return priceIdsChanged; + return newPriceIdsAdded; } function updatePriceFeeds( diff --git a/target_chains/ethereum/contracts/forge-test/PulseScheduler.t.sol b/target_chains/ethereum/contracts/forge-test/PulseScheduler.t.sol index c80a161f5b..1482cd2873 100644 --- a/target_chains/ethereum/contracts/forge-test/PulseScheduler.t.sol +++ b/target_chains/ethereum/contracts/forge-test/PulseScheduler.t.sol @@ -398,7 +398,7 @@ contract SchedulerTest is Test, SchedulerEvents, PulseSchedulerTestUtils { assertEq( status.priceLastUpdatedAt, 0, - "priceLastUpdatedAt should be reset to 0 after changing price IDs" + "priceLastUpdatedAt should be reset to 0 after adding new price IDs" ); // 4. Verify immediate update is possible From 4e88b552c167c9d5699f18802a2ebbadf93ffa1d Mon Sep 17 00:00:00 2001 From: Tejas Badadare Date: Tue, 27 May 2025 09:19:43 -0700 Subject: [PATCH 3/4] test: fix test --- .../ethereum/contracts/forge-test/PulseScheduler.t.sol | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/target_chains/ethereum/contracts/forge-test/PulseScheduler.t.sol b/target_chains/ethereum/contracts/forge-test/PulseScheduler.t.sol index 1482cd2873..0de7d2cb7f 100644 --- a/target_chains/ethereum/contracts/forge-test/PulseScheduler.t.sol +++ b/target_chains/ethereum/contracts/forge-test/PulseScheduler.t.sol @@ -380,12 +380,10 @@ contract SchedulerTest is Test, SchedulerEvents, PulseSchedulerTestUtils { "priceLastUpdatedAt should be set to the first update timestamp" ); - // 2. Update subscription to change price IDs + // 2. Update subscription to add price IDs (SchedulerState.SubscriptionParams memory currentParams, ) = scheduler .getSubscription(subscriptionId); - - // Create new price IDs (different from the original ones) - bytes32[] memory newPriceIds = createPriceIds(3); // Different number of price IDs + bytes32[] memory newPriceIds = createPriceIds(3); SchedulerState.SubscriptionParams memory newParams = currentParams; newParams.priceIds = newPriceIds; From bc64ced42e60f945683eedbf30b4e9d3625b3af8 Mon Sep 17 00:00:00 2001 From: Tejas Badadare Date: Tue, 27 May 2025 09:31:49 -0700 Subject: [PATCH 4/4] test: fix refs to mockParsePriceFeedUpdatesWithSlots --- .../ethereum/contracts/forge-test/PulseScheduler.t.sol | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/target_chains/ethereum/contracts/forge-test/PulseScheduler.t.sol b/target_chains/ethereum/contracts/forge-test/PulseScheduler.t.sol index 402db6ff7b..8e9d9e7456 100644 --- a/target_chains/ethereum/contracts/forge-test/PulseScheduler.t.sol +++ b/target_chains/ethereum/contracts/forge-test/PulseScheduler.t.sol @@ -359,7 +359,7 @@ contract SchedulerTest is Test, SchedulerEvents, PulseSchedulerTestUtils { PythStructs.PriceFeed[] memory priceFeeds; uint64[] memory slots; (priceFeeds, slots) = createMockPriceFeedsWithSlots(publishTime, 2); - mockParsePriceFeedUpdatesWithSlots(pyth, priceFeeds, slots); + mockParsePriceFeedUpdatesWithSlotsStrict(pyth, priceFeeds, slots); bytes[] memory updateData = createMockUpdateData(priceFeeds); vm.prank(pusher); @@ -413,7 +413,7 @@ contract SchedulerTest is Test, SchedulerEvents, PulseSchedulerTestUtils { PythStructs.PriceFeed[] memory priceFeeds; uint64[] memory slots; (priceFeeds, slots) = createMockPriceFeedsWithSlots(publishTime2, 3); // 3 feeds for new price IDs - mockParsePriceFeedUpdatesWithSlots(pyth, priceFeeds, slots); + mockParsePriceFeedUpdatesWithSlotsStrict(pyth, priceFeeds, slots); bytes[] memory updateData = createMockUpdateData(priceFeeds); // This should succeed even though we haven't waited for heartbeatSeconds @@ -433,7 +433,7 @@ contract SchedulerTest is Test, SchedulerEvents, PulseSchedulerTestUtils { // Verify that normal heartbeat criteria apply again for subsequent updates uint64 publishTime3 = SafeCast.toUint64(block.timestamp + 10); // Only 10 seconds later (priceFeeds, slots) = createMockPriceFeedsWithSlots(publishTime3, 3); - mockParsePriceFeedUpdatesWithSlots(pyth, priceFeeds, slots); + mockParsePriceFeedUpdatesWithSlotsStrict(pyth, priceFeeds, slots); updateData = createMockUpdateData(priceFeeds); // This should fail because we haven't waited for heartbeatSeconds since the last update