Typescript rewrite

[corollari]

Hi, when working on a liquidation bot I started typing this library to interact with it and, given that I had already started, I decided to go ahead and create a full set of ts typings for it.

Why not programatically convert the JSTDoc annotation to ts types?

That's actually the first thing I tried to do, concretely I tried using the following tools:

• https://github.com/otris/jsdoc-tsd
• https://github.com/fivetran/typescript-closure-tools
• https://github.com/englercj/tsd-jsdoc

Well, the first two just ran into runtime errors when parsing the code, the third one managed to do a little better but the output was full of any, some types seemed to be wrong and exports were not properly handled, so after spending some time trying to fix all that I realized that it'd be faster to just write the types myself.

Then I started writing a few declaration files, but ensuring type correctness without direct compiler passes turned out to be quite hard, so I just went ahead and rewrote all the files in typescript. Furthermore, I also created types for the libraries tbtc.js interacts with, for example bcoin or bcrypto, in order to check the correctness of those types too (interestingly that also lead to a small fix landing on bcoin).

Extra modifications

Adding types brought some possible problems to the surface, I ended up fixing the most simple ones (I tried not to make any runtime modifications) with the following changes:

• Added some typeguards
• Coalesced some imports, as they were importing the same object
• Changed mainnet string to 'main' to match the string used in bcoin (was fixed on lib/Address.js, but that was unused)
• Fixed some incorrect types
• Remove dead code (lib/Address.js)
• Some other minor fixes

There are some problems which I left untouched as I believe that it's better to just directly discuss them:

• src/Deposit.ts:386 - How is forTDT() supposed to work? I haven't been able to find any usage of it and the implementation seems to be stub
• How does electrum return numeric values? ElectrumClient seems to simply parse json responses, so I assumed that the numeric values would be number but I've run into some problems with that
• src/Deposit.ts:1301 - What object is being referenced by deposit?
• getLatestRedemptionDetails() may return `null``but the rest of the code seems to assume that calls to it will always successfully return proper details, am I missing something?

A note on modules

When I started working on my liquidation bot, the single biggest issue I had to contend with was the use of ES imports/exports in the published npm module. This seemed to be a problem caused by the fact that tbtc.js is marked as an ESM module by the inclusion of {type:"module"} on package.json, which lead to the following problems:

• When ts is configured to use one type of modules, running nodejs on the generated modules causes a runtime error to be thrown because of tbtc.js
• When configured to use a different type of modules, nodejs throws another error because of the other modules

This made my code unexecutable so I attempted several strategies to fix it:

• Use the esm package, which didn't fix the problem
• Bundle the code using webpack with bable, which should transpile all the code to a standard version. The problem with this is that one of the dependencies of tbtc.js (concretely one of the web3-* packages) contains an import to electron in it's code. Now, that assumes that it will be run inside electron and the electron package will be made available at runtime by electron, but because that's not the case all kinds of errors pop up:
  • If electron is bundled a runtime error is thrown on execution because some unspecified electron dependencies can't be found
  • If webpack is forced to ignore electron during bundling then a different runtime error is thrown
• Some other solutions which ended up failing

Ultimately I solved it by running babel against the module files and converting all imports and exports statements, but this resulted in abysmal usability. I believe that by publishing code which uses commonjs modules it would be possible to improve developer experience, and that's also one of the benefits of this PR, as typescript is configured to output that type of code.

Yet another note, this time on contract calls

A lot of the contract calls do not provide a from option field, instead, they rely on web3 using the from account that has been assigned to a contract and will be used by default. However, the account assigned as default is web3.eth.defaultAccount, which could be undefined, which doesn't result in a compiler error as the contract property that is being assigned to can also be undefined (under normal operation it is expected that from will be provided on the contract call) but will lead to a runtime error.

Should a check be added to prevent that?

[corollari]

Just fixed all merge conflicts 👍

[Shadowfiend]

Oof… Unfortunately I'm ahead of you on the typing here (see #65, the open PRs #70 and #71, and the just-pushed branch type-bins which I'm going to break down into another couple of PRs, which combined get us to strictNullCheck and noImplicitAny type-checking paired with pre-commit and PR linting for types). We're also unfortunately currently not interested in adding additional build steps to the library unless it proves completely unavoidable.

That said, I'm paying close attention to the feedback about modules (see #31, which also has some additional details about build steps), and do want to ensure solid interop with TypeScript (which as I understand it can extract types from JSDocs natively)---I'd be very interested in a minimal repo that demonstrates the build issue you're having with the module setup this package has, and an issue filed to that end.

Lastly:

A lot of the contract calls do not provide a from option field,

This is a known issue, currently tracked as #31 and in a few FIXMEs from the aforementioned PRs. The intent is to provide a way to specify from alongside the config when setting up the TBTC object, but we're definitely open to ideas here. Right now the typing PRs roughly preserve the current functionality by defaulting from to "" as a backstop, which should fail when creating the transactions. The points where that happens offer an anchor for adding proper config-based handling.

---

Apologies for pulling the air brake on this when it's clear you've put in a lot of time and effort---in general I encourage folks to discuss big efforts like this and the underlying motivation in issues before putting work in to make sure there isn't too much overlap of work or directional mismatch. As with the use of ES6 modules, it's very much a deliberate decision not to use TypeScript directly in this library---though not an ironclad one. Build steps introduce future complexity and maintenance burden that we're trying to avoid if possible. Whether that proves viable remains to be seen, but looking at your build issues using this with a TypeScript project is going to be a big first step here, and I'm going to keep this PR open until we've had a chance to look at that and make sure the issues can be dealt with.

[corollari]

which combined get us to strictNullCheck and noImplicitAny type-checking paired with pre-commit and PR linting for types

Just wanted to mention that even if this PR is discarded it may be interesting to apply some of the types changes (and/or dependency typings) made on this PR to the repo, as the code in this PR builds with strict checks while also taking into account the dependencies' types, so theoretically the type safety achieved should be higher.

I'd be very interested in a minimal repo

It seems you've already found my repo, which makes it trivial to reproduce the issue, but for anyone else reading this, this repo should serve an example for that. It should also be possible to explore the build issues I had with typescript, which I described in the OP, by just removing some of the last commits.

This is a known issue, currently tracked as #31

Is that issue reference wrong? That issue seems to be about the module system in use. It seems the correct issue is #32.

in general I encourage folks to discuss big efforts like this

I understand that, but in my mind this PR seemed a perfect fit for this repository:

• Better typechecking was something that was explicitly requested in Complete typing and enable ts-check across the repo #65 and, when I started working on this, none of the type PRs had been created yet
• Better interoperability with typescript seems like something that would be interesting for any library
• Bug fixes are generally always welcome

With that said, I agree that I should have probably communicated that before, but I didn't see any reason to reject the PR at that moment.