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

Fix validateUpgradeSafety to handle undefined ast #1127

Closed

Conversation

Setland34
Copy link

@Setland34 Setland34 commented Feb 13, 2025

Fixes #1126

Handle undefined ast in validateUpgradeSafety.

  • Add a check in checkNamespacesOutsideContract to verify if solcOutput.sources[source].ast is undefined before accessing its nodes property.
  • Log a warning and skip the check if solcOutput.sources[source].ast is undefined.
  • Add a test case in validate-upgrade-safety.test.ts to verify the behavior when solcOutput.sources[source].ast is undefined.

For more details, open the Copilot Workspace session.

Fixes OpenZeppelin#1126

Handle undefined `ast` in `validateUpgradeSafety`.

* Add a check in `checkNamespacesOutsideContract` to verify if `solcOutput.sources[source].ast` is undefined before accessing its `nodes` property.
* Log a warning and skip the check if `solcOutput.sources[source].ast` is undefined.
* Add a test case in `validate-upgrade-safety.test.ts` to verify the behavior when `solcOutput.sources[source].ast` is undefined.

---

For more details, open the [Copilot Workspace session](https://copilot-workspace.githubnext.com/OpenZeppelin/openzeppelin-upgrades/issues/1126?shareId=XXXX-XXXX-XXXX-XXXX).
@ericglau
Copy link
Member

Thanks for this PR. From the original issue, it looks like solcOutput.sources[source] is undefined, rather than .ast being undefined.

We'll investigate the original issue further, but this doesn't look like the right approach. The AST is required for validations so missing AST should not be suppressed with a warning. The checks in packages/core/src/cli/validate/build-info-file.ts already look for incomplete build info files and missing storage layouts, and from my testing, the AST should be present if these checks passed.

I'll close this PR but feel free to continue the discussion in the original issue or here if needed.

@ericglau ericglau closed this Feb 13, 2025
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.

validateUpgradeSafety: Cannot read properties of undefined (reading 'ast')
2 participants