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

Migration to hardhat #914

Merged
merged 78 commits into from
Jan 28, 2021
Merged

Conversation

jjgonecrypto
Copy link
Contributor

@jjgonecrypto jjgonecrypto commented Nov 19, 2020

🚧 WIP 🚧
(not a draft because of ci tests)

Critical TODOs

  • need smockit that will allow hardhat compilation cc @kfichter
  • upgrade solidity-coverage@0.7.12 cc @cgewecke (see Testing solidity-coverage hardhat plugin sc-forks/synthetix#2)
  • upgrade to hardhat-gas-reporter
  • fork support on mainnet
  • AST building support for docs generation. Requires a new version of: https://hardhat.org/plugins/buidler-source-descriptor.html
  • --use-ovm not working on compile as @eth-optimism/ovm-toolchain not supported for hardhat
  • --fail-oversize support on compile
  • gas reports not being published to codechecks
  • coverage reports not showing up on PRs
  • ovm-ignore being ignored in compilation
  • why are unit tests slower? (using resource: medium - since reverted to using resource: large)
  • remove legacy stuff, and introduce lightweight alternative

Nice to have

  • fork support for all testnets
  • legacy test contract support (we may decide to sunset this)
  • @eth-optimism/ovm-toolchain needs upgrading to support hardhat
  • slither-config.json has entry buidler_cache_directory
  • @nomiclabs/hardhat-truffle5 that uses @nomiclabs/truffle-contract@4.2.24 (to prevent truffle emitting event issues Upgrading Web3 >= 1.2.2 trufflesuite/truffle#2688) cc @alcuadrado - should speed up coverage

@eternauta1337 eternauta1337 self-assigned this Nov 25, 2020
* Restore ability to read contract sizes

* Linter fix
@eternauta1337 eternauta1337 marked this pull request as ready for review November 25, 2020 21:12
@codecov
Copy link

codecov bot commented Dec 18, 2020

Codecov Report

Merging #914 (371b8a0) into develop (2fdd05d) will decrease coverage by 1.89%.
The diff coverage is 89.12%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #914      +/-   ##
===========================================
- Coverage    97.55%   95.66%   -1.90%     
===========================================
  Files           61       75      +14     
  Lines         4097     5350    +1253     
  Branches       524      658     +134     
===========================================
+ Hits          3997     5118    +1121     
- Misses         100      232     +132     
Impacted Files Coverage Δ
contracts/EmptyCollateralManager.sol 0.00% <0.00%> (ø)
contracts/SynthetixStateWithLimitedSetup.sol 0.00% <0.00%> (ø)
contracts/ImportableRewardEscrowV2.sol 27.27% <27.27%> (ø)
contracts/BaseRewardEscrowV2.sol 80.66% <80.66%> (ø)
contracts/BinaryOptionMarketManager.sol 98.22% <84.21%> (-1.78%) ⬇️
contracts/RewardEscrowV2.sol 84.61% <84.61%> (ø)
contracts/CollateralManager.sol 84.94% <84.94%> (ø)
contracts/CollateralShort.sol 88.00% <88.00%> (ø)
contracts/MixinSystemSettings.sol 97.82% <94.11%> (-2.18%) ⬇️
contracts/ShortingRewards.sol 95.58% <95.58%> (ø)
... and 49 more

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 16b36ba...371b8a0. Read the comment docs.

Comment on lines +425 to +426
// Deterministic account #0 when using `npx hardhat node`
owner: '0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266',
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make this a constant instead of hard-coding it twice?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Twice? Where's the other instance?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah! I see it. It's in Deployer.js
Fixed

@jjgonecrypto jjgonecrypto merged commit 45553ff into develop Jan 28, 2021
@jjgonecrypto jjgonecrypto deleted the migrate-to-hardhat-master-trigger branch January 28, 2021 17:53
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.

3 participants