-
Notifications
You must be signed in to change notification settings - Fork 30
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
Conversation
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.
Seems like decent resulting data, but the fetching could be cleaner in a variety of ways for longevity of the codebase.
scripts/ApiParser.ts
Outdated
} | ||
} | ||
|
||
// const a: ApiParser = new ApiParser("https://polygonscan.com"); |
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.
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("/")) { |
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.
I think these comments can be removed?
const cookiesString: string = cookies | ||
.map((cookie) => `${cookie.name}=${cookie.value}`) | ||
.join("; "); | ||
// console.log(cookiesString, baseUrl); |
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.
Should be removed
}; | ||
const ftmScanHtmlParser = scanConfig["ftmscan"].htmlParser; | ||
const baseUrl = scanConfig["ftmscan"].website; | ||
await ftmScanHtmlParser.login(page, baseUrl); |
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.
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
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.
more thoughts left
website: string; | ||
}; | ||
export type ApiResponse = { | ||
d: { |
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.
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.
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.
@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; |
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.
I think this should be commented out but made to be optional? I don't remember
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.
@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
Will come back to |
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.