Skip to content
This repository has been archived by the owner on Jan 8, 2024. It is now read-only.

feat: refactor deploy script to use more dependency injection #27

Merged
merged 6 commits into from
Dec 17, 2021

Conversation

akramhussein
Copy link
Member

@akramhussein akramhussein commented Nov 25, 2021

What I did

How I did it

  • Pass chain as an arg
  • Add function for getting deployment dict per chain-id

How to verify it

Run brownie run deploy

Checklist

  • I have confirmed that my PR passes all linting checks
  • I have included test cases
  • I have updated the documentation

Outstanding work

Other notes

@akramhussein
Copy link
Member Author

Will need to update to address for #28

@akramhussein akramhussein marked this pull request as draft November 25, 2021 20:16
@akramhussein akramhussein changed the base branch from main to revert-670f72e5622dbffc3dd205926e75f74e3c800781 November 25, 2021 21:50
Base automatically changed from revert-670f72e5622dbffc3dd205926e75f74e3c800781 to main November 28, 2021 01:09
@akramhussein akramhussein marked this pull request as ready for review November 28, 2021 01:10
@akramhussein akramhussein force-pushed the update-deployment-script branch 2 times, most recently from 1ce2841 to 7bba08a Compare December 2, 2021 16:51
Copy link
Contributor

@abdullathedruid abdullathedruid left a comment

Choose a reason for hiding this comment

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

Yep, good improvement to deployment script.
We will obviously need to add further WETH addresses when we consider deploying to further chains (and possibly rename it to wrapped-native or something sufficiently generalisable)
I can't seem to get the command brownie run deploy --network mainnet-fork (which uses ganache) to work.
I get the error "AttributeError: 'AttributeDict' object has no attribute 'baseFeePerGas'"
When I do the hardhat fork it works perfectly.

I don't think we necessarily need to support ganache as hardhat provides all the necessary features for testing in a forked environment. It might be worth explicitly documenting that it only works with hardhat

@abdullathedruid
Copy link
Contributor

Happy to merge once you've decided what we do with ganache

@akramhussein
Copy link
Member Author

akramhussein commented Dec 17, 2021

It fails on ganache due to: "priority_fee": "2 gwei" being set. I suspect this is to do with being forked from istanbul not london

@akramhussein
Copy link
Member Author

akramhussein commented Dec 17, 2021

Looks like london fork support isn't live yet for ganache: trufflesuite/ganache#939.

For the time being I'm going to merge it in since this isn't something we can fix or is a blocker.

@akramhussein akramhussein merged commit 036b51f into main Dec 17, 2021
@akramhussein akramhussein deleted the update-deployment-script branch December 17, 2021 03:37
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants