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

Detect issues in parent initializer calls #1095

Open
wants to merge 54 commits into
base: master
Choose a base branch
from

Conversation

ericglau
Copy link
Member

@ericglau ericglau commented Oct 25, 2024

Reports if a contract is non-abstract and any of the following are true:

  1. Missing initializer: This contract does not appear to have an initializer, but parent contracts require initialization.
  2. Missing initializer call: This contract's initializer is missing a call to a parent initializer.
  3. Duplicate initializer call: This contract has duplicate calls to the same parent initializer function.
  4. Incorrect initializer order (warning): This contract does not call parent initializers in the correct order.

Note that reinitializers will be ignored for these validations.


Future improvements:

  • Allow users to annotate that a function is an initializer - see Detect issues in parent initializer calls #1095 (comment)
  • Warn that reinitializers (if detected) are not included in validations
  • Transitive parents are treated as not requiring initialization if a parent initializer calls them. This PR checks that the remaining parent contracts are initialized, but some parent contracts could have multiple initializer functions, where not all of those functions actually initialize their transitive parents. Add tracking of which specific parent initializers are called, so the rest's transitive parent calls can be reported as missing.

Fixes #160

@ericglau ericglau changed the title Check for initializer errors Detect issues in parent initializer calls Dec 10, 2024
@ericglau ericglau marked this pull request as ready for review December 10, 2024 21:01
@ericglau ericglau requested a review from a team December 10, 2024 21:01
@ericglau ericglau requested a review from Amxx December 17, 2024 22:32
(fnDef.modifiers.some(modifier =>
['initializer', 'reinitializer', 'onlyInitializing'].includes(modifier.modifierName.name),
) ||
['initialize', 'initializer', 'reinitialize', 'reinitializer'].includes(fnDef.name)) &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For another PR, I think it may be useful if we annotate initializers with @custom:initializer or similar.
The idea is that there are contracts with initializers that don't follow the initialize or initializer convention in its name.

A concrete example is any of the Signers we created for ERC4337 accounts:

abstract contract SignerRSA is AbstractSigner {
    error SignerRSAUninitializedSigner(bytes e, bytes n);

    bytes private _e;
    bytes private _n;

    /// @custom:initializer true
    function _initializeSigner(bytes memory e, bytes memory n) internal {
        if (_e.length != 0 || _n.length != 0) revert SignerRSAUninitializedSigner(e, n);
        _e = e;
        _n = n;
    }

...
}

packages/core/src/validate/run.ts Outdated Show resolved Hide resolved
@ericglau ericglau marked this pull request as ready for review January 11, 2025 06:17
@ericglau ericglau requested a review from ernestognw January 11, 2025 06:19
@ericglau ericglau marked this pull request as draft January 15, 2025 20:46
@ericglau
Copy link
Member Author

Added recursive checks in the initializer function, to detect cases where its called functions (e.g. helper functions or parent initializer calls) may have duplicate calls or incorrect order of calls to parent initializers.

@ericglau ericglau marked this pull request as ready for review January 16, 2025 01:59
@ericglau
Copy link
Member Author

Ready for re-re-review! @Amxx @ernestognw

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

Successfully merging this pull request may close these issues.

Detect issues in parent initializer calls
3 participants