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

Use ethers.Signer.getAddress() in OpenSeaSDK._getAvailableAccounts() #1491

Merged
merged 3 commits into from
Jun 26, 2024

Conversation

RLaursen
Copy link
Contributor

Motivation

The SDK does not currently work with ethers.Signer objects without an address property. ethers.Signer objects may asynchronously retrieve their own address via the .getAddress() method, they do not always have an address property.

Solution

Fallback to calling .getAddress() in OpenSeaSDK._getAvailableAccounts

Open Questions

Are the added tests sufficient/desired?

@RLaursen RLaursen changed the title Use ethers.SIgner.getAddress() in OpenSeaSDK._getAvailableAccounts Use ethers.Signer.getAddress() in OpenSeaSDK._getAvailableAccounts Jun 14, 2024
@RLaursen RLaursen changed the title Use ethers.Signer.getAddress() in OpenSeaSDK._getAvailableAccounts Use ethers.Signer.getAddress() in OpenSeaSDK._getAvailableAccounts() Jun 14, 2024
@ryanio
Copy link
Collaborator

ryanio commented Jun 14, 2024

thanks for the PR! do you have an example of instantiating a signer that has getAddress()?
also i don't think we need to modify the tests, just the src you included seems fine

@RLaursen
Copy link
Contributor Author

thanks for the PR! do you have an example of instantiating a signer that has getAddress()? also i don't think we need to modify the tests, just the src you included seems fine

Here's an example from our SDK :)

https://docs.stardust.gg/typescript-sdk/signers/ethers-v6/sign-transaction

@RLaursen
Copy link
Contributor Author

RLaursen commented Jun 14, 2024

I added some secrets I used for local testing to my fork, required gha code-quality / test (pull_request) should pass now I think (I will burn them EOD though)

@RLaursen RLaursen force-pushed the use-ethers-get-address-method branch from 1328e90 to 0670ed3 Compare June 18, 2024 16:24
@RLaursen
Copy link
Contributor Author

Is anything else required for this? @ryanio

@ryanio
Copy link
Collaborator

ryanio commented Jun 26, 2024

looks good to me! sorry for the delay, will prepare a release now

@ryanio ryanio merged commit ed45ca7 into ProjectOpenSea:main Jun 26, 2024
3 of 6 checks passed
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.

None yet

2 participants