Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clashing items on the reviewer and crafter checklists #37

Open
oddaf opened this issue Aug 12, 2024 · 0 comments
Open

Clashing items on the reviewer and crafter checklists #37

oddaf opened this issue Aug 12, 2024 · 0 comments

Comments

@oddaf
Copy link
Member

oddaf commented Aug 12, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant