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

Collateral onboarding docker container. #404

Closed
wants to merge 5 commits into from

Conversation

KirillDogadin-std
Copy link
Contributor

@KirillDogadin-std KirillDogadin-std commented Aug 9, 2022

collateral-onboarding/collateral-onboarding/onboard.sh Outdated Show resolved Hide resolved
collateral-onboarding/docker-compose.yml Outdated Show resolved Hide resolved
Comment on lines +4 to +5
make
dapp create DssSpell
Copy link
Contributor

Choose a reason for hiding this comment

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

This make all only builds them, but doesn't execute
What dapp create DssSpell actually do? Can 1+ particular spell be executed instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://github.com/makerdao/spells-mainnet#deploy refers to the make directive deploy

this directive make deploy sadly also runs a python script that performs a bunch of verifications and at least during the developing was a blocker. Thus in order to avoid running the python script i executed the command separately.

https://github.com/makerdao/spells-mainnet/blob/d000902ad35c1eb8425db71b05b17c716bf0a66a/Makefile#L9

SO what it actually does is deploying a compiled contract.

If one has to guarantee that the collateral is onboarded via the spell, they would have to adjust the solidity code in the repository (readme there covers what to edit)

Copy link
Contributor

Choose a reason for hiding this comment

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

The python script contains calls to the etherscan which would fail since we run it on the fork instead

collateral-onboarding/hardhat/Dockerfile Show resolved Hide resolved
Comment on lines 20 to 22
// Few blocks before WSTETH-A is taken at 14052147,
// https://etherscan.io/address/0x49a33a28c4c7d9576ab28898f4c9ac7e52ea457a
blockNumber: 14052140,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this specific block number? I imagine that we should actually fork without it to be on the latest state of the maker network

Otherwise, for now, you can take the block number where MCD_CLIP_WSTETH_A didn't exist yet (ie 13426393 - 1) and try to execute the spell which introduced it, then check if MCD_CLIP_WSTETH_A exists in the chainlog contract (which should at this point)

Copy link
Contributor Author

@KirillDogadin-std KirillDogadin-std Aug 9, 2022

Choose a reason for hiding this comment

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

well, there's no reason for a specific block number:

  • in order to test that e.g. the MCD_CLIP_WSTETH_A , i would also have to checkout different commit there
  • because currently there is no collateral being introduced by the latest version of the spell.

also why i claimed in the verbal discussion that this runs without failure and not that it works.

Copy link
Contributor

Choose a reason for hiding this comment

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

there's no reason for a specific block number
i would also have to checkout different commit there

Well, I still think that it makes sense to fork on a specific block number and maybe (as you suggest above) run a spell on a specific commit, in order to validate that contract that was not available is available after the execution

Copy link
Contributor

Choose a reason for hiding this comment

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

So the takeaway here is the additional test setup that might need to be created

Comment on lines +9 to +10
One has to specify `ALCHEMY_URL` variable in the `.env` file that points the hardhat to the fork url.
The `.env` file is to be located next to the `docker-compose.yml` file.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
One has to specify `ALCHEMY_URL` variable in the `.env` file that points the hardhat to the fork url.
The `.env` file is to be located next to the `docker-compose.yml` file.
In order to run the project, `collateral-onboarding/.env` file needs to be created with appropriate variables described below:
- `ALCHEMY_URL`: (required) [alchemy](https://www.alchemy.com) endpoint url (can be found in: apps table -> app -> view key)

Comment on lines +26 to +27
This narrows down to adding/removing the docker-compose directive `platform: linux/amd64`. If one desires to run the docker container separately then the
flag has to be provided along with the rest of the `docker build` command: `--platform linux/amd64`
Copy link
Contributor

Choose a reason for hiding this comment

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

To which service it needs to be added? Can it be done without editing via a flag or env var? Eg will this work?

DOCKER_DEFAULT_PLATFORM=linux/amd64 docker-compose up --build --force-recreate

Comment on lines +4 to +5
make
dapp create DssSpell
Copy link
Contributor

Choose a reason for hiding this comment

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

The python script contains calls to the etherscan which would fail since we run it on the fork instead

Comment on lines 20 to 22
// Few blocks before WSTETH-A is taken at 14052147,
// https://etherscan.io/address/0x49a33a28c4c7d9576ab28898f4c9ac7e52ea457a
blockNumber: 14052140,
Copy link
Contributor

Choose a reason for hiding this comment

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

there's no reason for a specific block number
i would also have to checkout different commit there

Well, I still think that it makes sense to fork on a specific block number and maybe (as you suggest above) run a spell on a specific commit, in order to validate that contract that was not available is available after the execution

Comment on lines 20 to 22
// Few blocks before WSTETH-A is taken at 14052147,
// https://etherscan.io/address/0x49a33a28c4c7d9576ab28898f4c9ac7e52ea457a
blockNumber: 14052140,
Copy link
Contributor

Choose a reason for hiding this comment

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

So the takeaway here is the additional test setup that might need to be created

@valiafetisov valiafetisov mentioned this pull request Aug 19, 2022
2 tasks
Comment on lines +18 to +19
RUN curl -L https://dapp.tools/install | sh

Copy link
Contributor

Choose a reason for hiding this comment

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

Would this also run without errors?

Suggested change
RUN curl -L https://dapp.tools/install | sh
RUN curl -L https://dapp.tools/install | sh
RUN seth --version

Copy link
Contributor Author

Choose a reason for hiding this comment

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

 => ERROR [tool-installs 6/6] RUN seth --version                                                                                                                                                                                                                                                                         0.4s
------
 > [tool-installs 6/6] RUN seth --version:
#12 0.215 /bin/sh: 1: seth: not found
------
executor failed running [/bin/sh -c seth --version]: exit code: 127

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks! From this I assume that dapp tools did not correctly install despite finishing without error code. Correct me if I am missing something here

@valiafetisov
Copy link
Contributor

This issue is closed via adding a complete instruction in #506

### Onboarding not yet deployed collateral
When a completely new collateral type support is being prepared, we need to ensure that it will work even before the Maker Protocol is changed via a [`spell`](https://docs.makerdao.com/smart-contract-modules/governance-module/spell-detailed-documentation). Usually a new spell is prepared in the [spells-mainnet](https://github.com/makerdao/spells-mainnet/pulls) repository. When it is there we need to fork the repository, compile the spell and deploy it into the hardhat fork. Currently the setup is as follows:
1. `rsync` or clone the repo to the desired x86 machine (tested with Docker 19.03.12 on Ubuntu 20.04)
2. `cd` into the `core/simulations/docker` and run `docker-compose up` to start the hardhat fork in one container and another container with installed [`dapp-tools`](https://github.com/dapphub/dapptools)
3. Shell into the `spells` container
- List avialble containers `docker container ls` (copy `CONTAINER ID` of the `docker_spells`)
- Shell into the container `docker exec -it 277a8d793341 sh`
4. Fix future `Invalid argument` `nix` error via `echo 'filter-syscalls = false' >> /etc/nix/nix.conf`
5. Open `nix` shell with useful tools `nix-shell -p cacert cachix curl git jq nix gnumake nano`
6. Update `dapp --version` to the supported version (currently 0.35.0)
- Install `duppgrade` via `curl https://rari-capital.github.io/duppgrade/install | bash`
- Execute `duppgrade` and wait
7. Clone branch containing the spell
- Clone the repo, eg `git clone https://github.com/makerdao/spells-mainnet spells && cd spells`
- Checkout correct branch `git checkout CES-795`
8. Fetch all libraries (that are linked via git submodules) using `dapp update`
9. Create keystore (that will be used by dapp-tools)
- Set hardhat private key into the file `echo 'ac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80' > ../private-key`
- Set password `echo '' > ../key-password`
- Import key to create a keystore folder `(printf "\n\n" && cat) | geth account import --datadir /root ../private-key`
- Press enter to finish the process
10. Prepare env vars
- `export ETH_KEYSTORE="/root/keystore"`
- `export ETH_PASSWORD="/root/key-password"`
- `export ETH_RPC_URL=http://core:8545`
- `export ETH_FROM="0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266"` - the address of the key from above
- `export ETH_GAS=3000000` - might need to be adjusted based on the `make estimate` output
- `export ETH_GAS_PRICE=1000000000000`
11. Compile the spell via `make`
12. Deploy the spell via `dapp create DssSpell`
13. Copy the bitecode of the spell into the `core/bytecode/compiledSpells.json` file, which will automatically update the `Onboard new collateral` simulation
14. Run `Onboard new collateral` locally to deploy compiled bytecode and execute the spell, create vault, liquidate vault to create auction
15. Run keeper and frontend against the simulation to validate that it worked

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