openzeppelin / openzeppelin-test-helpers Goto Github PK

Hi! A zero bytes32 constant would be very useful!
Thanks.

PaulRBerg reports on Twitter that Test helpers break compatibility with bignumber.js in their tests
(all tests that had "should.be.bignumber.equal" broke)

https://twitter.com/PaulRBerg/status/1163017370434265090?s=20

It might be better if the helpers were agnostic to the framework that is being used, and used web3.js directly.

The constants module is not documented in the README.

The current configuration system relies on global mutable values, and this may come back to bite us at some point.

We should consider changing either the "global" part, by allowing multiple instances configured in different ways, or changing the "mutable" part, for example using a config file.

These are the contents of the installed package directory, inside node_modules:

$ ls
contracts  LICENSE     node_modules  README.md  src   test-integration
index.js   migrations  package.json  scripts    test  truffle-config.js

Only index.js and src should be there, along with the readme and license files.

The time API functions are documented here https://docs.openzeppelin.com/test-helpers/0.5/api#time

However there is no proper definition of what units and types these functions return

• String vs. int vs. big num
• Seconds vs. milliseconds
• Epoch

@jamesmorgan suggested on the Telegram channel adding support for evm_snapshot and evm_revert. Given that their semantics are a bit strange, and that a loot of speed can be gained with proper usage of snapshots, I think this is a great idea.

I'm not sure on the API, we could definitely simply provide simple functions to call these, but it might be possible to achieve a more ergonomic solution. @jamesmorgan could you share more about your use case to see if we can tailor our solution to it? Thanks!

For the Mac folks with Node install with brew,

If you are getting an error like "error [email protected] install: node-gyp rebuild" when installing with npm, you should try to remove node and install it from the node site.

link

Reverted transactions still have a transaction receipt with has, used gas, etc.: we should return this object to the caller.

Do we want to promisify the current web3 provider and export that? How will this affect people using web3 1.x?

Also, we can't have people running const { web3 } = require('openzeppelin-test-helpers'), since that will clash with their own web3 instance.

await shouldFail.reverting.withMessage(this.safeMath.mul(a, b), 'SignedSafeMath: multiplication overflow');
returned success even when contract had two requires one with SignedSafeMath: multiplication overflow 1 and another with SignedSafeMath: multiplication overflow 2.
From looking at the code it seems it happens because of the use of "indexOf".
This seems like an unexpected behavior.

const { BN, constants, expectEvent, expectRevert, time } = require('openzeppelin-test-helpers/configure');

Rises the following error:

Error: Error: [email protected] detected, incompatible with requirement of [email protected]

Seems that [email protected] is forcing web3 to bump to version 1.2.

For some reason we forgot to add this restriction, see this comment.

Since integration tests are located in a subdirectory of the package itself, they inherit all of its dependencies due to Node's module resolution algorithm.

This causes situations where our integration tests will pass, but in isolation they wouldn't, due to missing dependencies.

I'm not sure what's the common way of doing this.

For the same reasons described in OpenZeppelin/openzeppelin-contracts#1687, we probably want to switch to expect-based naming (like expectEvent).

Truffle and web3 make it hard to get the events emitted during a contract's creation, and when doing a raw transaction (web3.eth.sendTransaction): we should improve our helpers so that this can be easily done. Some contract's events are not being tested due to this, including Ownable, Secondary and PaymentSplitter.

Related to OpenZeppelin/openzeppelin-contracts#1354.

The current helper attempts to match all values on the first event that it finds with a matching name. In the case of a value mismatch, it should keep searching the event list, as opposed to exiting immediately. See for example PaymentSplitter's constructor, where multiple PayeeAdded events are emitted in the same transaction.

hey everyone what's up
it's kind of surprising for me too since truffle should come with chai natively but I guess it only has it in form of internal use and doesn't provide it to external modules such as openzeppelin-test-helper
missing chai dependency and when I npm install it manually it fixes here's a snapshot

chai includes an (afaik) mandatory message when include fails, which causes the actual error log to contain the expected message twice, and be generally very confusing.

This is what you get when a test expects 'ERC777: token recipient contract has no implementer for ERC777TokensRecipient' but gets 'SafeMath: subtraction overflow':

AssertionError: Wrong failure type, expected 'ERC777: token recipient contract has no implementer for ERC777TokensRecipient': expected 'Returned error: VM Exception while processing transaction: revert SafeMath: subtraction overflow -- Reason given: SafeMath: subtraction overflow.' to include 'ERC777: token recipient contract has no implementer for ERC777TokensRecipient'

We should either opt-out of this chai behavior, or write a custom test function.

We are only testing @truffle/contracts.

The abi field for it does not have an inputs property, which causes a failure. This use case should be supported.

See #18 for the main changes.

We have to start a changelog and in this case give the steps needed to migrate from v0.2 to v0.3.

Hey.

A pattern I've often had to do was to either estimate the tx cost (gas * gasPrice) in wei beforehand OR after a transaction take gasUsed * gasPrice to calculate how much wei was spent in the transaction. This is used in transactions, for example, where someone withdraws ETH, to see if the balance AFTER withdrawal is correct [withdrawn ETH - tx fee to withdraw ETH].

Is this something that would be meaningful? Are there others who want a similar helper? If so, I might take a stab at it, but first want to gauge interest.

Something like:

estimatedTxCost(<contract method>, <tx data>)
or
txCost(<txReceipt>, <gasPrice>)

Unsure if this would be meaningful (or something like it). So want to hear from others.

#59 changed the error message reported when expectRevert is used and the messages don't match. As can be seen in the added test, the new message is simply Wrong kind of exception received, which is not very useful in figuring out what the actual exception was.

We should add back the expected and received strings.

balanceDifference currently has an uncomfortable API that uses a callback. I propose the following neater API.

• balanceTracker(owner: Account): Promise<BalanceTracker>
• BalanceTracker.get(): Promise<BigNum>

Example:

const ownerBalance = await balanceTracker(owner);
expect(await ownerBalance.get()).to.bignumber.equal('0');
await send.ether(owner, '10');
expect(await ownerBalance.get()).to.bignumber.equal('10');

See here for an instance where an event argument has a value with type uint64[3].

This was recreated using @openzeppelin/test-helpers v0.5.3

Using any part of the error message string in an expectRevert allows a test to succeed. Imagine the following code that works as expected:

Manager.sol

function setManager(address _mgr) public {
    require(managers[msg.sender] == true, "Must be manager");
    ...
}

ManagerTest.js

it("Should revert for a non-manager", async () => {
    const revertMsg = 'Must be manager'
    await expectRevert(manager.setManager(accounts[1], { from: accounts[0] }), revertMsg)
  })

However, this test will also pass if revertMsg = 'Must be, revertMsg = 'Must', or even revertMsg = 'M'.

I would assume that the revert message string should be identical to the expectRevert message.

In send.js as part of the EIP1820 deploy steps:

function ether (from, to, value) {
  return web3.eth.sendTransaction({ from, to, value, gasPrice: 0 });
}

The gas price here is hardcoded. I'm on a chain where I require non-zero gas price. Can this be configurable?

In some cases, event parameters need to be manually tested (e.g. because a value is only checked to be in a range as opposed to being exactly equal to another one). To support these scenarios, expectEvent should return an object with the event, so that these values can then be extracted and tested manually:

const event = expectEvent(receipt, 'MyEvent');
expect(event.args.param).to.be.greaterThan(50);

Since we're both supporting web3 and truffle workflows, which have different event formats in their receipts, it is not clear what format the returned object should have. In particular, there's a difference in how numbers are handled: truffle returns BN instances, while web3 uses regular strings.

expectEvent does not match events if there are internal transactions between contracts that emit events. Please correct if I am wrong but this is the behavior I was actually observing.

This might ve something internal to a contract class. My guess it only considers ABI of the contract that was target of to address in the tx. I added an example tx receipt where you can see only 2 of 3 raw event logs where decoded.

Suggested solutions - not sure if this is a bug or an enhancement

• Document the current behaviour so there is no mismanaged expectations what ABIs expectEvent can match
• If there is no global source of event ABIs, give a list of event ABIs or Contract instances to expectEvent as a parameter, so it explicitly knows what events to decode

const receipt = await newToken.send(staking.address, STAKE_PRICE, Buffer.from(''), { from: user });

 {
      tx: '0xb2d009b34da07e386f4fc9cfde74b043b107f91706bc554c811217ba2d04eb5f',
      receipt: {
        transactionHash: '0xb2d009b34da07e386f4fc9cfde74b043b107f91706bc554c811217ba2d04eb5f',
        transactionIndex: 0,
        blockHash: '0x318665891da411d835426036994a011086c5daf79396a6b6d111e730ad4a3989',
        blockNumber: 9,
        from: '0xee01a8b398992efe7fda65e9b9a2ed42ebc2b1fd',
        to: '0xd8c5c8dd412ce99f9fb17ff93fb4af23b50206e5',
        gasUsed: 201012,
        cumulativeGasUsed: 201012,
        contractAddress: null,
        logs: [ [Object], [Object] ],
        status: true,
        logsBloom: '0x00000000000000008000000000000800010000000000000000000200000000000000000000000000000000000000000000000000000000000000000000000004000000000000010000000008000000000000000000100000000000000000000000000000000000000000000000000000000000400000000000000010000000000000000000000000000000000000000000100000000000000000048000000000000000000100000000000000000000000000000000000000000000000000000000000002000000000000000000000000000000000000000000202000000080000000020000000010000000000000000000000000000000000000000000001000',
        rawLogs: [ [Object], [Object], [Object] ]
      },
      logs: [
        {
          logIndex: 0,
          transactionIndex: 0,
          transactionHash: '0xb2d009b34da07e386f4fc9cfde74b043b107f91706bc554c811217ba2d04eb5f',
          blockHash: '0x318665891da411d835426036994a011086c5daf79396a6b6d111e730ad4a3989',
          blockNumber: 9,
          address: '0xd8C5c8Dd412Ce99f9fB17ff93fb4af23b50206E5',
          type: 'mined',
          id: 'log_c8de3b85',
          event: 'Sent',
          args: [Result]
        },
        {
          logIndex: 1,
          transactionIndex: 0,
          transactionHash: '0xb2d009b34da07e386f4fc9cfde74b043b107f91706bc554c811217ba2d04eb5f',
          blockHash: '0x318665891da411d835426036994a011086c5daf79396a6b6d111e730ad4a3989',
          blockNumber: 9,
          address: '0xd8C5c8Dd412Ce99f9fB17ff93fb4af23b50206E5',
          type: 'mined',
          id: 'log_25687991',
          event: 'Transfer',
          args: [Result]
        }
      ]
    }```

It's a bit confusing at the moment, 'fixed' will make things clearer.

In #75 we assumed that in a Truffle environment the package @truffle/contract would be available to require, but it turns out that since Truffle bundles all of its dependencies this is not true.

Temporarily, we will add @truffle/contract as a dependency of test-helpers, but this may cause issues since it will be a different version than the Truffle environment where we're running.

Alternatively, we can use the global artifacts object and run artifacts.require but we haven't looked into this in detail and don't know if it will work.

The output of an expectRevert test may be more helpful if it included the expect-revert model of any other test.

Actual message is just

AssertionError: Wrong kind of exception received
      at expectException (node_modules/openzeppelin-test-helpers/src/expectRevert.js:13:14)
      at processTicksAndRejections (internal/process/next_tick.js:81:5)

Truffle contracts include a property with the hash of the transaction in which they were created, web3 contracts, on the other hand, don't provide this value.

We could find it by doing a simple binary search over all blocks, querying for the code stored at the contract's address.

This can then be used to implement expectEvent.inConstruction for non-Truffle contracts.

Hey there !
I was working in some unit tests for a SC, and thought that it would be awesome to add the option to be less strict in the args argument of the expectEvent. Let me explain why.

Setup

contract A {
   funct emitFirstEvent() {
       emit FirstEvent(args);
  }
}

contract B {
  funct secondFunction () {
     A.emitFirstEvent();
     emit SecondEvent(args);
  }
}

Logic
Let's say I'm testing my contract B. I'd like to test that A emitted firstEvent, but I don't really want to test if the args where emitted correctly, that's up to the unit tests of the A contract.

In case you agree that it would be cool to have this feature. Any suggestions on how you'd like to see this implemented ? I thought about making eventArgs = {} in expectEvent (receipt, eventName, eventArgs = {}) optional, but it may be misleading.

Hi

The advanceBlock function does not provide an id as a property. The web3-provider-ws package uses the id as follows:

WebsocketProvider.prototype._addResponseCallback = function(payload, callback) {
    var id = payload.id || payload[0].id;
    var method = payload.method || payload[0].method;

    this.responseCallbacks[id] = callback;
    this.responseCallbacks[id].method = method;

    // more code....
};

The line var id = payload.id || payload[0].id; will produce an error when an id is undefined

The same checks is presented in the new version of web3.

Thanks a lot!

I would like to assert that certain events don't get emitted under given circumstances.

Suggested syntax:
await dontExpectEvent.inTransaction(truffleReceipt, 'Foo');

With the command truffle migrate --reset --network kovan, I have an error raised by my truffle migration script.

It says :
Cannot configure openzeppelin-test-helpers a second time

Is this a bug or a misunderstanding on my part?

Here is the faulty code.

const Migrations = artifacts.require('Migrations')

require('openzeppelin-test-helpers/configure')({web3})

module.exports = function(deployer) {
  deployer.deploy(Migrations)
}

To use with embark I had to clone the repo locally and comment out the module:

module.exports = {
  balance: require('./src/balance'),
  BN,
  constants: require('./src/constants'),
  ether: require('./src/ether'),
  expectEvent: require('./src/expectEvent'),
  makeInterfaceId: require('./src/makeInterfaceId'),
  send: require('./src/send'),
  shouldFail: require('./src/shouldFail'),
  // singletons: require('./src/singletons'),
  time: require('./src/time'),
};

index.js is sort of standard, I guess because it's treated specially by node's require. If you require a path and the path is a directory, it will look for an index.js file in it.

with the Ganache GUI v2.0.1 - which is the latest stable release - shouldFail.reverting.withMessage in v0.3.2 or expectRevert in master both fails to test the reason string.

Because a regex to test a nodeInfo cannot match a pre-release or a meta-data specified in a semver documentation - https://semver.org/#spec-item-9 and Ganache GUI v2.0.1 uses ganache-core v2.5.5-beta.0 - nodeInfo is EthereumJS TestRPC/v2.5.5-beta.0/ethereum-js

expectRevert output:

openzeppelin-test-helpers WARN       expectRevert: revert reason checking only supported on Ganache v2.2.0 or newer.

I am trying to const oz = require('openzeppelin-test-helpers') but I am getting the following error:

/path/to/project/node_modules/openzeppelin-test-helpers/src/setup.js:2
const BN = web3.utils.BN;
           ^

ReferenceError: web3 is not defined

Why web3 is not imported inside setup.js ? Am I missing something obvious ?

For some reason there are global-looking web3 and artifacts variables in a Truffle migration, but these are not accessible from required modules, nor do they show up in the globals object. Truffle must be injecting them in some other way.

This breaks our ability to detect a Truffle environment or even use the global web3 object to automatically get the current provider. I don't see an easy way out of this. We should document that in migrations people will have to manually configure the provider.

We might have to make artifacts configurable too.

I've found myself many times, while using the ETH balance trackers, having to do:

web3.utils.fromWei(await ethBalanceTracker.delta())

It would be far more natural if we just could tell the balance tracker the units in which we want the returned data. Something like:

await ethBalanceTracker.delta({units: 'ether'})

or even:

await ethBalanceTracker.delta('ether')

Same goes for the get method of the tracker.
I think this would make the tests far cleaner and elegant. WDYT ?

Thankssss!

We could simply have reverting have the current withMessage behavior when it receives two arguments (the promise and the message).

Current:

await shouldFail.reverting(promise);
await shouldFail.reverting.withMessage(promise, 'error');

After this:

await shouldFail.reverting(promise);
await shouldFail.reverting(promise, 'error');

We could have simple projects that install the package and use it to test dummy contracts. This would allow us to e.g. test both truffle 4.x and truffle 5.x environments (related to #2).