Fix: add timeoutheight_sync_interval parameter to finetune sync frequency or abort it completely #361
Conversation
WalkthroughThe pull request introduces a new optional parameter Changes
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🔇 Additional comments (3)pyinjective/async_client.py (3)
The new optional parameter
The initialization logic:
The changes provide control over sync frequency, but we should verify that the default interval (20s) is sufficient to prevent load balancer limitations. Also applies to: 3455-3462 ✅ Verification successfulLet me gather more specific information about the timeoutheight sync interval configuration. Default interval of 20s is appropriate for load balancing The code review shows that:
The default value provides sufficient spacing between gRPC requests to prevent overloading, while still maintaining timely updates. No issues found with the current implementation. 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Check for any gRPC rate limiting or error patterns in the codebase
# that might indicate if the default interval needs adjustment.
# Search for gRPC error handling patterns
echo "Searching for gRPC error handling patterns..."
rg -A 5 "grpc\.RpcError|load.*balance|rate.*limit"
# Search for existing timeout configurations
echo "Searching for other timeout/interval configurations..."
rg "DEFAULT_.*INTERVAL|DEFAULT_.*TIMEOUT"
Length of output: 135505 Script: #!/bin/bash
# Search for DEFAULT_TIMEOUTHEIGHT_SYNC_INTERVAL definition and any related configurations
rg "DEFAULT_TIMEOUTHEIGHT_SYNC_INTERVAL" -A 3
# Search for any code that uses this interval
rg "timeoutheight_sync_interval" -A 3
Length of output: 2030 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
|
![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. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
pyproject.toml (1)
Critical security vulnerabilities found - dependency constraints needed
Several critical security issues require immediate attention:
ecdsawith "*" versioning is dangerous:
- CRITICAL severity vulnerability in versions < 0.13.3
- HIGH severity Minerva timing attack in versions <= 0.18.0 (recent: Jan 2024)
requestswith "*" versioning has multiple vulnerabilities:
- MODERATE severity issues in versions < 2.32.0
- Header leaks in versions < 2.31.0
grpciowith "*" versioning has HIGH severity issues:
- Multiple "Excessive Iteration" vulnerabilities in various versions
Required actions:
- Add minimum version constraints for security:
ecdsa = ">=0.18.1" requests = ">=2.32.0" grpcio = ">=1.56.2"- Document the reason for protobuf=5.26.1 pinning
- Consider pinning other dependencies with "*" versioning for better reproducibility
🔗 Analysis chain
Line range hint
24-38: Verify security implications of dependency versions.
- The aiohttp >= 3.9.4 constraint addresses a security issue (referenced in comment).
- The strict pinning of protobuf to 5.26.1 should be reviewed.
- Consider pinning more dependencies for better reproducibility.
Let's check for known vulnerabilities in key dependencies:
Security review needed for protobuf and other dependencies
The current protobuf version (5.26.1) is behind the latest version (5.28.3), though it's not affected by any known vulnerabilities. However:
- The strict pinning to protobuf 5.26.1 should be documented with a comment explaining the reason, similar to aiohttp.
- Several dependencies using "*" versioning (bech32, bip32, ecdsa, etc.) pose reproducibility and security risks.
Let's verify if these dependencies have known vulnerabilities:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for security advisories in critical dependencies gh api graphql -f query=' { securityVulnerabilities(first: 5, ecosystem: PIP, package: "protobuf") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } } ' # Check latest versions of critical dependencies echo "Latest protobuf version:" curl -s https://pypi.org/pypi/protobuf/json | jq '.info.version'Length of output: 1658
Script:
#!/bin/bash # Check for security advisories in other critical dependencies for pkg in "ecdsa" "web3" "requests" "grpcio"; do echo "Checking $pkg:" gh api graphql -f query=" { securityVulnerabilities(first: 3, ecosystem: PIP, package: \"$pkg\") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange } } } " doneLength of output: 3218
pyinjective/async_client.py (1)
3461-3462: Consider adding validation for the interval value.While the configurable interval achieves the goal of controlling sync frequency, very small values could still lead to high load. Consider adding validation to ensure the interval is not set too low.
def __init__( self, network: Network, insecure: Optional[bool] = None, credentials=None, timeoutheight_sync_interval: Optional[int] = DEFAULT_TIMEOUTHEIGHT_SYNC_INTERVAL, ): + if timeoutheight_sync_interval is not None and timeoutheight_sync_interval < 5: + raise ValueError("timeoutheight_sync_interval must be at least 5 seconds to prevent excessive load") self._timeoutheight_sync_interval = timeoutheight_sync_interval
🛑 Comments failed to post (1)
pyproject.toml (1)
3-3: 💡 Codebase verification
Version bump to 1.9.0-pre needs revision to align with release pattern
The version history shows that the project uses
rc(release candidate) suffix for pre-releases (v1.9.0-rc1 through rc4) instead of-pre. The version should be updated to follow this established pattern.
- Consider changing
version = "1.9.0-pre"toversion = "1.9.0-rc1"🔗 Analysis chain
Version bump to 1.9.0-pre looks appropriate.
The version increment from 1.8.0 to 1.9.0-pre aligns well with the introduction of the new
timeoutheight_sync_intervalparameter, which could potentially affect existing client behavior.Let's verify if this follows the project's versioning pattern:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check version history pattern gh api repos/InjectiveLabs/sdk-python/tags --jq '.[].name' | head -n 5Length of output: 123
Fixes the case, when the client produced too many requests and was limited by the grpc load balancer
Summary by CodeRabbit
New Features
Bug Fixes
Chores