feat: tokenlists resolver #282
Conversation
I think this will be helpful for testing and for apps using it too.
What's missing is the actual processing/resolving.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@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 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
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.
mktcode
requested review from
bonustrack and
wa0x6e
and removed request for
bonustrack and
wa0x6e
| 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.
Why is there a yarn.lock update, without a dependencies update?
There was a problem hiding this comment.
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.
Unit tests does not need such high timeout
There was a problem hiding this comment.
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.
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.
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.
As mentioned here #282 (comment) this is not the expected image
There was a problem hiding this comment.
This is not working due to something unrelated to this PR: the shortname oeth is not supported:
There was a problem hiding this comment.
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.https://github.com/stamp-labs/stamp/blob/0ee529ec4b2ebf1cbb24c921ee6ad3ea024d8994/src/helpers/tokenlists.ts#L9
The URLs are sorted based on keywords they contain (in their path).
https://github.com/stamp-labs/stamp/blob/0ee529ec4b2ebf1cbb24c921ee6ad3ea024d8994/src/helpers/tokenlists.ts#L85-L126
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.
https://github.com/stamp-labs/stamp/blob/c0f8a9c2503971b999c86da45a7d90a0d4d5876e/src/helpers/tokenlists.ts#L71-L85
When the resolver runs for the first time, it waits for the tokenlists data to be fetched and aggregated.
https://github.com/stamp-labs/stamp/blob/c0f8a9c2503971b999c86da45a7d90a0d4d5876e/src/resolvers/tokenlists.ts#L6-L10
https://github.com/stamp-labs/stamp/blob/51bff9d637a1771fcf532fff1d67706f46c2e802/src/helpers/tokenlists.ts#L135-L140
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.