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.

Sign up for GitHub

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

Jump to bottom

Create NonStandardFallbackClass #838

Closed
wants to merge 1 commit into from
Closed

Create NonStandardFallbackClass #838

wants to merge 1 commit into from

Conversation

adonesky1
Copy link
Contributor

@adonesky1 adonesky1 commented May 20, 2022 •
edited
Loading

  • ADDED:

    • WIP (Will add tests if/when we decide to move forward)

Checklist

  • Tests are included if applicable
  • Any added code is fully documented

Issue

Resolves #837

@adonesky1 adonesky1 force-pushed the create-non-standard-fallback-class branch from 74f1c91 to 1d85241 Compare May 20, 2022 15:22
@adonesky1 adonesky1 mentioned this pull request May 20, 2022
Merged
gantunesr
gantunesr approved these changes May 23, 2022
View reviewed changes
Copy link
Member

@gantunesr gantunesr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

FrederikBolding
FrederikBolding requested changes May 25, 2022
View reviewed changes
Copy link
Member

@FrederikBolding FrederikBolding left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a fine solution for finding at least some metadata from non-standard tokens. The important thing in my opinion is how we handle them in the extension. I.e. what happens when the extension sees UNKNOWN_STANDARD.

Added some small comments too.

src/assets/Standards/NonStandardFallBack.ts
}

try {
decimals = await this.erc20Standard.getTokenSymbol(address);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

symbol

src/constants.ts
@@ -10,6 +10,7 @@ export const RINKEBY_CHAIN_ID = '4';
export const ERC721 = 'ERC721';
export const ERC1155 = 'ERC1155';
export const ERC20 = 'ERC20';
export const UNKNOWN_STANDARD = 'UNKNOWN_STANDARD';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The important thing is how we handle this in the extension imo.

src/assets/Standards/CollectibleStandards/ERC721/ERC721Standard.ts

if (tokenURI) {
try {
const response = await timeoutFetch(tokenURI);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may potentially leak users IP. Needs to be an issue created if there isn't already.

@adonesky1
Copy link
Contributor Author

very stale

@adonesky1 adonesky1 closed this Sep 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FrederikBolding FrederikBolding FrederikBolding requested changes

@gantunesr gantunesr gantunesr approved these changes

@Gudahtt Gudahtt Awaiting requested review from Gudahtt

@wachunei wachunei Awaiting requested review from wachunei

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Create non-standard class to handle non-standard but common token read methods
3 participants
@adonesky1 @FrederikBolding @gantunesr