Skip to content

Conversation

@sifnoc
Copy link
Member

@sifnoc sifnoc commented Nov 5, 2022

This PR for solving the issue zkopru-network/zkopru#410

@sifnoc sifnoc requested a review from KimiWu123 November 5, 2022 15:47
Copy link

@KimiWu123 KimiWu123 left a comment

Choose a reason for hiding this comment

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

LGTM
Actually most of things I don't understand (not familiar enough for npm and front-end) and just have some minor questions.

README.md Outdated

## Development alongside @zkopru/client

the web wallet download `@zkopru/client` module from npm while installing. for more convenience to develop alongside @zkopru/client, It would be better to use a client package that is locally cloned.

Choose a reason for hiding this comment

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

question about this, it works well by using yarn link @zkopru/client as mentioned above Line16. Why do we need to have another command npm run install-dev for zkopru/client integration?

Copy link
Member Author

Choose a reason for hiding this comment

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

In my environment, yarn link requires permission to create a folder in the global node module.

I think it is okay setup and forgets in this way. If you use yarn link, have to unlink at some point, even though not worry about the permission.

Copy link
Member Author

@sifnoc sifnoc Nov 8, 2022

Choose a reason for hiding this comment

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

May need some more explanation in this section about using the script or yarn link :)

WEBSOCKET: 'wss://optimism-kovan.zkopru.network',
}

if (process.env.NODE_ENV == "development") {

Choose a reason for hiding this comment

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

Why not keep the config for testnet? If we changed to this, we can only support local dev env only.

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