feat(coordinator): add version check for sdk provers #1551
Conversation
WalkthroughThe changes introduce new functionality for handling SDK versions in the Changes
Poem
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
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: 2
🧹 Outside diff range and nitpick comments (1)
common/version/prover_version.go (1)
30-34: Add tracking and detailed documentation for future SDK version checks.The current implementation is a placeholder that accepts all SDK versions. To ensure this doesn't remain as technical debt:
- Document the expected format of SDK versions
- Add a TODO with specific checks that will be implemented
- Create a tracking issue for the implementation
Would you like me to help create a GitHub issue to track the implementation of SDK version validation?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- common/version/prover_version.go (3 hunks)
- common/version/version.go (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- common/version/version.go
🔇 Additional comments (1)
common/version/prover_version.go (1)
12-14: Verify the implications of accepting all SDK versions.While the implementation is clean, accepting all SDK versions without validation could potentially lead to compatibility issues. Consider adding validation logic or logging when SDK versions are encountered to track usage patterns.
Consider adding structured logging to track SDK version usage:
if strings.HasPrefix(proverVersion, "sdk") { + log.Info("Processing SDK prover version", "version", proverVersion) return CheckProverSDKVersion(proverVersion) }
| if strings.HasPrefix(proverVersion, "sdk") { | ||
| return CheckProverSDKWithMinVersion(proverVersion, minVersion) | ||
| } |
There was a problem hiding this comment.
Add validation for minVersion format in SDK version checks.
The current implementation skips semver validation for SDK versions. Consider:
- Validating the minVersion format even for SDK versions
- Documenting the expected format for SDK version constraints
if strings.HasPrefix(proverVersion, "sdk") {
+ if !strings.HasPrefix(minVersion, "sdk") {
+ log.Error("incompatible version formats", "proverVersion", proverVersion, "minVersion", minVersion)
+ return false
+ }
return CheckProverSDKWithMinVersion(proverVersion, minVersion)
}📝 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 strings.HasPrefix(proverVersion, "sdk") { | |
| return CheckProverSDKWithMinVersion(proverVersion, minVersion) | |
| } | |
| if strings.HasPrefix(proverVersion, "sdk") { | |
| if !strings.HasPrefix(minVersion, "sdk") { | |
| log.Error("incompatible version formats", "proverVersion", proverVersion, "minVersion", minVersion) | |
| return false | |
| } | |
| return CheckProverSDKWithMinVersion(proverVersion, minVersion) | |
| } |
| // CheckProverSDKWithMinVersion check prover sdk version is at least the minimum required version, it simply returns true for now, | ||
| // and more checks will be added as we evolve. | ||
| func CheckProverSDKWithMinVersion(proverVersion string, minVersion string) bool { | ||
| return true | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement basic version comparison for SDK versions.
The current implementation accepts any version combination, which could lead to compatibility issues. Consider implementing basic version comparison:
- Validate input parameters
- Parse and compare SDK version numbers
Here's a suggested implementation:
func CheckProverSDKWithMinVersion(proverVersion string, minVersion string) bool {
- return true
+ if !strings.HasPrefix(proverVersion, "sdk") || !strings.HasPrefix(minVersion, "sdk") {
+ log.Error("invalid SDK version format", "proverVersion", proverVersion, "minVersion", minVersion)
+ return false
+ }
+
+ // Extract version numbers (assuming format sdk-x.y.z)
+ proverParts := strings.Split(proverVersion, "-")
+ minParts := strings.Split(minVersion, "-")
+ if len(proverParts) != 2 || len(minParts) != 2 {
+ log.Error("invalid SDK version format", "proverVersion", proverVersion, "minVersion", minVersion)
+ return false
+ }
+
+ proverSemver, err1 := semver.NewVersion(proverParts[1])
+ minSemver, err2 := semver.NewVersion(minParts[1])
+ if err1 != nil || err2 != nil {
+ log.Error("invalid semver in SDK version", "error1", err1, "error2", err2)
+ return false
+ }
+
+ return proverSemver.Compare(minSemver) >= 0
}Committable suggestion was skipped due to low confidence.
Purpose or design rationale of this PR
Currently the sdk-provers are hard to get the "repo" commit hash and hard to follow the version check we are using in coordinator now: see https://github.com/scroll-tech/scroll-proving-sdk/blob/haoyu/sindri_tokio/src/utils.rs#L5-L9, we have to hardcode a version to bypass the version check for now.
This PR add a dedicated version check mechanism for sdk-prover. And it simply returns true for now.
PR title
Your PR title must follow conventional commits (as we are doing squash merge for each PR), so it must start with one of the following types:
Deployment tag versioning
Has
tagincommon/version.gobeen updated or have you addedbump-versionlabel to this PR?Breaking change label
Does this PR have the
breaking-changelabel?Summary by CodeRabbit
New Features
Bug Fixes
These changes improve the application's capability to manage SDK versions while ensuring existing functionalities remain intact.