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 SPDX Identifier and Foundry top-level key #6

Merged
merged 2 commits into from
Jul 16, 2022

Conversation

sambacha
Copy link
Contributor

Caught this when flattening the contracts into one.

@PaulRBerg
Copy link
Owner

Hi @sambacha thanks for your contribution.

Happy to accept the SPDX fix, but regarding changing the foundry.toml file, I think we should create another PR for it and discuss each change there.

Would you like to revert the last commit you made?

@sambacha
Copy link
Contributor Author

Hi @sambacha thanks for your contribution.

Happy to accept the SPDX fix, but regarding changing the foundry.toml file, I think we should create another PR for it and discuss each change there.

Would you like to revert the last commit you made?

Do you want me to just leave the top level change or revert the entire thing?

suggested toml config for downstream users:

```toml

memory_limit = 33554432
fuzz_max_global_rejects = 65_536
fuzz_max_local_rejects = 16_384
fuzz_runs = 512

ignored_error_codes = [
  1878,
  2018,
  3420,
  3716,
]

cache = true
cache_path = 'cache'
evm_version = 'london'
```
@sambacha
Copy link
Contributor Author

amended the offending changes, note that the top level directive of the profile is a requirement now for foundry. If your using the latest forge you will be greeted with a warning message informing you to update the configuration file.

@sambacha
Copy link
Contributor Author

Also, why not make the compiler version min. the requirement at least ^0.8.4? Should I open separate issues?

Cheers 🤠

@PaulRBerg
Copy link
Owner

PaulRBerg commented Jul 16, 2022

amended the offending changes

Thanks! Just wanted to separate concerns so I can understand the rationale better.

note that the top level directive of the profile is a requirement now for foundry

Foundry's moving so fast! I think they should also update this in their READMEs, they're still using [default]:

https://github.com/foundry-rs/foundry/tree/37e4376cfbf1d2ce35c0c4eb25f7131c9fc1f8e2/config

@PaulRBerg
Copy link
Owner

Why not make the compiler version min. the requirement at least ^0.8.4?

Big fan of custom errors too. But I guess it doesn't hurt to keep the pragma to >=0.8.0? Users can still use >=0.8.4 in their own tests file, there wouldn't be any clash.

@PaulRBerg PaulRBerg changed the title fix(defect): SPDX Identifier Fix SPDX Identifier and Foundry top-level key Jul 16, 2022
@PaulRBerg PaulRBerg added the bug label Jul 16, 2022
@PaulRBerg PaulRBerg merged commit 203ce65 into PaulRBerg:main Jul 16, 2022
@sambacha
Copy link
Contributor Author

amended the offending changes

Thanks! Just wanted to separate concerns so I can understand the rationale better.

note that the top level directive of the profile is a requirement now for foundry

Foundry's moving so fast! I think they should also update this in their READMEs, they're still using [default]:

foundry-rs/foundry@37e4376/config

yup, automate all these things otherwise its ngmi

Why not make the compiler version min. the requirement at least ^0.8.4?

Big fan of custom errors too. But I guess it doesn't hurt to keep the pragma to >=0.8.0? Users can still use >=0.8.4 in their own tests file, there wouldn't be any clash.

Will open an issue shortly, its related.

Thanks!

@sambacha sambacha deleted the patch-1 branch July 16, 2022 09:32
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