Skip to content

Conversation

@johnwhitton
Copy link
Collaborator

@johnwhitton johnwhitton commented Sep 1, 2022

TODO

  • Add miniwallets/deployments/ethLocal folder to .gitignore leave miniwallet/deployments folder
  • Review Information in deployments folder holds deployment info including addresses for DefaultProxyAdmin, MiniWallet_Proxy , MiniWallet, MiniWallet_Implementation
  • Review upgrade scenarios
  • Decide whether to merge this branch or leave it as a template for when we are ready to do an upgrade

Sample Deployment Log

johnlaptop miniwallet (proxyUpgrade) $ yarn deploy --network ethLocal
yarn run v1.22.19
warning ../../../package.json: No license field
$ npx hardhat deploy --tags deploy --network ethLocal
Nothing to compile
No need to generate any newer typings.
✅ Generated documentation for 37 contracts
 ·-----------------|--------------|----------------·
 |  Contract Name  ·  Size (KiB)  ·  Change (KiB)  │
 ··················|··············|·················
 |  MiniWallet     ·       9.128  ·                │
 ·-----------------|--------------|----------------·
chainId: 1337
operators: ["0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266","0x70997970C51812dc3A010C7d01b50e0d17dc79C8","0x3C44CdDdB6a900fa2b585dd299e03d12FA4293BC","0x90F79bf6EB2c4f870365E785982E1f101E93b906"]
deploying "DefaultProxyAdmin" (tx: 0xb01a14487b4eb95e5252e6e35c523268c71655eb621a8945fa3581b863f9bc8e)...: deployed at 0x5FbDB2315678afecb367f032d93F642f64180aa3 with 643983 gas
deploying "MiniWallet_Implementation" (tx: 0xefe7939947bd3df56344b115ca5c0622c368bf3e423175672578761acfcb2f69)...: deployed at 0xe7f1725E7734CE288F8367e1Bb143E90bb3F0512 with 2071284 gas
deploying "MiniWallet_Proxy" (tx: 0xc77ef2d33a495d02662d07fcf4dc4e8f243c3df609969ed4ba19094af7643340)...: deployed at 0x9fE46736679d2D9a65F0992F2272dE9f3c7fa6e0 with 1213663 gas
MiniWallet deployed to: 0x9fE46736679d2D9a65F0992F2272dE9f3c7fa6e0
MiniWallet Operator Threshold: 10
operatorCount : 4
Operator [0]: 0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266
Operator [1]: 0x70997970C51812dc3A010C7d01b50e0d17dc79C8
Operator [2]: 0x3C44CdDdB6a900fa2b585dd299e03d12FA4293BC
Operator [3]: 0x90F79bf6EB2c4f870365E785982E1f101E93b906
MiniWallet Global User Auth Limit: 1000000.0
MiniWallet Global User Auth Limit: 100000.0
✨  Done in 5.93s.
johnlaptop miniwallet (proxyUpgrade) $ yarn deploy-upgrade --network ethLocal
yarn run v1.22.19
warning ../../../package.json: No license field
$ npx hardhat deploy --tags upgrade --network ethLocal
Nothing to compile
No need to generate any newer typings.
✅ Generated documentation for 37 contracts
 ·-----------------|--------------|----------------·
 |  Contract Name  ·  Size (KiB)  ·  Change (KiB)  │
 ··················|··············|·················
 |  MiniWallet     ·       9.128  ·                │
 ·-----------------|--------------|----------------·
chainId: 1337
operators: ["0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266","0x70997970C51812dc3A010C7d01b50e0d17dc79C8","0x3C44CdDdB6a900fa2b585dd299e03d12FA4293BC","0x90F79bf6EB2c4f870365E785982E1f101E93b906"]
reusing "DefaultProxyAdmin" at 0x5FbDB2315678afecb367f032d93F642f64180aa3
deploying "MiniWallet_Implementation" (tx: 0xefac31f8f8ac2dbc23d08741cca224a1bf3c7692e2ccaf1da68209a5fccbe3b9)...: deployed at 0xCf7Ed3AccA5a467e9e704C703E8D87F634fB0Fc9 with 2071284 gas
executing DefaultProxyAdmin.upgrade (tx: 0xa7e7dcbde3988c5b9e6e6163253182f5bf93e6cadb97992b6deec49fd7f60e58) ...: performed with 38688 gas
MiniWallet_v2 deployed to: 0x9fE46736679d2D9a65F0992F2272dE9f3c7fa6e0
MiniWallet_v2 Operator Threshold: 10
operatorCount : 4
Operator [0]: 0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266
Operator [1]: 0x70997970C51812dc3A010C7d01b50e0d17dc79C8
Operator [2]: 0x3C44CdDdB6a900fa2b585dd299e03d12FA4293BC
Operator [3]: 0x90F79bf6EB2c4f870365E785982E1f101E93b906
MiniWallet_v2 Global User Auth Limit: 1000000.0
MiniWallet_v2 Global User Auth Limit: 100000.0
✨  Done in 5.65s.

@johnwhitton johnwhitton changed the title Proxy upgrade Contains MiniWallet Contracts refactor, MiniServer, Proxy Upgrade Deploys and NFT Scaffolding Sep 1, 2022
@johnwhitton johnwhitton changed the title Contains MiniWallet Contracts refactor, MiniServer, Proxy Upgrade Deploys and NFT Scaffolding Contains MiniWallet Contracts refactor, MiniServer, Proxy Upgrade Deploys Sep 1, 2022
@johnwhitton johnwhitton changed the title Contains MiniWallet Contracts refactor, MiniServer, Proxy Upgrade Deploys MiniWallet Contracts refactor, MiniServer, Proxy Upgrade Deploys Sep 1, 2022
@johnwhitton johnwhitton mentioned this pull request Sep 1, 2022
4 tasks
@polymorpher polymorpher changed the base branch from main to miniwallet September 2, 2022 22:29
Copy link
Owner

@polymorpher polymorpher left a comment

Choose a reason for hiding this comment

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

It's nice to have hardhat do the proxy-deployment in a simple function call, but I am concerned we see too little detail and have too little control. The generated .json files (in deployments/) are pretty hard to read, and may be easily confused with something that can be regenerated. Yet they are unique to the deployment and critical to managing the contract / upgrades in the future.

For each chain, we only need three things: proxy contract, deployed proxy address, and logic contract address. Rather than keeping a bunch of JSON files and irrelevant information in those files, I think it is better to just store these three pieces of information in a file inside a folder (similar to relayer/cache, but can be simpler, e.g. the versions can be hash of contracts instead). When we need to upgrade the contract later, we just read from that file and call relevant functions accordingly (i.e. (1) deploy a new logic contract, (2) redirect proxy to the address of the new logic contract). I have read https://github.com/wighawag/hardhat-deploy#deploying-and-upgrading-proxies and examined underlying types (ProxyOptionsBase, etc.) but I am not sure whether hardhat-deploy library supports manual selection (of logic contract address) during upgrade. It would be nice if it does, but even if it doesn't, it seems not hard to just implement on our own - should be just a few lines

@johnwhitton
Copy link
Collaborator Author

Closing this as we will progress this under PR #14 and track items under #13
The Deployment Improvements have been documented here

@johnwhitton johnwhitton closed this Sep 4, 2022
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