feat: add support for arabica-11 in download-genesis command #2978
Conversation
subhajit20
requested review from
evan-forbes,
rootulp and
cmwaters
as code owners
WalkthroughWalkthroughThe modification involves updating the Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| @@ -19,6 +19,7 @@ var chainIDToSha256 = map[string]string{ | |||
| "celestia": "9727aac9bbfb021ce7fc695a92f901986421283a891b89e0af97bc9fad187793", | |||
| "mocha-4": "0846b99099271b240b638a94e17a6301423b5e4047f6558df543d6e91db7e575", | |||
| "arabica-10": "fad0a187669f7a2c11bb07f9dc27140d66d2448b7193e186312713856f28e3e1", | |||
| "arabica-11": "" | |||
There was a problem hiding this comment.
The addition of "arabica-11" with an empty string as its value to the chainIDToSha256 map is a placeholder for future updates. Ensure that the actual SHA256 hash is added before the "arabica-11" network goes live to avoid potential issues with genesis file verification.
| @@ -19,6 +19,7 @@ | |||
| "celestia": "9727aac9bbfb021ce7fc695a92f901986421283a891b89e0af97bc9fad187793", | |||
| "mocha-4": "0846b99099271b240b638a94e17a6301423b5e4047f6558df543d6e91db7e575", | |||
| "arabica-10": "fad0a187669f7a2c11bb07f9dc27140d66d2448b7193e186312713856f28e3e1", | |||
| "arabica-11": "" | |||
| } | |||
|
|
|||
| func downloadGenesisCommand() *cobra.Command { | |||
There was a problem hiding this comment.
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.
| 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
downloadFilefunction 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.
There was a problem hiding this comment.
[blocking] We should update the error messages to also include arabica-11
|
Hey I have not found the arabica-11 from the network repo so I added the key to the map as of now and after you guys deployed the network I would add the sha256 hash to it. |
rootulp
changed the title
|
Thanks for the contribution @subhajit20 . The genesis file now exists at https://github.com/celestiaorg/networks/blob/master/arabica-11/genesis.json |
| @@ -19,6 +19,7 @@ | |||
| "celestia": "9727aac9bbfb021ce7fc695a92f901986421283a891b89e0af97bc9fad187793", | |||
| "mocha-4": "0846b99099271b240b638a94e17a6301423b5e4047f6558df543d6e91db7e575", | |||
| "arabica-10": "fad0a187669f7a2c11bb07f9dc27140d66d2448b7193e186312713856f28e3e1", | |||
| "arabica-11": "" | |||
| } | |||
|
|
|||
| func downloadGenesisCommand() *cobra.Command { | |||
There was a problem hiding this comment.
[blocking] We should update the error messages to also include arabica-11
| @@ -82,6 +84,10 @@ func isKnownChainID(chainID string) bool { | |||
| return contains(knownChainIDs, chainID) | |||
| } | |||
|
|
|||
| func chainIDs() string { | |||
Closes #2976
Testing
Updated the help and error messages to include arabica-11
Downloading arabica-11 genesis works