-
Notifications
You must be signed in to change notification settings - Fork 0
feat: implement token lists management with fetcher and parser support #10
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
Conversation
f3313c0
to
b07c664
Compare
7c0e25d
to
15d894a
Compare
8753428
to
a572056
Compare
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.
Thanks for splitting the logic into files. I feel we could go one step further and create packages with self-contained single-responsibility functionality, that way these modules become reusable in a more granular level.
I haven't though too much about it, but I imagine something like:
- A package in charge of fetching/parsing lists of token lists
- A package in charge of fetching/parsing token lists (doesn't care whether they come from, could be downloaded or a local file selected by the user)
- A package in charge of merging multiple token lists
The "Service" part that runs the timers and triggers periodical fetches. Hmm I've avoided putting such stuff in the wallet-sdk for now, and I haven't thought too much about it yet, but my general feeling is that something like that belongs more in the consumer (ie status-go) than the wallet-sdk. Anyway, it's a discussion I'd like to have!
pkg/tokenlists/config.go
Outdated
return c | ||
} | ||
|
||
func (c *Config) WithCommunityTokenStore(store CommunityTokenStore) *Config { |
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.
haven't finished reviewing, but this is a red flag. The concept of a community is something status-specific, it most likely doesn't belong in the wallet-sdk
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.
The concept of a community is something status-specific
Yes it is, and the wallet-sdk is here to simplify Status' needs. Was thinking whether to keep it on the stautsgo side or not, but then moved it here and let tokenlists
package to take care of everything token-related.
|
||
"go.uber.org/zap" | ||
) | ||
|
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.
the config struct should probably be defined here for clarity
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.
It can be, but added it to types.go since all types for the package are there.
pkg/common/chainid.go
Outdated
package common | ||
|
||
const ( | ||
EthereumNativeCrossChainID = "eth-native" |
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.
hmm if this really needs to be in common, it should probably be in a separate file.
My impression is this is token-related, so it should be in some token-related package
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.
Yes, can be moved there.
pkg/tokenlists/types.go
Outdated
LogoURI string `json:"logoUri"` | ||
|
||
CustomToken bool `json:"custom"` | ||
CommunityData *CommunityData `json:"communityData,omitempty"` |
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.
yeah this definitely doesn't belong here. If Status wants community data for some specific token it should get it from some community service.
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 said in the previous comment, was thinking which approach to go with, but then wallet-sdk is here mainly for our needs and is aware of custom tokens and community tokens and knows how to handle them if they are provided. If the client doesn't need them, there are default custom and community token storer that can be used. In that case the package behaves as a regular token manager.
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.
but then wallet-sdk is here mainly for our needs
I think we should treat this as a general-purpose library (that’s what an SDK is for). Ideally it should stay flexible enough so it can also be used in the Market-Proxy or even in Logos core, without assumptions about Status-client specific needs.
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 we claim in https://github.com/status-im/go-wallet-sdk/blob/master/docs/specs.md
Go Wallet SDK is a modular Go library intended to support the development of multi‑chain cryptocurrency wallets and blockchain applications. The SDK exposes self‑contained packages for common wallet tasks such as fetching account balances across many EVM chains and interacting with Ethereum JSON‑RPC.
a572056
to
52bb6d6
Compare
We can, ofc, if there is a need for that. That's how it is organized in stautsgo now. |
52bb6d6
to
79b8fc9
Compare
@friofry @dlipicar as suggested, constants moved to the token package, community data type removed, examples added for the package. For now, I kept it all in a single package and split the logic into related files. When it becomes a need, we can think of having a package for client, config, fetching, parsing and so on. |
- Added new package `tokenlists` for managing token lists. - Implemented `tokensList` struct for thread-safe state management. - Introduced `refreshWorker` for background updates of token lists. - Created fetchers for remote token lists and individual token lists. - Added parsers for CoinGecko, Uniswap and Status list token formats. - Included configuration options for token lists management. - Implemented validation for token lists against JSON schemas. - Added comprehensive tests for all new functionalities.
79b8fc9
to
bd11449
Compare
|
||
func printToken(token *tokenlists.Token) { | ||
fmt.Printf("Token Key: %s:\n", token.Key()) | ||
fmt.Printf(" Name: %s (%s)\n", token.Name, token.Symbol) |
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 ran the example app
Shouldn't it display Arbitrum or Base Name instead of Ethereum (ETH)
?
Token Key: 42161-0x707f635951193ddafbb40971a0fcaab8a6415160:
Name: Status (SNT)
Chain ID: 42161
Address: 0x707F635951193dDaFBB40971a0fCAAb8A6415160
Decimals: 18
Native: false
Custom: false
Logo: https://assets.coingecko.com/coins/images/779/thumb/status.png?1548610778
Token Key: 42161-0x0000000000000000000000000000000000000000:
Name: Ethereum (ETH)
Chain ID: 42161
Address: 0x0000000000000000000000000000000000000000
Decimals: 18
Native: true
Custom: false
Logo: https://raw.githubusercontent.com/trustwallet/assets/master/blockchains/ethereum/assets/0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2/logo.png
Token Key: 8453-0x0000000000000000000000000000000000000000:
Name: Ethereum (ETH)
Chain ID: 8453
Address: 0x0000000000000000000000000000000000000000
Decimals: 18
Native: true
Custom: false
Logo: https://raw.githubusercontent.com/trustwallet/assets/master/blockchains/ethereum/assets/0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2/logo.png
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.
You mean for Token Key: 42161-0x0000000000000000000000000000000000000000
a name should be Arbitrum Ethereum
?
That's not how I did it. The name and symbol for ETH token are the same accross chains. That's also the case for tokens with the same cross-chain id.
// process last fetched main list | ||
storedContent, err := tl.config.ContentStore.Get(tl.config.MainListID) | ||
if err != nil { | ||
tl.config.logger.Error("failed to get stored content for main list", zap.Error(err)) |
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.
The error is printed when ContentStore.Get is run for the first time. This seems an expected situation.
Could you change it to a warning?
=====================================
2025-09-18T13:48:14.089+0400 ERROR tokenlists/tokenslist.go:308 failed to get stored content for main list {"error": "content not found"}
github.com/status-im/go-wallet-sdk/pkg/tokenlists.(*tokensList).mergeMainList
/Users/ricko/status/go-wallet-sdk/pkg/tokenlists/tokenslist.go:308
github.com/status-im/go-wallet-sdk/pkg/tokenlists.(*tokensList).buildState
/Users/ricko/status/go-wallet-sdk/pkg/tokenlists/tokenslist.go:239
github.com/status-im/go-wallet-sdk/pkg/tokenlists.(*tokensList).Start
/Users/ricko/status/go-wallet-sdk/pkg/tokenlists/tokenslist.go:53
main.main
/Users/ricko/status/go-wallet-sdk/examples/tokenlists-usage/main.go:69
runtime.main
/opt/homebrew/Cellar/go@1.24/1.24.7/libexec/src/runtime/proc.go:283
symbol := EthereumNativeSymbol | ||
name := EthereumNativeName | ||
logoURI := "https://raw.githubusercontent.com/trustwallet/assets/master/blockchains/ethereum/assets/0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2/logo.png" | ||
if chainID == common.BSCMainnet || chainID == common.BSCTestnet { |
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.
Is it possible to retrieve the name programmatically instead of hardcoding it?
'Optimism', 'Arbitrum' and 'Base' are not supported here.
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.
Not really, toke lists usually don't contain entries for native tokens.
CustomTokenListID = "custom" | ||
|
||
// #nosec G101 | ||
listOfTokenListsSchema = `{ |
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.
It would be better if the schema was in a separate file
|
||
for _, tokenList := range remoteListOfTokenLists.TokenLists { | ||
wg.Add(1) | ||
go func(ctx context.Context) { |
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.
Maybe pass both params as args?
go func(ctx context.Context, tl tokenList)
currently ctx - param, tl - captured
) | ||
|
||
var ( | ||
ErrConfigNotProvided = errors.New("config not provided") |
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.
There are lots of errors in tokenlists.go
that are not listed here.
Should we declare here the errors that are part of the module interface and are returned to the developer using it?
currentState.tokenLists[tokenListID] = tokenList | ||
for _, token := range tokenList.Tokens { | ||
if _, exists := currentState.tokens[token.Key()]; !exists { | ||
currentState.tokens[token.Key()] = token |
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 where we can log duplicate tokens. I think this information might be useful
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 wonder if we can have a priority of the tokenlists when we merging duplicates?
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.
Token key is a unique thing, we process lists in a specific order which is defined in buildState()
. Different lists may contain the same tokens, that's ok, but since we have a strict order of processing them when the token appears for the first time we store it, all the same tokens (same chain+address) that may appear later will be skipped and no need to log them.
} | ||
|
||
// PrivacyGuard interface for checking privacy mode. | ||
type PrivacyGuard interface { |
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.
-
It's violation of the single responsibility principle: The token library should not be aware of the application's privacy policies.
-
Tight coupling: It creates a tight coupling between the application's business logic and the library.
-
Restricts reuse: Other applications may have different approaches to privacy.
I think there should be a general permissions mechanism for the entire go-wallet-sdk
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.
after discussing in the direct messages:
- I suggest remove the PrivacyGuard interface and add
autoRefresh
property to the manager. (it's part of the state, not an external dependency) RefreshNow
is misleading name because it works asynchronously. And it should be renamed toTriggerRefresh
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.
Yes, that's what we discussed, naming is definitely misleading and having an interface for this simple thing is maybe too much.
} | ||
defer func() { | ||
if err := resp.Body.Close(); err != nil { | ||
log.Println(err.Error()) |
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.
inconsistent logging
WithLogger(zap.NewNop()) | ||
|
||
// Create token list manager | ||
tl, err := tokenlists.NewTokensList(config) |
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.
shouldnt it match the package name TokenLists?
@friofry I am reorganizing all this, splitting into simple packages. |
Closing this one in favour of this one #20 |
tokenlists
for managing token lists.tokensList
struct for thread-safe state management.refreshWorker
for background updates of token lists.