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

Update docs to mention including ast #28

Merged
merged 6 commits into from
Mar 20, 2024
Merged

Conversation

gasperbr
Copy link
Contributor

With a recent update the ast has been removed from the default forge build output which causes scripts to fail with a no matching value found at ".ast.absolutePath" message.

With a recent update the ast has been removed from the default build output which causes scripts to fail with a `no matching value found at ".ast.absolutePath"` message.
@ericglau
Copy link
Member

Hi @gasperbr, thanks for the PR. Which version of forge are you running, can you share the output of forge --version ?

@ericglau
Copy link
Member

I see the issue if running with the default foundry.toml, e.g. without any of the following:

build_info = true
ast = true
extra_output = ["storageLayout"]

It looks like the error is occuring here: https://github.com/OpenZeppelin/openzeppelin-foundry-upgrades/blob/v0.1.0/src/internal/Utils.sol#L83

I think ideally a check should be added before this line, and if the JSON does not contain the expected ast key, a helpful error should be thrown to direct users to edit their foundry.toml as above.

Would you be interested in adding such a check? No worries if not, I could also add it. Thanks.

@ericglau ericglau changed the title Update README.md Update docs to include ast Mar 19, 2024
@ericglau ericglau changed the title Update docs to include ast Update docs to mention including ast Mar 19, 2024
@ericglau
Copy link
Member

Updated docs and added the check mentioned in my comment above.

@ericglau ericglau merged commit 3595893 into OpenZeppelin:main Mar 20, 2024
3 checks passed
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.

2 participants