Skip to content
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.

Sign up for GitHub

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: add support for arabica-11 in download-genesis command #2978

Merged
merged 3 commits into from
Jan 3, 2024
Merged

feat: add support for arabica-11 in download-genesis command #2978

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e307301
Added support for arabica-11 in download-genesis command
subhajit20 Jan 3, 2024
05f2684
Update cmd/celestia-appd/cmd/download_genesis.go
rootulp Jan 3, 2024
293b6ba
fix: error message
rootulp Jan 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions cmd/celestia-appd/cmd/download_genesis.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"io"
"net/http"
"os"
"strings"

"github.com/cosmos/cosmos-sdk/server"
"github.com/spf13/cobra"
Expand All @@ -19,20 +20,21 @@ var chainIDToSha256 = map[string]string{
"celestia": "9727aac9bbfb021ce7fc695a92f901986421283a891b89e0af97bc9fad187793",
"mocha-4": "0846b99099271b240b638a94e17a6301423b5e4047f6558df543d6e91db7e575",
"arabica-10": "fad0a187669f7a2c11bb07f9dc27140d66d2448b7193e186312713856f28e3e1",
"arabica-11": "77605cee57ce545b1be22402110d4baacac837bdc7fc3f5c74020abf9a08810f",
}

func downloadGenesisCommand() *cobra.Command {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error message in the RunE function of downloadGenesisCommand does not include "arabica-11" as a known chain-id. This should be updated to reflect the new addition to the chainIDToSha256 map.

- return fmt.Errorf("unknown chain-id: %s. Must be: celestia, mocha-4, or arabica-10", chainID)
+ return fmt.Errorf("unknown chain-id: %s. Must be: celestia, mocha-4, arabica-10, or arabica-11", chainID)
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.

Suggested change
func downloadGenesisCommand() *cobra.Command {
func downloadGenesisCommand() *cobra.Command {

Consider caching the result of getKeys(chainIDToSha256) outside of the isKnownChainID function to avoid recomputing the known chain IDs on every invocation, which could be a performance improvement if the command is run multiple times in a session.

The script output shows the downloadFile function, but it does not include any checks for the HTTP response status code. The function directly proceeds to create a file and copy the response body into it after making the HTTP GET request. There is no check to ensure that the response status code is 200 OK before writing the content to the file.

Based on the provided output, the review comment is correct. The downloadFile function should be updated to handle non-200 HTTP responses appropriately to prevent writing non-genesis file content if the URL is incorrect or the file is not found.

  • The downloadFile function lacks error handling for non-200 HTTP status codes.
  • The function writes the HTTP response body to a file without checking the status code, which could lead to incorrect file content being saved.
Analysis chain

Ensure that the downloadFile function has proper error handling for HTTP status codes other than 200 OK. This is important to prevent writing non-genesis file content if the URL is incorrect or the file is not found.

To verify the review comment, I need to see more of the downloadFile function, particularly the parts that handle the HTTP response status codes. I will generate a script to display more lines following the http.Get call.

Scripts Executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Verify that downloadFile handles non-200 HTTP responses appropriately.
rg --type go 'http.Get' --context 10 cmd/celestia-appd/cmd/download_genesis.go

Length of output: 438

Script:

#!/bin/bash
# Display more of the downloadFile function to inspect HTTP status code handling.
rg --type go 'func downloadFile' -A 30 cmd/celestia-appd/cmd/download_genesis.go

Length of output: 692

The computeSha256 function reads the entire file into memory, which could be optimized to handle large genesis files more efficiently by processing the file in chunks.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[blocking] We should update the error messages to also include arabica-11

cmd := &cobra.Command{
Use: "download-genesis [chain-id]",
Short: "Download genesis file from https://github.com/celestiaorg/networks",
Long: "Download genesis file from https://github.com/celestiaorg/networks.\n" +
"The first argument should be a known chain-id. Ex. celestia, mocha-4, or arabica-10.\n" +
fmt.Sprintf("The first argument should be a known chain-id. Ex. %s\n", chainIDs()) +
"If no argument is provided, defaults to celestia.\n",
Args: cobra.MaximumNArgs(1),
RunE: func(cmd *cobra.Command, args []string) error {
chainID := getChainIDOrDefault(args)
if !isKnownChainID(chainID) {
return fmt.Errorf("unknown chain-id: %s. Must be: celestia, mocha-4, or arabica-10", chainID)
return fmt.Errorf("unknown chain-id: %s. Must be: %s", chainID, chainIDs())
}
outputFile := server.GetServerContextFromCmd(cmd).Config.GenesisFile()
fmt.Printf("Downloading genesis file for %s to %s\n", chainID, outputFile)
Expand Down Expand Up @@ -82,6 +84,10 @@ func isKnownChainID(chainID string) bool {
return contains(knownChainIDs, chainID)
}

func chainIDs() string {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice addition 👍

return strings.Join(getKeys(chainIDToSha256), ", ")
}

// contains checks if a string is present in a slice.
func contains(slice []string, s string) bool {
for _, v := range slice {
Expand Down
Loading