Skip to content

Commit 5354fe6

Browse files
tejasbadadaretejasbadadare
committed
refactor: extract out subscription parameter validation and improve error handling
1 parent 3ef5e7f commit 5354fe6

File tree

4 files changed

+220
-63
lines changed

4 files changed

+220
-63
lines changed

Diff for: ‎target_chains/ethereum/contracts/contracts/pulse/scheduler/Scheduler.sol

+84-59
Original file line numberDiff line numberDiff line change
@@ -23,35 +23,8 @@ abstract contract Scheduler is IScheduler, SchedulerState {
2323
function createSubscription(
2424
SubscriptionParams memory subscriptionParams
2525
) external payable override returns (uint256 subscriptionId) {
26-
if (
27-
subscriptionParams.priceIds.length > MAX_PRICE_IDS_PER_SUBSCRIPTION
28-
) {
29-
revert TooManyPriceIds(
30-
subscriptionParams.priceIds.length,
31-
MAX_PRICE_IDS_PER_SUBSCRIPTION
32-
);
33-
}
34-
35-
// Validate update criteria
36-
if (
37-
!subscriptionParams.updateCriteria.updateOnHeartbeat &&
38-
!subscriptionParams.updateCriteria.updateOnDeviation
39-
) {
40-
revert InvalidUpdateCriteria();
41-
}
42-
43-
// If gas config is unset, set it to the default (100x multipliers)
44-
if (
45-
subscriptionParams.gasConfig.maxBaseFeeMultiplierCapPct == 0 ||
46-
subscriptionParams.gasConfig.maxPriorityFeeMultiplierCapPct == 0
47-
) {
48-
subscriptionParams
49-
.gasConfig
50-
.maxPriorityFeeMultiplierCapPct = DEFAULT_MAX_PRIORITY_FEE_MULTIPLIER_CAP_PCT;
51-
subscriptionParams
52-
.gasConfig
53-
.maxBaseFeeMultiplierCapPct = DEFAULT_MAX_BASE_FEE_MULTIPLIER_CAP_PCT;
54-
}
26+
// Validate params and set default gas config
27+
_validateAndPrepareSubscriptionParams(subscriptionParams);
5528

5629
// Calculate minimum balance required for this subscription
5730
uint256 minimumBalance = this.getMinimumBalance(
@@ -133,34 +106,8 @@ abstract contract Scheduler is IScheduler, SchedulerState {
133106
return;
134107
}
135108

136-
// Validate parameters for active or to-be-activated subscriptions
137-
if (newParams.priceIds.length > MAX_PRICE_IDS_PER_SUBSCRIPTION) {
138-
revert TooManyPriceIds(
139-
newParams.priceIds.length,
140-
MAX_PRICE_IDS_PER_SUBSCRIPTION
141-
);
142-
}
143-
144-
// Validate update criteria
145-
if (
146-
!newParams.updateCriteria.updateOnHeartbeat &&
147-
!newParams.updateCriteria.updateOnDeviation
148-
) {
149-
revert InvalidUpdateCriteria();
150-
}
151-
152-
// If gas config is unset, set it to the default (100x multipliers)
153-
if (
154-
newParams.gasConfig.maxBaseFeeMultiplierCapPct == 0 ||
155-
newParams.gasConfig.maxPriorityFeeMultiplierCapPct == 0
156-
) {
157-
newParams
158-
.gasConfig
159-
.maxPriorityFeeMultiplierCapPct = DEFAULT_MAX_PRIORITY_FEE_MULTIPLIER_CAP_PCT;
160-
newParams
161-
.gasConfig
162-
.maxBaseFeeMultiplierCapPct = DEFAULT_MAX_BASE_FEE_MULTIPLIER_CAP_PCT;
163-
}
109+
// Validate the new parameters, including setting default gas config
110+
_validateAndPrepareSubscriptionParams(newParams);
164111

165112
// Handle activation/deactivation
166113
if (!wasActive && willBeActive) {
@@ -195,6 +142,83 @@ abstract contract Scheduler is IScheduler, SchedulerState {
195142
emit SubscriptionUpdated(subscriptionId);
196143
}
197144

145+
/**
146+
* @notice Validates subscription parameters and sets default gas config if needed.
147+
* @dev This function modifies the passed-in params struct in place for gas config defaults.
148+
* @param params The subscription parameters to validate and prepare.
149+
*/
150+
function _validateAndPrepareSubscriptionParams(
151+
SubscriptionParams memory params
152+
) internal pure {
153+
// No zero‐feed subscriptions
154+
if (params.priceIds.length == 0) {
155+
revert EmptyPriceIds();
156+
}
157+
158+
// Price ID limits and uniqueness
159+
if (params.priceIds.length > MAX_PRICE_IDS_PER_SUBSCRIPTION) {
160+
revert TooManyPriceIds(
161+
params.priceIds.length,
162+
MAX_PRICE_IDS_PER_SUBSCRIPTION
163+
);
164+
}
165+
for (uint i = 0; i < params.priceIds.length; i++) {
166+
for (uint j = i + 1; j < params.priceIds.length; j++) {
167+
if (params.priceIds[i] == params.priceIds[j]) {
168+
revert DuplicatePriceId(params.priceIds[i]);
169+
}
170+
}
171+
}
172+
173+
// Whitelist size limit and uniqueness
174+
if (params.readerWhitelist.length > MAX_READER_WHITELIST_SIZE) {
175+
revert TooManyWhitelistedReaders(
176+
params.readerWhitelist.length,
177+
MAX_READER_WHITELIST_SIZE
178+
);
179+
}
180+
for (uint i = 0; i < params.readerWhitelist.length; i++) {
181+
for (uint j = i + 1; j < params.readerWhitelist.length; j++) {
182+
if (params.readerWhitelist[i] == params.readerWhitelist[j]) {
183+
revert DuplicateWhitelistAddress(params.readerWhitelist[i]);
184+
}
185+
}
186+
}
187+
188+
// Validate update criteria
189+
if (
190+
!params.updateCriteria.updateOnHeartbeat &&
191+
!params.updateCriteria.updateOnDeviation
192+
) {
193+
revert InvalidUpdateCriteria();
194+
}
195+
if (
196+
params.updateCriteria.updateOnHeartbeat &&
197+
params.updateCriteria.heartbeatSeconds == 0
198+
) {
199+
revert InvalidUpdateCriteria();
200+
}
201+
if (
202+
params.updateCriteria.updateOnDeviation &&
203+
params.updateCriteria.deviationThresholdBps == 0
204+
) {
205+
revert InvalidUpdateCriteria();
206+
}
207+
208+
// If gas config is unset, set it to the default (100x multipliers)
209+
if (
210+
params.gasConfig.maxBaseFeeMultiplierCapPct == 0 ||
211+
params.gasConfig.maxPriorityFeeMultiplierCapPct == 0
212+
) {
213+
params
214+
.gasConfig
215+
.maxPriorityFeeMultiplierCapPct = DEFAULT_MAX_PRIORITY_FEE_MULTIPLIER_CAP_PCT;
216+
params
217+
.gasConfig
218+
.maxBaseFeeMultiplierCapPct = DEFAULT_MAX_BASE_FEE_MULTIPLIER_CAP_PCT;
219+
}
220+
}
221+
198222
/**
199223
* @notice Internal helper to clear stored PriceFeed data for price IDs removed from a subscription.
200224
* @param subscriptionId The ID of the subscription being updated.
@@ -265,10 +289,11 @@ abstract contract Scheduler is IScheduler, SchedulerState {
265289

266290
// Parse price feed updates with an expected timestamp range of [-10s, now]
267291
// We will validate the trigger conditions and timestamps ourselves
268-
// using the returned PriceFeeds.
269292
uint64 curTime = SafeCast.toUint64(block.timestamp);
270293
uint64 maxPublishTime = curTime + FUTURE_TIMESTAMP_MAX_VALIDITY_PERIOD;
271-
uint64 minPublishTime = curTime - PAST_TIMESTAMP_MAX_VALIDITY_PERIOD;
294+
uint64 minPublishTime = curTime > PAST_TIMESTAMP_MAX_VALIDITY_PERIOD
295+
? curTime - PAST_TIMESTAMP_MAX_VALIDITY_PERIOD
296+
: 0;
272297
PythStructs.PriceFeed[] memory priceFeeds;
273298
uint64[] memory slots;
274299
(priceFeeds, slots) = pyth.parsePriceFeedUpdatesWithSlots{

Diff for: ‎target_chains/ethereum/contracts/contracts/pulse/scheduler/SchedulerErrors.sol

+17-4
Original file line numberDiff line numberDiff line change
@@ -2,18 +2,31 @@
22

33
pragma solidity ^0.8.0;
44

5+
// Authorization errors
6+
error Unauthorized();
7+
8+
// Subscription state errors
59
error InactiveSubscription();
610
error InsufficientBalance();
7-
error Unauthorized();
11+
error IllegalPermanentSubscriptionModification();
12+
13+
// Price feed errors
814
error InvalidPriceId(bytes32 providedPriceId, bytes32 expectedPriceId);
915
error InvalidPriceIdsLength(bytes32 providedLength, bytes32 expectedLength);
16+
error EmptyPriceIds();
17+
error TooManyPriceIds(uint256 provided, uint256 maximum);
18+
error DuplicatePriceId(bytes32 priceId);
19+
error PriceSlotMismatch();
20+
21+
// Update criteria errors
1022
error InvalidUpdateCriteria();
1123
error InvalidGasConfig();
12-
error PriceSlotMismatch();
13-
error TooManyPriceIds(uint256 provided, uint256 maximum);
1424
error UpdateConditionsNotMet();
15-
error IllegalPermanentSubscriptionModification();
1625
error TimestampOlderThanLastUpdate(
1726
uint256 providedUpdateTimestamp,
1827
uint256 lastUpdatedAt
1928
);
29+
30+
// Whitelist errors
31+
error TooManyWhitelistedReaders(uint256 provided, uint256 maximum);
32+
error DuplicateWhitelistAddress(address addr);

Diff for: ‎target_chains/ethereum/contracts/contracts/pulse/scheduler/SchedulerState.sol

+2
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,8 @@ import "@pythnetwork/pyth-sdk-solidity/PythStructs.sol";
77
contract SchedulerState {
88
/// Maximum number of price feeds per subscription
99
uint8 public constant MAX_PRICE_IDS_PER_SUBSCRIPTION = 255;
10+
/// Maximum number of addresses in the reader whitelist
11+
uint8 public constant MAX_READER_WHITELIST_SIZE = 255;
1012
/// Default max gas multiplier
1113
uint32 public constant DEFAULT_MAX_BASE_FEE_MULTIPLIER_CAP_PCT = 10_000;
1214
/// Default max fee multiplier

Diff for: ‎target_chains/ethereum/contracts/forge-test/PulseScheduler.t.sol

+117
Original file line numberDiff line numberDiff line change
@@ -1341,6 +1341,123 @@ contract SchedulerTest is Test, SchedulerEvents, PulseTestUtils {
13411341
});
13421342
}
13431343

1344+
function testSubscriptionParamValidations() public {
1345+
uint256 initialSubId = 0; // For update tests
1346+
1347+
// === Empty Price IDs ===
1348+
SchedulerState.SubscriptionParams
1349+
memory emptyPriceIdsParams = _createDefaultSubscriptionParams(1);
1350+
emptyPriceIdsParams.priceIds = new bytes32[](0);
1351+
1352+
vm.expectRevert(abi.encodeWithSelector(EmptyPriceIds.selector));
1353+
scheduler.createSubscription{value: 1 ether}(emptyPriceIdsParams);
1354+
1355+
initialSubId = addTestSubscription(); // Create a valid one for update test
1356+
vm.expectRevert(abi.encodeWithSelector(EmptyPriceIds.selector));
1357+
scheduler.updateSubscription(initialSubId, emptyPriceIdsParams);
1358+
1359+
// === Duplicate Price IDs ===
1360+
SchedulerState.SubscriptionParams
1361+
memory duplicatePriceIdsParams = _createDefaultSubscriptionParams(
1362+
2
1363+
);
1364+
bytes32 duplicateId = duplicatePriceIdsParams.priceIds[0];
1365+
duplicatePriceIdsParams.priceIds[1] = duplicateId;
1366+
1367+
vm.expectRevert(
1368+
abi.encodeWithSelector(DuplicatePriceId.selector, duplicateId)
1369+
);
1370+
scheduler.createSubscription{value: 1 ether}(duplicatePriceIdsParams);
1371+
1372+
initialSubId = addTestSubscription();
1373+
vm.expectRevert(
1374+
abi.encodeWithSelector(DuplicatePriceId.selector, duplicateId)
1375+
);
1376+
scheduler.updateSubscription(initialSubId, duplicatePriceIdsParams);
1377+
1378+
// === Too Many Whitelist Readers ===
1379+
SchedulerState.SubscriptionParams
1380+
memory largeWhitelistParams = _createDefaultSubscriptionParams(1);
1381+
uint whitelistLength = uint(scheduler.MAX_READER_WHITELIST_SIZE()) + 1;
1382+
address[] memory largeWhitelist = new address[](whitelistLength);
1383+
for (uint i = 0; i < whitelistLength; i++) {
1384+
largeWhitelist[i] = address(uint160(i + 1)); // Unique addresses
1385+
}
1386+
largeWhitelistParams.readerWhitelist = largeWhitelist;
1387+
1388+
vm.expectRevert(
1389+
abi.encodeWithSelector(
1390+
TooManyWhitelistedReaders.selector,
1391+
largeWhitelist.length,
1392+
scheduler.MAX_READER_WHITELIST_SIZE()
1393+
)
1394+
);
1395+
scheduler.createSubscription{value: 1 ether}(largeWhitelistParams);
1396+
1397+
initialSubId = addTestSubscription();
1398+
vm.expectRevert(
1399+
abi.encodeWithSelector(
1400+
TooManyWhitelistedReaders.selector,
1401+
largeWhitelist.length,
1402+
scheduler.MAX_READER_WHITELIST_SIZE()
1403+
)
1404+
);
1405+
scheduler.updateSubscription(initialSubId, largeWhitelistParams);
1406+
1407+
// === Duplicate Whitelist Address ===
1408+
SchedulerState.SubscriptionParams
1409+
memory duplicateWhitelistParams = _createDefaultSubscriptionParams(
1410+
1
1411+
);
1412+
address[] memory duplicateWhitelist = new address[](2);
1413+
duplicateWhitelist[0] = address(reader);
1414+
duplicateWhitelist[1] = address(reader); // Duplicate
1415+
duplicateWhitelistParams.readerWhitelist = duplicateWhitelist;
1416+
1417+
vm.expectRevert(
1418+
abi.encodeWithSelector(
1419+
DuplicateWhitelistAddress.selector,
1420+
address(reader)
1421+
)
1422+
);
1423+
scheduler.createSubscription{value: 1 ether}(duplicateWhitelistParams);
1424+
1425+
initialSubId = addTestSubscription();
1426+
vm.expectRevert(
1427+
abi.encodeWithSelector(
1428+
DuplicateWhitelistAddress.selector,
1429+
address(reader)
1430+
)
1431+
);
1432+
scheduler.updateSubscription(initialSubId, duplicateWhitelistParams);
1433+
1434+
// === Invalid Heartbeat (Zero Seconds) ===
1435+
SchedulerState.SubscriptionParams
1436+
memory invalidHeartbeatParams = _createDefaultSubscriptionParams(1);
1437+
invalidHeartbeatParams.updateCriteria.updateOnHeartbeat = true;
1438+
invalidHeartbeatParams.updateCriteria.heartbeatSeconds = 0; // Invalid
1439+
1440+
vm.expectRevert(abi.encodeWithSelector(InvalidUpdateCriteria.selector));
1441+
scheduler.createSubscription{value: 1 ether}(invalidHeartbeatParams);
1442+
1443+
initialSubId = addTestSubscription();
1444+
vm.expectRevert(abi.encodeWithSelector(InvalidUpdateCriteria.selector));
1445+
scheduler.updateSubscription(initialSubId, invalidHeartbeatParams);
1446+
1447+
// === Invalid Deviation (Zero Bps) ===
1448+
SchedulerState.SubscriptionParams
1449+
memory invalidDeviationParams = _createDefaultSubscriptionParams(1);
1450+
invalidDeviationParams.updateCriteria.updateOnDeviation = true;
1451+
invalidDeviationParams.updateCriteria.deviationThresholdBps = 0; // Invalid
1452+
1453+
vm.expectRevert(abi.encodeWithSelector(InvalidUpdateCriteria.selector));
1454+
scheduler.createSubscription{value: 1 ether}(invalidDeviationParams);
1455+
1456+
initialSubId = addTestSubscription();
1457+
vm.expectRevert(abi.encodeWithSelector(InvalidUpdateCriteria.selector));
1458+
scheduler.updateSubscription(initialSubId, invalidDeviationParams);
1459+
}
1460+
13441461
// Required to receive ETH when withdrawing funds
13451462
receive() external payable {}
13461463
}

0 commit comments

Comments
 (0)