Interacts using typechain #12

0xsarvesh · 2019-05-09T06:09:33Z

fixes #7

This PR generates type script contract interacts. Generated interacts are not committed, they should be generated as part of the build process.

Currently interacts are generated with npm run generate and post hook of npm install

Interacts are manually tested, to check script refer 0xsarvesh@a508b20

Also, check discussion about Typechain Vs 0x@ab-gen in the ticket

deepesh-kn

Nice work 💯
Just a comment related to git ignore of interacts.

deepesh-kn · 2019-05-09T07:55:23Z

.gitignore

@@ -14,3 +14,6 @@ build/

 # NPM package generated files:
 dist/contracts.json
+
+# Do not track generated wrapper, it should be part of build script
+interacts/*


I can see that you have added postinstall script.
Its mentioned in the typechain documentation not to commit the interacts. What will be the disadvantages if the interacts are committed?

I am thinking in general, If we publish an NPM package, will the interacts be available? Or the project that consumes the package needs to handle the interact generation separately?

The interacts would still be part of the published package in my opinion, just not part of the repository. That way, it is guaranteed that the interacts are always up to date with the latest code changes on the solidity layer.

Yes, Interacts will be available in the published package, it's just they are not tracked by git rather generated each time. This will make sure that everyone has the latest interacts.

At the time of publishing, the latest interacts will be generated(as a build step)and published.

Yes, Interacts will be available in the published package, it's just they are not tracked by git rather generated each time.

Will it not be confusing on github if there are modules/libraries/helpers written on top of generated interacts 💭 ? Written modules will be tracked in git but the dynamic interacts will not be.

e.g. https://github.com/OpenST/openst.js/blob/develop/lib/helper/User.js
User.js is written on top of multiple contract interacts. So we will commit User.js in github calling generated interacts methods which will not be tracked in github.
One option is we describe this clearly in readme but I just feel it will be confusing for open source community looking at the code.

What do you think 🤔 ?

In the contract repository, there shouldn't be any complex class which uses multiple interacts. They should go into Interaction layer like mosaic.js, openst.js where these thin interact will always be available with published npm of contracts in my opinion.

schemar · 2019-05-09T09:03:51Z

Reviewing!

schemar

Very nice 👍

Should we somehow provide something to the consumer to ease deployment or do we expect them to write that themselves? 🤔

schemar · 2019-05-09T09:12:16Z

.gitignore

@@ -14,3 +14,6 @@ build/

 # NPM package generated files:
 dist/contracts.json
+
+# Do not track generated wrapper, it should be part of build script
+interacts/*


The interacts would still be part of the published package in my opinion, just not part of the repository. That way, it is guaranteed that the interacts are always up to date with the latest code changes on the solidity layer.

schemar · 2019-05-09T09:15:29Z

ts-generator.json

+    "target": "web3-1.0.0",
+    "outDir" : "interacts"
+  }
+]


package.json

0xsarvesh · 2019-05-09T10:48:20Z

Very nice 👍

Should we somehow provide something to the consumer to ease deployment or do we expect them to write that themselves? 🤔

I also feel theoretically it should be possible to deploy contracts from generated interacts as they extend from web3.eth.contracts which has deploy method.

I have created a ticket to research on this. #13

abhayks1 · 2019-05-09T12:31:27Z

Nice work 💯Typechain looks like a simple to use option.

I have few questions comparing with our existing interacts approach:

How typechain takes care of validations for the interact methods? Does it also generates validations along with contract interact code? Raising this point because in the past we had discussions on adding validations for the JS layers and their respective unit tests 🤔
Referring to PR description:
Interacts are manually tested, to check script refer sarvesh-ost@a508b20

This means positive/negative unit tests needs to be manually written using sinon after generating the interacts covering all the cases. Right ❓

0xsarvesh · 2019-05-09T13:33:04Z

Nice work 💯Typechain looks like a simple to use option.

I have few questions comparing with our existing interacts approach:

How typechain takes care of validations for the interact methods? Does it also generates validations along with contract interact code? Raising this point because in the past we had discussions on adding validations for the JS layers and their respective unit tests 🤔

Typechain generates interacts in typescript where types are checked at compile time. Which means you can't pass a complex object at the place of string.
But checks like conversion decimal should be less than 5 or conversion rate should be greater than zero are not handled here.

Referring to PR description:
Interacts are manually tested, to check script refer sarvesh-ost@a508b20

This means positive/negative unit tests needs to be manually written using sinon after generating the interacts covering all the cases. Right ❓

No, I don't think unit testing of generated interacts is required that would mean we will be testing the generator tool(that it generates the correct classes). However, we should definitely use these interacts for unit testing of contract logic.

schemar

Almost done, just these minor remarks.

package.json

schemar

LGTM 🐧

Sarvesh Jain added 7 commits May 3, 2019 17:39

Added example of typechain

a508b20

Deleted commited contract wrappers

1bedfe4

added script of generating wrapper and added types into git ignore

b580551

Added post install script

2bf9cbc

Merge remote-tracking branch 'upstream/develop' into type_chain_example

40ffcfe

Added output directoryt and not generating interacts for Migrations

c13786c

Removed script for manual test

1591eab

deepesh-kn reviewed May 9, 2019

View reviewed changes

schemar self-assigned this May 9, 2019

schemar suggested changes May 9, 2019

View reviewed changes

schemar removed their assignment May 9, 2019

0xsarvesh mentioned this pull request May 9, 2019

Deployment of contracts #13

Open

Removed caret operator from the typeschain dependency

8531976

abhayks1 self-assigned this May 9, 2019

abhayks1 removed their assignment May 9, 2019

schemar suggested changes May 10, 2019

View reviewed changes

package.json Show resolved Hide resolved

schemar assigned schemar and unassigned schemar May 10, 2019

schemar approved these changes May 13, 2019

View reviewed changes

schemar merged commit 8a2b98f into OpenST:develop May 13, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Interacts using typechain #12

Interacts using typechain #12

0xsarvesh commented May 9, 2019

deepesh-kn left a comment

deepesh-kn May 9, 2019

schemar May 9, 2019

0xsarvesh May 9, 2019

abhayks1 May 9, 2019 •

edited

Loading

0xsarvesh May 9, 2019 •

edited

Loading

schemar commented May 9, 2019

schemar left a comment

schemar May 9, 2019

schemar May 9, 2019

0xsarvesh commented May 9, 2019

abhayks1 commented May 9, 2019 •

edited

Loading

0xsarvesh commented May 9, 2019

schemar left a comment

schemar left a comment

Interacts using typechain #12

Interacts using typechain #12

Conversation

0xsarvesh commented May 9, 2019

deepesh-kn left a comment

Choose a reason for hiding this comment

deepesh-kn May 9, 2019

Choose a reason for hiding this comment

schemar May 9, 2019

Choose a reason for hiding this comment

0xsarvesh May 9, 2019

Choose a reason for hiding this comment

abhayks1 May 9, 2019 • edited Loading

Choose a reason for hiding this comment

0xsarvesh May 9, 2019 • edited Loading

Choose a reason for hiding this comment

schemar commented May 9, 2019

schemar left a comment

Choose a reason for hiding this comment

schemar May 9, 2019

Choose a reason for hiding this comment

schemar May 9, 2019

Choose a reason for hiding this comment

0xsarvesh commented May 9, 2019

abhayks1 commented May 9, 2019 • edited Loading

0xsarvesh commented May 9, 2019

schemar left a comment

Choose a reason for hiding this comment

schemar left a comment

Choose a reason for hiding this comment

abhayks1 May 9, 2019 •

edited

Loading

0xsarvesh May 9, 2019 •

edited

Loading

abhayks1 commented May 9, 2019 •

edited

Loading