feat: add token file validation CI #45
Conversation
WalkthroughThe recent updates introduce a GitHub Actions workflow to validate JSON files during pull requests, ensuring they meet a specified structure. Additionally, a new Go program has been developed to validate these JSON files and interact with the Injective Protocol's APIs to retrieve market data based on the tokens specified in the files. Changes
Recent Review DetailsConfiguration used: CodeRabbit UI Files selected for processing (2)
Files skipped from review as they are similar to previous changes (1)
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 (
|
mmeloni
force-pushed
the
feat/ci-valid
branch
12 times, most recently
from
998141b to
499a620
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
.github/workflows/test.yaml
Outdated
| - name: Install and build | ||
| run: | | ||
| cd validator | ||
| go install . |
There was a problem hiding this comment.
Tip
Codebase Verification
The verification script confirmed that the token-meta-validator binary is not accessible in the PATH, indicating that the installation step in the workflow might not ensure the binary's availability for subsequent steps. This aligns with the initial concern raised in the review comment about ensuring the installed binary is accessible in the PATH.
Analysis chain
Ensure that the installed token-meta-validator binary is accessible in the PATH.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify if token-meta-validator binary is accessible
if ! command -v token-meta-validator &> /dev/null
then
echo "token-meta-validator could not be found"
exit 1
fi
Length of output: 131
|
why have a different repo for the validation command? It can be put in the |
done @hmoragrega |
internal/validator/main.go
Outdated
| for key, client := range f.exchangeClient { | ||
| spotMarketInfo := injective_spot_exchange_rpcpb.MarketResponse{} | ||
| derivativeMarketInfo := injective_derivative_exchange_rpcpb.MarketResponse{} | ||
| spotMarketInfo, err := client.GetSpotMarket(ctx, market) |
There was a problem hiding this comment.
I’m not sure se should go as deep as check markets here, for example, do this requests return non active markets? I would simply validate the market Id is sintactically correct,
There was a problem hiding this comment.
Its present a market white list that allow to skip not existent markets check
| log.Errorf("cannot create testnet exchange client: %s", err) | ||
| os.Exit(1) |
There was a problem hiding this comment.
nit: we can use log.Fatalf("cannot read token file: %w", err) as a one-liner. It already calls os.Exit(1)
| if err != nil { | ||
| continue | ||
| } |
There was a problem hiding this comment.
We should never ignore an error or we might have a bad time troubleshooting issues.
I understand that this is just a validation tool and it shouldn't really bother us with bugs but we should always handle the error or log it.
For this case, a simple log would work
There was a problem hiding this comment.
Its expected too much false positive errors here, but at the end Ill return last error if market is not found or in other cases
@hmoragrega, I am not sure if I agree to have this inside |
| tokenFile := flag.String("token-file", "token.json", "path to token file") | ||
| marketSkipList := flag.String("market-skip-list", "", "comma separated list of markets to skip") | ||
| flag.Parse() |
There was a problem hiding this comment.
Consider using a more descriptive flag usage message to improve user experience.
- tokenFile := flag.String("token-file", "token.json", "path to token file")
+ tokenFile := flag.String("token-file", "token.json", "Specify the path to the token JSON file to be validated.")
- marketSkipList := flag.String("market-skip-list", "", "comma separated list of markets to skip")
+ marketSkipList := flag.String("market-skip-list", "", "Specify a comma-separated list of market identifiers to skip during validation.")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.
| tokenFile := flag.String("token-file", "token.json", "path to token file") | |
| marketSkipList := flag.String("market-skip-list", "", "comma separated list of markets to skip") | |
| flag.Parse() | |
| tokenFile := flag.String("token-file", "token.json", "Specify the path to the token JSON file to be validated.") | |
| marketSkipList := flag.String("market-skip-list", "", "Specify a comma-separated list of market identifiers to skip during validation.") | |
| flag.Parse() |
| log.Fatalf("cannot read token file: %s", err) | ||
| } | ||
| isValid := json.Valid(tokens) | ||
| if !isValid { | ||
| log.Fatalf("invalid json: %s", tokens) | ||
| } |
There was a problem hiding this comment.
Ensure graceful handling of errors rather than using log.Fatalf which causes the program to exit immediately.
- log.Fatalf("cannot read token file: %s", err)
+ log.Errorf("cannot read token file: %s", err)
+ return
- log.Fatalf("invalid json: %s", tokens)
+ log.Errorf("invalid json: %s", tokens)
+ returnCommittable 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.
| log.Fatalf("cannot read token file: %s", err) | |
| } | |
| isValid := json.Valid(tokens) | |
| if !isValid { | |
| log.Fatalf("invalid json: %s", tokens) | |
| } | |
| log.Errorf("cannot read token file: %s", err) | |
| return | |
| isValid := json.Valid(tokens) | |
| if !isValid { | |
| log.Errorf("invalid json: %s", tokens) | |
| return | |
| } |
| if err != nil { | ||
| log.Fatalf("cannot decode token file: %s", err) |
There was a problem hiding this comment.
Duplicate JSON decoding: decoder.Decode and json.Unmarshal are both used to decode the same token file.
Consider removing one of the decoding steps to optimize performance and avoid redundancy.
| exchangeClient, err := exchange.NewExchangeClient(network) | ||
| if err != nil { | ||
| log.Errorf("cannot create mainnet exchange client: %s", err) | ||
| os.Exit(1) | ||
| } |
There was a problem hiding this comment.
Error handling could be improved by returning after logging the error, to prevent further execution.
- log.Errorf("cannot create mainnet exchange client: %s", err)
- os.Exit(1)
+ log.Errorf("cannot create mainnet exchange client: %s", err)
+ returnCommittable 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.
| exchangeClient, err := exchange.NewExchangeClient(network) | |
| if err != nil { | |
| log.Errorf("cannot create mainnet exchange client: %s", err) | |
| os.Exit(1) | |
| } | |
| exchangeClient, err := exchange.NewExchangeClient(network) | |
| if err != nil { | |
| log.Errorf("cannot create mainnet exchange client: %s", err) | |
| return | |
| } |
| log.Errorf("cannot create testnet exchange client: %s", err) | ||
| os.Exit(1) | ||
| } |
There was a problem hiding this comment.
Similar to previous comment, improve error handling by returning after logging.
- log.Errorf("cannot create testnet exchange client: %s", err)
- os.Exit(1)
+ log.Errorf("cannot create testnet exchange client: %s", err)
+ returnCommittable 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.
| log.Errorf("cannot create testnet exchange client: %s", err) | |
| os.Exit(1) | |
| } | |
| log.Errorf("cannot create testnet exchange client: %s", err) | |
| return | |
| } |
| _, err = finder.findMarketOnAllNetwork(context.TODO(), market) | ||
| if err != nil { | ||
| log.Fatalf("%s: market %s not found: %s. If this is expected is possible to whitelist this market using `--market-skip-list` flag.", key, market, err) | ||
| } | ||
| log.Infof("%s: market %s found", key, market) |
There was a problem hiding this comment.
Consider handling errors more gracefully instead of using log.Fatalf, which stops program execution.
- log.Fatalf("%s: market %s not found: %s. If this is expected is possible to whitelist this market using `--market-skip-list` flag.", key, market, err)
+ log.Errorf("%s: market %s not found: %s. Consider whitelisting this market using the `--market-skip-list` flag if this is expected.", key, market, err)
+ continueCommittable 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.
| _, err = finder.findMarketOnAllNetwork(context.TODO(), market) | |
| if err != nil { | |
| log.Fatalf("%s: market %s not found: %s. If this is expected is possible to whitelist this market using `--market-skip-list` flag.", key, market, err) | |
| } | |
| log.Infof("%s: market %s found", key, market) | |
| _, err = finder.findMarketOnAllNetwork(context.TODO(), market) | |
| if err != nil { | |
| log.Errorf("%s: market %s not found: %s. Consider whitelisting this market using the `--market-skip-list` flag if this is expected.", key, market, err) | |
| continue | |
| } | |
| log.Infof("%s: market %s found", key, market) |
| func (f MarketFinder) findMarketOnAllNetwork(ctx context.Context, market string) (string, error) { | ||
| ctx, cancel := context.WithTimeout(context.Background(), 1000*time.Second) | ||
| defer cancel() | ||
| var err error | ||
| for key, client := range f.exchangeClient { | ||
| spotMarketInfo := injective_spot_exchange_rpcpb.MarketResponse{} | ||
| derivativeMarketInfo := injective_derivative_exchange_rpcpb.MarketResponse{} | ||
| spotMarketInfo, err = client.GetSpotMarket(ctx, market) | ||
| if err != nil { | ||
| derivativeMarketInfo, err = client.GetDerivativeMarket(ctx, market) | ||
| if err != nil && | ||
| !strings.Contains(err.Error(), "not found") && | ||
| !strings.Contains(err.Error(), "502") { | ||
| return "", fmt.Errorf("cannot get market %s on %s: %s", market, key, err) | ||
| } | ||
| if err != nil { | ||
| continue | ||
| } | ||
| return derivativeMarketInfo.Market.Ticker, nil | ||
| } | ||
| return spotMarketInfo.Market.Ticker, nil | ||
| } | ||
| if err != nil { | ||
| return "", fmt.Errorf("market %s not found: %s", market, err) | ||
| } | ||
| return "", fmt.Errorf("market %s not found", market) | ||
| } |
There was a problem hiding this comment.
The method findMarketOnAllNetwork should accept a context from the caller instead of creating a new one to maintain control over the timeout across different calls.
- func (f MarketFinder) findMarketOnAllNetwork(ctx context.Context, market string) (string, error) {
- ctx, cancel := context.WithTimeout(context.Background(), 1000*time.Second)
+ func (f MarketFinder) findMarketOnAllNetwork(ctx context.Context, market string) (string, error) {
+ ctx, cancel := context.WithTimeout(ctx, 1000*time.Second)
defer cancel()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 (f MarketFinder) findMarketOnAllNetwork(ctx context.Context, market string) (string, error) { | |
| ctx, cancel := context.WithTimeout(context.Background(), 1000*time.Second) | |
| defer cancel() | |
| var err error | |
| for key, client := range f.exchangeClient { | |
| spotMarketInfo := injective_spot_exchange_rpcpb.MarketResponse{} | |
| derivativeMarketInfo := injective_derivative_exchange_rpcpb.MarketResponse{} | |
| spotMarketInfo, err = client.GetSpotMarket(ctx, market) | |
| if err != nil { | |
| derivativeMarketInfo, err = client.GetDerivativeMarket(ctx, market) | |
| if err != nil && | |
| !strings.Contains(err.Error(), "not found") && | |
| !strings.Contains(err.Error(), "502") { | |
| return "", fmt.Errorf("cannot get market %s on %s: %s", market, key, err) | |
| } | |
| if err != nil { | |
| continue | |
| } | |
| return derivativeMarketInfo.Market.Ticker, nil | |
| } | |
| return spotMarketInfo.Market.Ticker, nil | |
| } | |
| if err != nil { | |
| return "", fmt.Errorf("market %s not found: %s", market, err) | |
| } | |
| return "", fmt.Errorf("market %s not found", market) | |
| } | |
| func (f MarketFinder) findMarketOnAllNetwork(ctx context.Context, market string) (string, error) { | |
| ctx, cancel := context.WithTimeout(ctx, 1000*time.Second) | |
| defer cancel() | |
| var err error | |
| for key, client := range f.exchangeClient { | |
| spotMarketInfo := injective_spot_exchange_rpcpb.MarketResponse{} | |
| derivativeMarketInfo := injective_derivative_exchange_rpcpb.MarketResponse{} | |
| spotMarketInfo, err = client.GetSpotMarket(ctx, market) | |
| if err != nil { | |
| derivativeMarketInfo, err = client.GetDerivativeMarket(ctx, market) | |
| if err != nil && | |
| !strings.Contains(err.Error(), "not found") && | |
| !strings.Contains(err.Error(), "502") { | |
| return "", fmt.Errorf("cannot get market %s on %s: %s", market, key, err) | |
| } | |
| if err != nil { | |
| continue | |
| } | |
| return derivativeMarketInfo.Market.Ticker, nil | |
| } | |
| return spotMarketInfo.Market.Ticker, nil | |
| } | |
| if err != nil { | |
| return "", fmt.Errorf("market %s not found: %s", market, err) | |
| } | |
| return "", fmt.Errorf("market %s not found", market) | |
| } |
This PR add token meta json file validation step with this script:
https://github.com/InjectiveLabs/token-meta-validator
First it run a semantic content validtion check then it fetch provided markets to check existance.
If content is not matching provided struct schema it will error:
Summary by CodeRabbit