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

work with local testchain snapshot #26

Merged
merged 14 commits into from
Apr 4, 2019
Merged

work with local testchain snapshot #26

merged 14 commits into from
Apr 4, 2019

Conversation

levity
Copy link
Contributor

@levity levity commented Mar 28, 2019

This is the set of changes I've had to make so far to get the portal working with the local testchain.

starting the local testchain

cd $DAIJS # your dai.js path
git checkout dev
git pull
git submodule update --init --recursive
yarn test:net -s mcd-step-4 --fast

if you also want to make changes to dai.js / dai-plugin-mcd

Here's how you would get things running and auto-refreshing when you make changes to either dai.js or the MCD plugin:

one-time prep:

npm i -g sane yalc

terminal 2:

cd $DAIJS
sane "yarn build:backend && cd dist && yalc publish && cd $PORTAL && yalc link @makerdao/dai" src --wait=3

terminal 3:

cd $DAIJS/lib/dai-plugin-mcd
sane "yalc publish && cd $PORTAL && yalc link @makerdao/dai-plugin-mcd" src --wait=3

terminal 4:

cd $PORTAL # your mcd portal path
yarn start

@levity
Copy link
Contributor Author

levity commented Mar 28, 2019

Haven't done much testing yet, so there's probably more to do to get it working all the way through. And the issue with price feeds being Median on Kovan and DSValue in the snapshots is a pain; the workaround I put in place is a temporary hack.

@levity
Copy link
Contributor Author

levity commented Mar 28, 2019

TODO

  • remove redundant testchain addresses (rely on plugin)
  • come up with a better way to toggle between Median and DSValue

to avoid confusion between "testnet" and "testchain"
- get rpcUrl, multicallAddress, all other addresses, & the list of ilks from maker instance
- remove WatcherProvider since components should not need to interact with the watcher directly
- use the MCD plugin's CdpType.getPrice() for price values
@vercel
Copy link

vercel bot commented Apr 2, 2019

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

Latest deployment for this branch: https://mcd-cdp-portal-git-local-testchain.mkr-js-prod1.now.sh

@levity
Copy link
Contributor Author

levity commented Apr 2, 2019

OK, this is ready for review!

It is set up by default to handle an environment with Median price feeds. To handle an environment with DSValue price feeds, add simplePriceFeeds=1 to the URL query params.

To test this against a local testchain, please run the commands described in the "terminal 1" section above to start the testchain, and then fire up the MCD portal with ?network=test&simplePriceFeeds=1.

@levity levity changed the title work with local testchain snapshot (WIP) work with local testchain snapshot Apr 2, 2019
* cdp creation flow: show all ilks regardless of whether we have all addresses
* center spaceship illustration even when above a long string
* forward all search queries after programatic navigation
* don’t redirect at all if MM wallet connection has failed
@jparklev
Copy link
Contributor

jparklev commented Apr 3, 2019

was able to open a cdp on the local chain using MM with one of these keys imported. made this PR. i think we still need added tokens to accept a string data.address so that we can use addressOverrides for tokens, too. will be back 😴

@adrianleb
Copy link
Contributor

My first thought was - why am I looking at npm linking instructions if all I want is to run a testchain instance and this being a PR about local testchain?

These were the steps I took to get things running after reading your comments:

  • Make sure seth is installed

  •  git clone @makerdao/dai.js
     cd dai.js && yarn && git submodule update --init --recursive
     yarn test:net -s mcd-step-4 --fast
    
  • Add localhost:2000 as a custon network to Metamask

  • Run mcd-cdp-portal as usual

  • Navigate to http://localhost:3000?network=test&simplePriceFeeds=1

I'm still not able to develop on mcd-cdp-portal, where can one find a wallet with seeded balances?

Def. think it's useful having instructions on how to run all things all the time, but truth is the 80% use case is just developing the frontend using published versions of said sdk libs. it makes less and less sense having testchain be a dai.js submodule if one is not actively working on the sdk.

That aside, I think this is a great step for the developer experience around this project 🙏.

@b-pmcg
Copy link
Contributor

b-pmcg commented Apr 3, 2019

I was able to get it up and running, but I had to make some adjustments to handle added contracts with no network properties.

@b-pmcg
Copy link
Contributor

b-pmcg commented Apr 3, 2019

I'm still not able to develop on mcd-cdp-portal, where can one find a wallet with seeded balances?

@adrianleb using the mnemonic that's built into the deploy script outputs "ETH_FROM": "0x16fb96a5fa0427af0c8f7cf1eb4870231c8154b6" to testchain/lib/out/addresses.json. The private key for that (474beb999fed1b3af2ea048f963833c686a0fba05f5724cb6417cf3b8ee9697e) can be found in test/helpers/testAccounts.json and you can send yourself ETH/COL1, etc.

We should probably add this to the testchain setup documentation.

@levity
Copy link
Contributor Author

levity commented Apr 3, 2019

I'm still not able to develop on mcd-cdp-portal, where can one find a wallet with seeded balances?

@adrianleb using the mnemonic that's built into the deploy script outputs "ETH_FROM": "0x16fb96a5fa0427af0c8f7cf1eb4870231c8154b6" to testchain/lib/out/addresses.json. The private key for that (474beb999fed1b3af2ea048f963833c686a0fba05f5724cb6417cf3b8ee9697e) can be found in `test/helpers/testAccounts.json' and you can send yourself ETH/COL1, etc.

We should probably add this to the testchain setup documentation.

oh yeah, we should just bundle a script that makes it easy to send ETH/COLn from the creator address to the user's metamask account

@levity
Copy link
Contributor Author

levity commented Apr 3, 2019

My first thought was - why am I looking at npm linking instructions if all I want is to run a testchain instance and this being a PR about local testchain?

@adrianleb my bad, i should have edited that out. as you've noticed, the dai and dai-plugin-mcd deps were updated so you don't need yalc link to use this. edited the original comment for clarity

@levity
Copy link
Contributor Author

levity commented Apr 3, 2019

it makes less and less sense having testchain be a dai.js submodule if one is not actively working on the sdk.

@adrianleb You can actually do all this with a standalone copy of https://github.com/makerdao/testchain; just substitute scripts/launch for yarn test:net. i just provided instructions based on dai.js because everyone has a copy already.

@levity
Copy link
Contributor Author

levity commented Apr 3, 2019

@jparklev did you mean to put "QOL tweaks" on develop?

@jparklev
Copy link
Contributor

jparklev commented Apr 3, 2019

@levity that commit should have been two really, a few of the tweaks I needed to get cdps working for me here, the others could have been committed to develop first. Though they all arose for me in the context of this PR

@levity
Copy link
Contributor Author

levity commented Apr 3, 2019

I was able to get it up and running, but I had to make some adjustments to handle added contracts with no network properties.

Fixed in the last commit! Lemme know if there's anything else to do here before merging

@jparklev
Copy link
Contributor

jparklev commented Apr 4, 2019

@levity I think we might hit a little snag with the kovan deployment, where b/c pip refers to the oracle security module, the current getPrice returns a funny value for the gem/usd feed price, which makes the cdp creation flow wonky (fails on formatNumber rn, though that's an easy fix). We could drop a manual getStorageAt back in for now until #101 lands? How does that feel?

Otherwise LGTM :)

@levity
Copy link
Contributor Author

levity commented Apr 4, 2019

@jparklev ok, i'll review makerdao/dai.js#101, then once that's merged, ship a new point release and then update the deps here

which reads price from the vat
@levity
Copy link
Contributor Author

levity commented Apr 4, 2019

@jparklev i've got to jump on a call now, so if all looks good, feel free to merge

@jparklev jparklev merged commit 4d29629 into develop Apr 4, 2019
@jparklev jparklev deleted the local-testchain branch April 12, 2019 19:06
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.

4 participants