(fix) Fix read only examples #246
Conversation
|
Warning Rate limit exceeded@PavelInjective has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 26 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe changes involve modifications to various Go example files, primarily focusing on how data is retrieved and processed. Key alterations include dynamic fetching of block heights and market IDs instead of using hardcoded values, as well as updates to network configurations and transaction IDs. Additionally, there are changes to the request parameters for fetching transactions, emphasizing limiting the number of results instead of specifying historical points. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Example
participant ExchangeClient
participant ChainClient
User->>Example: Start example
Example->>ChainClient: GetLatestBlockHeight()
ChainClient-->>Example: Return latest block height
Example->>ChainClient: GetBlock(blockHeight)
ChainClient-->>Example: Return block data
Example->>User: Display transactions
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? 🪧 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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
Why are we changing the examples? Were they broken? |
PavelInjective
force-pushed
the
fix-read-only-examples
branch
from
fd43148
to
fa9e283
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (3)
examples/explorer/5_TxsRequest/example.go (1)
Line range hint
1-38: Suggestions for improving the exampleWhile the example demonstrates the basic usage of the explorer client, consider the following improvements to make it more robust and flexible:
- Error handling: Handle the error from
json.MarshalIndent. For example:str, err := json.MarshalIndent(res, "", " ") if err != nil { fmt.Printf("Error marshaling JSON: %v\n", err) return }
- Context: Instead of using
context.Background(), consider adding a timeout to prevent the request from hanging indefinitely:ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) defer cancel()
- Configurability: Make the network and limit configurable. For example:
network := common.LoadNetwork(os.Getenv("NETWORK_TYPE"), os.Getenv("NETWORK_ENDPOINT")) limit, _ := strconv.Atoi(os.Getenv("TX_LIMIT")) if limit == 0 { limit = 10 // Default value } req := explorerPB.GetTxsRequest{ Limit: uint64(limit), }These changes would make the example more flexible and closer to production-ready code.
examples/chain/auth/query/1_Account/example.go (1)
Line range hint
1-71: Consider enhancing the example for better usability and error handling.While the example effectively demonstrates how to query an account using the Injective SDK, there are a few suggestions to improve it:
- Replace hardcoded values with environment variables or command-line flags for better flexibility.
- Improve error handling, especially for the account query result.
- Add comments to explain key steps in the process.
Here's a suggested refactor for the
mainfunction:func main() { network := common.LoadNetwork("mainnet", "lb") tmClient, err := rpchttp.New(network.TmEndpoint, "/websocket") if err != nil { log.Fatalf("Failed to create tendermint client: %v", err) } // Use environment variables for sensitive information home := os.Getenv("HOME") chainId := os.Getenv("CHAIN_ID") privateKey := os.Getenv("PRIVATE_KEY") address := os.Getenv("QUERY_ADDRESS") senderAddress, cosmosKeyring, err := chainclient.InitCosmosKeyring( home+"/.injectived", "injectived", "file", "inj-user", "12345678", privateKey, false, ) if err != nil { log.Fatalf("Failed to initialize Cosmos keyring: %v", err) } clientCtx, err := chainclient.NewClientContext( chainId, senderAddress.String(), cosmosKeyring, ) if err != nil { log.Fatalf("Failed to create client context: %v", err) } clientCtx = clientCtx.WithNodeURI(network.TmEndpoint).WithClient(tmClient) queryClient := authtypes.NewQueryClient(clientCtx) ctx, cancelFn := context.WithCancel(context.Background()) defer cancelFn() res, err := queryClient.Account(ctx, &authtypes.QueryAccountRequest{ Address: address, }) if err != nil { log.Fatalf("Failed to query account: %v", err) } var account types.EthAccount err = clientCtx.Codec.UnmarshalInterface(res.GetAccount(), &account) if err != nil { log.Fatalf("Failed to unmarshal account: %v", err) } fmt.Printf("Account details for %s:\n%s\n", address, account.String()) }This refactored version:
- Uses environment variables for sensitive information.
- Improves error handling with more descriptive messages.
- Uses
log.Fatalffor critical errors to provide stack traces.- Adds comments to explain the purpose of key steps.
- Uses
UnmarshalInterfaceinstead ofMustUnmarshalfor safer deserialization.Remember to set the necessary environment variables before running the example:
export CHAIN_ID="your-chain-id" export PRIVATE_KEY="your-private-key" export QUERY_ADDRESS="address-to-query"examples/chain/8_OfflineSigning/example.go (1)
90-98: Approve with suggestions: Dynamic market ID retrieval implemented.The change to dynamically fetch the market ID is a good improvement over hardcoding. However, consider the following suggestions:
- Add a comment explaining why you're using the first market ID.
- Consider adding a fallback or error handling if no markets are returned.
- For a more robust example, you might want to filter for a specific market or allow the user to choose one.
Here's a suggested improvement:
marketResponse, err := exchangeClient.GetSpotMarkets(ctx, &spotExchangePB.MarketsRequest{}) if err != nil { panic(err) } if len(marketResponse.Markets) == 0 { panic("No markets available") } // For this example, we're using the first available market marketId := marketResponse.Markets[0].MarketId fmt.Printf("Using market ID: %s\n", marketId)This change adds better error handling and a comment explaining the choice of market ID.
🛑 Comments failed to post (5)
examples/exchange/derivatives/1_Market/example.go (2)
19-19: 🛠️ Refactor suggestion
Consider using a dynamic approach for obtaining the market ID.
While the market ID has been updated, using a hardcoded value in example code can lead to maintenance issues if the specific market becomes invalid or changes. Consider implementing a more dynamic approach to obtain a valid market ID, such as:
- Fetching a list of available markets and using the first one.
- Allowing the user to input a market ID as a command-line argument.
- Using environment variables to configure the market ID.
This would make the example more robust and easier to maintain.
Here's a sample implementation using a command-line argument:
import ( // ... existing imports ... "os" ) func main() { // ... existing code ... var marketID string if len(os.Args) > 1 { marketID = os.Args[1] } else { marketID = "0x95698a9d8ba11660f44d7001d8c6fb191552ece5d9141a05c5d9128711cdc2e0" // fallback to default } res, err := exchangeClient.GetDerivativeMarket(ctx, marketID) // ... rest of the code ... }
21-21: 🛠️ Refactor suggestion
Improve error handling for better user feedback.
The current error handling prints the error but allows the program to continue executing, which may lead to unexpected behavior or confusing output. Consider enhancing the error handling to provide more context and exit the program when an error occurs.
Here's a suggested improvement:
if err != nil { fmt.Fprintf(os.Stderr, "Error fetching derivative market: %v\n", err) os.Exit(1) }This change will:
- Print the error message to stderr with context.
- Exit the program with a non-zero status code, indicating an error occurred.
examples/chain/7_GetBlock/example.go (2)
21-21: 🛠️ Refactor suggestion
Simplify block height calculation
The current implementation calculates 99% of the latest block height using floating-point arithmetic, which might lead to unnecessary precision loss.
Consider simplifying the calculation to use integer arithmetic:
-res, err := tmClient.GetBlock(clientCtx, int64(float64(latestBlockHeight)*0.99)) +res, err := tmClient.GetBlock(clientCtx, latestBlockHeight - latestBlockHeight/100)This change maintains the intent of fetching a block slightly before the latest one while avoiding potential floating-point precision issues.
📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.res, err := tmClient.GetBlock(clientCtx, latestBlockHeight - latestBlockHeight/100)
18-20:
⚠️ Potential issueConsider improving error handling
While the added error handling is a good start, the code continues to execute even if an error occurs when fetching the latest block height. This could lead to unexpected behavior or errors in the subsequent code.
Consider modifying the error handling to return from the function if an error occurs:
if err != nil { - fmt.Println(err) + fmt.Println("Error fetching latest block height:", err) + return }This change ensures that the example doesn't attempt to use an invalid block height if the initial fetch fails.
📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.if err != nil { fmt.Println("Error fetching latest block height:", err) return }examples/explorer/5_TxsRequest/example.go (1)
24-24: 🛠️ Refactor suggestion
Shift from historical filtering to result limiting
The change from using
Before: 7158400toLimit: 10in theGetTxsRequeststructure represents a significant shift in how transactions are requested:
Removal of
Before: This eliminates the ability to specify a historical point (block height or timestamp) for transaction retrieval. This might impact users who relied on this for accessing specific historical data.Addition of
Limit: This introduces a way to control the number of transactions returned, which is a common practice for pagination and managing response sizes.While this change simplifies the API usage and is suitable for many use cases, it's worth considering the following:
- Does this change align with the intended use cases for this example?
- Are there scenarios where users might still need to specify a historical point?
- Is the limit of 10 transactions sufficient for demonstrating the API's capabilities?
Consider adding a comment explaining the purpose of the
Limitfield and its implications. For example:req := explorerPB.GetTxsRequest{ // Limit the number of transactions returned to 10. // Adjust this value based on your specific requirements. Limit: 10, }This will help users understand how to modify the example for their specific needs.
|
Yes, they are not functional (e.g. some market does not exist, some height is too old or some settings have changed). I would expect all the read-only examples to work. |
|
No, that is not the case. The examples are there to show how to use the SDK, but the users are expected to configure the parameters correctly. We prefer to have examples with simple code and out of date parameters rather than complex examples because we are requesting values from the chain to use as the parameters for the example, shifting the focus of the script and maybe confusing the user |
Summary by CodeRabbit
New Features
Bug Fixes
Chores