Fix solidity compiler warnings and fail build on warnings

[maurelian]

Running yarn build in the contracts package outputs a bunch of warnings.

1. We should fix the warnings

There are no serious risks from these warnings, but we should fix them because it's easy to do, and it's easy to become desensitized to them, which results in a gradually increasing number of warnings.

2. The build process should fail if there are warnings

Better to just prevent them from creeping in in the first place. I'm not sure what the right way to detect and fail on a warning is, but one way would be to parse the JSON files written to packages/contracts/artifacts/build-info, for an entry in the output.errors array. Alternative approaches are also welcome.

[0xB548E]

1. Looks like small warnings need to be fixed like removing unused variables for overriden functions and a little bit more extras. Will create a PR in order to fix those for sure.

2. In case of yarn v2.4 it would be pretty simple by using logFilters in order to do pattern matching for exit status in case of warnings. But in this case we could add step in build for checking warning using something similar to: (echo 'artifacts/build-info/' && echo '(cd artifacts/build-info && ls -t *.json | head -1)') | tr -d '\n' | xargs cat | jq --exit-status '.output.errors | length == 0'

May look complicated, but it just comes down to getting the latest build file and using jq in order to check length of output.errors array like suggested

Let me know if these comments make sense and I will create PR with the changes.

[141p]

1. Looks like small warnings need to be fixed like removing unused variables for overriden functions and a little bit more extras. Will create a PR in order to fix those for sure.
2. In case of yarn v2.4 it would be pretty simple by using logFilters in order to do pattern matching for exit status in case of warnings. But in this case we could add step in build for checking warning using something similar to: (echo 'artifacts/build-info/' && echo '(cd artifacts/build-info && ls -t *.json | head -1)') | tr -d '\n' | xargs cat | jq --exit-status '.output.errors | length == 0'

May look complicated, but it just comes down to getting the latest build file and using jq in order to check length of output.errors array like suggested

Let me know if these comments make sense and I will create PR with the changes.

Hi, i´ve been looking at this today too. In my opinion:

1. The solidity warnings should be easy to get rid off but it looks like they are having a discussion around the implementation of some of those functions here. Also it looks like the current implementation is needed to past the test as they are set now. I don´t know if it would be possible to straight up eliminate those functions for the moment since having some code that isn´t used does not make musch sense to me, or maybe just changing the tests that reference those functions. I´m a noob so there might be a reason and im not getting it.

2. I was writing a script to execute between yarn build:contracts and yarn autogen:artifacts that checks for warnigs but your solution seems better and cleaner. I think it might be worth to keep an option (yarn build:ignore:warnings or something like that) to ignore warnings in case you are just doing some fast prototyping.

[0xB548E]

Hey, thanks for the response!

1. Let's see what's the current status from someone from the team, it's possible that we are missing some context here.

2. My way is a little bit hacky way of solving this. Your separate script could be probably a little bit more readable. Let me know if you want to continue with that. Also, instead of adding yarn build:ignore:warnings we could allow warnings to be in build, but fail on test so you can still prototype with warnings but the on CI it will fail so you won't be able to merge until warnings get removed.

[maurelian]

Hey friends, thanks for your interest here!

One consideration that might impact here is that most of the contracts are currently considered 'frozen' as we've recently completed an upgrade. But that being said, the kind of changes required to resolve these warnings are usually very small, and would be easy to merge even if we don't do so right away. I will ask some others who to chime in who will have a better idea about how to procede.

As far as the discussion about failing the build:

1. I don't think the failure should happen in the yarn scripts, that'd be a bit too much clutter in the package.json.
2. Other options include:
   • Causing a failure in the GH workflow file for this package.
   • Writing a lightweight wrapper around the hardhat compile task to take an optional flag, like --fail-on-warn, and parse the output (or artifacts) to fail if there are any warnings.

[0xB548E]

I've tried workflow based approach in #1902 PR, but I need approval in order to actually run and test it using Github Actions.

Also, tried running workflow locally with act but it has some strange issue, probably not worth debugging atm.

[smartcontracts]

@0xB548E just gave you approval for the workflow run

[0xB548E]

Thanks, runs smooth! Added a little bit better log message in case of detected warnings. Let me know if there's something else to add.

[smartcontracts]

Closing this. Let's not touch the legacy contracts anymore.