-
Notifications
You must be signed in to change notification settings - Fork 21
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
feat: tokenlists resolver #282
base: master
Are you sure you want to change the base?
Conversation
I think this will be helpful for testing and for apps using it too.
What's missing is the actual processing/resolving.
@bonustrack I moved everything to the resolver and made sure the fetches run in parallel. The little test I wrote still passes but you didn't comment on the extra headers yet. This needs improvement but I think it is a good way of testing more things. We can decide per environment what headers are being sent. Some headers can be exclusively for testing. |
Shouldn't the change on cache be in another PR? I don't see how it's related to tokenlists |
Oh sry, I didn't mean "missing" but just this EIP<some number> prefix.... didn't test any of these variations. |
I suggest gathering all relevant testable cases here now and adding them to the test suite as good as possible. The test I added is more a stub and only ensures that The unit test for URL pattern replacement should also be extended and the implementation then too of course. |
@mktcode We should add a test case with a token and network prefix, we could have network prefix with both chain id and network short name like |
@bonustrack Ok, I can take care of that during this week. Suggestion: Start issues with a continuously updated section, titled "To be tested:" |
This sorting isn't relevant. Response times are. Sorting must be the last step.
@@ -6290,7 +6290,7 @@ [email protected]: | |||
resolved "https://registry.yarnpkg.com/scrypt-js/-/scrypt-js-3.0.1.tgz#d314a57c2aef69d1ad98a138a21fe9eafa9ee312" | |||
integrity sha512-cdwTTnqPu0Hyvf5in5asVdZocVDTNRmR7XEcJuIzMjJeSHybHl7vpB66AzwTaIg6CLSbtjcxc8fqcySfnTkccA== | |||
|
|||
[email protected], semver@^7.3.2, semver@^7.3.5, semver@^7.3.7, semver@^7.5.3: | |||
[email protected], semver@^7.3.2, semver@^7.3.5, semver@^7.5.3: |
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.
Why is there a yarn.lock update, without a dependencies update?
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.
Idk. Maybe because it hasn't been pushed before. Might be my mistake as well. Just delete it and run yarn again.
@@ -0,0 +1,53 @@ | |||
import { replaceURIPatterns, sortByKeywordMatch } from '../../src/helpers/tokenlists'; | |||
|
|||
jest.setTimeout(60_000); |
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.
Unit tests does not need such high timeout
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.
True. Please just take over this branch. I won't make any more changes to it.
|
||
export default async function resolve(address: string, chainId: string) { | ||
try { | ||
await updateExpiredAggregatedTokenList(); |
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.
If we remove the await
here:
- the first request triggering the token list refresh will always return immediately with nothing, with the list refreshing async
- a request waiting for list refresh will return immediately with current list, instead of waiting for new list, which is refreshing async
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.
That's the details I tried to clarify but without success. I still feel like my approach of completely decoupling the list updates and the resolver is the better one but I'm not making the decisions here. Please just take from this PR what makes sense to you.
const imageBuffer = await image.raw().toBuffer(); | ||
|
||
const fingerprint = getImageFingerprint(imageBuffer.toString('hex')); | ||
const expectedFingerprint = 'ac601f072065d4d03e6ef906c1dc3074d7ad52b9c715d0db6941ec89bf2073a1'; |
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.
As mentioned here #282 (comment) this is not the expected image
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.
This is not working due to something unrelated to this PR: the shortname oeth
is not supported:
Lines 50 to 58 in 3b50d97
export function shortNameToChainId(shortName: string) { | |
if (shortName === 'eth') return '1'; | |
if (shortName === 'bsc') return '56'; | |
if (shortName === 'ftm') return '250'; | |
if (shortName === 'matic') return '137'; | |
if (shortName === 'arb1') return '42161'; | |
return null; | |
} |
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.
this is not the expected image
yes, you'll have to decide what images make most sense to test.
Closes: #24
Tests
The (e2e) tests check previously generated fingerprints of the returned images. Of courtse that will make tests fail, when one of these images changes. The images we test should be chosen accordingly. Correct invalidation of changed/updated images should probably be a test of its own.
The tests could be far more comprehensive but I didn't want to cause too much refactoring. The fingerprinting should help to test more things already and reduce manual review.
Implementation
The tokenlists data is aggregated into a map, containing all found image URLs for a
<chainId>-<address>
pair.stamp/src/helpers/tokenlists.ts
Line 9 in 0ee529e
The URLs are sorted based on keywords they contain (in their path).
stamp/src/helpers/tokenlists.ts
Lines 85 to 126 in 0ee529e
Before that, known URL patterns related to image size, are replaced. Like for coingecko, we know we can replace small and thumb with large to get a larger image.
stamp/src/helpers/tokenlists.ts
Lines 71 to 85 in c0f8a9c
When the resolver runs for the first time, it waits for the tokenlists data to be fetched and aggregated.
stamp/src/resolvers/tokenlists.ts
Lines 6 to 10 in c0f8a9c
stamp/src/helpers/tokenlists.ts
Lines 135 to 140 in 51bff9d
When the cache expires, an update is triggered by the first incoming request. This request waits for the update to complete. Other concurrent requests will use the existing list, which may be empty after a server restart. While they could wait for the first request to finish updating, I'm still struggling to understand the rationale behind this approach.
I'm curious about the reasons for not running updates at regular intervals, starting with the server initialization. Additionally, I'm wondering why it's necessary for the resolver to determine the TTL starting timestamp.
Personally, I might consider a different approach, such as running a daily or hourly GitHub workflow to update the data in the Redis cache. This way, the application wouldn't need to handle these updates directly. The dependabot-like approach I suggested earlier is not even necessary.
Btw, I didn't have the time to look into sentry integration yet.
@bonustrack I'm working on another project now, so, like I said, I won't have a lot of time anymore to work on this one. But I will check back in a couple of weeks/months. Let me know when release day get's closer.