Default user limits per DON family #241
Conversation
| // - if this user has no override, use the default user limit set for this DON | ||
| // - if this user has an override, use that unless it is bigger than the global cap set for this DON | ||
| uint32 cap = cfg.defaultUserLimit; | ||
| if (cfg.userOverride[owner].enabled) { | ||
| cap = cfg.userOverride[owner].value < cfg.limitValue.value ? cfg.userOverride[owner].value : cfg.limitValue.value; | ||
| } |
There was a problem hiding this comment.
What if we want to intentionally limit some users below the don-user limit? A scenario here would be to limit users after pausing their workflows so they cannot reactivate them.
There was a problem hiding this comment.
Well, in that case, I think the only way to do that would be to explicitly set the user override to either zero (if we want to disallow deploying workflows completely) or set it to a much lower value than the current defaultUserLimit.
| // It reverts with CallerIsNotWorkflowOwner | ||
| vm.prank(s_owner); | ||
| s_registry.setDONLimit(s_donFamily, 10, true); | ||
| s_registry.setDONLimit(s_donFamily, 100, 10); |
There was a problem hiding this comment.
Nit for future (don't change it now) - it helps to readability to have vars for these numbers, e.g., DON_WORKFLOWS_LIMIT and DEFAULT_USER_WORKFLOWS_LIMIT
| eventType: EventType.DONCapacitySet, | ||
| timestamp: uint32(block.timestamp), | ||
| payload: abi.encode(donHash, newCapacity) | ||
| payload: abi.encode(donHash, donLimit) |
There was a problem hiding this comment.
just verifying we don't need userDefaultLimit here?
There was a problem hiding this comment.
Deividas said it's not needed in one of his previous comments so I removed it
| if (!ov.enabled) { | ||
| // → was OFF, now turning ON with new cap | ||
| // → was OFF, now turning ON with new the new limit override | ||
| ov.enabled = true; | ||
| ov.value = limit; | ||
| ov.value = userLimit; | ||
| s_donOverrideUsers[donHash].add(user); | ||
| emit UserDONLimitSet(user, donFamily, limit); | ||
| } else if (ov.value != limit) { | ||
| // → was ON with a different cap, so update it | ||
| ov.value = limit; | ||
| emit UserDONLimitSet(user, donFamily, limit); | ||
| emit UserDONLimitSet(user, donFamily, userLimit); | ||
| } else if (ov.value != userLimit) { | ||
| // → was ON with a different limit, so update it with a new override | ||
| ov.value = userLimit; | ||
| emit UserDONLimitSet(user, donFamily, userLimit); | ||
| } else { | ||
| // → already ON at exactly this cap, nothing to do | ||
| // → already ON at exactly this limit, nothing to do | ||
| return; | ||
| } |
There was a problem hiding this comment.
nit. imo a lot of this logic can be simplified by simply doing
ov.enabled = true;
ov.value = userLimit;
emit UserDONLimitSet(user, donFamily, userLimit);
There was a problem hiding this comment.
there's not much extra gas sent when value is written to the same value (if the storage slot doesn't change) and also prevents silent acknowledgement by the contract without any logs
There was a problem hiding this comment.
I've simplified it, please take a look
| // Global limit must be explicitly enabled (zero or missing = disallowed) | ||
| if (!cfg.limitValue.enabled) revert DonLimitNotSet(donFamily); | ||
| // global limit must be explicitly enabled (zero means it is disabled) | ||
| if (cfg.limit == 0) revert DonLimitNotSet(donFamily); |
There was a problem hiding this comment.
just checking, is this function called when a workflow is deleted as well? in that case we would want to allow that
There was a problem hiding this comment.
No, it is called on upsert, activate, activateBatch and updateDONFamily
When setting a DON family active workflows limit, ensure that the default user limit is also configured. With a separate function, the contract admin can always override the default user limit, but never above the global DON family limit. The default user limit ensures fairness for all users who depend on having access to a chunk of a DON family capacity.