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

Init web3-test-utils Package #5426

Closed
wants to merge 45 commits into from
Closed

Conversation

spacesailor24
Copy link
Contributor

No description provided.

@spacesailor24 spacesailor24 added the 4.x 4.0 related label Sep 13, 2022
@spacesailor24 spacesailor24 self-assigned this Sep 13, 2022
@render
Copy link

render bot commented Sep 13, 2022

@lgtm-com
Copy link

lgtm-com bot commented Sep 13, 2022

This pull request introduces 1 alert when merging 56d0609 into bc39fe9 - view on LGTM.com

new alerts:

  • 1 for Useless comparison test

@@ -104,6 +104,7 @@ export class Web3Context<
| SupportedProviders<API>
| Web3ContextInitOptions<API, RegisteredSubs>,
) {
console.log(providerOrContext)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think its for local debugging so should be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes this PR is still a draft and not ready for review

@@ -37,7 +37,8 @@
"jest-when": "^3.5.1",
"prettier": "^2.7.1",
"ts-jest": "^28.0.7",
"typescript": "^4.7.4"
"typescript": "^4.7.4",
"web3-test-utils": "0.1.0-alpha.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

This will create cyclic dependency? as testing utils will use accounts pkg for account creation.

@@ -42,7 +42,8 @@
"typescript": "^4.7.4",
"web3-eth-abi": "^4.0.1-alpha.0",
"web3-eth-accounts": "^4.0.1-alpha.0",
"web3-providers-http": "^4.0.1-alpha.0"
"web3-providers-http": "^4.0.1-alpha.0",
"web3-test-utils": "0.1.0-alpha.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

same here, cyclic dependency question?

return { address: newAccount.address, privateKey: newAccount.privateKey };
}

const fundGethAccount = async (fundeeAddress: Address, fundAmount: BigInt) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Mostly logic for funding test accounts will remain same, we just can pass from address ( or coinbase in case of geth, or one of 10 pre funded ganache acct) or any provided by user. so can we omit diff implementation for diff nodes.

@lgtm-com
Copy link

lgtm-com bot commented Sep 14, 2022

This pull request introduces 1 alert when merging 6e0210e into 7e236d0 - view on LGTM.com

new alerts:

  • 1 for Useless comparison test

@lgtm-com
Copy link

lgtm-com bot commented Sep 14, 2022

This pull request introduces 1 alert when merging 4b23a08 into 7e236d0 - view on LGTM.com

new alerts:

  • 1 for Useless comparison test

@lgtm-com
Copy link

lgtm-com bot commented Sep 14, 2022

This pull request introduces 1 alert when merging 0d1c8b1 into 7e236d0 - view on LGTM.com

new alerts:

  • 1 for Useless comparison test

@lgtm-com
Copy link

lgtm-com bot commented Sep 14, 2022

This pull request introduces 1 alert when merging 1291e52 into 7e236d0 - view on LGTM.com

new alerts:

  • 1 for Useless comparison test

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4.x 4.0 related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants