Skip to content

Replace @ethersproject imports with ethers#144

Merged
cgewecke merged 5 commits intoSetProtocol:masterfrom
rootulp:rp/remove-ethersproject-imports
Sep 11, 2021
Merged

Replace @ethersproject imports with ethers#144
cgewecke merged 5 commits intoSetProtocol:masterfrom
rootulp:rp/remove-ethersproject-imports

Conversation

@rootulp
Copy link
Copy Markdown
Contributor

@rootulp rootulp commented Sep 10, 2021

Motivation in this comment. I expect we'll encounter fewer dependency mismatches for ethers subpackages if we import from the umbrella package. Also the ethers docs say:

Most users will prefer to use the umbrella package, but for those with more specific needs, individual components can be imported.

Testing

yarn build succeeds

Motivation in [this comment](SetProtocol#131 (comment)).
I expect we'll encounter fewer dependency mismatches for ethers
subpackages if we import from the umbrella package. Also the
[ethers docs](https://github.com/ethers-io/ethers.js/tree/master/packages/bignumber) say:

> Most users will prefer to use the umbrella package, but for those with
more specific needs, individual components can be imported.
@cgewecke
Copy link
Copy Markdown
Contributor

Wow!! Will check this out...

@cgewecke cgewecke self-requested a review September 10, 2021 13:27
Copy link
Copy Markdown
Contributor

@cgewecke cgewecke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is super cool.

The only additional thing to do here is remove the resolutions section in package.json.

"resolutions": {
"@ethersproject/abi": "5.4.1",
"@ethersproject/bignumber": "5.4.1",
"@ethersproject/contracts": "5.4.1",
"@ethersproject/providers": "5.4.5"
},

(Tried this locally and the build was clean) You just need to

  • delete the resolutions
  • run rm -rf node_modules
  • run yarn

...and the yarn lock should update appropriately.

Thanks for the suggestion cgewecke! Agreed these resolutions are no
longer necessary. `yarn build` succeeds. - @rootulp
@rootulp rootulp requested a review from cgewecke September 11, 2021 01:55
Copy link
Copy Markdown
Contributor

@cgewecke cgewecke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

(Added a couple additional changes in one test file that uses ethereum-waffle mock contract fixtures)

@cgewecke cgewecke merged commit fe09ee7 into SetProtocol:master Sep 11, 2021
@rootulp rootulp deleted the rp/remove-ethersproject-imports branch September 11, 2021 21:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants