Skip to content

buidler-refactor #164

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

Closed
wants to merge 3 commits into from
Closed

buidler-refactor #164

wants to merge 3 commits into from

Conversation

thegostep
Copy link
Contributor

@thegostep thegostep commented Aug 28, 2020

@thegostep thegostep marked this pull request as draft August 28, 2020 16:46
@thegostep thegostep self-assigned this Aug 28, 2020
@thegostep thegostep force-pushed the buidler-refactor branch 8 times, most recently from 5e45b6a to 1e0fbdf Compare September 21, 2020 14:39
@thegostep thegostep marked this pull request as ready for review September 21, 2020 14:43
@thegostep thegostep requested review from brandoniles, aalavandhan and ahnaguib and removed request for brandoniles and aalavandhan September 21, 2020 14:43
@aalavandhan
Copy link
Member

@thegostep can you make the test file renames register as moves rather than deletion/additions.

https://stackoverflow.com/questions/433111/how-to-make-git-mark-a-deleted-and-a-new-file-as-a-file-move

@thegostep
Copy link
Contributor Author

thegostep commented Sep 28, 2020

@thegostep can you make the test file renames register as moves rather than deletion/additions.

@nithinkrishna I took a crack at this - unfortunately I don't think it is possible for git to track diffs between js and ts files.

Instead I've staged the files as js so that the diffs work and then converted to ts in separate commit

@aalavandhan
Copy link
Member

aalavandhan commented Oct 5, 2020

As far as I can see this doesn't change any of the contracts. It only updates the testing environment.

I like the buidler + ethers framework a lot after using it for a couple of projects.

Will wait for @brandoniles or @ahnaguib to have another look before merging

@thegostep
Copy link
Contributor Author

Here is a great tutorial on process for upgrades: https://forum.openzeppelin.com/t/openzeppelin-upgrades-step-by-step-tutorial-for-hardhat/3580

Copy link
Member

@brandoniles brandoniles left a comment

Choose a reason for hiding this comment

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

Thanks Stephane, I will always support typed language!

I see that package-lock.json is removed and node_modules is added to gitignore. Will we still get reproducible builds?

AFAICT, the only changes to the solidity files are formatting. I assume this matches the new linter settings. As a general note, I'd recommend separating the code changes from the test changes when the behavior is meant to be the same. It gives more confidence that neither introduces problems on their own.

@thegostep
Copy link
Contributor Author

I see that package-lock.json is removed and node_modules is added to gitignore. Will we still get reproducible builds?

Yes - I switched the package manager from npm to yarn. See yarn.lock for version locking

@thegostep
Copy link
Contributor Author

AFAICT, the only changes to the solidity files are formatting. I assume this matches the new linter settings. As a general note, I'd recommend separating the code changes from the test changes when the behavior is meant to be the same. It gives more confidence that neither introduces problems on their own.

Fair - I can split into seperate PRs if you prefer. Just say the word ✨

@thegostep
Copy link
Contributor Author

following @brandoniles suggestion, this PR is superseded by #176 and #177

@thegostep thegostep closed this Dec 23, 2020
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