Module Management

[0xJem]

Completes #9

Implements:

• Module keycodes (Keycode) and versioned keycodes (Veecode)
• Install modules and subsequent versions
• Sunset modules
• execOnModule and prohibiting certain functions
• Continuous integration

[Oighty]

Overall, this looks good. I have a few smallish changes.

[Oighty]

I was actually hoping to avoid having JS packages in this repo, but it may be unavoidable.

I was planning to use forge fmt instead of prettier for formatting, but there isn't a linter equivalent to replace solhint.

[0xJem]

I can shift it over to forge fmt, at least

[Oighty]

I generally like the idea of having the Parent be able to call functions in the course of operations, but restrict them from being called out of scope. I don't know if this is the right pattern though.

There are a few different options.

1. We could have a function on the Module that returns a list of functions that are callable externally, similar to requestPermissions on Default modules. We could call it governanceFunctions or something. You could then check your calldata against that prior to executing in execOnModule. These could be stored on install or fetched at the beginning of this function.
2. Having a variable on the Parent contract that is set when the execOnModule function is entered. Modules could have a modifier that checks whether this variable is set on the parent contract. This could be used to separate onlyParent and onlyGovernance functionality. Requires a re-entrant call back to Parent, but probably not an issue.

[0xJem]

#2 seems more simple, as it would just be a modifier attached to the protected functions, e.g.

modifier noGovernance() {
    if (Module(owner).isExecOnModule()) revert NoGovernance();

    _;
}

[0xJem]

I've added this - please take a look

[0xJem]

onlyInternal modifier added, along with the infra to support it

[Oighty]

I think the implementation is close, but not quite there.

I think we only need one execOnModule function, but with the format of execOnModuleInternal. To be more explicit on this:

The idea is that there are standard functions on the Auction, Derivative, etc. modules that are called in the general user flows, such as purchase. We don't want those to be able to be called by governance via execOnModule. They could be if the only access control is onlyParent. However, some functions like setMinDuration should be callable by governance.

Therefore, there are two groups:

1. Restricted functions only callable by AuctionHouse outside of execOnModule.
2. Restricted functions that should be accessible by Governance via execOnModule.

I would then propose the following refactor:

• change isExecOnModuleInternal to isExecOnModule
• delete execOnModule as currently written and replace with the execOnModuleInternal, renamed to just execOnModule
• onlyInternal modifier should check if isExecOnModule and fail if it is true.

We would then use onlyInternal for case 1 above and onlyParent for case 2 (Governance type functions).

[0xJem]

Ok that makes sense! I've pushed the changes.

[Oighty]

I think the implementation is close, but not quite there.

I think we only need one execOnModule function, but with the format of execOnModuleInternal. To be more explicit on this:

The idea is that there are standard functions on the Auction, Derivative, etc. modules that are called in the general user flows, such as purchase. We don't want those to be able to be called by governance via execOnModule. They could be if the only access control is onlyParent. However, some functions like setMinDuration should be callable by governance.

Therefore, there are two groups:

1. Restricted functions only callable by AuctionHouse outside of execOnModule.
2. Restricted functions that should be accessible by Governance via execOnModule.

I would then propose the following refactor:

• change isExecOnModuleInternal to isExecOnModule
• delete execOnModule as currently written and replace with the execOnModuleInternal, renamed to just execOnModule
• onlyInternal modifier should check if isExecOnModule and fail if it is true.

We would then use onlyInternal for case 1 above and onlyParent for case 2 (Governance type functions).

[Oighty]

Looks good. Going to merge.