Chore/update ethers #188
Chore/update ethers #188
Conversation
|
One to-do left is the whole lock file stuff. I commited a package-lock, but I guess you're still using yarn? |
Yes, yarn is still used here. |
There was a problem hiding this comment.
@fermentfan yarn.lock must be updated and package-lock.json removed before this PR can be merged.
As it is, it is failing CI tests due to the lockfiles state.
That being said, I really appreciate the work you put into this task! The PR looks great
There was a problem hiding this comment.
@fermentfan please remove the package-lock.json and update the yarn.lock file to be able to merge this PR
|
@mirceanis tidied up the yarn stuff! Btw I didn't find any actual yarn version that you're targeting in that repository so I just installed the newest via NPM. Is this intended? |
src/config/deployments.ts
Outdated
| { chainId: 42, registry: '0xdca7ef03e98e0dc2b855be647c39abe984fcf21b', name: 'kovan', legacyNonce: true }, | ||
| { chainId: 30, registry: '0xdca7ef03e98e0dc2b855be647c39abe984fcf21b', name: 'rsk', legacyNonce: true }, | ||
| { chainId: 31, registry: '0xdca7ef03e98e0dc2b855be647c39abe984fcf21b', name: 'rsk:testnet', legacyNonce: true }, | ||
| { chainId: BigInt(1), registry: '0xdca7ef03e98e0dc2b855be647c39abe984fcf21b', name: 'mainnet', legacyNonce: true }, |
There was a problem hiding this comment.
Configuration should not rely on types that are not usable in a json/yaml file.
While it is fine to accept bigint as an option, the default should remain a number or string.
There was a problem hiding this comment.
Hmm... I definitely get the irritation around the bigint usage for chainIDs and I also don't 100% understand the decision to use it for that specific thing, but I'd not like to deviate from ethers there. It's their domain, which they own and there might be a good reason for it. Especially due to the behaviour of losing precision when reading very long integers from JSON into a javascript number that might lead to very intransparent problems. If the whole ecosystem moves with ethers you'd always need to translate it.
Thinking about this, one would also need to implement such logic (like reading the deployment configuration from a file) so why not solve this on that level then?
There was a problem hiding this comment.
For the cases where the chainId would be too big to fit into a number, we still have the option of specifying it as a hex string. Do you have an example of such a network?
Since this is a config for ethr-did-resolver, it makes sense to have some logic for parsing it in this library instead of deferring to ethers.
As an example where such a config gets loaded from a file, we have the Veramo CLI.
In that case, the agent configuration is specified as a YML file which dynamically loads all the plugins specified in that file including their configs. That includes the configuration for this resolver where there is no opportunity to run any extra code on that level.
There was a problem hiding this comment.
I pushed the alternative number configuration! please review :)
src/configuration.ts
Outdated
| provider?: Provider | ||
| chainId?: string | number | ||
| provider?: Provider | null | ||
| chainId?: string | bigint |
There was a problem hiding this comment.
Please revert to using numbers.
It's fine to accept bigint, but numbers must be accepted to allow configs to be serialized to plain config files.
| const cleanResults: (ERC1056Event | undefined)[] = results.filter((result) => result !== undefined) | ||
| // THIS IS THE GIGA HACK JUST TO REMOVE THE POSSIBLE UNDEFINED FROM THE ARRAY | ||
| // THAT IS INTRODUCED BY THE .MAP ABOVE | ||
| type cleanResult = Exclude<typeof results[0], undefined> |
I think it's yarn 1.x |
# [9.0.0](8.1.2...9.0.0) (2023-09-27) ### Bug Fixes * **deps:** update ethers to v6 ([#188](#188)) ([2785e61](2785e61)) ### BREAKING CHANGES * **deps:** this update uses ethers v6 which has a sufficiently different API from v5 that will likely need attention. While the API of this library hasn't changed, it is likely that an update will need attention so this is marked as a breaking change and a new major version is released.
|
🎉 This PR is included in version 9.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Long story short I update ethers and it was a pain.