Initial pass at converting from TruffleContract to Web3 #10

liamzebedee · 2020-03-12T10:40:57Z

This PR refactors our code to use Web3 as our library for interacting with contracts, replacing using of TruffleContract.

Conversion notes

Web3 returns strings instead of BN's for numbers.
eg.

const creationCost = await this.systemContract.methods.createNewDepositFeeEstimate().call()
// creationCost = '2188250000000000'

Some contract calls involving external systems require us to insert gas estimates (see 2cfd25f for more)

TruffleContract automatically calls estimateGas for many transactions. For Web3, it uses the gas estimates contained in the contract ABI's. Unfortunately, these gas estimates in the ABI's do not include estimation of calls to abstract contracts (interfaces of externals, eg. IBondedECDSAKeepVendor).

- using .returnValues instead of .args - setting defaultAccount for Deposit/Keep contract

see: #11

This is more concise than the long lookup. web3-utils is installed as a peer dependency of the web3 package.

Manual merge based off a `git diff`. It was easier than messing around with rebase.

For JS code that doesn't explictly declare its exports using ES6 Module Syntax (import/export), instead using CommonJS syntax (module.exports = xyz;), we can only import the default export. See this article for a good reference - https://medium.com/the-node-js-collection/an-update-on-es6-modules-in-node-js-42c958b890c

Shadowfiend

Did a long pass here. This mostly looks awesome, few remarks on things we can abstract better, couple of other improvements and tweaks that we can make separately.

Shadowfiend · 2020-03-20T14:34:47Z

src/Deposit.js

-    return await this.systemContract.getAllowedLotSizes()
+    return (await this.systemContract.methods.getAllowedLotSizes().call()).map(
+      toBN
+    )


What does web3 return instead of BN here?

Looks like the answer might be strings… If that's the case, I'm interested in using BigNumber rather than BN. tbtc-dapp already uses BigNumber, and switching back and forth between that and BN is a real pain.

😬 Switching back-and-forth is a pain.

We do have inconsistent usage of big numbers:

we use BigNumber 3x in tbtc-dapp and a handful of times in tbtc

we use BN 90% of the time in tbtc tests

Why BigNumber over BN? 🤔

BigNumber supports decimal math, BN only supports integers. BN is more performant. Right tool for the right job, but I think for frontend stuff being able to do decimal math is a strong win---in the case of the dApp, it's necessary, since we do signing fee math and display values in TBTC, not tSats or whatever we want to call them 😉

Yup, thought there was a reason 😉

Do you think it's a small amount of work? Wondering if I can get the hardware wallet stuff done first before we do this. dApp works currently without it.

Also tSats is palindromic, mmmmm I like it...

Yeah let's handle this after.

src/Deposit.js

Shadowfiend · 2020-03-20T14:40:25Z

src/Deposit.js

+      const contract = new web3.eth.Contract(artifact.abi)
+      contract.options.address = lookupAddress(artifact)
+      contract.options.from = web3.eth.defaultAccount
+      this[propertyName] = contract


Crazy question here: do we want to store contract.methods? I feel like that's the only thing we actually interact with anywhere, and it would make the remaining interactions much cleaner. If we did that we'd probably want to name the properties e.g. tokenMethods, systemMethods, depositTokenMethods, etc, of course.

We probably still need access to the contract stuff for e.g. events, but I wonder if we don't invert the importance and give the *Methods objects a .contract property that references the contract.

Ha, hadn't thought of that.

tokenMethods.balanceOf(address).call() vs. .tokenContract.methods.balanceOf(address).call() - how much cleaner does it seem? Moving in the direction of TruffleContract's API, I would want to remove .call too, so it's as simple as tokenMethods.balanceOf(address).

My opinion - could focus on improving other aspects first.

src/EthereumHelpers.js

Shadowfiend · 2020-03-20T16:00:08Z

src/Redemption.js

@@ -94,25 +97,28 @@ export default class Redemption {
          "SignatureSubmitted",
          { digest: redemptionDigest }
        )
-        const { r, s, recoveryID } = signatureEvent.args
+        const { r, s, recoveryID } = signatureEvent.returnValues


Are r and s here not strings now?

Yeah, they are? Confused as to what you're getting at.

Sorry, I asked this here for some reason, but it's because below you toString both of them.

Yeah. I think single byte256 words are converted into BN/buffer instances by the ABI decoder? Not sure, to be perfectly honest.

Shadowfiend · 2020-03-20T16:00:49Z

src/lib/BitcoinTxParser.js

@@ -1,4 +1,5 @@
-import { TX } from "bcoin/lib/primitives/index.js"
+import BcoinPrimitives from "bcoin/lib/primitives/index.js"
+const { TX } = BcoinPrimitives


Why this change?

I mentioned it in the commit description. GitHub doesn't surface this though :/

For JS code that doesn't explictly declare its exports using ES6 Module Syntax (import/export), instead using CommonJS syntax (module.exports = xyz;), we can only import the default export.
See this article for a good reference - https://medium.com/the-node-js-collection/an-update-on-es6-modules-in-node-js-42c958b890c

This is… Strange. Our current code works fine, right? In the browser and on the command line with Node 12 latest?

To get this running, I have to run:

$ nvm use v12 # node v12.0.0 $ node --experimental-modules --experimental-json-modules --async-stack-traces example-local.js

Without this modified import syntax, the error that is thrown is:

(node:42107) ExperimentalWarning: The ESM module loader is experimental. (node:42107) ExperimentalWarning: Importing JSON modules is an experimental feature. This feature could change at any time file:///Users/liamz/go/src/github.com/keep-network/tbtc.js/src/lib/BitcoinTxParser.js:3 import { TX } from "bcoin/lib/primitives/index.js" ^^ SyntaxError: The requested module 'bcoin/lib/primitives/index.js' does not provide an export named 'TX' at ModuleJob._instantiate (internal/modules/esm/module_job.js:92:21) at async ModuleJob.run (internal/modules/esm/module_job.js:107:20) at async Loader.import (internal/modules/esm/loader.js:164:24) ```

I'd be interested in seeing how you ran it @Shadowfiend - I should've asked before. 😸 For reference, node v10 with --harmony wasn't working for me, but v12 was.

Yeah, v12 only, but never ran into this. Anyway, no need to block for now I guess.

This improves readability in the console.

Shadowfiend · 2020-03-24T00:59:22Z

This feels real close… Interested in figuring out why that change for BcoinPrimitives wasn't necessary before…

I'm also interested in seeing a helper for the 0x replacement and leading .slice(2) that takes us from SPV proof land to Bitcoin land in the output scripts. Happy to merge now if you want to do that in a follow-up, just let me know.

liamzebedee · 2020-03-24T09:24:12Z

@Shadowfiend I'll do a follow-up for the 0x replacement tomorrow. 👍

Shadowfiend

Oookay, let's land this!

This was broken after we swapped from TruffleContract to web3.js in keep-network/tbtc.js#10

liamzebedee mentioned this pull request Mar 19, 2020

findScript fails to find existing tx due to extra bytes #11

Closed

liamzebedee marked this pull request as ready for review March 19, 2020 08:51

liamzebedee force-pushed the from-exotic-fruits-to-web3 branch 3 times, most recently from 24d74cb to b82ddcb Compare March 20, 2020 04:56

liamzebedee added 12 commits March 20, 2020 17:19

Initial pass at converting from TruffleContract to Web3

90c6978

Fetch deposit creationCost from chain again.

98364a2

Convert readEventFromTransaction, getEvent to use Web3

a3ff641

Minor fixups to Web3 logic:

8a392fb

- using .returnValues instead of .args - setting defaultAccount for Deposit/Keep contract

Minor fixes to names

4d035a4

fix: slice off two bytes of redeemerOutputScript

31fc6d7

see: #11

Deposit and Redemption flows successfully running with web3 library

f981c08

Use toBN through web3-utils import

b774536

This is more concise than the long lookup. web3-utils is installed as a peer dependency of the web3 package.

Simplify usages of estimateGas where possible

0eea2f3

Fix remaining BN usages

fcf7871

Prettify

f148ac6

liamzebedee force-pushed the from-exotic-fruits-to-web3 branch 2 times, most recently from a3c4e8d to 0addbb3 Compare March 20, 2020 07:42

Merge master into branch 'from-exotic-fruits-to-web3'

cacc5c5

Manual merge based off a `git diff`. It was easier than messing around with rebase.

liamzebedee force-pushed the from-exotic-fruits-to-web3 branch from 0addbb3 to cacc5c5 Compare March 20, 2020 07:45

Fix remaining linter errors

4807f17

liamzebedee changed the title ~~[WIP] Initial pass at converting from TruffleContract to Web3~~ Initial pass at converting from TruffleContract to Web3 Mar 20, 2020

liamzebedee requested a review from Shadowfiend March 20, 2020 08:02

Shadowfiend suggested changes Mar 20, 2020

View reviewed changes

liamzebedee added 3 commits March 23, 2020 16:42

Improve legibility of isLotSizeAllowed call.

3c1aaaa

fix: use toNumber instead of toString when printing numbers

6f2ab55

This improves readability in the console.

Minor doc touchups.

4aaebf4

Shadowfiend approved these changes Mar 24, 2020

View reviewed changes

Shadowfiend merged commit e93a8ba into master Mar 24, 2020

Shadowfiend deleted the from-exotic-fruits-to-web3 branch March 24, 2020 16:39

liamzebedee mentioned this pull request Mar 25, 2020

Tranche Deux: Add helper for converting ABI-coded bytes into their underlying bytes #12

Merged

liamzebedee added a commit to keep-network/tbtc-dapp that referenced this pull request Apr 1, 2020

fix: call to deposit.contract.signerFee

69ab5e4

This was broken after we swapped from TruffleContract to web3.js in keep-network/tbtc.js#10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial pass at converting from TruffleContract to Web3 #10

Initial pass at converting from TruffleContract to Web3 #10

liamzebedee commented Mar 12, 2020 •

edited

Loading

Shadowfiend left a comment

Shadowfiend Mar 20, 2020

Shadowfiend Mar 20, 2020

liamzebedee Mar 23, 2020

Shadowfiend Mar 24, 2020

liamzebedee Mar 24, 2020

Shadowfiend Mar 24, 2020

Shadowfiend Mar 20, 2020

liamzebedee Mar 23, 2020

Shadowfiend Mar 20, 2020

liamzebedee Mar 23, 2020

Shadowfiend Mar 24, 2020

liamzebedee Mar 24, 2020 •

edited

Loading

Shadowfiend Mar 20, 2020

liamzebedee Mar 23, 2020

Shadowfiend Mar 24, 2020

liamzebedee Mar 24, 2020 •

edited

Loading

liamzebedee Mar 24, 2020

Shadowfiend Mar 24, 2020

Shadowfiend commented Mar 24, 2020

liamzebedee commented Mar 24, 2020

Shadowfiend left a comment

Initial pass at converting from TruffleContract to Web3 #10

Initial pass at converting from TruffleContract to Web3 #10

Conversation

liamzebedee commented Mar 12, 2020 • edited Loading

Conversion notes

Shadowfiend left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liamzebedee Mar 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liamzebedee Mar 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shadowfiend commented Mar 24, 2020

liamzebedee commented Mar 24, 2020

Shadowfiend left a comment

Choose a reason for hiding this comment

liamzebedee commented Mar 12, 2020 •

edited

Loading

liamzebedee Mar 24, 2020 •

edited

Loading

liamzebedee Mar 24, 2020 •

edited

Loading