fix(abigen): implement solc binary fallback for Apple Silicon Macs #3572
Conversation
- Add fallback mechanism that downloads native solc binaries when Docker fails - Detect Apple Silicon platform and use appropriate binary - Cache downloaded binaries to prevent repeated downloads - Implement race-safe file locking for concurrent processes - Add tests for binary downloads and direct compilation - Update documentation with new fallback mechanism information Fixes #3366 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThis pull request updates the Solidity compilation workflow and documentation. The README now explains a fallback mechanism using a downloaded Changes
Sequence Diagram(s)sequenceDiagram
participant U as User/Developer
participant C as compileSolidity
participant D as Docker
participant B as getSolcBinary
participant P as Platform Utils
U->>C: Request Solidity compilation
C->>D: Attempt Docker compilation
alt Docker success
D-->>C: Contract Data
C-->>U: Return contract data
else Docker fails
C->>B: Request solc binary
B->>P: determinePlatform()
P-->>B: Platform information
B->>B: Construct download URL with getSolcURL()
B-->>C: Return solc binary path
C->>C: Invoke solc binary directly
C-->>U: Return contract data
end
Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches
🪧 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
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3572 +/- ##
===================================================
+ Coverage 14.21232% 15.49407% +1.28175%
===================================================
Files 300 431 +131
Lines 29390 37950 +8560
Branches 105 105
===================================================
+ Hits 4177 5880 +1703
- Misses 24744 31379 +6635
- Partials 469 691 +222
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Deploying sanguine-fe with
|
| Latest commit: |
fff69c3
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://236c2b55.sanguine-fe.pages.dev |
| Branch Preview URL: | https://feature-abigen-solc-fallback.sanguine-fe.pages.dev |
![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 comments (1)
tools/abigen/internal/generate.go (1)
128-238: 🛠️ Refactor suggestionCheck error from runFile.Close()
At line 191, the return value of
runFile.Close()is not handled. If closing the file fails, you could miss important errors.- defer runFile.Close() + defer func() { + if cerr := runFile.Close(); cerr != nil { + fmt.Fprintf(os.Stderr, "failed to close docker run file: %v\n", cerr) + } + }()🧰 Tools
🪛 golangci-lint (1.62.2)
191-191: Error return value of
runFile.Closeis not checked(errcheck)
201-201: shadow: declaration of "err" shadows declaration at line 131
(govet)
🪛 GitHub Check: Lint (tools)
[failure] 191-191:
Error return value ofrunFile.Closeis not checked (errcheck)
🧹 Nitpick comments (5)
tools/abigen/README.md (1)
129-159: Improve heading level consistencyThe heading at line 129 skips directly to "###". The lint tool recommends not skipping heading levels; consider switching to "##" or adjusting other headings for consistency and readability.
-### Solc Binary Fallback for Apple Silicon and Other Platforms +## Solc Binary Fallback for Apple Silicon and Other Platforms🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
129-129: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3(MD001, heading-increment)
tools/abigen/internal/generate_test.go (1)
90-114: Account for network variability in testsNetworking tests may fail in offline environments or when unexpected connectivity issues arise. Consider handling these cases to prevent spurious failures outside CI.
🧰 Tools
🪛 golangci-lint (1.62.2)
112-112: SA1019: filepath.HasPrefix has been deprecated since Go 1.0 because it shouldn't be used: HasPrefix does not respect path boundaries and does not ignore case when required.
(staticcheck)
tools/abigen/internal/generate.go (3)
426-452: Ensure comment style complianceLine 426: The comment is missing a trailing period, as flagged by lint. Consider adding punctuation to conform to style guidelines.
-// determinePlatform returns the platform-specific string for downloading solc +// determinePlatform returns the platform-specific string for downloading solc.🧰 Tools
🪛 golangci-lint (1.62.2)
426-426: Comment should end in a period
(godot)
🪛 GitHub Check: Lint (tools)
[failure] 426-426:
Comment should end in a period (godot)
453-483: Add a period at the end of the commentLine 453: The lint rules require comments to end with a period.
-// getSolcURL returns the URL to download the solc binary for the given version and platform +// getSolcURL returns the URL to download the solc binary for the given version and platform.🧰 Tools
🪛 golangci-lint (1.62.2)
453-453: Comment should end in a period
(godot)
454-454: getSolcURL - result 1 (error) is always nil
(unparam)
🪛 GitHub Check: Lint (tools)
[failure] 453-453:
Comment should end in a period (godot)
484-521: Terminate the function comment with punctuationLine 484: The function comment should end with a period to meet style conventions.
-// isSemverGreaterOrEqual checks if version a is greater than or equal to version b +// isSemverGreaterOrEqual checks if version a is greater than or equal to version b.🧰 Tools
🪛 golangci-lint (1.62.2)
484-484: Comment should end in a period
(godot)
🪛 GitHub Check: Lint (tools)
[failure] 484-484:
Comment should end in a period (godot)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
tools/abigen/README.md(2 hunks)tools/abigen/internal/export_test.go(1 hunks)tools/abigen/internal/generate.go(4 hunks)tools/abigen/internal/generate_test.go(2 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
tools/abigen/README.md
129-129: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
🪛 golangci-lint (1.62.2)
tools/abigen/internal/generate_test.go
143-143: Error return value of os.RemoveAll is not checked
(errcheck)
151-151: Error return value of os.Setenv is not checked
(errcheck)
152-152: Error return value of os.Setenv is not checked
(errcheck)
[medium] 146-146: G306: Expect WriteFile permissions to be 0600 or less
(gosec)
112-112: SA1019: filepath.HasPrefix has been deprecated since Go 1.0 because it shouldn't be used: HasPrefix does not respect path boundaries and does not ignore case when required.
(staticcheck)
tools/abigen/internal/generate.go
191-191: Error return value of runFile.Close is not checked
(errcheck)
201-201: shadow: declaration of "err" shadows declaration at line 131
(govet)
366-366: Error return value of lock.Close is not checked
(errcheck)
394-394: Error return value of resp.Body.Close is not checked
(errcheck)
405-405: Error return value of tmpFile.Close is not checked
(errcheck)
426-426: Comment should end in a period
(godot)
453-453: Comment should end in a period
(godot)
484-484: Comment should end in a period
(godot)
[medium] 316-316: G301: Expect directory permissions to be 0750 or less
(gosec)
[medium] 329-329: G302: Expect file permissions to be 0600 or less
(gosec)
[medium] 339-339: G304: Potential file inclusion via variable
(gosec)
[medium] 348-348: G302: Expect file permissions to be 0600 or less
(gosec)
[medium] 356-356: G304: Potential file inclusion via variable
(gosec)
[medium] 372-372: G302: Expect file permissions to be 0600 or less
(gosec)
[medium] 390-390: G107: Potential HTTP request made with variable url
(gosec)
342-342: if err != nil && os.IsExist(err) has complex nested blocks (complexity: 5)
(nestif)
454-454: getSolcURL - result 1 (error) is always nil
(unparam)
307-307: calculated cyclomatic complexity for function getSolcBinary is 20, max is 10
(cyclop)
327-327: shadow: declaration of "err" shadows declaration at line 309
(govet)
347-347: shadow: declaration of "err" shadows declaration at line 309
(govet)
371-371: shadow: declaration of "err" shadows declaration at line 309
(govet)
🪛 GitHub Check: Lint (tools)
tools/abigen/internal/generate_test.go
[failure] 143-143:
Error return value of os.RemoveAll is not checked (errcheck)
[failure] 151-151:
Error return value of os.Setenv is not checked (errcheck)
[failure] 152-152:
Error return value of os.Setenv is not checked (errcheck)
tools/abigen/internal/generate.go
[failure] 191-191:
Error return value of runFile.Close is not checked (errcheck)
[failure] 366-366:
Error return value of lock.Close is not checked (errcheck)
[failure] 394-394:
Error return value of resp.Body.Close is not checked (errcheck)
[failure] 405-405:
Error return value of tmpFile.Close is not checked (errcheck)
[failure] 426-426:
Comment should end in a period (godot)
[failure] 453-453:
Comment should end in a period (godot)
[failure] 484-484:
Comment should end in a period (godot)
⏰ Context from checks skipped due to timeout of 90000ms (17)
- GitHub Check: Go Coverage (1.22.x, services/scribe)
- GitHub Check: Go Generate (Module Changes) (tools)
- GitHub Check: Go Coverage (1.22.x, services/rfq)
- GitHub Check: Go Generate (Module Changes) (services/scribe)
- GitHub Check: Go Coverage (1.22.x, services/explorer)
- GitHub Check: Go Generate (Module Changes) (services/rfq)
- GitHub Check: Go Coverage (1.22.x, services/cctp-relayer)
- GitHub Check: Go Generate (Module Changes) (services/explorer)
- GitHub Check: Go Coverage (1.22.x, contrib/promexporter)
- GitHub Check: Go Generate (Module Changes) (services/cctp-relayer)
- GitHub Check: Go Coverage (1.22.x, contrib/opbot)
- GitHub Check: Go Generate (Module Changes) (contrib/promexporter)
- GitHub Check: Go Generate (Module Changes) (contrib/opbot)
- GitHub Check: Go Generate (Module Changes) (agents)
- GitHub Check: SonarCloud
- GitHub Check: Build (1.22.x)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (1)
tools/abigen/internal/export_test.go (1)
23-26: Good addition for test coverageExporting the
getSolcBinaryfunction for testing is straightforward and well-justified. The doc comment concisely explains its purpose, enhancing clarity for test code.
| // TestCompileSolidityWithoutDocker tests that our fallback mechanism works end-to-end | ||
| func TestCompileSolidityWithoutDocker(t *testing.T) { | ||
| // Skip in CI to avoid unnecessary downloads | ||
| if os.Getenv("CI") != "" { | ||
| t.Skip("Skipping in CI environment") | ||
| } | ||
|
|
||
| // Create a temporary Solidity file | ||
| content := ` | ||
| // SPDX-License-Identifier: MIT | ||
| pragma solidity ^0.8.4; | ||
|
|
||
| contract SimpleStorage { | ||
| uint256 private value; | ||
|
|
||
| function setValue(uint256 _value) public { | ||
| value = _value; | ||
| } | ||
|
|
||
| function getValue() public view returns (uint256) { | ||
| return value; | ||
| } | ||
| } | ||
| ` | ||
|
|
||
| tempDir, err := os.MkdirTemp("", "solc-test-*") | ||
| Nil(t, err) | ||
| defer os.RemoveAll(tempDir) | ||
|
|
||
| tempFile := filepath.Join(tempDir, "SimpleStorage.sol") | ||
| err = os.WriteFile(tempFile, []byte(content), 0644) | ||
| Nil(t, err) | ||
|
|
||
| // Force docker to fail by setting an invalid environment variable | ||
| originalPath := os.Getenv("PATH") | ||
| os.Setenv("DOCKER_HOST", "unix:///non-existent-path") | ||
| defer os.Setenv("PATH", originalPath) | ||
|
|
||
| // Compile using our fallback mechanism | ||
| contracts, err := internal.CompileSolidity("0.8.4", tempFile, 200, nil) | ||
| Nil(t, err) | ||
|
|
||
| // Verify that we got the contract | ||
| NotNil(t, contracts) | ||
| NotEqual(t, 0, len(contracts)) | ||
|
|
||
| // Check that the compiled contract actually exists | ||
| var found bool | ||
| for name, contract := range contracts { | ||
| if strings.Contains(name, "SimpleStorage") { | ||
| found = true | ||
| NotEmpty(t, contract.Code) | ||
| NotNil(t, contract.Info.AbiDefinition) | ||
| } | ||
| } | ||
|
|
||
| True(t, found, "SimpleStorage contract not found in compiled output") | ||
| } |
There was a problem hiding this comment.
Handle potential errors from environment calls
Lines 143, 151, and 152 do not check the return values of os.RemoveAll and os.Setenv. Ignoring these errors can cause subtle test-side effects or leave behind stale resources.
- defer os.RemoveAll(tempDir)
+ defer func() {
+ if err := os.RemoveAll(tempDir); err != nil {
+ t.Logf("could not remove tempDir: %v", err)
+ }
+ }()
- os.Setenv("DOCKER_HOST", "unix:///non-existent-path")
+ if err := os.Setenv("DOCKER_HOST", "unix:///non-existent-path"); err != nil {
+ t.Fatalf("failed to set DOCKER_HOST: %v", err)
+ }Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 golangci-lint (1.62.2)
143-143: Error return value of os.RemoveAll is not checked
(errcheck)
151-151: Error return value of os.Setenv is not checked
(errcheck)
152-152: Error return value of os.Setenv is not checked
(errcheck)
[medium] 146-146: G306: Expect WriteFile permissions to be 0600 or less
(gosec)
🪛 GitHub Check: Lint (tools)
[failure] 143-143:
Error return value of os.RemoveAll is not checked (errcheck)
[failure] 151-151:
Error return value of os.Setenv is not checked (errcheck)
[failure] 152-152:
Error return value of os.Setenv is not checked (errcheck)
|
|
||
| // Check if we already have the binary | ||
| if _, err := os.Stat(solcPath); err == nil { | ||
| // Binary exists, make sure it's executable | ||
| if err := os.Chmod(solcPath, 0755); err != nil { | ||
| return "", fmt.Errorf("could not set executable bit on cached solc: %w", err) | ||
| } | ||
| return solcPath, nil | ||
| } | ||
|
|
||
| // Use a file lock to prevent concurrent downloads | ||
| lockFile := filepath.Join(cacheDir, fmt.Sprintf("solc-%s.lock", normalizedVersion)) | ||
|
|
||
| // Create lock file with exclusive lock | ||
| lock, err := os.OpenFile(lockFile, os.O_CREATE|os.O_WRONLY|os.O_EXCL, 0600) | ||
|
|
||
| // If lock file exists, another process is downloading, wait a bit and retry | ||
| if err != nil && os.IsExist(err) { | ||
| // Another process is downloading, wait a bit and retry checking for the binary | ||
| time.Sleep(500 * time.Millisecond) | ||
|
|
||
| // Check again if the binary exists now | ||
| if _, err := os.Stat(solcPath); err == nil { | ||
| if err := os.Chmod(solcPath, 0755); err != nil { | ||
| return "", fmt.Errorf("could not set executable bit on cached solc: %w", err) | ||
| } | ||
| return solcPath, nil | ||
| } | ||
|
|
||
| // Binary still doesn't exist, something might be wrong with the other process | ||
| // Try to acquire the lock again | ||
| lock, err = os.OpenFile(lockFile, os.O_CREATE|os.O_WRONLY|os.O_EXCL, 0600) | ||
| if err != nil { | ||
| return "", fmt.Errorf("could not acquire lock for downloading solc: %w", err) | ||
| } | ||
| } else if err != nil { | ||
| return "", fmt.Errorf("could not create lock file: %w", err) | ||
| } | ||
|
|
||
| // Make sure to clean up the lock file when we're done | ||
| defer func() { | ||
| lock.Close() | ||
| _ = os.Remove(lockFile) | ||
| }() | ||
|
|
||
| // Double-check if binary exists now (might have been downloaded by another process) | ||
| if _, err := os.Stat(solcPath); err == nil { | ||
| if err := os.Chmod(solcPath, 0755); err != nil { | ||
| return "", fmt.Errorf("could not set executable bit on cached solc: %w", err) | ||
| } | ||
| return solcPath, nil | ||
| } | ||
|
|
||
| // Determine platform and architecture | ||
| platform := determinePlatform() | ||
|
|
||
| // URL to download the binary from | ||
| downloadURL, err := getSolcURL(normalizedVersion, platform) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
|
|
||
| fmt.Fprintf(os.Stderr, "Downloading solc %s for %s from %s\n", normalizedVersion, platform, downloadURL) | ||
|
|
||
| // Download the binary | ||
| resp, err := http.Get(downloadURL) | ||
| if err != nil { | ||
| return "", fmt.Errorf("could not download solc: %w", err) | ||
| } | ||
| defer resp.Body.Close() | ||
|
|
||
| if resp.StatusCode != http.StatusOK { | ||
| return "", fmt.Errorf("could not download solc: HTTP %d", resp.StatusCode) | ||
| } | ||
|
|
||
| // Create a temporary file to download to | ||
| tmpFile, err := os.CreateTemp(cacheDir, "solc-download-*") | ||
| if err != nil { | ||
| return "", fmt.Errorf("could not create temporary file: %w", err) | ||
| } | ||
| defer tmpFile.Close() | ||
|
|
||
| // Copy the response to the temporary file | ||
| _, err = io.Copy(tmpFile, resp.Body) | ||
| if err != nil { | ||
| return "", fmt.Errorf("could not write downloaded solc to disk: %w", err) | ||
| } | ||
|
|
||
| // Make the file executable | ||
| if err := os.Chmod(tmpFile.Name(), 0755); err != nil { | ||
| return "", fmt.Errorf("could not set executable bit on downloaded solc: %w", err) | ||
| } | ||
|
|
||
| // Move the temporary file to the final location | ||
| if err := os.Rename(tmpFile.Name(), solcPath); err != nil { | ||
| return "", fmt.Errorf("could not move downloaded solc to final location: %w", err) | ||
| } | ||
|
|
||
| return solcPath, nil | ||
| } |
There was a problem hiding this comment.
Address lock/file-handling issues and reduce complexity
- The return values of
lock.Close()(line 366),resp.Body.Close()(line 394), andtmpFile.Close()(line 405) are not handled. - Using
0755for binaries may breach stricter security guidelines recommending0600or0700. - The function’s cyclomatic complexity of 20 suggests refactoring to improve maintainability.
// Example approach for closing lock:
defer func() {
if lerr := lock.Close(); lerr != nil {
fmt.Fprintf(os.Stderr, "warning: could not close solc lock file: %v\n", lerr)
}
}()Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 golangci-lint (1.62.2)
366-366: Error return value of lock.Close is not checked
(errcheck)
394-394: Error return value of resp.Body.Close is not checked
(errcheck)
405-405: Error return value of tmpFile.Close is not checked
(errcheck)
[medium] 316-316: G301: Expect directory permissions to be 0750 or less
(gosec)
[medium] 329-329: G302: Expect file permissions to be 0600 or less
(gosec)
[medium] 339-339: G304: Potential file inclusion via variable
(gosec)
[medium] 348-348: G302: Expect file permissions to be 0600 or less
(gosec)
[medium] 356-356: G304: Potential file inclusion via variable
(gosec)
[medium] 372-372: G302: Expect file permissions to be 0600 or less
(gosec)
[medium] 390-390: G107: Potential HTTP request made with variable url
(gosec)
342-342: if err != nil && os.IsExist(err) has complex nested blocks (complexity: 5)
(nestif)
307-307: calculated cyclomatic complexity for function getSolcBinary is 20, max is 10
(cyclop)
327-327: shadow: declaration of "err" shadows declaration at line 309
(govet)
347-347: shadow: declaration of "err" shadows declaration at line 309
(govet)
371-371: shadow: declaration of "err" shadows declaration at line 309
(govet)
🪛 GitHub Check: Lint (tools)
[failure] 366-366:
Error return value of lock.Close is not checked (errcheck)
[failure] 394-394:
Error return value of resp.Body.Close is not checked (errcheck)
[failure] 405-405:
Error return value of tmpFile.Close is not checked (errcheck)
The URL for downloading solc binaries for Apple Silicon was incorrect. Fix the URL to use 'solc-macos' instead of 'solc-macos-aarch64'. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tools/abigen/internal/generate.go (1)
307-424:⚠️ Potential issueAddress lock/file-handling issues and reduce complexity
The
getSolcBinaryfunction has several issues:
- The return values of
lock.Close()(line 366),resp.Body.Close()(line 394), andtmpFile.Close()(line 405) are not handled.- Using
0755for directories and binaries may breach stricter security guidelines recommending0750for directories and0700for binaries.- The function's cyclomatic complexity of 20 exceeds the maximum recommended value of 10.
- Multiple instances of variable shadowing with
errat lines 327, 347, and 371.Consider refactoring this function to address these issues:
func getSolcBinary(version string) (string, error) { // Get user's home directory for cache placement homeDir, err := os.UserHomeDir() if err != nil { return "", fmt.Errorf("could not determine user's home directory: %w", err) } // Create cache directory if it doesn't exist cacheDir := filepath.Join(homeDir, ".cache", "solc") - if err := os.MkdirAll(cacheDir, 0755); err != nil { + if err := os.MkdirAll(cacheDir, 0750); err != nil { return "", fmt.Errorf("could not create cache directory: %w", err) } // Normalize version string (remove 'v' prefix if present) normalizedVersion := strings.TrimPrefix(version, "v") // Path to the cached binary solcPath := filepath.Join(cacheDir, fmt.Sprintf("solc-%s", normalizedVersion)) // Check if we already have the binary - if _, err := os.Stat(solcPath); err == nil { + if _, statErr := os.Stat(solcPath); statErr == nil { // Binary exists, make sure it's executable - if err := os.Chmod(solcPath, 0755); err != nil { + if err := os.Chmod(solcPath, 0700); err != nil { return "", fmt.Errorf("could not set executable bit on cached solc: %w", err) } return solcPath, nil } // ... rest of the function with similar fixes ... // Make sure to clean up the lock file when we're done defer func() { - lock.Close() + if closeErr := lock.Close(); closeErr != nil { + fmt.Fprintf(os.Stderr, "warning: could not close solc lock file: %v\n", closeErr) + } _ = os.Remove(lockFile) }() // ... rest of the function with similar fixes ... // Download the binary resp, err := http.Get(downloadURL) if err != nil { return "", fmt.Errorf("could not download solc: %w", err) } - defer resp.Body.Close() + defer func() { + if closeErr := resp.Body.Close(); closeErr != nil { + fmt.Fprintf(os.Stderr, "warning: could not close response body: %v\n", closeErr) + } + }() // ... rest of the function with similar fixes ... // Create a temporary file to download to tmpFile, err := os.CreateTemp(cacheDir, "solc-download-*") if err != nil { return "", fmt.Errorf("could not create temporary file: %w", err) } - defer tmpFile.Close() + defer func() { + if closeErr := tmpFile.Close(); closeErr != nil { + fmt.Fprintf(os.Stderr, "warning: could not close temporary file: %v\n", closeErr) + } + }()🧰 Tools
🪛 golangci-lint (1.62.2)
366-366: Error return value of
lock.Closeis not checked(errcheck)
394-394: Error return value of
resp.Body.Closeis not checked(errcheck)
405-405: Error return value of
tmpFile.Closeis not checked(errcheck)
[medium] 316-316: G301: Expect directory permissions to be 0750 or less
(gosec)
[medium] 329-329: G302: Expect file permissions to be 0600 or less
(gosec)
[medium] 339-339: G304: Potential file inclusion via variable
(gosec)
[medium] 348-348: G302: Expect file permissions to be 0600 or less
(gosec)
[medium] 356-356: G304: Potential file inclusion via variable
(gosec)
[medium] 372-372: G302: Expect file permissions to be 0600 or less
(gosec)
[medium] 390-390: G107: Potential HTTP request made with variable url
(gosec)
342-342:
if err != nil && os.IsExist(err)has complex nested blocks (complexity: 5)(nestif)
307-307: calculated cyclomatic complexity for function getSolcBinary is 20, max is 10
(cyclop)
327-327: shadow: declaration of "err" shadows declaration at line 309
(govet)
347-347: shadow: declaration of "err" shadows declaration at line 309
(govet)
371-371: shadow: declaration of "err" shadows declaration at line 309
(govet)
🧹 Nitpick comments (3)
tools/abigen/internal/generate.go (3)
426-451: End comment with a periodThe function comment should end with a period for consistency.
-// determinePlatform returns the platform-specific string for downloading solc +// determinePlatform returns the platform-specific string for downloading solc.🧰 Tools
🪛 golangci-lint (1.62.2)
426-426: Comment should end in a period
(godot)
453-483: Function returns unused errorThe
getSolcURLfunction always returns nil for the error result but still includes it in the signature. Consider removing the error return or implementing proper error handling where needed.-func getSolcURL(version, platform string) (string, error) { +func getSolcURL(version, platform string) string { // Base URL for solc binaries baseURL := "https://github.com/ethereum/solidity/releases/download/v" + version // For apple silicon (arm64), we need to use a slightly different approach. // The apple silicon binaries are only available starting with more recent solc versions if platform == "macosx-aarch64" { // For Apple Silicon, check if we're using a version that has native binaries if isSemverGreaterOrEqual(version, "0.8.5") { // The correct filename is "solc-macos" with no architecture suffix - return fmt.Sprintf("%s/solc-macos", baseURL), nil + return fmt.Sprintf("%s/solc-macos", baseURL) } // Fall back to x86_64 version that will run using Rosetta 2 platform = "macosx-amd64" } // Handle specific platform formatting switch platform { case "macosx-amd64": - return fmt.Sprintf("%s/solc-macos", baseURL), nil + return fmt.Sprintf("%s/solc-macos", baseURL) case "linux-amd64": - return fmt.Sprintf("%s/solc-static-linux", baseURL), nil + return fmt.Sprintf("%s/solc-static-linux", baseURL) case "windows-amd64": - return fmt.Sprintf("%s/solc-windows.exe", baseURL), nil + return fmt.Sprintf("%s/solc-windows.exe", baseURL) default: // Default to Linux AMD64 as fallback - return fmt.Sprintf("%s/solc-static-linux", baseURL), nil + return fmt.Sprintf("%s/solc-static-linux", baseURL) } }Also, the function comment should end with a period for consistency.
-// getSolcURL returns the URL to download the solc binary for the given version and platform +// getSolcURL returns the URL to download the solc binary for the given version and platform.🧰 Tools
🪛 golangci-lint (1.62.2)
453-453: Comment should end in a period
(godot)
454-454: getSolcURL - result 1 (error) is always nil
(unparam)
485-522: End comment with a periodThe function comment should end with a period for consistency.
-// isSemverGreaterOrEqual checks if version a is greater than or equal to version b +// isSemverGreaterOrEqual checks if version a is greater than or equal to version b.🧰 Tools
🪛 golangci-lint (1.62.2)
485-485: Comment should end in a period
(godot)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tools/abigen/internal/generate.go(4 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
tools/abigen/internal/generate.go
191-191: Error return value of runFile.Close is not checked
(errcheck)
201-201: shadow: declaration of "err" shadows declaration at line 131
(govet)
366-366: Error return value of lock.Close is not checked
(errcheck)
394-394: Error return value of resp.Body.Close is not checked
(errcheck)
405-405: Error return value of tmpFile.Close is not checked
(errcheck)
426-426: Comment should end in a period
(godot)
453-453: Comment should end in a period
(godot)
485-485: Comment should end in a period
(godot)
[medium] 316-316: G301: Expect directory permissions to be 0750 or less
(gosec)
[medium] 329-329: G302: Expect file permissions to be 0600 or less
(gosec)
[medium] 339-339: G304: Potential file inclusion via variable
(gosec)
[medium] 348-348: G302: Expect file permissions to be 0600 or less
(gosec)
[medium] 356-356: G304: Potential file inclusion via variable
(gosec)
[medium] 372-372: G302: Expect file permissions to be 0600 or less
(gosec)
[medium] 390-390: G107: Potential HTTP request made with variable url
(gosec)
342-342: if err != nil && os.IsExist(err) has complex nested blocks (complexity: 5)
(nestif)
454-454: getSolcURL - result 1 (error) is always nil
(unparam)
307-307: calculated cyclomatic complexity for function getSolcBinary is 20, max is 10
(cyclop)
327-327: shadow: declaration of "err" shadows declaration at line 309
(govet)
347-347: shadow: declaration of "err" shadows declaration at line 309
(govet)
371-371: shadow: declaration of "err" shadows declaration at line 309
(govet)
⏰ Context from checks skipped due to timeout of 90000ms (19)
- GitHub Check: Go Coverage (1.22.x, tools)
- GitHub Check: Go Coverage (1.22.x, services/scribe)
- GitHub Check: Go Generate (Module Changes) (tools)
- GitHub Check: Go Coverage (1.22.x, services/rfq)
- GitHub Check: Go Generate (Module Changes) (services/scribe)
- GitHub Check: Go Coverage (1.22.x, services/explorer)
- GitHub Check: Go Generate (Module Changes) (services/rfq)
- GitHub Check: Go Coverage (1.22.x, services/cctp-relayer)
- GitHub Check: Go Generate (Module Changes) (services/explorer)
- GitHub Check: Go Coverage (1.22.x, contrib/promexporter)
- GitHub Check: Go Generate (Module Changes) (services/cctp-relayer)
- GitHub Check: Go Coverage (1.22.x, contrib/opbot)
- GitHub Check: Go Generate (Module Changes) (contrib/promexporter)
- GitHub Check: Go Generate (Module Changes) (contrib/opbot)
- GitHub Check: Go Generate (Module Changes) (agents)
- GitHub Check: Lint (tools)
- GitHub Check: Build (1.22.x)
- GitHub Check: SonarCloud
- GitHub Check: Cloudflare Pages
🔇 Additional comments (3)
tools/abigen/internal/generate.go (3)
191-191: Check return value of runFile.Close()The return value of
runFile.Close()is not checked, which could hide errors during file closing. Consider handling the error or explicitly ignoring it with an underscore.- defer runFile.Close() + defer func() { + if err := runFile.Close(); err != nil { + fmt.Fprintf(os.Stderr, "Warning: could not close run file: %v\n", err) + } + }()🧰 Tools
🪛 golangci-lint (1.62.2)
191-191: Error return value of
runFile.Closeis not checked(errcheck)
201-201: Variable 'err' shadows outer declarationThe declaration of
erron this line shadows the outer declaration at line 131, which could lead to unexpected behavior.- contract, err := compiler.ParseCombinedJSON(stdout.Bytes(), string(solContents), version, version, strings.Join(args, " ")) + contract, parseErr := compiler.ParseCombinedJSON(stdout.Bytes(), string(solContents), version, version, strings.Join(args, " ")) + if parseErr != nil { + return nil, fmt.Errorf("could not parse json: %w", parseErr) + } - if err != nil { - return nil, fmt.Errorf("could not parse json: %w", err) - }🧰 Tools
🪛 golangci-lint (1.62.2)
201-201: shadow: declaration of "err" shadows declaration at line 131
(govet)
128-238: This looks good with some minor improvementsThe fallback mechanism for Apple Silicon Macs looks well-implemented with clear error messaging and proper handling of both Docker and direct binary approaches. The change aligns with the PR objectives to improve user experience on Apple Silicon Macs.
Consider adding some logging to indicate whether Docker-based compilation or direct binary execution is being used, which would help with debugging.
🧰 Tools
🪛 golangci-lint (1.62.2)
191-191: Error return value of
runFile.Closeis not checked(errcheck)
201-201: shadow: declaration of "err" shadows declaration at line 131
(govet)
| default: | ||
| // Default to Linux AMD64 as fallback | ||
| return fmt.Sprintf("%s/solc-static-linux", baseURL), nil |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Potential URL construction issue for Windows binary
The URL for Windows binaries might need adjustment depending on the Solidity version. For newer Solidity versions (≥ 0.7.2), the binary is named solc-windows-amd64-v{version}+commit.{commit}.exe.
🌐 Web query:
How are solc Windows binary files named in GitHub releases?
💡 Result:
Solidity's Windows binaries on GitHub releases follow a consistent naming convention. The primary pattern for recent versions (0.8.0 and later) is solc-windows.exe, with the version number included in the release tag. For example:
- Version 0.8.0:
https://github.com/ethereum/solidity/releases/download/v0.8.0/solc-windows.exe[2]
This pattern applies to modern releases, though older versions (pre-0.6.2) used different structures, such as solc_mac for macOS in third-party repositories[2]. The current naming convention avoids platform ambiguity by explicitly including windows in the filename.
For verification, checksums and alternative download paths are available via the solc-bin repository[3][4], which serves as the canonical source for compiler binaries.
Citations:
- 1: https://docs.soliditylang.org/en/latest/installing-solidity.html
- 2: https://solcbin.github.io
- 3: https://github.com/ethereum/solc-bin
- 4: https://github.com/crytic/solc-select?files=1
- 5: https://www.npmjs.com/package/solc
- 6: https://github.com/ethereum/solidity/blob/develop/ReleaseChecklist.md
- 7: https://github.com/blockscout/solc-bin
- 8: Overhaul binary release process ethereum/solidity#9258
- 9: https://github.com/ethereum/solidity/releases
- 10: Solidity
0.8.18binaries built with too new version ofGLIBCfor support of Ubuntu 20.04.5 LTS on WSL ethereum/solidity#13921
Action Required: Update Windows Binary URL Construction
After checking the latest GitHub releases for Solidity, it’s clear that modern Windows binaries (from version 0.8.0 onward) are named simply solc-windows.exe (with the version included only in the release tag), rather than following the older pattern of solc-windows-amd64-v{version}+commit.{commit}.exe. If your tool is meant to support multiple Solidity versions, especially newer ones, please review and adjust the URL construction logic accordingly. Consider adding conditional logic to handle both the legacy and modern naming conventions if necessary.
- File:
tools/abigen/internal/generate.go - Action: Ensure that the URL for Windows binaries is correctly generated according to the current naming convention (
solc-windows.exe) for modern releases, and review legacy support if needed.
Regenerated contracts in: - agents/contracts - services/rfq/contracts Fixed the URL format for solc binary download on Apple Silicon 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
…mode When using the solc binary fallback mechanism, we now normalize the contract names and metadata paths to match the Docker-based format, which has 'solidity/' prefix. This helps minimize diffs when switching between Docker and direct binary modes. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
trajan0x
requested review from
dwasse,
aureliusbtc and
ChiTimesChi
as code owners
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
agents/contracts/test/receiptharness/receiptharness.abigen.go (1)
1-1004: 💡 Verification agent🧩 Analysis chain
Verify that all contract bytecode has been updated consistently across the codebase.
While the bytecode changes in this file appear correct, it's important to ensure that all related contract files have been updated consistently to maintain compatibility.
🏁 Script executed:
#!/bin/bash # Description: Check that all .abigen.go files have updated bytecode # Search for any other .abigen.go files to ensure they're also updated echo "Finding all abigen.go files in the repository:" find . -name "*.abigen.go" | sort # Count the number of files that contain Bin fields with updated values echo "Updated .abigen.go files (containing new bytecode):" find . -name "*.abigen.go" | xargs grep -l "Bin: \"0x" | wc -lLength of output: 317
Bytecode Consistency Check – Updates Required
- The updated bytecode in this file appears correct.
- However, a repository-wide scan returned 0 occurrences of updated "Bin:" fields in all *.abigen.go files.
- Please verify that the contract bindings have been regenerated consistently so that every related file reflects the new bytecode.
♻️ Duplicate comments (2)
tools/abigen/internal/generate.go (2)
348-348: 🛠️ Refactor suggestionRefactor getSolcBinary to reduce complexity
The
getSolcBinaryfunction has a high cyclomatic complexity (20, max is 10).Consider breaking it down into smaller, focused functions:
func getSolcBinary(version string) (string, error) { + return getSolcBinaryWithRetry(version, 1) +} + +func getSolcBinaryWithRetry(version string, attempt int) (string, error) { + if attempt > 3 { + return "", fmt.Errorf("exceeded maximum retry attempts for getting solc binary") + } + // Get user's home directory for cache placement homeDir, err := os.UserHomeDir() if err != nil { return "", fmt.Errorf("could not determine user's home directory: %w", err) } // Create cache directory if it doesn't exist cacheDir := filepath.Join(homeDir, ".cache", "solc") if err := os.MkdirAll(cacheDir, 0750); err != nil { return "", fmt.Errorf("could not create cache directory: %w", err) } // Normalize version string (remove 'v' prefix if present) normalizedVersion := strings.TrimPrefix(version, "v") // Path to the cached binary solcPath := filepath.Join(cacheDir, fmt.Sprintf("solc-%s", normalizedVersion)) // Check if we already have the binary if cachedPath, err := checkExistingBinary(solcPath); err == nil { + return cachedPath, nil + } + + // Use a file lock to prevent concurrent downloads + lockFile := filepath.Join(cacheDir, fmt.Sprintf("solc-%s.lock", normalizedVersion)) + + // Try to acquire lock + lock, err := acquireLock(lockFile) + if err != nil { + if os.IsExist(err) { + // Another process is downloading, wait and retry + time.Sleep(500 * time.Millisecond) + return getSolcBinaryWithRetry(version, attempt+1) + } + return "", err + } + + // Make sure to clean up the lock file when we're done + defer releaseLock(lock, lockFile) + + // Double-check if binary exists now (might have been downloaded by another process) + if cachedPath, err := checkExistingBinary(solcPath); err == nil { + return cachedPath, nil + } + + // Download the binary + return downloadSolcBinary(normalizedVersion, solcPath, cacheDir) +} + +func checkExistingBinary(solcPath string) (string, error) { + if _, err := os.Stat(solcPath); err == nil { + // Binary exists, make sure it's executable + if err := os.Chmod(solcPath, 0755); err != nil { + return "", fmt.Errorf("could not set executable bit on cached solc: %w", err) + } + return solcPath, nil + } + return "", fmt.Errorf("binary does not exist") +} + +func acquireLock(lockFile string) (*os.File, error) { + // Create lock file with exclusive lock + lock, err := os.OpenFile(lockFile, os.O_CREATE|os.O_WRONLY|os.O_EXCL, 0600) + if err != nil { + return nil, err + } + return lock, nil +} + +func releaseLock(lock *os.File, lockFile string) { + if err := lock.Close(); err != nil { + fmt.Fprintf(os.Stderr, "warning: failed to close lock file: %v\n", err) + } + if err := os.Remove(lockFile); err != nil { + fmt.Fprintf(os.Stderr, "warning: failed to remove lock file: %v\n", err) + } +} + +func downloadSolcBinary(normalizedVersion, solcPath, cacheDir string) (string, error) {🧰 Tools
🪛 golangci-lint (1.62.2)
348-348: calculated cyclomatic complexity for function getSolcBinary is 20, max is 10
(cyclop)
346-465:⚠️ Potential issueAddress error handling and file management issues
Several error return values are not being checked, and there are potential security concerns with file permissions.
Apply these fixes:
- defer func() { - lock.Close() - _ = os.Remove(lockFile) - }() + defer func() { + if closeErr := lock.Close(); closeErr != nil { + fmt.Fprintf(os.Stderr, "warning: failed to close lock file: %v\n", closeErr) + } + if rmErr := os.Remove(lockFile); rmErr != nil { + fmt.Fprintf(os.Stderr, "warning: failed to remove lock file: %v\n", rmErr) + } + }()- if err := os.MkdirAll(cacheDir, 0755); err != nil { + if err := os.MkdirAll(cacheDir, 0750); err != nil {- defer resp.Body.Close() + defer func() { + if closeErr := resp.Body.Close(); closeErr != nil { + fmt.Fprintf(os.Stderr, "warning: failed to close response body: %v\n", closeErr) + } + }()- defer tmpFile.Close() + defer func() { + if closeErr := tmpFile.Close(); closeErr != nil { + fmt.Fprintf(os.Stderr, "warning: failed to close temporary file: %v\n", closeErr) + } + }()🧰 Tools
🪛 golangci-lint (1.62.2)
407-407: Error return value of
lock.Closeis not checked(errcheck)
435-435: Error return value of
resp.Body.Closeis not checked(errcheck)
446-446: Error return value of
tmpFile.Closeis not checked(errcheck)
[medium] 357-357: G301: Expect directory permissions to be 0750 or less
(gosec)
[medium] 370-370: G302: Expect file permissions to be 0600 or less
(gosec)
[medium] 380-380: G304: Potential file inclusion via variable
(gosec)
[medium] 389-389: G302: Expect file permissions to be 0600 or less
(gosec)
[medium] 397-397: G304: Potential file inclusion via variable
(gosec)
[medium] 413-413: G302: Expect file permissions to be 0600 or less
(gosec)
[medium] 431-431: G107: Potential HTTP request made with variable url
(gosec)
383-383:
if err != nil && os.IsExist(err)has complex nested blocks (complexity: 5)(nestif)
348-348: calculated cyclomatic complexity for function getSolcBinary is 20, max is 10
(cyclop)
368-368: shadow: declaration of "err" shadows declaration at line 350
(govet)
388-388: shadow: declaration of "err" shadows declaration at line 350
(govet)
412-412: shadow: declaration of "err" shadows declaration at line 350
(govet)
🪛 GitHub Check: codecov/patch
[warning] 352-353: tools/abigen/internal/generate.go#L352-L353
Added lines #L352 - L353 were not covered by tests
[warning] 358-359: tools/abigen/internal/generate.go#L358-L359
Added lines #L358 - L359 were not covered by tests
[warning] 369-373: tools/abigen/internal/generate.go#L369-L373
Added lines #L369 - L373 were not covered by tests
[warning] 384-392: tools/abigen/internal/generate.go#L384-L392
Added lines #L384 - L392 were not covered by tests
[warning] 397-400: tools/abigen/internal/generate.go#L397-L400
Added lines #L397 - L400 were not covered by tests
[warning] 402-403: tools/abigen/internal/generate.go#L402-L403
Added lines #L402 - L403 were not covered by tests
[warning] 413-416: tools/abigen/internal/generate.go#L413-L416
Added lines #L413 - L416 were not covered by tests
[warning] 425-426: tools/abigen/internal/generate.go#L425-L426
Added lines #L425 - L426 were not covered by tests
[warning] 433-434: tools/abigen/internal/generate.go#L433-L434
Added lines #L433 - L434 were not covered by tests
[warning] 438-439: tools/abigen/internal/generate.go#L438-L439
Added lines #L438 - L439 were not covered by tests
[warning] 444-445: tools/abigen/internal/generate.go#L444-L445
Added lines #L444 - L445 were not covered by tests
[warning] 451-452: tools/abigen/internal/generate.go#L451-L452
Added lines #L451 - L452 were not covered by tests
[warning] 456-457: tools/abigen/internal/generate.go#L456-L457
Added lines #L456 - L457 were not covered by tests
[warning] 461-462: tools/abigen/internal/generate.go#L461-L462
Added lines #L461 - L462 were not covered by tests🪛 GitHub Check: Lint (tools)
[failure] 407-407:
Error return value oflock.Closeis not checked (errcheck)
[failure] 435-435:
Error return value ofresp.Body.Closeis not checked (errcheck)
[failure] 446-446:
Error return value oftmpFile.Closeis not checked (errcheck)
🧹 Nitpick comments (7)
agents/contracts/inbox/itransactor_generated.go (1)
55-62: Note the removal of ownership management methods.The
AcceptOwnershipmethod has been removed from this interface, indicating a significant change in the contract's ownership management pattern. This is consistent with similar removals across other interfaces in this PR and suggests a simplification or refactoring of the ownership transfer mechanism.agents/contracts/lightinbox/itransactor_generated.go (1)
35-46: Code changes maintain cross-implementation consistency and support PR objectives.The parameter type changes from
uint8to*big.IntforstateIndexand updated contract method identifiers (0x0b6b985c, 0x62389709, 0x0db27e77) ensure that both regular and light inbox implementations remain synchronized. These interface adjustments likely support the PR's primary objective of implementing solc binary fallback for Apple Silicon Macs by ensuring consistent contract ABI handling across different compilation methods.While the connection to the solc binary fallback mechanism isn't immediately obvious from these interface changes alone, they're an essential part of ensuring compatibility when contracts are compiled using different methods (Docker vs. direct binary).
tools/abigen/internal/generate.go (5)
431-447: Add timeout to HTTP requestThe HTTP request to download the solc binary doesn't have a timeout, which could lead to hanging connections.
Consider adding a timeout to the HTTP request:
- resp, err := http.Get(downloadURL) + client := &http.Client{ + Timeout: 5 * time.Minute, + } + resp, err := client.Get(downloadURL)🧰 Tools
🪛 golangci-lint (1.62.2)
435-435: Error return value of
resp.Body.Closeis not checked(errcheck)
446-446: Error return value of
tmpFile.Closeis not checked(errcheck)
[medium] 431-431: G107: Potential HTTP request made with variable url
(gosec)
🪛 GitHub Check: codecov/patch
[warning] 433-434: tools/abigen/internal/generate.go#L433-L434
Added lines #L433 - L434 were not covered by tests
[warning] 438-439: tools/abigen/internal/generate.go#L438-L439
Added lines #L438 - L439 were not covered by tests
[warning] 444-445: tools/abigen/internal/generate.go#L444-L445
Added lines #L444 - L445 were not covered by tests🪛 GitHub Check: Lint (tools)
[failure] 435-435:
Error return value ofresp.Body.Closeis not checked (errcheck)
[failure] 446-446:
Error return value oftmpFile.Closeis not checked (errcheck)
467-492: Improve error handling in determinePlatform function.The function falls back to Linux if the platform is not recognized, but doesn't warn the user about this fallback.
Consider logging a warning when falling back to a default platform:
case runtime.GOOS == "windows": return "windows-amd64" default: // Default to Linux, which is most commonly supported + fmt.Fprintf(os.Stderr, "warning: unsupported platform %s/%s, falling back to linux-amd64\n", runtime.GOOS, runtime.GOARCH) return "linux-amd64"🧰 Tools
🪛 golangci-lint (1.62.2)
467-467: Comment should end in a period
(godot)
🪛 GitHub Check: codecov/patch
[warning] 472-476: tools/abigen/internal/generate.go#L472-L476
Added lines #L472 - L476 were not covered by tests
[warning] 480-483: tools/abigen/internal/generate.go#L480-L483
Added lines #L480 - L483 were not covered by tests
[warning] 486-490: tools/abigen/internal/generate.go#L486-L490
Added lines #L486 - L490 were not covered by tests🪛 GitHub Check: Lint (tools)
[failure] 467-467:
Comment should end in a period (godot)
191-191: Add error handling for runFile.Close()The return value of
runFile.Close()is not checked.- defer runFile.Close() + defer func() { + if err := runFile.Close(); err != nil { + fmt.Fprintf(os.Stderr, "warning: failed to close run file: %v\n", err) + } + }()🧰 Tools
🪛 golangci-lint (1.62.2)
191-191: Error return value of
runFile.Closeis not checked(errcheck)
🪛 GitHub Check: Lint (tools)
[failure] 191-191:
Error return value ofrunFile.Closeis not checked (errcheck)
526-563: Add edge case handling to isSemverGreaterOrEqual functionThe function doesn't handle pre-release versions or build metadata (e.g., "1.2.3-alpha" or "1.2.3+build.1").
Consider enhancing the function to handle these cases by removing anything after a dash or plus before comparing versions:
func isSemverGreaterOrEqual(a, b string) bool { + // Remove pre-release versions and build metadata + if idx := strings.Index(a, "-"); idx != -1 { + a = a[:idx] + } + if idx := strings.Index(a, "+"); idx != -1 { + a = a[:idx] + } + if idx := strings.Index(b, "-"); idx != -1 { + b = b[:idx] + } + if idx := strings.Index(b, "+"); idx != -1 { + b = b[:idx] + } aParts := strings.Split(a, ".") bParts := strings.Split(b, ".") // Rest of function unchanged🧰 Tools
🪛 golangci-lint (1.62.2)
526-526: Comment should end in a period
(godot)
🪛 GitHub Check: codecov/patch
[warning] 527-537: tools/abigen/internal/generate.go#L527-L537
Added lines #L527 - L537 were not covered by tests
[warning] 540-547: tools/abigen/internal/generate.go#L540-L547
Added lines #L540 - L547 were not covered by tests
[warning] 550-557: tools/abigen/internal/generate.go#L550-L557
Added lines #L550 - L557 were not covered by tests🪛 GitHub Check: Lint (tools)
[failure] 526-526:
Comment should end in a period (godot)
467-467: Fix comment style for consistencyComments should end with periods per Go style guidelines.
- // determinePlatform returns the platform-specific string for downloading solc + // determinePlatform returns the platform-specific string for downloading solc. - // getSolcURL returns the URL to download the solc binary for the given version and platform + // getSolcURL returns the URL to download the solc binary for the given version and platform. - // isSemverGreaterOrEqual checks if version a is greater than or equal to version b + // isSemverGreaterOrEqual checks if version a is greater than or equal to version b.Also applies to: 494-494, 526-526
🧰 Tools
🪛 golangci-lint (1.62.2)
467-467: Comment should end in a period
(godot)
🪛 GitHub Check: Lint (tools)
[failure] 467-467:
Comment should end in a period (godot)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (40)
agents/contracts/bondingmanager/filterer_generated.go(0 hunks)agents/contracts/bondingmanager/icaller_generated.go(0 hunks)agents/contracts/bondingmanager/itransactor_generated.go(1 hunks)agents/contracts/bondingmanager/mocks/i_bonding_manager.go(1 hunks)agents/contracts/gasoracle/filterer_generated.go(0 hunks)agents/contracts/gasoracle/icaller_generated.go(0 hunks)agents/contracts/gasoracle/itransactor_generated.go(1 hunks)agents/contracts/gasoracle/mocks/i_gas_oracle.go(2 hunks)agents/contracts/inbox/filterer_generated.go(1 hunks)agents/contracts/inbox/icaller_generated.go(0 hunks)agents/contracts/inbox/itransactor_generated.go(2 hunks)agents/contracts/inbox/mocks/i_inbox.go(12 hunks)agents/contracts/lightinbox/filterer_generated.go(1 hunks)agents/contracts/lightinbox/icaller_generated.go(0 hunks)agents/contracts/lightinbox/itransactor_generated.go(2 hunks)agents/contracts/lightinbox/mocks/i_light_inbox.go(12 hunks)agents/contracts/lightmanager/filterer_generated.go(0 hunks)agents/contracts/lightmanager/icaller_generated.go(0 hunks)agents/contracts/lightmanager/itransactor_generated.go(1 hunks)agents/contracts/lightmanager/mocks/i_light_manager.go(1 hunks)agents/contracts/origin/filterer_generated.go(1 hunks)agents/contracts/origin/icaller_generated.go(0 hunks)agents/contracts/origin/itransactor_generated.go(1 hunks)agents/contracts/origin/mocks/i_origin.go(6 hunks)agents/contracts/test/attestationharness/attestationharness.abigen.go(3 hunks)agents/contracts/test/basemessageharness/basemessageharness.abigen.go(6 hunks)agents/contracts/test/gasdata/gasdataharness.abigen.go(3 hunks)agents/contracts/test/headerharness/headerharness.abigen.go(2 hunks)agents/contracts/test/headerharness/headerharness.contractinfo.json(1 hunks)agents/contracts/test/messageharness/messageharness.abigen.go(9 hunks)agents/contracts/test/pingpongclient/pingpongclient.abigen.go(3 hunks)agents/contracts/test/receiptharness/receiptharness.abigen.go(3 hunks)agents/contracts/test/requestharness/requestharness.abigen.go(2 hunks)agents/contracts/test/requestharness/requestharness.contractinfo.json(1 hunks)agents/contracts/test/snapshotharness/snapshotharness.abigen.go(9 hunks)agents/contracts/test/stateharness/stateharness.abigen.go(5 hunks)agents/contracts/test/testclient/testclient.abigen.go(4 hunks)agents/contracts/test/tipsharness/tipsharness.abigen.go(2 hunks)services/rfq/contracts/testcontracts/mockerc20/mockerc20.abigen.go(2 hunks)tools/abigen/internal/generate.go(4 hunks)
💤 Files with no reviewable changes (9)
- agents/contracts/lightinbox/icaller_generated.go
- agents/contracts/inbox/icaller_generated.go
- agents/contracts/bondingmanager/icaller_generated.go
- agents/contracts/origin/icaller_generated.go
- agents/contracts/gasoracle/icaller_generated.go
- agents/contracts/lightmanager/icaller_generated.go
- agents/contracts/lightmanager/filterer_generated.go
- agents/contracts/gasoracle/filterer_generated.go
- agents/contracts/bondingmanager/filterer_generated.go
✅ Files skipped from review due to trivial changes (1)
- agents/contracts/test/tipsharness/tipsharness.abigen.go
🧰 Additional context used
🪛 golangci-lint (1.62.2)
tools/abigen/internal/generate.go
191-191: Error return value of runFile.Close is not checked
(errcheck)
244-244: if !strings.HasPrefix(key, "solidity/") has complex nested blocks (complexity: 16)
(nestif)
201-201: shadow: declaration of "err" shadows declaration at line 131
(govet)
407-407: Error return value of lock.Close is not checked
(errcheck)
435-435: Error return value of resp.Body.Close is not checked
(errcheck)
446-446: Error return value of tmpFile.Close is not checked
(errcheck)
467-467: Comment should end in a period
(godot)
494-494: Comment should end in a period
(godot)
526-526: Comment should end in a period
(godot)
[medium] 357-357: G301: Expect directory permissions to be 0750 or less
(gosec)
[medium] 370-370: G302: Expect file permissions to be 0600 or less
(gosec)
[medium] 380-380: G304: Potential file inclusion via variable
(gosec)
[medium] 389-389: G302: Expect file permissions to be 0600 or less
(gosec)
[medium] 397-397: G304: Potential file inclusion via variable
(gosec)
[medium] 413-413: G302: Expect file permissions to be 0600 or less
(gosec)
[medium] 431-431: G107: Potential HTTP request made with variable url
(gosec)
383-383: if err != nil && os.IsExist(err) has complex nested blocks (complexity: 5)
(nestif)
495-495: getSolcURL - result 1 (error) is always nil
(unparam)
348-348: calculated cyclomatic complexity for function getSolcBinary is 20, max is 10
(cyclop)
368-368: shadow: declaration of "err" shadows declaration at line 350
(govet)
388-388: shadow: declaration of "err" shadows declaration at line 350
(govet)
412-412: shadow: declaration of "err" shadows declaration at line 350
(govet)
🪛 GitHub Check: codecov/patch
tools/abigen/internal/generate.go
[warning] 200-205: tools/abigen/internal/generate.go#L200-L205
Added lines #L200 - L205 were not covered by tests
[warning] 212-214: tools/abigen/internal/generate.go#L212-L214
Added lines #L212 - L214 were not covered by tests
[warning] 219-220: tools/abigen/internal/generate.go#L219-L220
Added lines #L219 - L220 were not covered by tests
[warning] 230-230: tools/abigen/internal/generate.go#L230
Added line #L230 was not covered by tests
[warning] 258-259: tools/abigen/internal/generate.go#L258-L259
Added lines #L258 - L259 were not covered by tests
[warning] 273-275: tools/abigen/internal/generate.go#L273-L275
Added lines #L273 - L275 were not covered by tests
[warning] 352-353: tools/abigen/internal/generate.go#L352-L353
Added lines #L352 - L353 were not covered by tests
[warning] 358-359: tools/abigen/internal/generate.go#L358-L359
Added lines #L358 - L359 were not covered by tests
[warning] 369-373: tools/abigen/internal/generate.go#L369-L373
Added lines #L369 - L373 were not covered by tests
[warning] 384-392: tools/abigen/internal/generate.go#L384-L392
Added lines #L384 - L392 were not covered by tests
[warning] 397-400: tools/abigen/internal/generate.go#L397-L400
Added lines #L397 - L400 were not covered by tests
[warning] 402-403: tools/abigen/internal/generate.go#L402-L403
Added lines #L402 - L403 were not covered by tests
[warning] 413-416: tools/abigen/internal/generate.go#L413-L416
Added lines #L413 - L416 were not covered by tests
[warning] 425-426: tools/abigen/internal/generate.go#L425-L426
Added lines #L425 - L426 were not covered by tests
[warning] 433-434: tools/abigen/internal/generate.go#L433-L434
Added lines #L433 - L434 were not covered by tests
[warning] 438-439: tools/abigen/internal/generate.go#L438-L439
Added lines #L438 - L439 were not covered by tests
[warning] 444-445: tools/abigen/internal/generate.go#L444-L445
Added lines #L444 - L445 were not covered by tests
[warning] 451-452: tools/abigen/internal/generate.go#L451-L452
Added lines #L451 - L452 were not covered by tests
[warning] 456-457: tools/abigen/internal/generate.go#L456-L457
Added lines #L456 - L457 were not covered by tests
[warning] 461-462: tools/abigen/internal/generate.go#L461-L462
Added lines #L461 - L462 were not covered by tests
[warning] 472-476: tools/abigen/internal/generate.go#L472-L476
Added lines #L472 - L476 were not covered by tests
[warning] 480-483: tools/abigen/internal/generate.go#L480-L483
Added lines #L480 - L483 were not covered by tests
[warning] 486-490: tools/abigen/internal/generate.go#L486-L490
Added lines #L486 - L490 were not covered by tests
[warning] 502-506: tools/abigen/internal/generate.go#L502-L506
Added lines #L502 - L506 were not covered by tests
[warning] 509-509: tools/abigen/internal/generate.go#L509
Added line #L509 was not covered by tests
[warning] 514-515: tools/abigen/internal/generate.go#L514-L515
Added lines #L514 - L515 were not covered by tests
[warning] 518-522: tools/abigen/internal/generate.go#L518-L522
Added lines #L518 - L522 were not covered by tests
[warning] 527-537: tools/abigen/internal/generate.go#L527-L537
Added lines #L527 - L537 were not covered by tests
[warning] 540-547: tools/abigen/internal/generate.go#L540-L547
Added lines #L540 - L547 were not covered by tests
[warning] 550-557: tools/abigen/internal/generate.go#L550-L557
Added lines #L550 - L557 were not covered by tests
🪛 GitHub Check: Lint (tools)
tools/abigen/internal/generate.go
[failure] 191-191:
Error return value of runFile.Close is not checked (errcheck)
[failure] 407-407:
Error return value of lock.Close is not checked (errcheck)
[failure] 435-435:
Error return value of resp.Body.Close is not checked (errcheck)
[failure] 446-446:
Error return value of tmpFile.Close is not checked (errcheck)
[failure] 467-467:
Comment should end in a period (godot)
[failure] 494-494:
Comment should end in a period (godot)
[failure] 526-526:
Comment should end in a period (godot)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: Go Coverage (1.22.x, services/scribe)
- GitHub Check: Go Coverage (1.22.x, services/rfq)
- GitHub Check: Go Coverage (1.22.x, services/explorer)
- GitHub Check: Go Generate (Module Changes) (tools)
- GitHub Check: Go Generate (Module Changes) (services/scribe)
- GitHub Check: Go Generate (Module Changes) (services/rfq)
- GitHub Check: Go Generate (Module Changes) (services/explorer)
- GitHub Check: Go Generate (Module Changes) (services/cctp-relayer)
- GitHub Check: Go Generate (Module Changes) (contrib/promexporter)
- GitHub Check: Go Generate (Module Changes) (contrib/opbot)
- GitHub Check: Go Generate (Module Changes) (agents)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (77)
agents/contracts/gasoracle/itransactor_generated.go (1)
33-34: Parameter name improvementThe parameter rename from
summitTipWei_tosummitTipWeiimproves code consistency by removing the trailing underscore. This aligns better with Go naming conventions.agents/contracts/gasoracle/mocks/i_gas_oracle.go (1)
479-495: Parameter name consistency updateThe parameter rename from
summitTipWei_tosummitTipWeiin the mock implementation matches the change in the interface definition, maintaining consistency across the codebase.agents/contracts/bondingmanager/itransactor_generated.go (1)
51-54: Method name clarity improvementRenaming
ResolveDisputeWhenStucktoResolveStuckDisputeimproves readability while maintaining the function's purpose. The new name is more concise and follows a more natural English word order.agents/contracts/lightmanager/itransactor_generated.go (1)
35-38: Consistent method naming across managersThe rename of
ResolveDisputeWhenStucktoResolveStuckDisputemirrors the same change in the Bonding Manager, maintaining naming consistency across different manager interfaces. This kind of consistency helps with codebase maintainability.agents/contracts/lightmanager/mocks/i_light_manager.go (1)
735-756: Function signature change reflects updated dispute resolution mechanism.The replacement of
ResolveProposedAgentRootwithResolveStuckDisputewith additional parameters (domain and slashedAgent) indicates a refinement in how disputes are resolved in the system. Since this is an auto-generated mock file, the changes appropriately reflect modifications to the underlying interface.agents/contracts/bondingmanager/mocks/i_bonding_manager.go (1)
965-986: API standardization improves codebase consistency.The rename of this function to
ResolveStuckDisputestandardizes the dispute resolution API across different interfaces in the codebase (matching the same change in the LightManager). This consistency improves maintainability and makes the API more intuitive for developers working across different components of the system.agents/contracts/origin/filterer_generated.go (1)
14-25: Event handling methods added for newDispatchedevent.The interface has been expanded to include standard event handling methods for the new
Dispatchedevent. These methods properly follow the established pattern with appropriate documentation and consistent parameter naming.agents/contracts/origin/itransactor_generated.go (1)
33-34: Parameter naming improvement inResolveDisputemethod.The parameters now have more descriptive names (
slashedIndexandhonestIndex) which better clarify their purpose, replacing what appears to have been less specific parameter naming previously.agents/contracts/origin/mocks/i_origin.go (7)
85-106: Added mock implementation forFilterDispatchedmethod.This mock implementation correctly reflects the added method in the interface and follows the established pattern for mocking filter methods.
108-129: Updated mock implementation forFilterInitializedmethod.The return type and function signatures have been properly updated to maintain consistency with interface changes.
436-457: Added mock implementation forParseDispatchedmethod.This implementation correctly handles the parsing of logs for the new
Dispatchedevent, maintaining the established pattern for event parsing in the codebase.
459-480: Updated mock implementation forParseInitializedmethod.The return type has been updated to
OriginInitializedto reflect changes in the interface contract structure.
590-611: Updated parameter names inResolveDisputemock method.The parameter naming has been improved from
rivalIndextohonestIndex(as seen in the implementation), making the code more descriptive and consistent with the updated interface.
793-814: Added mock implementation forWatchDispatchedmethod.This implementation properly handles event subscription for the new
Dispatchedevent, following the established pattern for event watching in the codebase.
816-837: Updated mock implementation forWatchInitializedmethod.The mock method has been updated to handle the new return type and function signatures, maintaining consistency with interface changes.
agents/contracts/inbox/mocks/i_inbox.go (1)
946-967: Parameter type upgrade enhances scalability.The change from
uint8to*big.Intfor thestateIndexparameter significantly increases the range of possible state indices from 0-255 to virtually unlimited. This improves the contract's scalability for systems that may need to handle more than 256 states.agents/contracts/inbox/itransactor_generated.go (1)
43-54: Interface updates reflect underlying Solidity contract changes.The updated method identifiers (e.g.,
0x0b6b985c,0x62389709) and parameter type changes fromuint8to*big.IntforstateIndexmaintain alignment with the corresponding Solidity contract modifications. These changes enable handling larger state indices and ensure proper ABI compatibility between Go and Solidity.agents/contracts/lightinbox/mocks/i_light_inbox.go (1)
716-737: Consistent parameter type changes across inbox implementations.The changes to
stateIndexfromuint8to*big.Intin the LightInbox mock match those in the regular Inbox interfaces. This consistent approach across different inbox variants ensures uniform behavior and allows for greater scalability in both implementations.agents/contracts/lightinbox/filterer_generated.go (1)
74-97:❓ Verification inconclusive
Type change for stateIndex parameter and updated event signatures
The
stateIndexparameter type has been changed fromuint8touint256in bothInvalidStateWithAttestationandInvalidStateWithSnapshotevents. This significantly increases the range of possible state indices from 0-255 to a much larger range. Additionally, the event signatures (hex identifiers) have been updated to reflect these type changes.Make sure that consumers of these events are updated to handle the larger integer type properly.
🏁 Script executed:
#!/bin/bash # Check if any code is still using the old parameter type or event signature # Search for possible references to uint8 stateIndex in the codebase rg -A 2 -B 2 "InvalidStateWithAttestation.*uint8" --type go rg -A 2 -B 2 "InvalidStateWithSnapshot.*uint8" --type go # Search for references to the old event signatures rg "0x802f82273c009c05274153f0f1bde4e9e06244157a2914d4b84d266a3fa82ff1" --type go rg "0xf649e0d8f524df47b57dbcc2cda237f72096391dff21abc122acebd5112f4de0" --type goLength of output: 284
Action: Confirm that consumers are updated for the new uint256 type and revised event signatures.
Based on our search:
- Both the
InvalidStateWithAttestationandInvalidStateWithSnapshotevent bindings now useuint256for thestateIndexparameter—as shown in the fileagents/contracts/lightinbox/filterer_generated.goat lines 74–97.- No residual references to the old
uint8type or outdated event signature hex values were found in the codebase.Please double-check that all downstream consumers correctly handle the expanded range provided by
uint256.agents/contracts/inbox/filterer_generated.go (2)
122-133:✅ Verification successful
Ownership transfer mechanism simplified
The
OwnershipTransferStartedevent and related methods have been removed while keeping theOwnershipTransferredevent. This suggests a change from a two-step ownership transfer process to a simpler one-step approach.Ensure that any code relying on the two-step ownership transfer process is updated to work with the new mechanism.
🏁 Script executed:
#!/bin/bash # Check if there are any direct calls to the removed methods that need to be updated # Look for potential usage of the removed methods rg "FilterOwnershipTransferStarted|WatchOwnershipTransferStarted|ParseOwnershipTransferStarted" --type go # Look for any code that might be handling both ownership events rg -A 5 "case.*OwnershipTransfer" --type goLength of output: 153
I'll run another search to verify that no code in the repository still references "OwnershipTransferStarted" (which would indicate any lingering reliance on the removed two-step mechanism). This should provide further confidence that the migration to the one-step ownership transfer process is complete.
🏁 Script executed:
#!/bin/bash # Search the entire repository for any occurrence of "OwnershipTransferStarted" rg "OwnershipTransferStarted" --type goLength of output: 39
Ownership Transfer Process Updated
Based on the additional verification using shell scripts, no references to the removed
OwnershipTransferStartedevent or its related methods were found across the codebase. This confirms that the code now fully implements the simplified, one-step ownership transfer process. Please ensure that any external dependencies or tests expecting the two-step process are updated accordingly.
98-121:❓ Verification inconclusive
Type change for stateIndex parameter and updated event signatures
Similar to the changes in the lightinbox package, the
stateIndexparameter type has been changed fromuint8touint256in bothInvalidStateWithAttestationandInvalidStateWithSnapshotevents, with corresponding updates to the event signatures (hex identifiers).This change allows for a much larger range of state indices and maintains consistency between the inbox and lightinbox interfaces.
🏁 Script executed:
#!/bin/bash # Check for any struct definitions that might need updating to match the new parameter type # Look for struct definitions for these events rg -A 5 -B 2 "type.*InvalidStateWithAttestation struct" --type go rg -A 5 -B 2 "type.*InvalidStateWithSnapshot struct" --type go # Check if there are any remaining references to the removed OwnershipTransferStarted methods rg "OwnershipTransferStarted" --type go | grep -v "//.*OwnershipTransferStarted"Length of output: 213
Review Update – Verify Type Update and Signature Consistency
The changes for updating the
stateIndexparameter fromuint8touint256, along with the updated event signatures for theInvalidStateWithAttestationandInvalidStateWithSnapshotevents, appear to be in line with the corresponding updates made in the lightinbox package. Automated searches (including checks for related struct definitions and residual references toOwnershipTransferStarted) produced no output. However, as the absence of output may be inconclusive, please verify manually that all corresponding struct definitions and usages have been updated appropriately.
- File to verify:
agents/contracts/inbox/filterer_generated.go(lines 98-121)- Checklist:
- Confirm that the
stateIndexparameter is now correctly usinguint256.- Ensure that all event signature identifiers match the updated values.
- Verify that there are no lingering references to the removed
OwnershipTransferStartedmethods.agents/contracts/test/headerharness/headerharness.contractinfo.json (1)
3-3: Pragma relaxation aligns with cross-platform compilation needs.The change from
pragma solidity =0.8.17;topragma solidity 0.8.17;(removing the equality sign) relaxes the compiler version requirement. This modification supports the PR objective of implementing a fallback for downloadingsolcbinaries, as it allows for compilation with compatible patch versions which may be more readily available across different platforms, particularly Apple Silicon Macs.agents/contracts/test/stateharness/stateharness.abigen.go (1)
35-35: Bytecode changes reflect successful cross-platform compilation.The updated binary data across all contract metadata structures (
GasDataLibMetaData.Bin,MemViewLibMetaData.Bin,NumberLibMetaData.Bin,StateHarnessMetaData.Bin, andStateLibMetaData.Bin) indicates that the contracts have been successfully recompiled. These changes are the expected outcome of implementing the solc binary fallback mechanism for Apple Silicon Macs.Since the ABI remains unchanged while only the bytecode is modified, this confirms that the functional interface of the contracts is preserved across different compilation methods. This is exactly what we'd want to see when implementing a fallback compilation pipeline.
Also applies to: 208-208, 381-381, 571-571, 1214-1214
agents/contracts/test/testclient/testclient.abigen.go (7)
712-712: Bytecode update for RequestLibMetaDataThe compiled bytecode for RequestLib has been updated, likely as a result of the solc binary fallback implementation for Apple Silicon Macs.
884-885: ABI update for TestClientMetaDataThe ABI definition for TestClient contract has been updated with renamed parameter.
891-891: Bytecode update for TestClientMetaDataThe compiled bytecode for TestClient has been updated, which is expected as part of the solc binary fallback implementation.
1157-1159: Parameter name update in TestClientSessionThe parameter name change is consistently applied to the TestClientSession implementation.
1164-1166: Parameter name update in TestClientTransactorSessionThe parameter name change is consistently applied to the TestClientTransactorSession implementation.
1449-1449: Bytecode update for TypeCastsMetaDataThe compiled bytecode for TypeCasts library has been updated as part of the solc binary fallback implementation.
1148-1151:❓ Verification inconclusive
Parameter name change from 'optimisticPeriod' to 'optimisticSeconds'
The function parameter has been renamed from
optimisticPeriodtooptimisticSecondswhich improves clarity by explicitly stating the time unit.
🏁 Script executed:
#!/bin/bash # Check if this parameter name change is consistent across all files echo "Checking for any remaining instances of 'optimisticPeriod' in the codebase that might need updating:" rg "optimisticPeriod" --type goLength of output: 3035
Parameter Naming Update in TestClient Binding Approved
The parameter in
SendMessagewithinagents/contracts/test/testclient/testclient.abigen.gohas been successfully changed fromoptimisticPeriodtooptimisticSeconds, which clearly indicates the time unit and improves clarity.
- Verified Change: The diff confirms that the updated parameter name appears as intended in the testclient binding.
- Other Occurrences: The search shows that
optimisticPeriodis still used in origin contract bindings (e.g., inagents/contracts/origin/itransactor_generated.goand associated mocks) and in test files. These instances likely reflect auto-generated code or adhere to the original contract ABI conventions.- Action Item: Please review if the naming should be standardized across all bindings. If the divergence is intentional (to preserve ABI compatibility or for other reasons), no further action is needed on those files.
agents/contracts/test/basemessageharness/basemessageharness.abigen.go (6)
47-47: Bytecode update for BaseMessageHarnessMetaDataThe compiled bytecode for BaseMessageHarness has been updated, reflecting the change in compilation method introduced by the solc binary fallback.
534-534: Bytecode update for BaseMessageLibMetaDataThe compiled bytecode for BaseMessageLib has been updated as part of the solc binary fallback implementation.
707-707: Bytecode update for MemViewLibMetaDataThe compiled bytecode for MemViewLib has been updated as part of the solc binary fallback implementation.
880-880: Bytecode update for MerkleMathMetaDataThe compiled bytecode for MerkleMath has been updated as part of the solc binary fallback implementation.
1053-1053: Bytecode update for RequestLibMetaDataThe compiled bytecode for RequestLib has been updated consistently with other libraries as part of the solc binary fallback implementation.
1226-1226: Bytecode update for TipsLibMetaDataThe compiled bytecode for TipsLib has been updated, consistent with all other libraries in this file, reflecting the change in compilation process.
agents/contracts/test/requestharness/requestharness.contractinfo.json (1)
1-1: Changes to bytecode are consistent with Solidity versioning update.The bytecode and metadata in this contractinfo.json file have been updated, likely as a result of the changes in how the Solidity compiler is invoked through the new fallback mechanism for Apple Silicon Macs.
agents/contracts/test/headerharness/headerharness.abigen.go (2)
46-46: Bytecode update for HeaderHarnessMetaData is consistent with the PR objective.The updated bytecode reflects the changes in the compilation process resulting from the new solc binary fallback mechanism implemented for Apple Silicon Macs.
502-502: Bytecode update for HeaderLibMetaData is consistent with the PR objective.The updated bytecode here also reflects the changes in the compilation process from the new solc binary fallback mechanism.
agents/contracts/test/receiptharness/receiptharness.abigen.go (3)
35-35: Bytecode update for MemViewLibMetaData is consistent with the PR objective.The updated bytecode reflects the changes in the compilation process resulting from the new solc binary fallback mechanism.
224-224: Bytecode update for ReceiptHarnessMetaData is consistent with the PR objective.The updated bytecode aligns with the expected changes from implementing the solc binary fallback mechanism for Apple Silicon Macs.
835-835: Bytecode update for ReceiptLibMetaData is consistent with the PR objective.This bytecode update is in line with the compilation changes introduced by the solc binary fallback mechanism.
agents/contracts/test/snapshotharness/snapshotharness.abigen.go (8)
35-35: Binary data updated in GasDataLibMetaData.The bytecode for GasDataLibMetaData has been updated as part of the solc binary fallback implementation.
208-208: Binary data updated in MemViewLibMetaData.The bytecode for MemViewLibMetaData has been updated, consistent with recompilation using the fallback solc binary.
381-381: Binary data updated in MerkleMathMetaData.The bytecode for MerkleMathMetaData has been updated, likely a result of the solc binary fallback compilation.
554-554: Binary data updated in NumberLibMetaData.The bytecode for NumberLibMetaData has been updated, consistent with the recompilation process.
726-726: ABI and binary updates in SnapshotHarnessMetaData.The ABI, signature, and compiled bytecode for SnapshotHarnessMetaData have been updated. Notable change includes the
statefunction signature which now accepts auint256parameter type instead ofuint8.Also applies to: 734-734, 737-737
1162-1162: Binary data updated in SnapshotLibMetaData.The bytecode for SnapshotLibMetaData has been updated, consistent with other binary updates in this file.
1335-1335: Binary data updated in StateLibMetaData.The bytecode for StateLibMetaData has been updated, completing the set of binary updates in this file.
1097-1100:❓ Verification inconclusive
Function signature updated for State method.
The State method parameter type has been changed from
uint8to*big.Int(representing uint256 in Solidity), which improves compatibility with larger state indices.Also applies to: 1114-1117, 1121-1124
🏁 Script executed:
#!/bin/bash # Check if there are any usages of the SnapshotHarness.State function to ensure they've been updated echo "Checking for usages of the SnapshotHarness.State function..." rg -A 2 "SnapshotHarness.*State.*stateIndex" --glob "*.go" | grep -v "snapshotharness.abigen.go"Length of output: 227
Verify Signature Update for SnapshotHarness.State Function
TheStatefunction signature inagents/contracts/test/snapshotharness/snapshotharness.abigen.gohas been updated to use*big.Intfor thestateIndexparameter (matching Solidity’suint256). This change is reflected in lines 1097–1100, 1114–1117, and 1121–1124. However, since the search for external usages of this updated function returned no output, please manually verify that all callers ofSnapshotHarness.State(if any exist outside the auto-generated file) are adjusted accordingly.agents/contracts/test/requestharness/requestharness.abigen.go (2)
42-42: Binary data updated in RequestHarnessMetaData.The bytecode for RequestHarnessMetaData has been updated as part of the solc binary fallback implementation.
374-374: Binary data updated in RequestLibMetaData.The bytecode for RequestLibMetaData has been updated, consistent with other binary updates throughout the PR.
agents/contracts/test/messageharness/messageharness.abigen.go (9)
35-35: Bytecode update aligns with solc fallback implementation.This change to the
BaseMessageLibMetaData.Binfield reflects the recompiled bytecode from the new solc binary fallback mechanism for Apple Silicon Macs, as mentioned in the PR objectives. The bytecode changes themselves are expected when recompiling contracts with potentially different versions or configurations of the Solidity compiler.
208-208: Bytecode update reflects consistent recompilation.The
ByteStringMetaData.Binfield has been updated with new bytecode, in line with the other contract metadata updates in this file. This is part of the systematic recompilation using the newly implemented solc binary fallback mechanism.
381-381: Consistent bytecode update for HeaderLib contract.This update to the
HeaderLibMetaData.Binfield is part of the broader pattern of bytecode updates throughout the auto-generated files, consistent with the implementation of the solc binary fallback mechanism.
554-554: Bytecode modification for MemViewLib contract.The binary update for the
MemViewLibMetaData.Binfield follows the same pattern as other contracts in this file, indicating consistent recompilation across all components.
727-727: Updated MerkleMath contract bytecode.This change to the
MerkleMathMetaData.Binfield is expected as part of the recompilation process with the new solc binary fallback system for Apple Silicon Macs.
908-908: MessageHarness contract bytecode updated.The significant update to the
MessageHarnessMetaData.Binfield reflects the comprehensive recompilation strategy implemented for this PR, ensuring all contracts work properly with the new solc binary fallback.
1271-1271: MessageLib contract bytecode refreshed.This bytecode update for the
MessageLibMetaData.Binfield is consistent with the changes seen across all contract metadata in this file, supporting the PR's goal of reliable compilation on Apple Silicon Macs.
1444-1444: Final bytecode update for RequestLib contract.The
RequestLibMetaData.Binfield has been updated with new bytecode, completing the set of bytecode updates for all contracts in this file and ensuring compatibility with the new solc binary fallback mechanism.
1617-1617: TipsLib contract bytecode updated properly.The update to the
TipsLibMetaData.Binfield is another example of the systematic recompilation of all contracts, ensuring consistent behavior across the codebase with the new solc binary fallback implementation.agents/contracts/test/gasdata/gasdataharness.abigen.go (3)
50-50: GasDataHarness contract bytecode updated appropriately.The
GasDataHarnessMetaData.Binfield has been updated with new bytecode, consistent with the implementation of the solc binary fallback mechanism for Apple Silicon Macs. This is part of the systematic recompilation approach seen across all contract files.
630-630: GasDataLib contract bytecode refreshed.The update to the
GasDataLibMetaData.Binfield reflects the recompilation with the fallback solc binary mechanism, ensuring this library contract functions correctly on all platforms, including Apple Silicon Macs.
803-803: NumberLib contract bytecode properly updated.This final bytecode update for the
NumberLibMetaData.Binfield completes the set of changes in this file, ensuring all contract components have been consistently recompiled using the new solc binary fallback system.agents/contracts/test/pingpongclient/pingpongclient.abigen.go (3)
724-724: Expected binary update due to solc compilation changesThis change updates the compiled bytecode for the PingPongClient contract. This is an expected result of the implementation of the solc binary fallback mechanism introduced in this PR for Apple Silicon Macs.
1717-1717: Expected binary update due to solc compilation changesThis change updates the compiled bytecode for the RequestLib contract. This is an expected result of the implementation of the solc binary fallback mechanism introduced in this PR for Apple Silicon Macs.
1890-1890: Expected binary update due to solc compilation changesThis change updates the compiled bytecode for the TypeCasts contract. This is an expected result of the implementation of the solc binary fallback mechanism introduced in this PR for Apple Silicon Macs.
agents/contracts/test/attestationharness/attestationharness.abigen.go (3)
48-48: Expected binary update due to solc compilation changesThis change updates the compiled bytecode for the AttestationHarness contract. This is an expected result of the implementation of the solc binary fallback mechanism introduced in this PR for Apple Silicon Macs.
566-566: Expected binary update due to solc compilation changesThis change updates the compiled bytecode for the AttestationLib contract. This is an expected result of the implementation of the solc binary fallback mechanism introduced in this PR for Apple Silicon Macs.
739-739: Expected binary update due to solc compilation changesThis change updates the compiled bytecode for the MemViewLib contract. This is an expected result of the implementation of the solc binary fallback mechanism introduced in this PR for Apple Silicon Macs.
tools/abigen/internal/generate.go (4)
189-214: Good implementation of fallback mechanism for Docker-based compilationI like the approach of trying Docker first and then falling back to a direct binary. This maintains backward compatibility while adding support for environments where Docker may not be available or functioning correctly.
🧰 Tools
🪛 golangci-lint (1.62.2)
191-191: Error return value of
runFile.Closeis not checked(errcheck)
201-201: shadow: declaration of "err" shadows declaration at line 131
(govet)
🪛 GitHub Check: codecov/patch
[warning] 200-205: tools/abigen/internal/generate.go#L200-L205
Added lines #L200 - L205 were not covered by tests
[warning] 212-214: tools/abigen/internal/generate.go#L212-L214
Added lines #L212 - L214 were not covered by tests🪛 GitHub Check: Lint (tools)
[failure] 191-191:
Error return value ofrunFile.Closeis not checked (errcheck)
217-231: Handle potential vulnerabilities when executing downloaded binariesThe implementation provides a good fallback mechanism, but executing binaries downloaded from the internet poses security risks.
Consider adding checksum verification to ensure the downloaded binary has not been tampered with:
// In getSolcBinary function + // Verify checksum of downloaded file + expectedChecksum, err := getSolcChecksum(normalizedVersion, platform) + if err != nil { + return "", fmt.Errorf("could not retrieve checksum for solc: %w", err) + } + actualChecksum, err := calculateFileChecksum(solcPath) + if err != nil { + return "", fmt.Errorf("could not calculate checksum for downloaded solc: %w", err) + } + if expectedChecksum != actualChecksum { + os.Remove(solcPath) + return "", fmt.Errorf("checksum verification failed for downloaded solc") + }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 219-220: tools/abigen/internal/generate.go#L219-L220
Added lines #L219 - L220 were not covered by tests
[warning] 230-230: tools/abigen/internal/generate.go#L230
Added line #L230 was not covered by tests
239-277: Good normalization of contract namesThe normalization logic ensures compatibility between Docker and direct binary execution output formats. This is crucial for maintaining a consistent interface regardless of the compilation method used.
🧰 Tools
🪛 golangci-lint (1.62.2)
244-244:
if !strings.HasPrefix(key, "solidity/")has complex nested blocks (complexity: 16)(nestif)
🪛 GitHub Check: codecov/patch
[warning] 258-259: tools/abigen/internal/generate.go#L258-L259
Added lines #L258 - L259 were not covered by tests
[warning] 273-275: tools/abigen/internal/generate.go#L273-L275
Added lines #L273 - L275 were not covered by tests
494-524: Update Windows binary URL constructionThe URL for Windows binaries might need adjustment for newer Solidity versions.
#!/bin/bash # Check recent Solidity release formats for Windows binaries # Get recent releases echo "Checking recent Solidity releases for Windows binary formats..." curl -s https://api.github.com/repos/ethereum/solidity/releases?per_page=5 | \ jq -r '.[].assets[] | select(.name | contains("windows")) | [.browser_download_url, .name] | @tsv' | \ sort -u🧰 Tools
🪛 golangci-lint (1.62.2)
494-494: Comment should end in a period
(godot)
495-495: getSolcURL - result 1 (error) is always nil
(unparam)
🪛 GitHub Check: codecov/patch
[warning] 502-506: tools/abigen/internal/generate.go#L502-L506
Added lines #L502 - L506 were not covered by tests
[warning] 509-509: tools/abigen/internal/generate.go#L509
Added line #L509 was not covered by tests
[warning] 514-515: tools/abigen/internal/generate.go#L514-L515
Added lines #L514 - L515 were not covered by tests
[warning] 518-522: tools/abigen/internal/generate.go#L518-L522
Added lines #L518 - L522 were not covered by tests🪛 GitHub Check: Lint (tools)
[failure] 494-494:
Comment should end in a period (godot)services/rfq/contracts/testcontracts/mockerc20/mockerc20.abigen.go (3)
2711-2725: ABI and function signatures correctly updated to include new ERC20 token management capabilities.The MockERC20 contract interface has been enhanced with burn and mint capabilities, which are reflected in the updated ABI and Sigs map. These additions provide essential token supply management functions that were previously missing.
3106-3146: New Burn and Mint function bindings properly implemented.The implementation of the Go bindings for both burn and mint functions follows the same pattern as other ERC20 functions. Each function is correctly implemented with three variants (direct transactor call, session-based call, and transactor session call) for flexibility in different calling contexts.
2725-2725:✅ Verification successful
Verify binary compatibility with Apple Silicon.
The updated contract binary appears to be compatible with the new solc compilation fallback mechanism. This change is crucial for the PR objective of supporting contract compilation on Apple Silicon Macs when Docker is unavailable.
🏁 Script executed:
#!/bin/bash # Ensure this generated binding works correctly on Apple Silicon # Check if we can find any references to the new Apple Silicon support in the codebase echo "Searching for references to Apple Silicon support in the tooling:" rg -i "apple|silicon|m1|m2|arm" tools/abigen/Length of output: 2022
Apple Silicon Compatibility Verified
- The updated contract binary correctly integrates the new solc compilation fallback mechanism.
- Verification of the tooling in
tools/abigen/README.mdandtools/abigen/internal/generate.goconfirms that native support for Apple Silicon (arm64) is implemented.- These changes fulfill the PR objective of supporting contract compilation on Apple Silicon Macs without Docker.
Summary
solcbinaries directly when Docker fails~/.cache/solc/for better performanceTest plan
Fixes #3366
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Dispatchedevent in theIOriginFiltererinterface, allowing for log retrieval and subscriptions.Documentation
Tests
TestClientandSnapshotHarnesscontracts.