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

[Not ready] Ftmscan #52

Closed
wants to merge 5 commits into from
Closed

[Not ready] Ftmscan #52

wants to merge 5 commits into from

Conversation

dawsbot
Copy link
Owner

@dawsbot dawsbot commented Apr 23, 2024

  1. FIXED We need to ensure image urls are not relative paths but rather absolute paths. Otherwise consumers need to know what the base URL is for each image.
  2. Token names need to be fully expanded and they are not
  3. Token symbols need to be fully expanded and they are not

@dawsbot dawsbot changed the title [Not ready ] Ftmscan tests [Not ready] Ftmscan Apr 23, 2024
@kylewandishin
Copy link
Collaborator

kylewandishin commented Apr 29, 2024

i have designed the api logic to pull tokens directly on polygon and ftmscan pullers and am currently working on a ApiParser class to pull the full token info
Screen Shot 2024-04-29 at 11 16 50 am

scripts/ApiParser.ts Outdated Show resolved Hide resolved
Copy link
Owner Author

@dawsbot dawsbot left a comment

Choose a reason for hiding this comment

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

Seems like decent resulting data, but the fetching could be cleaner in a variety of ways for longevity of the codebase.

}
}

// const a: ApiParser = new ApiParser("https://polygonscan.com");
Copy link
Owner Author

Choose a reason for hiding this comment

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

I think these comments can be deleted?

const website = ($(tableCells[5]).find("a").attr("href") || "") // had to change .attr("data-original-title") to .attr("href") for arbiscan
.toLowerCase();
// image path is relative to prepend with root URL
// if (tokenImage.startsWith("/")) {
Copy link
Owner Author

Choose a reason for hiding this comment

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

I think these comments can be removed?

scripts/HtmlParser/HtmlParser.test.ts Show resolved Hide resolved
const cookiesString: string = cookies
.map((cookie) => `${cookie.name}=${cookie.value}`)
.join("; ");
// console.log(cookiesString, baseUrl);
Copy link
Owner Author

Choose a reason for hiding this comment

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

Should be removed

scripts/scan-config.ts Outdated Show resolved Hide resolved
};
const ftmScanHtmlParser = scanConfig["ftmscan"].htmlParser;
const baseUrl = scanConfig["ftmscan"].website;
await ftmScanHtmlParser.login(page, baseUrl);
Copy link
Owner Author

Choose a reason for hiding this comment

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

I believe this part requires user interaction, so we can name the test file as .integration.test.ts so that if we ever want to run unit tests in ci, we can call those .unit.test.ts

Copy link
Owner Author

@dawsbot dawsbot left a comment

Choose a reason for hiding this comment

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

more thoughts left

website: string;
};
export type ApiResponse = {
d: {
Copy link
Owner Author

Choose a reason for hiding this comment

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

Let's avoid ever naming variables something like "d". Programming languages as-high-level-as JavaScript benefit from being specific with variable names. Short ones don't make the program faster and they just make coming back as a dev more difficult.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@dawsbot i followed the naming convention of the original api call as those are the exact field names they use. i will alter this if that is a bad practice

data: Array<{
tokenName: string;
tokenSymbol: string;
// tokenImage: string;
Copy link
Owner Author

Choose a reason for hiding this comment

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

I think this should be commented out but made to be optional? I don't remember

Copy link
Collaborator

Choose a reason for hiding this comment

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

@dawsbot i commented the tokenImage field out as we do not have support for any chains yet. I would like to purpose once all chains have base functionality (pulling all other fields) that we add tokenImages as a new PR in one clean sweep. this is because it seems we shouldn't want just one field to be optional and do this process chain by chain involves having it optional to avoid linting errors and pre commit issues

@dawsbot
Copy link
Owner Author

dawsbot commented May 29, 2024

Will come back to

@dawsbot dawsbot closed this May 29, 2024
@dawsbot dawsbot deleted the ftmscan branch July 3, 2024 19:39
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