makerdao/pe-checklists

Clashing items on the reviewer and crafter checklists

Opened this issue · 0 comments

The mainnet Spell reviewer and crafter checklists do not agree regarding the use of dss-interfaces.

On the reviewer checklist the use of dss-interfaces is mandatory, the CL states it SHOULD be used unless the function call is not present there:

  • Static Interfaces
    • No unused static interfaces
    • Declared static interface not present in the dss-interfaces, OTHERWISE should be imported from there

On the crafter checklist, however, there are other situations in which the use of a static interface is acceptable:

  • IF some actions require using interfaces
    • Prefer using DssExecLib actions where possible (to avoid adding interfaces where not required)
    • Avoid multi-import layout / importing from Interfaces.sol (see issue #69)
    • Prefer single import layout (e.g. import { VatAbstract } from "dss-interfaces/dss/VatAbstract.sol";)
    • Use static interfaces IF not present in dss-interfaces OR present in dss-interfaces but outdated OR only a few function interfaces are needed

Since we rarely call more than a few functions in a contract, the crafter CL pushes the crafter to go against the review item, creating unnecessary friction during the review process. Both CLs should be aligned and push for a single pattern.