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

Rename getAddress -> getAccountAddress #323

Closed
wants to merge 1 commit into from

Conversation

voltrevo
Copy link

@voltrevo voltrevo commented Aug 4, 2023

In ethers v6, you can no longer get the address of a contract synchronously, so they've added an async .getAddress() method.

This means using getAddress as a contract method breaks the ethers API. .getAddress continues to return the address of the factory, and the contract method for getting the account address is inaccessible. The types generated by typechain also incorrectly show the contract method, but the JS actually resolves to ethers' .getAddress.

Incorrect types show custom getAddress defined in solidity:

Screen Shot 2023-08-04 at 3 07 19 pm

Actually resolves to the ethers' builtin to get the address of the contract, regardless of the parameters passed:

Screen Shot 2023-08-04 at 3 09 08 pm

This PR renames getAddress to getAccountAddress to fix this problem.

Passes yarn hardhat test.

@drortirosh
Copy link
Contributor

yes, we could do that, since these are basically samples, not "core" contracts.
Still, I think you could do contract.functions.getAccount(...) in the application. can't you?

@voltrevo
Copy link
Author

voltrevo commented Aug 8, 2023

yes, we could do that, since these are basically samples, not "core" contracts.

Cool. Any blockers here?

Still, I think you could do contract.functions.getAccount(...) in the application. can't you?

Not in v6 it turns out:

Screen Shot 2023-08-08 at 10 07 43 am

(I checked that .functions is a thing in v5. Must have been removed.)

I was able to use simpleAccountFactory.createAccount.staticCall(...) as a workaround though.

Doesn't solve the intellisense/typescript issue, but that also has a workaround. No hard blockers here, just clunky.

@shahafn
Copy link
Contributor

shahafn commented Aug 8, 2023

If ethers v6 has the potential to shadow a contract method, without exposing any way to access it, then it sounds like a bug in ethers that should be fixed there, no?
Did you open an issue for that over there as well?

@voltrevo
Copy link
Author

voltrevo commented Aug 9, 2023

@shahafn Good point. I didn't open an issue over there but I've now found this one:
ethers-io/ethers.js#4165

Turns out there is another workaround:

simpleAccountFactory.getFunction('getAccount')(...)

Commenters over there still see it as an issue for ethers though. I believe it's also worth fixing here to improve DX.

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.

3 participants