Skip to content

fix slither crashing in CI and local#1516

Closed
artdgn wants to merge 2 commits intoSynthetixio:developfrom
artdgn:develop
Closed

fix slither crashing in CI and local#1516
artdgn wants to merge 2 commits intoSynthetixio:developfrom
artdgn:develop

Conversation

@artdgn
Copy link
Copy Markdown
Contributor

@artdgn artdgn commented Sep 21, 2021

Context: was working through the different script targets and tests and this one was broken, so thought it might be nice to fix it.

Details: slither was not working in both CI and locally, example run. It seems it has been broken for a while (at least 4mo back, haven't check earlier).

changes:

testing: local runs of slither, unit tests (npx hardhat test --grep "Rewards")

@artdgn
Copy link
Copy Markdown
Contributor Author

artdgn commented Sep 21, 2021

@artdgn
Copy link
Copy Markdown
Contributor Author

artdgn commented Sep 21, 2021

ci/circleci: job-test-deploy-script fails with:

Error: No compiled source for: StakingRewardssETHUniswapV1. The source file is set to StakingRewards.sol - is that correct?

I don't how that's related to my changes.

And fork-tests seems to have been broken before as well.

@codecov
Copy link
Copy Markdown

codecov bot commented Sep 22, 2021

Codecov Report

Merging #1516 (a45e30d) into develop (96be7e1) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #1516   +/-   ##
========================================
  Coverage    95.47%   95.47%           
========================================
  Files           75       75           
  Lines         1744     1745    +1     
  Branches       545      545           
========================================
+ Hits          1665     1666    +1     
  Misses          79       79           
Impacted Files Coverage Δ
contracts/ShortingRewards.sol 100.00% <ø> (ø)
contracts/StakingRewards.sol 100.00% <ø> (ø)
contracts/RewardsDistributionRecipient.sol 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 96be7e1...a45e30d. Read the comment docs.

@artdgn
Copy link
Copy Markdown
Contributor Author

artdgn commented Sep 22, 2021

@justinjmoses CI is passing now (it's the third approach: not importing the OZ Math library at all).

The added internal method is just for the min function, which is very minimal and is tested through the tests that already exist for the methods that were using it before.

@artdgn artdgn force-pushed the develop branch 5 times, most recently from 7433b68 to 33cb9c9 Compare September 22, 2021 08:09
@artdgn
Copy link
Copy Markdown
Contributor Author

artdgn commented Sep 22, 2021

Now the report is stored as a CI artifact, example

details: slither was not working in both CI and running locally

changes:
  - pass the correct artifacts path in config
  - avoid using OZ Math import to prevent name
   collision that was causing slither to fail
  - modify local slither target to install slither in virtual env
  instead of globally

testing: local runs of slither, unit tests (`npx hardhat test --grep "Rewards"`)
@artdgn
Copy link
Copy Markdown
Contributor Author

artdgn commented Sep 23, 2021

Closed and replaced by #1519

@artdgn artdgn closed this Sep 23, 2021
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.

1 participant