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

Improve event typing and update web3 for the web3-1.0.0 target #152

Conversation

JamesLefrere
Copy link
Contributor

Hey @krzkaczor , this is a great project! I had a go at improving the types for events, so that event listeners/event callbacks have properly typed params:

Screenshot 2019-05-03 at 20 54 52

While I was at it, I updated the web3 version; the biggest change here is probably the BigNumber change in web3: web3/web3.js#2570

Changes

  • Upgrade the web3-1.0.0 target's web3 version to the latest beta (54)
  • Adjust test setup and assertions to reflect web3 changes
  • Parse event inputs in order to improve the typing of contract events for the web3-1.0.0 target
  • Update existing types to reflect updated web3 (e.g. EstimateGasOptions)

Let me know what you think. Cheers!

* Upgrade the `web3-1.0.0` target's `web3` version to the latest beta (54)
* Adjust test setup and assertions to reflect `web3` changes
* Parse event inputs in order to improve the typing of contract events for the `web3-1.0.0` target
* Update existing types to reflect updated `web3` (e.g. `EstimateGasOptions`)
@@ -17,7 +17,7 @@
},
"scripts": {
"tsc": "tsc --noEmit",
"mocha": "NODE_ENV=test mocha --require ts-node/register.js './**/*.spec.web3.ts'",
"mocha": "NODE_ENV=test mocha --require ts-node/register.js './**/*.spec.web3.ts' --timeout 5000",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some of the tests took over 2s (Mocha's default) on my computer.

@@ -10,7 +10,9 @@ export let web3: Web3;
export let accounts: string[];

export async function createNewBlockchain() {
const web3 = new Web3(ganache.provider());
const web3 = new Web3(ganache.provider(), undefined, {
transactionConfirmationBlocks: 1,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't look too deeply into this, but without this option, Web3 seemed to hang around for more confirmations.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm i saw similar behaviour with SOME web3 versions in the past.


describe("ContractWithOverloads", () => {
it("should work", async () => {
const contract = (await deployContract("ContractWithOverloads")) as ContractWithOverloads;
const contract = await deployContract<ContractWithOverloads>("ContractWithOverloads");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know which style is preferred; this is just like the other test now.

@coveralls
Copy link

coveralls commented May 4, 2019

Pull Request Test Coverage Report for Build 204

  • 3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 88.438%

Totals Coverage Status
Change from base Build 201: 0.0%
Covered Lines: 393
Relevant Lines: 428

💛 - Coveralls

Copy link
Member

@krzkaczor krzkaczor left a comment

Choose a reason for hiding this comment

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

Great contribution, thank you! 1 comment.

});

it("should allow to pass signed values in multiple ways", async () => {
const contract = await deployContract<DumbContract>("DumbContract");

expect(await contract.methods.returnSigned(2).call()).to.be.eq("2");
expect(await contract.methods.returnSigned("2").call()).to.be.eq("2");
expect((await contract.methods.returnSigned(2).call()).toString()).to.be.eq("2");
Copy link
Member

Choose a reason for hiding this comment

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

according to new typings this should be BN, right? Could you add some kind of type assertion as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, I've now done this in 44e6062 .

It looks like there's still some BigNumber/BN confusion to sort out in web3: web3/web3.js#2468

@JamesLefrere JamesLefrere force-pushed the feature/improve-event-typing-and-update-web3 branch from 70b30bd to 44e6062 Compare May 5, 2019 10:53
@krzkaczor krzkaczor merged commit be25b3e into dethcrypto:master May 24, 2019
@krzkaczor
Copy link
Member

krzkaczor commented May 24, 2019

Merged! sorry that it took so long and thanks for contribution!

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.

None yet

3 participants