close body when fetching abi #208
Conversation
WalkthroughThe function responsible for retrieving a contract's ABI was updated to ensure the HTTP response body is always closed after use and to add explicit error handling for non-200 HTTP responses. No changes were made to function signatures or public APIs. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant ABIHelper
Caller->>ABIHelper: GetABIForContract(contractAddress)
ABIHelper->>ABIHelper: HTTP GET request for ABI
ABIHelper->>ABIHelper: defer resp.Body.Close()
ABIHelper->>ABIHelper: Check if response status is 200
alt Status is 200
ABIHelper->>ABIHelper: Parse ABI from response body
ABIHelper-->>Caller: Return ABI
else Status is not 200
ABIHelper-->>Caller: Return error (unexpected status code)
end
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/common/abi.go (2)
43-45: Good error handling improvement.Adding a status code check ensures that the ABI is only parsed from successful responses. This prevents potential parsing errors or unexpected behavior when the API returns error responses.
However, you might consider providing more specific error messages for common status codes:
if resp.StatusCode != http.StatusOK { - return nil, fmt.Errorf("failed to get contract abi: unexpected status code %d", resp.StatusCode) + if resp.StatusCode == http.StatusNotFound { + return nil, fmt.Errorf("failed to get contract abi: contract not found for %s on chain %s", contract, chainId) + } else { + return nil, fmt.Errorf("failed to get contract abi: unexpected status code %d for %s", resp.StatusCode, url) + } }
34-52: Consider adding a timeout to the HTTP request.The current implementation uses the default HTTP client without a timeout, which could lead to long-running requests if the API is slow to respond.
You could create a client with a timeout:
func GetABIForContract(chainId string, contract string) (*abi.ABI, error) { url := fmt.Sprintf("%s/abi/%s/%s", config.Cfg.API.ThirdwebContractApi, chainId, contract) + client := &http.Client{ + Timeout: 10 * time.Second, + } - resp, err := http.Get(url) + resp, err := client.Get(url) if err != nil { return nil, fmt.Errorf("failed to get contract abi: %v", err) } defer resp.Body.Close() if resp.StatusCode != http.StatusOK { return nil, fmt.Errorf("failed to get contract abi: unexpected status code %d", resp.StatusCode) } abi, err := abi.JSON(resp.Body) if err != nil { return nil, fmt.Errorf("failed to load contract abi: %v", err) } return &abi, nil }Don't forget to add
"time"to the imports if you implement this suggestion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/common/abi.go(1 hunks)
🔇 Additional comments (2)
internal/common/abi.go (2)
41-41: Good addition for proper resource management.Adding
defer resp.Body.Close()ensures that the HTTP response body is properly closed after use, preventing potential resource leaks. This is a Go best practice that should be applied to all HTTP requests.
15-32:✅ Verification successful
Verify that cache mechanism properly handles non-200 responses.
Now that
GetABIForContractreturns errors for non-200 responses, ensure thatGetABIForContractWithCachecorrectly handles these new error cases. The current implementation appears to be handling all errors in the same way by setting the cached value to nil, which is appropriate.
🏁 Script executed:
#!/bin/bash # Search for any other callers of GetABIForContract to ensure they handle errors properly rg "GetABIForContract\(" --type go -A 5 -B 5Length of output: 1168
Cache correctly handles non-200 responses
GetABIForContractWithCachealready treats any error (including non-200 HTTP responses) by cachingniland returningnil, which aligns with the intended behavior. No other callers ofGetABIForContractwere found in the codebase.
No further changes needed.
TL;DR
Added proper HTTP response handling in the GetABIForContract function.
What changed?
defer resp.Body.Close()to ensure the response body is properly closed after useHow to test?
Why make this change?
This change improves error handling and resource management in the GetABIForContract function. The previous implementation didn't close the response body, which could lead to resource leaks, and didn't check the HTTP status code, which could lead to unexpected behavior when the API returns an error status.
Summary by CodeRabbit