-
-
Notifications
You must be signed in to change notification settings - Fork 203
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: if image starts with 'ipfs://', the return value should be string type #4985
fix: if image starts with 'ipfs://', the return value should be string type #4985
Conversation
Hi @GinMu, thanks for submitting this PR. Glancing at the code there does seem to be a bug with It would be good to have a test which confirms the bugfix and then we can merge this. If you'd like to add this then that would be great, but otherwise we could do that. That's the only blocker I can see for this PR. |
Hi @mcmire , have added test case. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay, this LGTM! I notice that we also have a similar issue with ERC1155Standard.ts
, but we can address that in a different PR.
@@ -204,7 +204,7 @@ export class ERC721Standard { | |||
const object = await response.json(); | |||
image = object?.image; | |||
if (image?.startsWith('ipfs://')) { | |||
image = getFormattedIpfsUrl(ipfsGateway, image, true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for the future, the absence of await
should have produced a type error, because getFormattedIpfsUrl
returns a Promise, whereas the type on image
in the return type is string | undefined
. However, the type of the image
variable here is any
, which is happening because the type of object
is any
. We should give object
a type annotation to fix this. But we can do that in a separate PR too.
…g type (#4985) ## Explanation return promise value if image starts with 'ipfs://' at `ERC721Standard.ts` ## References ## Changelog ## Checklist - [x] I've updated the test suite for new or updated code as appropriate - [ ] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [ ] I've highlighted breaking changes using the "BREAKING" category above as appropriate - [ ] I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes --------- Co-authored-by: Elliot Winkler <[email protected]>
Explanation
return promise value if image starts with 'ipfs://' at
ERC721Standard.ts
References
Changelog
Checklist