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

Update ABICoder dependency #3490

Merged
merged 10 commits into from
May 7, 2020
Merged

Update ABICoder dependency #3490

merged 10 commits into from
May 7, 2020

Conversation

ryanio
Copy link
Collaborator

@ryanio ryanio commented Apr 27, 2020

Description

This PR aims to resolve #3484 by updating the web3-eth-abi's ethers dep to @ethersproject/abi.

To maintain existing functionality, these cases were added to encodeParameters:

  • Intercept odd length bytes to make them even.
  • Intercept incorrect length bytes32 to make them fixed.
  • Recursively intercept tuples to apply the same bytes and bytes32 rules above.
    • Adds an extra test for an incorrect-length bytes32 nested inside a tuple.
  • Adds test for Buffer input value

Type of change

  • Dependency upgrade (non-breaking change which fixes an issue)

Checklist:

  • I have selected the correct base branch.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation.
  • My changes generate no new warnings.
  • Any dependent changes have been merged and published in downstream modules.
  • I ran npm run dtslint with success and extended the tests and types if necessary.
  • I ran npm run test:unit with success.
  • I have executed npm run test:cov and my test cases do cover all lines and branches of the added code.
  • I ran npm run build-all and tested the resulting file/'s from dist folder in a browser.
  • I have updated the CHANGELOG.md file in the root folder.
  • I have tested my code on the live network.

@ryanio ryanio added help wanted In Progress Currently being worked on 1.x 1.0 related issues labels Apr 27, 2020
@ryanio ryanio added Review Needed Maintainer(s) need to review and removed help wanted In Progress Currently being worked on labels Apr 28, 2020
@ryanio ryanio force-pushed the updateABICoderDep branch 3 times, most recently from b2b11b3 to 4b08c04 Compare May 4, 2020 20:43
@cgewecke cgewecke mentioned this pull request May 4, 2020
13 tasks
@ryanio ryanio force-pushed the updateABICoderDep branch 3 times, most recently from 5bc42c5 to c9260ca Compare May 4, 2020 22:48
@cgewecke
Copy link
Collaborator

cgewecke commented May 5, 2020

Couple notes:

@ryanio Is is correct that there's an upstream fix pending for the last remaining failure in the mosaic_e2e test?

It has a message like:

 Error: invalid modifier (argument="name", value="memory", code=INVALID_ARGUMENT, version=abi/5.0.0-beta.152)
      at Logger.makeError (node_modules/@ethersproject/logger/lib/index.js:178:21)

Cause is web3-eth-abi called directly with a payload that looks like this:

web3.eth.abi.encodeParameters(
      [
        'bytes32',
        'uint256',
        'bytes32',
        'address[] memory', // Ethers V5 doesn't like this 'memory' keyword
        'uint256[] memory',
        'uint256',
      ],
      [
        KERNEL_TYPEHASH,
        height.toNumber(),
        parent,
        updatedValidators,
        updatedReputation,
        gasTarget.toNumber(),
      ],
    )

Apart from that this PR LGTM and is a huge step forward 💯 💯

@ryanio
Copy link
Collaborator Author

ryanio commented May 6, 2020

Thanks!!

Yes @ricmoo said he already added the functionality and release should be imminent. 👍

edit: released! :) updated in 5499d44

@cgewecke cgewecke self-requested a review May 6, 2020 15:19
cgewecke
cgewecke previously approved these changes May 6, 2020
@ryanio ryanio merged commit ccf06a0 into 1.x May 7, 2020
@cgewecke cgewecke added this to Done in v1.2.8 May 8, 2020
@ryanio ryanio mentioned this pull request May 8, 2020
13 tasks
@Polycarpik Polycarpik deleted the updateABICoderDep branch August 12, 2020 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.x 1.0 related issues Review Needed Maintainer(s) need to review
Projects
No open projects
v1.2.8
  
Done
Development

Successfully merging this pull request may close these issues.

Removing ethers from the dependencies
2 participants