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

Rebase cinemarket to merge into master and clean up repo #122

Merged
merged 197 commits into from
May 21, 2019

Conversation

TimDaub
Copy link
Collaborator

@TimDaub TimDaub commented May 20, 2019

Fixes #110

This PR cleans up the cinemarket branch to make it mergeable into master. The cinemarket branch includes the sunDAI branch. Lots of stuff has been deleted as we don't need it anymore. For details see the list below.

For reviewing this PR, I'm not exactly sure what would be a good strategy. I'm sure there's still bugs, but we'll find them in continuing development and I'll fix them for "free" as part of the issue #110. But in other PRs and not this one.

TODOs:

@TimDaub TimDaub force-pushed the fix/rebase-cinemarket-progress branch 2 times, most recently from 43c3374 to b0b35f7 Compare May 20, 2019 15:46
@TimDaub TimDaub marked this pull request as ready for review May 20, 2019 16:13
@TimDaub TimDaub changed the title Rebase cinemarket to merge into master Rebase cinemarket to merge into master and clean up repo May 20, 2019
@TimDaub TimDaub force-pushed the fix/rebase-cinemarket-progress branch from b0b35f7 to 1a5c149 Compare May 21, 2019 08:47
@TimDaub TimDaub force-pushed the fix/rebase-cinemarket-progress branch from 1a5c149 to 820d692 Compare May 21, 2019 09:20
@TimDaub TimDaub force-pushed the fix/rebase-cinemarket-progress branch from 820d692 to a122529 Compare May 21, 2019 09:37
Copy link
Member

@iamonuwa iamonuwa left a comment

Choose a reason for hiding this comment

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

Left some reviews. PTAL.

@@ -5,17 +5,14 @@
"dependencies": {
Copy link
Member

Choose a reason for hiding this comment

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

ethereumutil-js is required but never part of either the devDependencies or the dependencies.

Copy link
Member

Choose a reason for hiding this comment

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

Can't resolve 'web3-utils' because of dapparatus version used.

Copy link
Collaborator Author

@TimDaub TimDaub May 21, 2019

Choose a reason for hiding this comment

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

You're right, we need ethereumutil-js. I didn't get any errors not being able to resolve web3-utils after adding it when doing the following:

$ rm -rf node_modules
$ rm package-lock.json
$ npm i

Can you confirm that adding ethereumutil-js fixed the issue (for npm at least)?

Copy link
Member

@iamonuwa iamonuwa left a comment

Choose a reason for hiding this comment

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

LGTM! Good work @TimDaub

@TimDaub TimDaub merged commit b2b7343 into master May 21, 2019
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.

Rebase cinemarket branch and merge into master
7 participants