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

F/add deployment script #37

Open
wants to merge 10 commits into
base: dev
Choose a base branch
from
Open

F/add deployment script #37

wants to merge 10 commits into from

Conversation

eugenPtr
Copy link
Collaborator

No description provided.

@eugenPtr eugenPtr requested a review from Willgg June 22, 2022 21:23
Copy link
Contributor

@Willgg Willgg left a comment

Choose a reason for hiding this comment

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

I added 2 pending comments

"ethereum-waffle": "^3.4.4",
"ethers": "^5.6.6",
"hardhat": "^2.9.5",
"hardhat-gas-reporter": "^1.0.8"
"hardhat-gas-reporter": "^1.0.8",
"sol-merger": "^4.1.1"
Copy link
Contributor

Choose a reason for hiding this comment

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

why did you add sol-merger ? you can compile the contracts with hardhat compile command

Copy link
Collaborator Author

@eugenPtr eugenPtr Jun 23, 2022

Choose a reason for hiding this comment

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

I added sol-merger to flatten the contracts in order to manually verify them on etherscan. I had some issues verifying some contracts programatically through hardhat-etherscan.js

const swapper = await Swapper.deploy()
await swapper.deployed()
console.log('Swapper deployed to: ', swapper.address)
console.log('IMPORTANT: Add swapper address to /packages/hardhat/src/addresses.js')
Copy link
Contributor

Choose a reason for hiding this comment

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

you could code the addition of the src addresses directly from this file so that you don't need to add it manually

Copy link
Collaborator Author

@eugenPtr eugenPtr Jun 23, 2022

Choose a reason for hiding this comment

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

Good catch, I missed this. Will repair it

@Willgg
Copy link
Contributor

Willgg commented Jun 23, 2022

the branch has some conflicts with dev branch. Please resolves conflicts locally and push again. Here is what i get when i git rebase dev locally :

warning: Cannot merge binary files: .yarn/install-state.gz (HEAD vs. deb6858 (feat: #24 added deployment and verification script))
CONFLICT (modify/delete): packages/hardhat/scripts/deploy.js deleted in HEAD and modified in deb6858 (feat: #24 added deployment and verification script). Version deb6858 (feat: #24 added deployment and verification script) of packages/hardhat/scripts/deploy.js left in tree.
Auto-merging packages/hardhat/package.json
CONFLICT (content): Merge conflict in packages/hardhat/package.json
CONFLICT (add/add): Merge conflict in packages/hardhat/package-lock.json
Auto-merging packages/hardhat/package-lock.json
Auto-merging packages/hardhat/hardhat.config.js
CONFLICT (content): Merge conflict in packages/hardhat/hardhat.config.js
Auto-merging packages/hardhat/contracts/Governance.sol
Auto-merging .yarn/install-state.gz
CONFLICT (content): Merge conflict in .yarn/install-state.gz
error: could not apply deb6858... feat: #24 added deployment and verification script
Resolve all conflicts manually, mark them as resolved with
"git add/rm <conflicted_files>", then run "git rebase --continue".
You can instead skip this commit: run "git rebase --skip".
To abort and get back to the state before "git rebase", run "git rebase --abort".
Could not apply deb6858... feat: #24 added deployment and verification script

@eugenPtr
Copy link
Collaborator Author

Thanks for this thorough review.

I'll modify the hardhat scripts to update the constant variables in address.js and I'll make another attempt at rebasing correctly. I'll let you know if I have any difficulties.

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.

2 participants