Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

migrate to hardhat #121

Merged
merged 3 commits into from
Mar 17, 2021
Merged

migrate to hardhat #121

merged 3 commits into from
Mar 17, 2021

Conversation

ralph-pichler
Copy link
Member

@ralph-pichler ralph-pichler commented Mar 8, 2021

makes our framework here more similar to storage-incentives repo

  • moves to hardhat (using truffle plugin for test compatibility)
  • moves to using signtypeddata_v4 in tests
  • moves to yarn instead of npm
  • moves to github ci instead of travis

@ralph-pichler ralph-pichler force-pushed the hardhat branch 2 times, most recently from b900284 to 3983353 Compare March 8, 2021 08:34
Copy link
Collaborator

@Eknir Eknir left a comment

Choose a reason for hiding this comment

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

I verified that everything works as expected -- it does. There are some minor issues which may be tackled now, or otherwise considered for a separate issue. I would like @vandot s feedback on .github/workflows before merging.

@@ -0,0 +1,41 @@
name: CI
Copy link
Collaborator

Choose a reason for hiding this comment

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

I can't review this. Perhaps ask @vandot ?

Copy link
Member Author

Choose a reason for hiding this comment

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

same ci as in storage-incentives repo.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Eknir syntax and yaml look good, I hope that steps/runs do what is needed

Copy link
Collaborator

Choose a reason for hiding this comment

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

@ralph-pichler - can this somehow be validated before merging?

Copy link
Member Author

Choose a reason for hiding this comment

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

@Eknir yes, you can just check the ci output on this PR

package.json Outdated
"abigen": "./abigen/gen.sh ERC20SimpleSwap SimpleSwapFactory Migrations",
"solhint": "solhint contracts/ERC20SimpleSwap.sol contracts/SimpleSwapFactory.sol",
"lint": "solhint contracts/ERC20SimpleSwap.sol contracts/SimpleSwapFactory.sol",
"coverage": "truffle run coverage"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Update this? See line 18 of README

Copy link
Member Author

Choose a reason for hiding this comment

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

it is updated already.

@Eknir
Copy link
Collaborator

Eknir commented Mar 17, 2021

Thanks for addressing my comments. Let's wait for @vandot to approve the github workflows and then it is good to go!

README.md Outdated
```

To also generate coverage information use `npm run coverage` instead.
To also generate coverage information use `yarn hardhat coverage` instead.
Copy link
Collaborator

Choose a reason for hiding this comment

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

use yarn coverage

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed. also in ci.

@ralph-pichler ralph-pichler merged commit 9748522 into master Mar 17, 2021
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants