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

TypeChain ver 1.0.0 #172

Merged
merged 50 commits into from Oct 20, 2019
Merged

TypeChain ver 1.0.0 #172

merged 50 commits into from Oct 20, 2019

Conversation

krzkaczor
Copy link
Member

@krzkaczor krzkaczor commented Sep 21, 2019

Goals:

  • complete ABI parsing to typesafe structures
    • use a discriminative union for EvmTypes
    • support overrides
  • split targets to separate packages
  • support custom targets
  • add prebublish hooks for subpackages
  • better error messages on missing target packages
  • readme
  • readme for each package
  • revive truffle example

Related #150
Closes #65
Closes #106

@coveralls
Copy link

coveralls commented Sep 27, 2019

Coverage Status

Coverage increased (+0.6%) to 90.894% when pulling 732f5db on kk/parser-rewrite into b81cb90 on master.

Copy link
Contributor

@quezak quezak left a comment

Choose a reason for hiding this comment

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

Looks good! Only some minor questions.

packages/core/lib/cli/cli.ts Outdated Show resolved Hide resolved
packages/core/lib/parser/abiParser.ts Outdated Show resolved Hide resolved
packages/core/lib/parser/parseEvmType.ts Show resolved Hide resolved
packages/core/lib/parser/parseEvmType.ts Show resolved Hide resolved
packages/core/lib/parser/parseEvmType.ts Show resolved Hide resolved
packages/typechain-target-ethers/lib/generation.ts Outdated Show resolved Hide resolved
@quezak
Copy link
Contributor

quezak commented Oct 1, 2019

I'll try to test the new version with our project (on ethers) later today, to see if there are no regressions.

@quezak
Copy link
Contributor

quezak commented Oct 2, 2019

Just tested the newest beta version with ethers target, works fine!

Two issues though:

  1. At first, I didn't install the typechain-target-ethers package, and the error message is rather cryptic. If I didn't read the code before, I would have no clue I have to install it. If target is one of the "official" targets, the error should say to install the package, and for other targets, say something about invalid target package (or a link to custom targets FAQ in the future).
   0[quezak@jagdsabel]~/...>: npx typechain --target ethers --outDir $TYPECHAIN_OUT_DIR "build/contracts/*.json"
Error occured:  module.default is not a constructor
  1. I've reviewed changes in the generated files. The big difference is that the functions are now also available on contract object's toplevel, which is correct with how ethers generates them. The difference I'm curious about is about contract variable getters -- they now also have the transactionOverrides parameter. It's not necessary there, maybe even invalid -- constant functions are not called as transactions. Example:

    • solidity:
    bool public isActive = true;
    address public owner;
  • typechain 0.3.19:
    isActive(): Promise<boolean>;
    owner(): Promise<string>;
  • typechain 1.0.0-beta.2 + typechain-target-ethers 1.0.0-beta.1:
    isActive(overrides?: TransactionOverrides): Promise<boolean>;
    owner(overrides?: TransactionOverrides): Promise<string>;

@krzkaczor
Copy link
Member Author

ad 2. it can be related to a bug in isConstantFn. Thanks for bringing this up!

@krzkaczor
Copy link
Member Author

I just fixed ad2 👍, I updated todo in the PR description.

@krzkaczor
Copy link
Member Author

ad 1, oh actually the error message was supposed to be much more civilized but you managed to hit another bug 😆 Fixing it now. It will say something like:

$ npx typechain --target x
Error occured:  Couldnt find x. Tried loading: typechain-target-x, /Users/krzkaczor/Workspace/ethereum-ts/TypeChain/x.
Perhaps you forgot to install typechain-target-x?

@eswarasai
Copy link
Contributor

eswarasai commented Oct 7, 2019

@krzkaczor - I got the below error message when I haven't installed web3-1.0.0 target.

➜  ETHSurvey git:(develop) ✗ npm run typechain 

> ethsurvey@0.0.1 typechain ~/ETHSurvey
> typechain --target=web3-1.0.0 --outDir ./src/types './build/contracts/*.json'

Error occured:  Couldnt find web3-1.0.0. Tried loading: typechain-target-web3-1.0.0, ~/ETHSurvey/web3-1.0.0.
Perhaps you forgot to install typechain-target-web3-1.0.0?

When I tried to do npm install typechain-target-web3-1.0.0, it threw a missing package error. As the package available on npm is under name typechain-target-web3-1. We need to fix the error message being displayed as well as probably change the targets now to web3-v1 and web3-v2 rather than 1.0.0 and 2.0.0. Let me know your thoughts. Thanks!

Edit: Also, after installing, I had to manually go to node_modules and rename the package from typechain-target-web3-1 to typechain-target-web3-1.0.0, in order for it to work.

@krzkaczor
Copy link
Member Author

krzkaczor commented Oct 7, 2019

Thanks for taking a look at it!
So the name of the target changed from web3-1.0.0 to web3-1. TBH I don't see anything particularly wrong with this naming scheme ie. web3-1 and web3-2 but maybe adding v would make it clearer. @quezak thoughts?

I didnt want to use web3-1.0.0 name as it clearly confusing (it works with web3-1.2.x etc.)

@quezak
Copy link
Contributor

quezak commented Oct 7, 2019

yeah, I think web3-v1 would be clearer.

web3-1 souds like it's a web package with version 3.1 :D web3-1.0.0 is also confusing, as it indeed looks like a version specification for web3, and not related to typechain.

<p align="center">
<a href="https://blog.neufund.org/introducing-typechain-typescript-bindings-for-ethereum-smart-contracts-839fc2becf22">Medium post</a> | <a href="https://www.youtube.com/watch?v=9x6AkShGkwU">DappCon Video</a>
</p>
</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

We could add a link to the main TypeChain README to all target package readmes -- in case someone lands there from a google search or something.

@eswarasai
Copy link
Contributor

@krzkaczor - Let me know if you need any help here in wrapping up this PR. I'll get started with the web3-v2 target once this is merged 🙂

@quezak
Copy link
Contributor

quezak commented Oct 10, 2019

@krzkaczor heads up -- I've renamed the web3-v1 integration test dir too, plus some other occurences you missed.

# Conflicts:
#	packages/typechain-target-ethers/lib/generation.ts
@krzkaczor krzkaczor merged commit 3958eea into master Oct 20, 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.

ABI Parser extraction and improvements Support stateMutability
5 participants