-
Notifications
You must be signed in to change notification settings - Fork 365
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
fix: name clashes #887
base: master
Are you sure you want to change the base?
fix: name clashes #887
Conversation
read ethers built-ins instead of maintaining a property list
🦋 Changeset detectedLatest commit: eaff379 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@jfschwarz thanks for this PR! Can you have a look at failing CI ( |
Thank you, @krzkaczor! The failing CI typecheck tasks actually only reveal that ethers-v5 suffered under the same issue, so I went ahead and applied the fix also to that package. Unfortunately, the dev tooling in that package appears a bit broken for me. I'm getting plenty of lint errors in target-ethers-v5 that don't seem to make make sense, not sure what's going on... I would appreciate if you could run the CI pipeline again to see if my latest changes check out. |
Unfortunately there is a bug in typechain and we get some name clashes in the generated code. There is a PR that fixes this, but until it gets merged I’ve added a patch. More info on the name clash: dethcrypto/TypeChain#887 Once this PR is merged the patch could be removed.
Unfortunately there is a bug in typechain and we get some name clashes in the generated code. There is a PR that fixes this, but until it gets merged I’ve added a patch. More info on the name clash: dethcrypto/TypeChain#887 Once this PR is merged the patch could be removed.
Hey @krzkaczor, I finally got my local project tooling set up correctly and have now fixed the bug causing the tests to fail. We have a few projects that need upgrading from Ethers 5 to 6, but we are currently blocked by the issue addressed in this PR. We understand that the project has been deprecated. However, we would greatly appreciate it if you could take a moment to review and release this fix, saving us from the need to fork TypeChain. 🙏 Let me know if I can support you in any way. |
closes #886
The ethers v6
reservedKeywords
set was quite outdated. It had ethers v5 contract property names that no longer exist for v6 contracts (e.g.:provider
) and was missing some v6 contract property names (e.g.:target
). Rather than attempting to manually keep thereservedKeywords
list up-to-date, I'd propose to directly read the list of built-in properties from the BaseContract class.The
then
property is a peculiar case. It's not a property of BaseContract, but still won't be passed through by ethers as it is kept in a specialpassProperties
list (see: ethers v6 BaseContract Proxy).