-
Notifications
You must be signed in to change notification settings - Fork 46
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/load_official_tokens_list #226
Conversation
…om the official tokens lists for each environment
WalkthroughThe recent updates primarily enhance token management across various components. Major changes include integrating a Changes
Sequence Diagram(s) (Beta)sequenceDiagram
participant User
participant MarketsAssistant
participant TokenMetadata
participant Network
participant ExchangeClient
User->>MarketsAssistant: Request token representation
MarketsAssistant->>TokenMetadata: Fetch token details
TokenMetadata-->>MarketsAssistant: Provide token information
MarketsAssistant-->>User: Return token representation
User->>ExchangeClient: Request network information
ExchangeClient->>Network: Fetch network details
Network-->>ExchangeClient: Provide token list URL
ExchangeClient-->>User: Return network information
Poem
TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
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.
Actionable comments posted: 2
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files ignored due to path filters (2)
go.mod
is excluded by!**/*.mod
go.sum
is excluded by!**/*.sum
Files selected for processing (8)
- client/chain/chain_test.go (1 hunks)
- client/chain/markets_assistant.go (5 hunks)
- client/chain/markets_assistant_test.go (3 hunks)
- client/common/network.go (8 hunks)
- client/core/tokens_file_loader.go (1 hunks)
- client/core/tokens_file_loader_test.go (1 hunks)
- client/exchange/exchange.go (2 hunks)
- client/exchange/exchange_test_support.go (3 hunks)
Files skipped from review due to trivial changes (1)
- client/chain/chain_test.go
Additional comments not posted (13)
client/core/tokens_file_loader.go (1)
9-22
: TheTokenMetadata
struct is well-defined and covers all necessary token attributes.client/core/tokens_file_loader_test.go (2)
12-62
: The testTestLoadTokensFromUrl
is well-implemented and covers the successful scenario of loading tokens.
64-73
: The testTestLoadTokensFromUrlReturnsNoTokensWhenRequestErrorHappens
correctly handles the error scenario, ensuring robust error handling inLoadTokens
.client/chain/markets_assistant_test.go (2)
Line range hint
5-93
: The testTestMarketAssistantCreationUsingMarketsFromExchange
is comprehensive, testing the initialization and functionality ofMarketsAssistant
with mock data.
Line range hint
90-125
: The testTestMarketAssistantCreationWithAllTokens
effectively verifies the initialization ofMarketsAssistant
with all tokens, ensuring comprehensive coverage.client/chain/markets_assistant.go (2)
24-31
: TheTokenMetadata
interface is clearly defined, providing essential methods for token attribute access.
Line range hint
32-288
: TheMarketsAssistant
struct and its methods are well-implemented, effectively managing market and token data with robust initialization and unique symbol handling.client/common/network.go (2)
18-20
: Constants for token list URLs are correctly defined.
204-204
: Addition ofOfficialTokensListUrl
to theNetwork
struct is appropriate for managing token list URLs based on the network environment.client/exchange/exchange_test_support.go (2)
20-20
: Addition of theNetwork
field to theMockExchangeClient
struct enhances testing capabilities by allowing simulation of network configurations.
308-310
: Implementation of theGetNetwork
method inMockExchangeClient
is correct and necessary for accessing the network configuration in tests.client/exchange/exchange.go (2)
93-93
: Addition ofGetNetwork()
method to theExchangeClient
interface is correctly implemented.
965-967
: Implementation ofGetNetwork()
inexchangeClient
correctly returns the network configuration.
@@ -218,6 +221,7 @@ func LoadNetwork(name string, node string) Network { | |||
ChainCookieAssistant: &DisabledCookieAssistant{}, | |||
ExchangeCookieAssistant: &DisabledCookieAssistant{}, | |||
ExplorerCookieAssistant: &DisabledCookieAssistant{}, | |||
OfficialTokensListUrl: MainnetTokensListUrl, |
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 initialization of OfficialTokensListUrl
in LoadNetwork
is correctly implemented for different network types.
Consider adding a default case in the switch statement to handle unexpected network names gracefully.
+ default:
+ panic("Unsupported network type")
Also applies to: 241-241, 257-257, 312-312, 352-352, 366-366
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
OfficialTokensListUrl: MainnetTokensListUrl, | |
OfficialTokensListUrl: MainnetTokensListUrl, | |
+ default: | |
+ panic("Unsupported network type") |
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 agree with 🤖 here. Makes sense to handle this
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.
@keithbaum, thank you for your feedback and agreement! If there's anything more you need help with regarding this or any other aspect of the project, feel free to let me know. Happy coding! 🚀
func LoadTokens(tokensFileUrl string) ([]TokenMetadata, error) { | ||
var tokensMetadata []TokenMetadata | ||
response, err := http.Get(tokensFileUrl) | ||
if err != nil { | ||
return tokensMetadata, err | ||
} | ||
if 400 <= response.StatusCode { | ||
return tokensMetadata, fmt.Errorf("failed to load tokens from %s: %s", tokensFileUrl, response.Status) | ||
} | ||
defer response.Body.Close() | ||
|
||
decoder := json.NewDecoder(response.Body) | ||
err = decoder.Decode(&tokensMetadata) | ||
if err != nil { | ||
return make([]TokenMetadata, 0), err | ||
} | ||
|
||
return tokensMetadata, nil | ||
} |
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.
Consider refining the error handling in LoadTokens
to cover all client errors (status codes 400-499) instead of just 400 and above.
- if 400 <= response.StatusCode {
+ if response.StatusCode >= 400 && response.StatusCode < 500 {
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
func LoadTokens(tokensFileUrl string) ([]TokenMetadata, error) { | |
var tokensMetadata []TokenMetadata | |
response, err := http.Get(tokensFileUrl) | |
if err != nil { | |
return tokensMetadata, err | |
} | |
if 400 <= response.StatusCode { | |
return tokensMetadata, fmt.Errorf("failed to load tokens from %s: %s", tokensFileUrl, response.Status) | |
} | |
defer response.Body.Close() | |
decoder := json.NewDecoder(response.Body) | |
err = decoder.Decode(&tokensMetadata) | |
if err != nil { | |
return make([]TokenMetadata, 0), err | |
} | |
return tokensMetadata, nil | |
} | |
func LoadTokens(tokensFileUrl string) ([]TokenMetadata, error) { | |
var tokensMetadata []TokenMetadata | |
response, err := http.Get(tokensFileUrl) | |
if err != nil { | |
return tokensMetadata, err | |
} | |
if response.StatusCode >= 400 && response.StatusCode < 500 { | |
return tokensMetadata, fmt.Errorf("failed to load tokens from %s: %s", tokensFileUrl, response.Status) | |
} | |
defer response.Body.Close() | |
decoder := json.NewDecoder(response.Body) | |
err = decoder.Decode(&tokensMetadata) | |
if err != nil { | |
return make([]TokenMetadata, 0), err | |
} | |
return tokensMetadata, nil | |
} |
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.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (3)
- client/chain/markets_assistant_test.go (3 hunks)
- client/core/tokens_file_loader_test.go (1 hunks)
- client/exchange/exchange_test_support.go (3 hunks)
Files skipped from review as they are similar to previous changes (3)
- client/chain/markets_assistant_test.go
- client/core/tokens_file_loader_test.go
- client/exchange/exchange_test_support.go
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.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- client/common/network.go (8 hunks)
Files skipped from review as they are similar to previous changes (1)
- client/common/network.go
Solves CHAIN-101
Summary by CodeRabbit
New Features
TokenMetadata
interface for retrieving token information.Bug Fixes
Keyring
tonil
.Tests
Enhancements
TokenMetadata
interface.ExchangeClient
to include a method for retrieving network information.