Skip to content

Conversation

@gulshanvasnani
Copy link
Contributor

@gulshanvasnani gulshanvasnani commented Feb 19, 2019

PR contains :-

  1. Added docker.js and docker-compose.yml to facilitate use of geth docker.
  2. Modified test-cases to use geth docker.
  3. Removed files helper.js and Web3WalletHelper.js.

@abhayks1 abhayks1 changed the title Integrated docker geth Docker geth integration and cleanup Feb 19, 2019
Copy link
Contributor

@benjaminbollen benjaminbollen left a comment

Choose a reason for hiding this comment

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

LGTM; I'm outside of this discussion, but at a glance, it looks good.

// // wallet3, wallet9 are the owners.
// wallet1 and wallet2 are the owners.
it('Should create a user wallet', async function() {
await auxiliaryWeb3.eth.accounts.wallet.create(1);
Copy link
Contributor

Choose a reason for hiding this comment

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

what s the difference with await auxiliaryWeb3.eth.accounts.wallet.create(3); ?

Copy link
Contributor Author

@gulshanvasnani gulshanvasnani Feb 19, 2019

Choose a reason for hiding this comment

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

@benjaminbollen , Thank you.
Used await auxiliaryWeb3.eth.accounts.wallet.create(3); to create 3 accounts.
Committing the fix.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand the system when we seem to call await auxiliaryWeb3.eth.accounts.wallet.create at various places throughout the tests;

this may be less important, and can be addressed later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@benjaminbollen
By default, Docker geth provides only 3-accounts.
So, to use more accounts,we need to create it dynamically.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok!

Copy link
Contributor

@benjaminbollen benjaminbollen left a comment

Choose a reason for hiding this comment

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

LGTM

@benjaminbollen benjaminbollen merged commit a2d39b9 into OpenST:develop Feb 19, 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.

2 participants