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

Updates Web3 to v1.x #572

Merged
merged 44 commits into from Jan 22, 2019

Conversation

Projects
None yet
3 participants
@ajsantander
Copy link
Contributor

ajsantander commented Jan 9, 2019

Fix #563
Fix #535

Goals of this PR

This PR intends to update the Web3 dependency in ZeppelinOS from v0.x to v1.x. It does not intend to upgrade Truffle v4.x to v5.x tho, so you will see, especially in tests, that Web3 v0.x is coexisting with Web3 v1.x. If this sounds weird, it's because it is, but it's not that bad. Another PR that updates Truffle to v5 should remove this bizarre intermediate state. Also note that this PR simply attempts to "make the code run" with Web3 v1 and does not focus on design changes, i.e. it does not yet take advantage of several design optimizations that could be implemented because of the way Web3 v1 works, and how it's typed. Another PR should focus on this specifically and hopefully remove a lot of code that will no longer be needed.

Changes that you will see in the code:

This should be helpful for reviewing, and also should be very informative to understand what actually changed and how to use Web3 v1 in the code in the future.

  1. Despite this PR not intending to make design changes, there is ONE exception: ContractFactory no longer injects methods at the top level of a contract, as Truffle does. This is meant as yet another step towards interacting with Web3 directly and not via Truffle. So, you will see things like:
    await myContract.methods.initialize(...PARAMS).send()
    or
    await myContract.methods.owner().call()
    instead of the old
    await myContract.initialize(...PARAMS)
    or
    await myContract.owner()
    This is laborious, but more explicit, because whenever we see a .methods in the future, we will know that we are interacting with a contract (as opposed to other custom objects we may have) directly via Web3.

  2. Something to be aware of, is that Web3 v1 unfortunately contaminates txParams in some of its methods. That is, if you pass a txParams object to a transaction's send method, this method may alter txParams. So in the code you will see things like:
    await myContract.methods.someMethod(...PARAMS).send({ ...txParams })
    Notice how a clone of txParams is passed via deconstruction. This cloning protects txParams from such unwanted modifications. We should raise an issue in Web3 for this.

  3. So far, 2 tests (tests, not files) that use Sinon stubs where not ported. This is due to the problem not being trivial, and should probably be handled in a separate PR. More info in a note in the code:

    // TODO: (STUB estimateGas problem) the tests below are disabled because

  4. Web3 v1 uses the same ABI encoder we are using in encodeCall (ethers/abi-coder) which is super strict in terms of how it handles parameters. For example, empty bytes objects can no longer be represented as an empty string ''. So you will see in the code things like Buffer.from('') instead.

  5. Web3 v1 uses checksummed addresses, so at the start of most tests, you will see a mapping that looks like:
    accounts = accounts.map(utils.toChecksumAddress)
    Which uses Web3 v1's sub-package 'web3-utils'. This mapping needs to be performed because the tests still use Web3 v0 via Truffle 4, and it does not produce checksummed addresses. utils.toChecksumAddress is not only used to map accounts in tests, but any address that comes from a contract obtained via Truffle artifacts, so you should see it in other parts of the code as well.

  6. Changes in event parsing. Mainly, a tx's logs array was renamed to events, and it is now an object. Also, a single event's args property was renamed to returnValues.

  7. Numbers returned as string instead of BigNumbers. So,

const value = myContract.getValue();
value.toNumber();

becomes

const value = myContract.methods.getValue().call();
parseInt(value, 10);
  1. Contracts.ts default artifacts use to be set synchronously in tests (setup.js), but since getting accounts is only async now in Web3 v1, the from address from getDefaultTxParams() is loaded lazily now, and no longer obtained before starting tests.

  2. General minor changes in the data structures of the objects returned by Web3, such as Transaction and TransactionReceipt objects, and in the way the API is called.

  3. contractClass.new(...PARAMS) previously used to deploy contracts is replaced by contractClass.deploy(...PARAMS). Now, this and all method calls return a TransactionObject, and it is this object that contains methods like .send() or .call() or .estimateGas().

  4. Notice that Web3 v1 uses promises (PromiEvents actually (?)) instead of callbacks, so there is no longer a need to promisify Web3 calls.

  5. During the migration, I found that the helper decodeLogs.ts was not really used, so I removed it. Let me know if this was expected for external use and I will put it back (and adapt it to Web3 v1).

Tasks

  • Migrate lib to Web3 v1.
  • Migrate cli to Web3 v1.

@facuspagnuolo facuspagnuolo self-assigned this Jan 9, 2019

@ajsantander ajsantander force-pushed the refactor/web3-v1 branch from 0dfbbca to 555629e Jan 10, 2019

@ajsantander ajsantander requested a review from facuspagnuolo Jan 10, 2019

@facuspagnuolo
Copy link
Contributor

facuspagnuolo left a comment

Looking good!

Show resolved Hide resolved packages/lib/src/artifacts/ZWeb3.ts Outdated
Show resolved Hide resolved packages/lib/src/artifacts/ZWeb3.ts Outdated
Show resolved Hide resolved packages/lib/test/src/artifacts/ZWeb3.test.js Outdated
Show resolved Hide resolved packages/lib/test/src/artifacts/ZWeb3.test.js Outdated
@ajsantander

This comment has been minimized.

Copy link
Contributor Author

ajsantander commented Jan 16, 2019

By this point, all of lib's tests are passing with Web3 v1.

@spalladino

This comment has been minimized.

Copy link
Member

spalladino commented Jan 16, 2019

RE Change 5: should we use this chance to move from truffle 4 to 5? I'm open to doing so if it makes things easier.

@ajsantander

This comment has been minimized.

Copy link
Contributor Author

ajsantander commented Jan 17, 2019

@spalladino, to answer your question:

RE Change 5: should we use this chance to move from truffle 4 to 5? I'm open to doing so if it makes things easier.

In this PR, it might complicate things. I'd say we do it in a different PR, or at least after all tests pass in this PR.

@@ -3,6 +3,7 @@
"private": true,
"devDependencies": {
"lerna": "~3.3.2",
"tslint": "^5.11.0"
"tslint": "^5.11.0",
"typescript": "^3.2.2"

This comment has been minimized.

Copy link
@ajsantander

ajsantander Jan 17, 2019

Author Contributor

This is necessary for tslint to work properly on the project.

@@ -46,6 +47,7 @@
"semver": "^5.5.0",
"truffle-config": "^1.0.6",
"truffle-resolver": "^4.0.4",
"web3": "^1.0.0-beta.37",

This comment has been minimized.

Copy link
@ajsantander

ajsantander Jan 17, 2019

Author Contributor

The Web3 dependency is used in cli only to map addresses to checksummed addresses in tests, so after we migrate to Truffle 5, it could be removed altogether.


await sendTransaction(this.instance.initialize, [42, 'foo', [1,2,3]]).should.be.rejectedWith(/always failing transaction/);
});
// TODO: (STUB estimateGas problem) the tests below are disabled because

This comment has been minimized.

Copy link
@ajsantander

ajsantander Jan 17, 2019

Author Contributor

These are the only 2 tests that I couldn't port in this PR.

This comment has been minimized.

Copy link
@spalladino

spalladino Jan 17, 2019

Member

Use .skip instead of commenting them out, so they show up on the CI as pending

_.mapKeys(slots, (value, key) => {
if(isNaN(parseInt(key, 10))) mappedSlots.push(parseInt(value, 10));
});
mappedSlots.should.deep.eq([0, 1, 3, 5, 7])

This comment has been minimized.

Copy link
@ajsantander

ajsantander Jan 17, 2019

Author Contributor

Yup. This is not very nice... The new returned value not only contains keys like with string names, but also keys with numerical names (in string format). Not sure why this is. Such keys need to be purged in this test.

tslint.json Outdated
@@ -46,7 +46,8 @@
"no-consecutive-blank-lines": [true, 1],
"no-var-requires": [false],
"curly": [true, "ignore-same-line"],
"adjacent-overload-signatures": [false]
"adjacent-overload-signatures": [false],
"no-string-literal": [false]

This comment has been minimized.

Copy link
@ajsantander

ajsantander Jan 17, 2019

Author Contributor

The linter wasn't allowing things like myObject['myProp'].

@ajsantander

This comment has been minimized.

Copy link
Contributor Author

ajsantander commented Jan 17, 2019

Ok, all done!! Lib and cli tests should be passing now with Web3 v1 🎉

@spalladino
Copy link
Member

spalladino left a comment

Awesome work @ajsantander!!!

@@ -13,5 +13,8 @@ module.exports = {
solc: {
version: '0.4.24'
}
},
mocha: {
bail: true

This comment has been minimized.

Copy link
@spalladino

spalladino Jan 17, 2019

Member

Remember to not commit this after you finish with this PR

@@ -40,6 +40,7 @@
},
"homepage": "https://github.com/zeppelinos/zos/tree/master/packages/lib#readme",
"dependencies": {
"@types/web3": "^1.0.14",

This comment has been minimized.

Copy link
@spalladino
@@ -266,7 +266,7 @@ contract('push script', function([_, owner]) {
if (!this.networkFile.appAddress) return;
const app = await App.fetch(this.networkFile.appAddress);
const packageInfo = await app.getPackage(libName)
packageInfo.version.should.be.semverEqual(libVersion)
packageInfo.version.map(Number).should.be.semverEqual(libVersion)

This comment has been minimized.

Copy link
@spalladino

spalladino Jan 17, 2019

Member

We could move the .map(Number) inside the semverEqual helper actually. But I'm fine with leaving this as is.

const result = await deployed.double(10);
result.toNumber().should.eq(20);
const result = await deployed.methods.double(10).call();
parseInt(result, 10).should.eq(20);

This comment has been minimized.

Copy link
@spalladino

spalladino Jan 17, 2019

Member

For the future, I'd consider having a custom helper instead of plain parseInt. I wouldn't rule out the fact that web3 may switch to native bignumbers instead of using strings, and then we would need a different function for parsing them. Centralizing how we go from web3-integers (currently strings) to real ints would help in these scenarios.

}

public async getPackage(name): Promise<{ package: Package, version: string }> {
const [address, version] = await this.appContract.getPackage(name);
const thepackage = await Package.fetch(address, this.txParams);
const { ['0']: address, ['1']: version } = await this.appContract.methods.getPackage(name).call();

This comment has been minimized.

Copy link
@spalladino

spalladino Jan 17, 2019

Member

I'm afraid to ask why this change was needed. How are tuples returned now in web3 1.0?!

This comment has been minimized.

Copy link
@ajsantander

ajsantander Jan 18, 2019

Author Contributor

If a Solidity function returns named tuples, like
(Solidity) function myFunction() public view returns (address adr1, address adr2)
You can do
(Javascript) const { adr1, adr2 } = await contract.methods.myFunction().call()

But if the Solidity function does not name the members of the tuple:
(Solidity) function myFunction() public view returns (address, address)
You retrieve them in Web3 v1 like
(Javascript) const { ['0']: adr1, ['1']: adr2 } = await contract.methods.myFunction().call()

Since Web3 v0 exposed tuples as arrays, this use to work:
(Javascript) const [ adr1, adr2 ] = await contract.myFunction()

const [address, version] = await this.appContract.getPackage(name);
const thepackage = await Package.fetch(address, this.txParams);
const { ['0']: address, ['1']: version } = await this.appContract.methods.getPackage(name).call();
const thepackage = await Package.fetch(address, { ...this.txParams });

This comment has been minimized.

Copy link
@spalladino

spalladino Jan 17, 2019

Member

Can we push further down the need to clone txParams? I'd try to do it only on the calls to web3, and not when passing them through our intermediate objects.


await sendTransaction(this.instance.initialize, [42, 'foo', [1,2,3]]).should.be.rejectedWith(/always failing transaction/);
});
// TODO: (STUB estimateGas problem) the tests below are disabled because

This comment has been minimized.

Copy link
@spalladino

spalladino Jan 17, 2019

Member

Use .skip instead of commenting them out, so they show up on the CI as pending

@ajsantander ajsantander force-pushed the refactor/web3-v1 branch from 0652d39 to 24e762b Jan 18, 2019

@ajsantander

This comment has been minimized.

Copy link
Contributor Author

ajsantander commented Jan 18, 2019

Rebased to master with some important changes to note:
(@spalladino thanks for reviewing, please add the points below to your review)

A recent change from master, in Transactions.ts transformed checks like
if(txParams.gas) // do something
to
if(txParams.gas || Contracts.getArtifactsDefaults) // do something
and I was getting out of gas errors with that. Considering that ContractFactory.ts' life is not expected to continue for much longer, I solved the issue by doing:

  const defaultGas = Contracts.getArtifactsDefaults().gas;
  if (!txParams.gas && defaultGas) txParams.gas = defaultGas;
  if (txParams.gas) // do something

Since Web3 uses checksummed addresses and that I had to checksum the addresses in Addresses.ts uint256toAddress, I started getting invalid address errors for addresses like 0x00da0452681c72356863b9048a52ce2630ed22b5. The problem was that Web3's getStorageAt removes padded zeroes, so such address was becoming 0xda0452681c72356863b9048a52ce2630ed22b5 (notice the removal of the first two 0's). I fixed that by left padding the incoming "uint256" to 64 characters before actually removing the 0's. As far as I understand, even though this bug should have existed before this PR, it shouldn't have had very bad implications. The resulting code is:

export function uint256ToAddress(uint256: string): string {
  const padded = utils.leftPad(uint256.toString(), 64);
  const address = padded.replace('0x000000000000000000000000', '0x');
  return utils.toChecksumAddress(address);
}
@ajsantander

This comment has been minimized.

Copy link
Contributor Author

ajsantander commented Jan 21, 2019

@spalladino all the changes requested by your review have been implemented!

@facuspagnuolo
Copy link
Contributor

facuspagnuolo left a comment

Amazing job @ajsantander, let's move forward with this PR and start removing ContractFactory and ZWeb3

@facuspagnuolo facuspagnuolo merged commit f75aaaf into master Jan 22, 2019

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/travis-ci/push The Travis CI build failed
Details

@facuspagnuolo facuspagnuolo deleted the refactor/web3-v1 branch Jan 22, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.