Bring Truffle to 4.0.4 to be able to properly tests asserts on revert.

[simondlr]

I was busy rebasing #98 (ERC721) and realised EIP20 tests were failling. The reason was that it was written pre-Byzantium where invalid opcode was thrown, not REVERT. I bumped to Truffle 4.0.4 which has REVERT in it. I cleaned up the tests for this and added in assertRevert helper. This helper is ES6 and requires some Babel transpilers.

[maurelian]

Please pin the versions.

[skmgoldin]

Why introduce babel at all? It doesn't seem to actually be used anywhere.

[simondlr]

It's for the assertRevert.js that uses ES6. It needs to be transpiled.

[simondlr]

Unless I have it wrong? Maybe one can remove it somehow? But it was needed for adding assertRevert. Do you know how to change both to not have to use Babel? I admit, transpilers always confuzzle me.

[skmgoldin]

What needs to be transpiled? I don't see any super exotic JS in assertRevert.js

[skmgoldin]

What Node JS version are you using? We develop against Node 8 for this repo.

[simondlr]

Maybe it's just the import statement then in the tests? vs require? Lemme try that.

[maurelian]

please pin package.json version.

I wonder if we can enforce that with eslint?

[simondlr]

Fixed the pending issues. Removed Babel. Wasn't really needed @skmgoldin @maurelian .

[skmgoldin]

Delete this line

[skmgoldin]

Might as well bump to latest, which is 4.0.5

[skmgoldin]

I like using assertRevert on things that return promises, but for things that only take callbacks might it be more legible to just do the check explicitly inside the callback itself? What do you think @maurelian ?

I think this code is right, it's just a little hairy.

[maurelian]

Agree, I think I've done the same thing in a previous version of the test suite, but it feels like wrapping and unwrapping at the same time.

[simondlr]

I'm not 100% sure how to do this without bungling up the usage of async/await?

Here's how I understand it: if it's a callback and not using async/await, then it will resolve in its own asynchronous thread, and then the following lines (49+) will execute independently of the sendTransaction. Won't this break the linear asyncs?

Doesn't this need to be chained as usual then with .then() style promises (or more traditional callback hell)?

Is that the intent? I'm not 100% sure I grok how to fix this?

[maurelian]

Good point. I didn't consider that there's another await call made after this. I was able to get this work though:

web3.eth.sendTransaction(
  { from: accounts[0], to: HST.address, value: web3.toWei('10', 'Ether') },
  async (err, res) => {
    assert(err);
    assert.strictEqual(res, undefined);

    const balanceAfter = await HST.balanceOf.call(accounts[0]);
    assert.strictEqual(balanceAfter.toNumber(), 10000);
  },
);

And I think one callback is reasonable.

Another option could be to promisify it at the top of the file:

const promiseSend = function (tx) {
  return new Promise((resolve, reject) => {
    web3.eth.sendTransaction(tx, (err, res) => {
      if (err) { reject(err); }
      resolve(res);
    });
  });
};

then the test itself looks like any other one.

[skmgoldin]

Now that I think about it, if we use just the callback, then we have to add a done to this function I think. Because it doesn't return a promise. https://mochajs.org/#asynchronous-code

But the second thing you wrote I think would work @maurelian

[skmgoldin]

I think I'm okay with merging this as-is though and coming back to this question later.

[maurelian]

Now that I think about it, if we use just the callback, then we have to add a done to this function I think. Because it doesn't return a promise.

FWIW, I had tried that, and got a 'resolution method overspecified' error. The sample I shared was working.

[skmgoldin]

LGTM. LGTY, @maurelian?

[maurelian]

LGTM