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

Fix sm integration #191

Closed
wants to merge 16 commits into from
Closed

Fix sm integration #191

wants to merge 16 commits into from

Conversation

kylezs
Copy link
Contributor

@kylezs kylezs commented Jun 25, 2021

@kylezs kylezs added the CFE label Jul 5, 2021
@kylezs
Copy link
Contributor Author

kylezs commented Jul 9, 2021

Currently blocked due to an issue with ganache. I can't solve the issue with this happening consistently:
trufflesuite/ganache#532

When calling eth_getLogs. Might try to find a version of ganache that actually works.

Comment on lines +20 to +23
sudo apt install nodejs
sudo apt-get update
sudo apt install npm
sudo npm install -g ganache-cli

Choose a reason for hiding this comment

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

FWIW these instructions will only work on linux distros with apt, and most of us use macOS 😉

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't know so many used Mac, Still better then no setup instructions? Maybe i should mention that the instructions are for Linux distros like Ubuntu.

- [Script](https://github.com/chainflip-io/chainflip-eth-contracts/blob/master/scripts/deploy_and.py) that creates the events expected by the test
```sh
sudo apt-get install pip
pip install eth-brownie

Choose a reason for hiding this comment

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

In the eth-contracts repo we use poetry as a dependency manager to prevent conflicts, maybe it would be worth copying the install instructions from there for the ETH part of this install process?

Copy link
Contributor

Choose a reason for hiding this comment

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

I did read those instructions when i was trying to get this working, but because the setup.sh downloads poetry automatically, i didn't add it to the instructions. Maybe its worth a mentions though.

@morelazers
Copy link

@j4m1ef0rd I'm not sure if you chatted with Kyle about it beforehand, but usually I would prefer we don't commit directly to each others' branches, even if it is only to update a README. Exceptional circumstances might change this policy.

Is there a reason you didn't make a PR?

@j4m1ef0rd
Copy link
Contributor

@morelazers I didn't know that this draft PR existed. I was working a little with kyle, I thought my job was to clean up what was in this branch and get it into develop, even if the test still didn't work. Am I not doing this the right way?

@morelazers morelazers closed this Aug 3, 2021
@kylezs kylezs deleted the fix-sm-integration branch August 3, 2021 09:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants