Consider making `_hasSchedulePassed` internal virtual in AccessControlDefaultAdminRules

[QEDK]

In AccessControlDefaultAdminRules.sol, you can't execute on inclusion block + scheduled delay but rather, inclusion block + scheduled delay + 1. This is a rather strange pattern since time limits are usually enforced as block.timestamp >= expectedTimestamp. I presume this is intended to induce a delay when schedule timestamp == block.timestamp but has been left undocumented.

💻 Environment
v5.3.0 OZ
v1 Foundry

📝 Details
I think this should be documented or fixed. I don't strictly view it as a bug but it's a definite pitfall.

🔢 Code to reproduce bug

• multicall([...], [...])
  • beginDefaultAdminTransfer(...)
  • acceptDefaultAdminTransfer()

[ernestognw]

Hi @QEDK, thanks for reporting

If I remember correctly we did it on purpose to really enforce two steps on transferring the admin. Otherwise, if the delay is 0 the admin can be transferred in a single transaction. The general principle would be to just avoid delays to take effect within the same block. Same principle should be applied to AccessManager

[QEDK]

Hi @QEDK, thanks for reporting

If I remember correctly we did it on purpose to really enforce two steps on transferring the admin. Otherwise, if the delay is 0 the admin can be transferred in a single transaction. The general principle would be to just avoid delays to take effect within the same block. Same principle should be applied to AccessManager

Yeah I figured, would be good to add it as a point here and in the docs. I can PR either, just waiting on consensus here.

[ernestognw]

Yeah it makes sense to add a bullet there. Feel free to open a PR 😄

Also, curious to hear what you are building with the AccessControlDefaultAdminRules contract. Do you have any opinion on that over the AccessManager?

[QEDK]

Yeah it makes sense to add a bullet there. Feel free to open a PR 😄

Also, curious to hear what you are building with the AccessControlDefaultAdminRules contract. Do you have any opinion on that over the AccessManager?

We use it as the access control for the AvailBridge. It works best for us because we only have one role beyond the admin. AccessManager is pretty encompassing of all needs but only if you need the ability for an arbitrary number of arbitrary roles with scheduling. Also, do you have any thoughts if we should have a standard delay (i.e. <=)? I will do the PR accordingly.

[ernestognw]

Got it, thanks for your feedback. I understand the AccessManager would be overkill for AvailBridge.

So for a standard delay one thing we can do is making _hasSchedulePassed internal virtual, so that you guys can override it with the desired behavior. However, I'd like to get someone else's opinion

cc @arr00 @luiz-lvj @gonzaotc

[QEDK]

Got it, thanks for your feedback. I understand the AccessManager would be overkill for AvailBridge.

So for a standard delay one thing we can do is making _hasSchedulePassed internal virtual, so that you guys can override it with the desired behavior. However, I'd like to get someone else's opinion

cc @arr00 @luiz-lvj @gonzaotc

Opened a PR to fix the documentation! I can PR the other change if you guys assent as well. 🖖

P.S. Do I need to update the changeset?

[ernestognw]

Just merged your PR, thanks @QEDK
Will keep this issue open if anyone else wants to weight in regarding making _hasSchedulePassed virtual

P.S. Do I need to update the changeset?

No worries, it's just documentation updates so we don't need a changelog entry