Modify Excubiae framework design improve reuse of deployed specific Excubia contracts

[0xjei]

Description

The actual design of Excubiae centralises the definition of the custom access control and prevention of unwanted access logic within the _check() method. Consequently, the developer is required to extend the Excubia contract and implement the logic for the _check() method only.

This design has several advantages, including reduced gas cost and the ease of creating one's own custom contract (one method override). However, this approach presents a significant challenge when attempting to reuse the same logic defined in an Excubia within a different contract. Indeed, interfacing with an Excubia and extending or reusing the _check(), would result in a tentative update to the execution of both logics (custom access control and prevention of unwanted access).

This is a problematic scenario because it is undesirable for an external Excubia to have the ability to modify the status directly and thereby render any records of users who have already passed invalid. Furthermore, it would be highly limiting from the perspective of aggregating multiple Excubias.

With regard to this latter point, if it were not possible to reuse the aforementioned _check(), it would be necessary to deploy the precise same Excubia, which would result in increased costs and the replication of on-chain logic.

In consequence, this PR introduces a novel design comprising:

• an internal method, _check(), which will exclusively address custom access control logic; and
• an internal method, _pass(), which will exclusively prevent unwanted access.

In addition, there will be two external methods (check() and pass()), which can be called externally to ascertain whether the custom access control logic is eligible to be passed (without requesting to pass the gate) or to attempt to pass the gate.

This approach allows for the aggregation of multiple Excubia instances and the invocation of the public interface, represented by the check() function. Based on the outcomes of these checks, the implementation of the logic preventing unwanted access can be initiated in the _pass() function. This will result in a change to the state of the aggregation contract, rather than that of the individual aggregates. This approach allows for the aggregation of the aggregation contract itself, representing a significant advancement in the composability of Excubia.

The following are the disadvantages for the new design:

• Limited increased gas costs: the deployment of EASExcubia incurs an additional expense of approximately 31k gas, while the execution of setGate() and pass() requires a mere 1k and 200 gas, respectively. This is a great trade-off since you deploy once and you do not need to duplicate deploys of Excubia anymore.
• The necessity for developers to incorporate custom code for both _pass() and _check() methods.
• The potential for the inclusion of redundant, low-cost code in both methods.

Other information

Additional modifications include the introduction of a 'GateSet' event to the 'setGate()' method, the adjustment of the supported Solidity version to 0.8.20, enhancements to the comments and interfaces, and the incorporation of missing tests to achieve comprehensive coverage.

Checklist

• My code follows the style guidelines of this project
• I have performed a self-review of my code
• I have commented my code, particularly in hard-to-understand areas
• My changes generate no new warnings
• I have run yarn format and yarn compile without getting any errors
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes

[0xjei]

@sripwoud is the following CI behaviour correct?

[sripwoud]

@0xjei

@sripwoud is the following CI behaviour correct?

I think mostly correct yes. (maybe some loose ends here I need to investigate)
#8 added slither in the ci. The corresponding GH action can write comments in PR and also upload analysis results in the code scanning section. So seeing an alert as a PR comment here was expected.
About the content of the alert, it indeed looks like slither was having false positive here: I submitted an issue in their repo crytic/slither#2500 (don't delete your refactor/design branch after merging so they can still see it)

[0xjei]

@0xjei

@sripwoud is the following CI behaviour correct?

I think mostly correct yes. (maybe some loose ends here I need to investigate) #8 added slither in the ci. The corresponding GH action can write comments in PR and also upload analysis results in the code scanning section. So seeing an alert as a PR comment here was expected. About the content of the alert, it indeed looks like slither was having false positive here: I submitted an issue in their repo crytic/slither#2500 (don't delete your refactor/design branch after merging so they can still see it)

sweet, tysm for the clarification here <3

btw, is this related to tests hanging on the CI?

[sripwoud]

the hanging check was due to a misconfiguration of the branch protection rules.
There are no tests check anymore, but test (imt), test (excubiae) etc

[0xjei]

the hanging check was due to a misconfiguration of the branch protection rules. There are no tests check anymore, but test (imt), test (excubiae) etc

lovely, tysm

[ctrlc03]

maybe instead of double check (which now is techincally possible by calling check directly), we should make it more clear, maybe "double pass"? or even expand a bit more and explain that ideally pass/_pass should record the data used to pass the check and prevent the same data from being used twice

[0xjei]

yes, that's a good point. In fact, the check() is not doing besides answering with "you're eligible / you're not eligible". Therefore, instead of double check is something like "avoid to pass the gate twice".

[ctrlc03]

Thanks, left a comment

[ctrlc03]

LGTM thanks