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

[web3@1.0.0] Generated typings don't have BigNumber as a type #103

Open
eswarasai opened this issue Sep 27, 2018 · 9 comments
Open

[web3@1.0.0] Generated typings don't have BigNumber as a type #103

eswarasai opened this issue Sep 27, 2018 · 9 comments

Comments

@eswarasai
Copy link
Contributor

With web3@1.0.0 as target, generated types no longer support BigNumber for the contracts. As an example, this is the typing generated with web3@0.26.0 -- https://github.com/MARKETProtocol/types/blob/develop/types/MarketCollateralPool.ts#L178 and the below is generated using web3@1.0.0 target which has string instead of BigNumber -- https://github.com/MARKETProtocol/types/blob/feature/upgrade-web3-1.0.0/types/MarketCollateralPool.d.ts#L44

BigNumber might need some additional parsing while generating the types I guess. Would be really great if this can be supported soon. Thanks :)

@eswarasai eswarasai changed the title Generated typings don't have BigNumber as a type [web3@1.0.0] Generated typings don't have BigNumber as a type Sep 27, 2018
@krzkaczor
Copy link
Member

@eswarasai this is expected behavior. Targets like web3@1.0.0 or truffle will generate typings for API provided by given libraries. This means that decision to use a string in public API was made by web3 team, not ethereum-ts/TypeChain.

You can always write automatic transformer on userland using a combination of conditional types and dynamically wrapping responses from web3. I know it's not the cleanest solution but this way we can stay out of discussions what API should TypeChain provide in generated wrappers.

I think it raises a broader question: should TypeChain allow to customize targets. For example, one can always write own target and just copy web3 target with some changes. I think we should at least document and support easily this use case.

@NicolasMahe
Copy link

It seems that web3@1.0.0 actually support BigNumber as well as BN for anything in Wei (transaction value and gas price at least).

Source:
https://web3js.readthedocs.io/en/1.0/web3-eth.html?highlight=BigNumber#sendtransaction

@sebastianst
Copy link

Note that web3.utils etc. use BN from BN.js internally. Some functions don't even support BigNumber but only BN, like web3.utils.toWei, so I suggest to change the exported type to BN to make it compatible to web3.js.

@hickscorp
Copy link

@sebastianst How do you do that? The .ts files generated by TypeChain import BigNumber and there's no way to override that. Did you find a solution to this problem?

@sebastianst
Copy link

I ended up with some hacks in the scripts section of the package.json file:

"scripts": {
    "postinstall": "patch node_modules/truffle-typings/index.d.ts < patches/truffle-typings.d.patch || true",
    "build:contracts": "truffle compile",
    "build:types": "typechain --target truffle './build/**/*.json'",
    "postbuild:types": "sed -i -e 's/BigNumber/BN/g' -e '/bignumber\\.js/d' types/truffle-contracts/index.d.ts",
    "build:js": "tsc -p .",
    "build": "yarn build:contracts &&  yarn build:types && yarn build:js",
    "test": "truffle test"
}

The patch looks like

--- /home/self/truffle-typings/index.d.ts	2019-02-04 13:23:01.173368580 +0100
+++ truffle.d.ts	2019-02-04 13:24:30.993372687 +0100
@@ -3,15 +3,19 @@
  */
 /// <reference types="chai" />
 /// <reference types="mocha" />
+
+declare type BN = import("web3-utils").BN;
+declare type Web3 = import("web3").default;
+
 declare const assert: Chai.AssertStatic;
 declare const expect: Chai.ExpectStatic;
 
+declare const web3: Web3;
+
 declare function contract(name: string, test: (accounts: Truffle.Accounts) => void): void;
 
 declare const artifacts: Truffle.Artifacts;
 
-declare const web3: any;
-
 /**
  * Namespace
  */
@@ -19,10 +23,10 @@
   type Accounts = string[];
 
   interface TransactionDetails {
-    from: string;
-    gas?: number | string;
-    gasPrice?: number | string;
-    value?: number | string;
+    from?: string;
+    gas?: BN | number | string;
+    gasPrice?: BN | number | string;
+    value?: BN | string;
   }
 
   export interface TransactionLog {

But I didn't test this for the last three months, so this probably has to be adjusted.

Does that answer your question?

@hickscorp
Copy link

hickscorp commented Jun 12, 2019

@sebastianst Thanks a lot. This is pretty much the same as we came up with as well.
I think the TypeChain should allow for "configurable" BigNumber library. It is such a great tool, I think it would benefit from some little flexibility.

@krzkaczor
Copy link
Member

@hickscorp @sebastianst I agree that we need "custom" target or something like this. Let me know if you want to work on this

@hickscorp
Copy link

@krzkaczor I'd be very happy to help but I've literally never worked on a tooling npm package - I'm a bit scared that I'll go against good practices. If you would however like me to help with specs for whoever would like to take on the development part, happy to do a clean ticket recap'ing what should be done.

@krzkaczor
Copy link
Member

@hickscorp don't worry about it, you did a fine job on truffle-typings repo ;)

Some background on this feature. TypeChain under the hood uses https://github.com/krzkaczor/ts-generator which abstracts away some parts regarding type generation. What we could do is allow typechain users to provide custom "plugin" for ts-generator. All existing targets are just plugins for ts-generator.

CLI usage could look something like:

typechain --target=./typechain-generator.ts --outDir app/contracts './node_modules/neufund-contracts/build/contracts/*.json'

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

No branches or pull requests

5 participants