feat(wallet)_: Auto refresh token list #6399
Open
+30,362
−16,546
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
saledjenic
force-pushed
the
feat/add-auto-refresh-option-to-settings
branch
from
763a01b to
7c986fc
Compare
Jenkins BuildsClick to see older builds (82)
|
saledjenic
force-pushed
the
auto-refresh-token-list
branch
from
00e797d to
05c6026
Compare
saledjenic
force-pushed
the
feat/add-auto-refresh-option-to-settings
branch
from
7c986fc to
28bf764
Compare
saledjenic
force-pushed
the
auto-refresh-token-list
branch
4 times, most recently
from
c7f1074 to
a0f246a
Compare
saledjenic
force-pushed
the
auto-refresh-token-list
branch
from
a0f246a to
2d970fe
Compare
saledjenic
changed the title
saledjenic
requested review from
alaibe,
friofry,
shivekkhurana,
dlipicar and
briansztamfater
saledjenic
force-pushed
the
auto-refresh-token-list
branch
from
2d970fe to
7408f84
Compare
friofry
reviewed
Mar 13, 2025
| // If the remote list url is not set (empty string provided), the hardcoded default list will be used. | ||
| // The auto-refresh interval is used to fetch the list of token lists from the remote source and update the local cache. | ||
| // The auto-refresh check interval is used to check if the auto-refresh should be triggered. | ||
| func (t *TokenLists) Start(remoteListUrl string, autoRefreshInterval time.Duration, autoRefreshCheckInterval time.Duration) { |
There was a problem hiding this comment.
Do not store Contexts inside a struct type; instead, pass a Context explicitly to each function that needs it, - documentation for context
This approach isn't ideal because contexts in Go are meant to flow down from the top (entry point) of the application. Each component typically receives a context parameter from its caller, rather than generating a new background context internally. Ideally, we would refactor so that: Add a context parameter to the wallet.Start() function. This ensures the caller explicitly controls the lifecycle and cancellation of routines.
Eventually, we might(should) manage a single "root context" at the StatusNode (get_status_node.go) level (the top-level entry point). This would allow coordinated cancellation of all services (30 services at the moment) at shutdown.
There was a problem hiding this comment.
@friofry I agree, but that requires refactoring of the upper code. If I just remove it from the struct I won't be able to cancel the context on calling Stop(). I've added "todo" and will try to address that in the follow-up PR.
| { | ||
| "id": "uniswap", | ||
| "sourceUrl": "https://ipfs.io/ipns/tokens.uniswap.org", | ||
| "schema": "https://uniswap.org/tokenlist.schema.json" |
There was a problem hiding this comment.
is this a valid URL ?
curl -X GET https://ipfs.io/ipns/tokens.uniswap.org
curl: (35) Recv failure: Operation timed out
There was a problem hiding this comment.
I don't see any issues here. Also, trying this command returns an instant response on my side.
There was a problem hiding this comment.
We have many users in different countries.
Probably making this data available from our market proxy will allow us to solve this problem.
| } | ||
|
|
||
| func downloadTokens(client *http.Client, key string, source defaulttokenlists.TokensSource) { | ||
| response, err := client.Get(source.SourceURL) |
There was a problem hiding this comment.
Token list sounds like a big chunk of data that rarely change.
I suggest optimizing it:
- store etag header for the downloaded token list. And do not download what we already have. (
If-None-Match:orIf-Modified-Sinceand do not download if returns 304) - Introduction of context cancellation of HTTP request
- Addition of connection and request timeouts against hanging requests
func fetchContentWithCaching(ctx context.Context, url string, etag string) ([]byte, string, bool, error) {
req, err := http.NewRequestWithContext(ctx, "GET", url, nil)
if err != nil {
return nil, "", false, err
}
// ETag
if etag != "" {
req.Header.Add("If-None-Match", etag)
}
// Timeouts
transport := &http.Transport{
DialContext: (&net.Dialer{
Timeout: dialTimeout, // Timeout for establishing a connection
}).DialContext,
TLSHandshakeTimeout: tlsHandshakeTimeout, // Timeout for TLS handshake
ResponseHeaderTimeout: responseHeaderTimeout, // Timeout for receiving response headers
}
client := &http.Client{
Transport: transport,
Timeout: requesetTimeout * time.Second,
}
resp, err := client.Do(req)
if err != nil {
return nil, "", false, err
}
defer resp.Body.Close()
// HTTP 304 Not Modified
if resp.StatusCode == http.StatusNotModified {
return nil, etag, true, nil
}
// Content changed
body, err := ioutil.ReadAll(resp.Body)
if err != nil {
return nil, "", false, err
}
// update ETag
newEtag := resp.Header.Get("ETag")
if newEtag == "" {
newEtag = etag // if etag is missing
}
return body, newEtag, false, nil
}
There was a problem hiding this comment.
I like this idea. Will do it today in the follow-up PR.
Actually, I see now that we have thirdparty.NewHTTPClientWithDetailedTimeouts and will extend it with the new function func fetchContentWithCaching(ctx context.Context, url string, etag string) ([]byte, string, bool, error) and use it from there in a new PR.
There was a problem hiding this comment.
Cool! It would be great to use the same http client
saledjenic
force-pushed
the
feat/add-auto-refresh-option-to-settings
branch
from
28bf764 to
fc68b55
Compare
saledjenic
force-pushed
the
auto-refresh-token-list
branch
from
7408f84 to
77acac4
Compare
|
@friofry thanks for the review and good comments. The last commit is about the changes you proposed. Once you agree I will move the code to appropriate commits, but this way is easier for you to review. I will handle context and etag in the follow-up PRs. |
saledjenic
force-pushed
the
feat/add-auto-refresh-option-to-settings
branch
from
fc68b55 to
106d96e
Compare
saledjenic
force-pushed
the
auto-refresh-token-list
branch
from
77acac4 to
056ebaf
Compare
Sounds like a good plan to make these changes as a separate PR. |
friofry
approved these changes
Mar 14, 2025
…ist from it added
downloader and analyzer moved to more appropriate location and accordingly updated to the changes made before. `make download-tokens` should be used to update initial tokens lists for the app. `make analyze-token-stores` should be used to analyze initial tokens lists.
Old token stores are not needed anymore since the tokens lists are being fetched dynamically and maintained differently.
saledjenic
force-pushed
the
auto-refresh-token-list
branch
from
056ebaf to
bce20bd
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR depends on:
Changes in this PR are not breaking changes and include the following:
download-tokensandanalyze-token-storesare updated.token.Managertype.wallet.token-lists.updatedsignal will be emitted