Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,11 @@ abstract contract Scheduler is IScheduler, SchedulerState {
bool wasActive = currentParams.isActive;
bool willBeActive = newParams.isActive;

// Check for permanent subscription restrictions
if (currentParams.isPermanent) {
_validatePermanentSubscriptionUpdate(currentParams, newParams);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can also effectively disable a subscription by (1) changing the readerWhitelist or (2) making the gas config so limiting that no tx ever lands on chain?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm I think you could also do it by changing the updateCriteria

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah good points -- will disallow changing the whitelist or the config. truly "permanent."

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just realized they can also just set isActive to false 🤦 updated


// If subscription is inactive and will remain inactive, no need to validate parameters
if (!wasActive && !willBeActive) {
// Update subscription parameters
Expand Down Expand Up @@ -385,6 +390,103 @@ abstract contract Scheduler is IScheduler, SchedulerState {
revert UpdateConditionsNotMet();
}

/**
* @notice Internal helper to validate modifications to a permanent subscription.
* @param currentParams The current subscription parameters (storage).
* @param newParams The proposed new subscription parameters (memory).
*/
function _validatePermanentSubscriptionUpdate(
SubscriptionParams storage currentParams,
SubscriptionParams memory newParams
) internal view {
// Cannot disable isPermanent flag once set
if (!newParams.isPermanent) {
revert IllegalPermanentSubscriptionModification();
}

// Cannot deactivate a permanent subscription
if (!newParams.isActive) {
revert IllegalPermanentSubscriptionModification();
}

// Cannot remove price feeds from a permanent subscription
if (newParams.priceIds.length < currentParams.priceIds.length) {
revert IllegalPermanentSubscriptionModification();
}

// Check that all existing price IDs are preserved (adding is allowed, not removing)
for (uint i = 0; i < currentParams.priceIds.length; i++) {
bool found = false;
for (uint j = 0; j < newParams.priceIds.length; j++) {
if (currentParams.priceIds[i] == newParams.priceIds[j]) {
found = true;
break;
}
}
if (!found) {
revert IllegalPermanentSubscriptionModification();
}
}

// Cannot change reader whitelist settings for permanent subscriptions
if (newParams.whitelistEnabled != currentParams.whitelistEnabled) {
revert IllegalPermanentSubscriptionModification();
}

// Check if the set of addresses in the whitelist is the same
if (
newParams.readerWhitelist.length !=
currentParams.readerWhitelist.length
) {
revert IllegalPermanentSubscriptionModification();
}
uint256 n = newParams.readerWhitelist.length;
bool[] memory currentVisited = new bool[](n);
uint256 matchesFound = 0;
for (uint256 i = 0; i < n; i++) {
bool foundInCurrent = false;
for (uint256 j = 0; j < n; j++) {
if (
!currentVisited[j] &&
newParams.readerWhitelist[i] ==
currentParams.readerWhitelist[j]
) {
currentVisited[j] = true;
foundInCurrent = true;
matchesFound++;
break;
}
}
if (!foundInCurrent) {
revert IllegalPermanentSubscriptionModification();
}
}

// Cannot change update criteria for permanent subscriptions
if (
newParams.updateCriteria.updateOnHeartbeat !=
currentParams.updateCriteria.updateOnHeartbeat ||
newParams.updateCriteria.heartbeatSeconds !=
currentParams.updateCriteria.heartbeatSeconds ||
newParams.updateCriteria.updateOnDeviation !=
currentParams.updateCriteria.updateOnDeviation ||
newParams.updateCriteria.deviationThresholdBps !=
currentParams.updateCriteria.deviationThresholdBps
) {
revert IllegalPermanentSubscriptionModification();
}

// Cannot change gas config for permanent subscriptions
if (
newParams.gasConfig.maxBaseFeeMultiplierCapPct !=
currentParams.gasConfig.maxBaseFeeMultiplierCapPct ||
newParams.gasConfig.maxPriorityFeeMultiplierCapPct !=
currentParams.gasConfig.maxPriorityFeeMultiplierCapPct
) {
revert IllegalPermanentSubscriptionModification();
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these gas parameters could cause some problems still. The chain usage will change over time, and these parameters may need to be tweaked. If you can't change them, then they could initially be set too low, and then the chain gets more congested and then you don't get updates.

Maybe we need to set these to infinity for the permanent subscriptions? not sure exactly what to do.

(this is a problem we can resolve later also, so I'm approving this PR)

}

/// FETCH PRICES

/**
Expand Down Expand Up @@ -487,9 +589,7 @@ abstract contract Scheduler is IScheduler, SchedulerState {

/// BALANCE MANAGEMENT

function addFunds(
uint256 subscriptionId
) external payable override onlyManager(subscriptionId) {
function addFunds(uint256 subscriptionId) external payable override {
if (!_state.subscriptionParams[subscriptionId].isActive) {
revert InactiveSubscription();
}
Expand All @@ -508,6 +608,11 @@ abstract contract Scheduler is IScheduler, SchedulerState {
subscriptionId
];

// Prevent withdrawals from permanent subscriptions
if (params.isPermanent) {
revert IllegalPermanentSubscriptionModification();
}

if (status.balanceInWei < amount) {
revert InsufficientBalance();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ error InvalidGasConfig();
error PriceSlotMismatch();
error TooManyPriceIds(uint256 provided, uint256 maximum);
error UpdateConditionsNotMet();
error IllegalPermanentSubscriptionModification();
error TimestampOlderThanLastUpdate(
uint256 providedUpdateTimestamp,
uint256 lastUpdatedAt
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ contract SchedulerState {
address[] readerWhitelist;
bool whitelistEnabled;
bool isActive;
bool isPermanent;
UpdateCriteria updateCriteria;
GasConfig gasConfig;
}
Expand Down
Loading
Loading