Updating Blocked chains #3478
Updating Blocked chains #3478
Conversation
The changes in this pull request involve updates to the Go workflow configuration, the introduction of a new Golang CI Lint Version Manager, and enhancements to documentation files. The workflow modifications include dynamic version handling for golangci-lint and improved job execution conditions. Additionally, the .golangci-version file has been updated to specify a new version. Documentation in CONTRIBUTING.md, README.md, and a new README.md for the Golang CI Lint Version Manager has been added to clarify contribution processes and setup instructions. Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Co-authored-by: [email protected] <[email protected]> Co-authored-by: Trajan0x <[email protected]>
* making correct variables mandated to generate call data * updating docs * additional unit test
- @synapsecns/[email protected] - @synapsecns/[email protected]
* making correct variables mandated to generate call data * updating docs * additional unit test * updating incentivized pools * constants * constants
- @synapsecns/[email protected] - @synapsecns/[email protected] - @synapsecns/[email protected] - @synapsecns/[email protected] - @synapsecns/[email protected]
* paused chain + updated SDK * fix: use correct address for tests --------- Co-authored-by: ChiTimesChi <[email protected]>
- @synapsecns/[email protected] - @synapsecns/[email protected] - @synapsecns/[email protected] - @synapsecns/[email protected]
Co-authored-by: Trajan0x <[email protected]>
Defi-Moses
requested review from
trajan0x,
dwasse,
aureliusbtc,
bigboydiamonds,
abtestingalpha and
ChiTimesChi
as code owners
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 golangci-lint (1.62.2)level=warning msg="[config_reader] The configuration option WalkthroughThis pull request introduces a comprehensive set of changes across multiple components of the Sanguine project, focusing on dependency management, linting configuration, and API enhancements. The changes span various modules, including Go projects, TypeScript packages, and configuration files. Key modifications include updating Go toolchain versions, removing Rookout dependencies, introducing a Golang CI Lint version manager, and refining API endpoint requirements. Changes
Sequence DiagramsequenceDiagram
participant Developer
participant GolangCILint
participant Project
participant CodeCoverage
Developer->>GolangCILint: Run linting
GolangCILint->>Project: Read .golangci-version
Project-->>GolangCILint: Return specific version
GolangCILint->>Project: Perform linting
GolangCILint->>CodeCoverage: Generate coverage report
Possibly related PRs
Suggested labels
Suggested reviewers
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
Documentation and Community
|
github-actions
bot
added
M-ci
Bundle ReportChanges will decrease total bundle size by 3.29MB (-10.1%) ⬇️. This is within the configured threshold ✅ Detailed changes
ℹ️ *Bundle size includes cached data from a previous commit |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🔭 Outside diff range comments (1)
contrib/golang-ci-lint/Makefile (1)
Line range hint
23-24: Add safety checks to docker-clean targetThe docker-clean target removes all containers without confirmation, which could be dangerous in shared environments.
docker-clean: ## stops and removes all containers at once - docker ps -aq | xargs docker stop | xargs docker rm + @echo "This will stop and remove ALL Docker containers." + @read -p "Are you sure? [y/N] " confirm && [ $$confirm = "y" ] || exit 1 + @docker ps -aq | xargs -r docker stop | xargs -r docker rm docker network prune
🧹 Nitpick comments (14)
services/rfq/api/rest/server_test.go (1)
434-436: Possible improvement: ExpandvalidateChainID
If additional chain ID constraints arise (e.g., restricting to a certain known set of chain IDs), consider expandingvalidateChainIDto handle them.services/rfq/api/docs/swagger.yaml (1)
313-316: Enhanced endpoint description
The updated explanation (“Initiate an Active Request-For-Quote and return the best quote available”) better communicates the endpoint’s purpose.services/rfq/api/docs/swagger.json (1)
2-5: Enhance API documentation with essential metadataThe API specification is missing crucial documentation elements in the
infosection:
- API version
- Description
- Terms of service
- License information
Add these fields to improve API documentation:
"info": { + "version": "1.0.0", + "title": "RFQ Service API", + "description": "API for managing quote requests and responses in the bridge system", + "termsOfService": "https://example.com/terms/", + "license": { + "name": "Apache 2.0", + "url": "http://www.apache.org/licenses/LICENSE-2.0.html" + }, "contact": {} }contrib/golang-ci-lint/main.go (2)
87-102: Ensure full trust chain verification for download URLs.Great job restricting downloads to GitHub releases. However, consider verifying the TLS certificate chain more explicitly (e.g., pinned certificate or pinned public key) if you want stronger security guarantees against potential man-in-the-middle attacks.
104-175: Consider splittingvalidatePathinto smaller functions.
validatePathis quite large and handles multiple responsibilities (e.g., MacOS /private prefix, symlink evaluation, root directory checks, directory traversal checks, etc.). Splitting it into smaller helper functions might improve readability and maintainability.contrib/golang-ci-lint/permissions/permissions_windows.go (1)
9-12: Windows ACL considerations.Calling
os.Chmodon Windows only partially reflects real ACL-based permissions. For robust Windows security, consider a separate ACL handling mechanism to fully reflect user/group permissions.contrib/golang-ci-lint/permissions/permissions.go (1)
29-38: Consolidate permission settings for maintainability.Currently,
SetFilePermissionsimplements separate logic for Windows and Unix inline. Consider pushing all OS-specific logic into the respective Unix/Windows functions for a more modular design.services/explorer/graphql/server/graph/queryutils.go (2)
1875-1884: Enums vs. numeric constants for bridging modules.Defining descriptive constants is a good practice. However, consider using typed enums or iota-based typed constants in Go to make the code more readable and prevent mistakes with raw integers.
1891-1891: RFQ bridging future-proofing.Similarly, any expansions to the RFQ bridging mechanism should continue to use
ModuleSynapseRFQor higher. Ensure tests verify that numeric event codes align with these modules.docker/golang-ci-lint.Dockerfile (1)
3-8: Metadata clarity is helpful.These labels provide excellent transparency about the image. Keep them updated to maintain correct references, descriptions, and documentation URLs.
contrib/golang-ci-lint/.goreleaser.yml (1)
47-54: Consider including additional documentation filesThe archive only includes README.md. Consider including other important documentation files like LICENSE, CHANGELOG, etc.
files: - README.md + - LICENSE* + - CHANGELOG* + - CONTRIBUTING*contrib/golang-ci-lint/README.md (3)
99-100: Fix macOS spelling in documentation.The Apple operating system should be written as "macOS" instead of "MacOS".
- - MacOS (/private/var) symlink handling: + - macOS (/private/var) symlink handling: - - MacOS: Handles /private/var symlinks and temp directories + - macOS: Handles /private/var symlinks and temp directoriesAlso applies to: 109-109
🧰 Tools
🪛 LanguageTool
[grammar] ~99-~99: The operating system from Apple is written “macOS”.
Context: ...amd64, darwin-arm64, windows-amd64 - MacOS (/private/var) symlink handling: -...(MAC_OS)
51-51: Replace hard tab with spaces in Makefile example.The Makefile example contains a hard tab that should be replaced with spaces for consistent formatting across the documentation.
- go run github.com/synapsecns/sanguine/contrib/golang-ci-lint run --fix --config=.golangci.yml + go run github.com/synapsecns/sanguine/contrib/golang-ci-lint run --fix --config=.golangci.yml🧰 Tools
🪛 Markdownlint (0.37.0)
51-51: Column: 1
Hard tabs(MD010, no-hard-tabs)
29-31: Add language specifier to the code block.Add a language specifier to the fenced code block for better syntax highlighting.
-``` +```text 1.61.0<details> <summary>🧰 Tools</summary> <details> <summary>🪛 Markdownlint (0.37.0)</summary> 29-29: null Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used: .coderabbit.yaml** **Review profile: CHILL** **Plan: Pro** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 7d2326b46022a54cc6579e3daa18c1aa42e57ddd and 97a185f25958eef7e3b164dca6f74f58336dbc17. </details> <details> <summary>⛔ Files ignored due to path filters (13)</summary> * `agents/go.sum` is excluded by `!**/*.sum` * `contrib/golang-ci-lint/go.sum` is excluded by `!**/*.sum` * `contrib/opbot/go.sum` is excluded by `!**/*.sum` * `contrib/promexporter/go.sum` is excluded by `!**/*.sum` * `contrib/screener-api/go.sum` is excluded by `!**/*.sum` * `core/go.sum` is excluded by `!**/*.sum` * `ethergo/go.sum` is excluded by `!**/*.sum` * `go.work` is excluded by `!**/*.work` * `services/cctp-relayer/go.sum` is excluded by `!**/*.sum` * `services/explorer/go.sum` is excluded by `!**/*.sum` * `services/omnirpc/go.sum` is excluded by `!**/*.sum` * `services/rfq/go.sum` is excluded by `!**/*.sum` * `services/scribe/go.sum` is excluded by `!**/*.sum` </details> <details> <summary>📒 Files selected for processing (76)</summary> * `.codecov.yml` (1 hunks) * `.github/workflows/go.yml` (2 hunks) * `.gitignore` (1 hunks) * `.golangci-version` (1 hunks) * `.golangci.yml` (2 hunks) * `CONTRIBUTING.md` (1 hunks) * `README.md` (1 hunks) * `agents/go.mod` (0 hunks) * `contrib/golang-ci-lint/.codecov.yml` (1 hunks) * `contrib/golang-ci-lint/.goreleaser.yml` (1 hunks) * `contrib/golang-ci-lint/Makefile` (1 hunks) * `contrib/golang-ci-lint/README.md` (1 hunks) * `contrib/golang-ci-lint/go.mod` (1 hunks) * `contrib/golang-ci-lint/main.go` (1 hunks) * `contrib/golang-ci-lint/main_test.go` (1 hunks) * `contrib/golang-ci-lint/permissions/permissions.go` (1 hunks) * `contrib/golang-ci-lint/permissions/permissions_unix.go` (1 hunks) * `contrib/golang-ci-lint/permissions/permissions_windows.go` (1 hunks) * `contrib/opbot/go.mod` (0 hunks) * `contrib/promexporter/go.mod` (0 hunks) * `contrib/promexporter/internal/gql/explorer/models.gen.go` (1 hunks) * `contrib/screener-api/go.mod` (0 hunks) * `core/go.mod` (0 hunks) * `core/metrics/README.md` (0 hunks) * `core/metrics/base.go` (0 hunks) * `core/metrics/internal/const.go` (0 hunks) * `core/metrics/rookout.go` (0 hunks) * `docker/golang-ci-lint.Dockerfile` (1 hunks) * `docs/bridge/CHANGELOG.md` (1 hunks) * `docs/bridge/docs/02-Bridge/02-REST-API.md` (1 hunks) * `docs/bridge/package.json` (2 hunks) * `ethergo/go.mod` (0 hunks) * `make/go.Makefile` (1 hunks) * `packages/explorer-ui/CHANGELOG.md` (1 hunks) * `packages/explorer-ui/package.json` (2 hunks) * `packages/rest-api/CHANGELOG.md` (1 hunks) * `packages/rest-api/package.json` (2 hunks) * `packages/rest-api/src/controllers/bridgeController.ts` (1 hunks) * `packages/rest-api/src/routes/bridgeRoute.ts` (1 hunks) * `packages/rest-api/src/tests/bridgeRoute.test.ts` (2 hunks) * `packages/rest-api/swagger.json` (1 hunks) * `packages/sdk-router/CHANGELOG.md` (1 hunks) * `packages/sdk-router/package.json` (1 hunks) * `packages/sdk-router/src/constants/chainIds.ts` (3 hunks) * `packages/sdk-router/src/constants/testValues.ts` (1 hunks) * `packages/sdk-router/src/sdk.test.ts` (2 hunks) * `packages/synapse-constants/CHANGELOG.md` (1 hunks) * `packages/synapse-constants/package.json` (1 hunks) * `packages/synapse-constants/src/constants/tokens/poolMaster.ts` (2 hunks) * `packages/synapse-interface/CHANGELOG.md` (1 hunks) * `packages/synapse-interface/constants/tokens/poolMaster.ts` (2 hunks) * `packages/synapse-interface/package.json` (2 hunks) * `packages/synapse-interface/public/blacklist.json` (1 hunks) * `packages/synapse-interface/public/pauses/v1/paused-chains.json` (1 hunks) * `packages/widget/CHANGELOG.md` (1 hunks) * `packages/widget/package.json` (2 hunks) * `services/cctp-relayer/go.mod` (0 hunks) * `services/explorer/go.mod` (0 hunks) * `services/explorer/graphql/server/graph/queryutils.go` (1 hunks) * `services/omnirpc/go.mod` (0 hunks) * `services/rfq/api/db/activequoterequeststatus_string.go` (1 hunks) * `services/rfq/api/db/api_db.go` (1 hunks) * `services/rfq/api/docs/docs.go` (3 hunks) * `services/rfq/api/docs/swagger.json` (1 hunks) * `services/rfq/api/docs/swagger.yaml` (4 hunks) * `services/rfq/api/rest/rfq.go` (5 hunks) * `services/rfq/api/rest/rfq_test.go` (4 hunks) * `services/rfq/api/rest/server.go` (1 hunks) * `services/rfq/api/rest/server_test.go` (4 hunks) * `services/rfq/go.mod` (0 hunks) * `services/rfq/relayer/inventory/circle.go` (1 hunks) * `services/rfq/relayer/inventory/synapse.go` (1 hunks) * `services/rfq/relayer/quoter/quoter.go` (2 hunks) * `services/rfq/relayer/relconfig/getters.go` (2 hunks) * `services/scribe/go.mod` (0 hunks) * `tools/abigen/internal/etherscan/etherscan.go` (1 hunks) </details> <details> <summary>💤 Files with no reviewable changes (15)</summary> * core/metrics/README.md * core/metrics/rookout.go * core/metrics/internal/const.go * core/metrics/base.go * services/cctp-relayer/go.mod * agents/go.mod * services/omnirpc/go.mod * contrib/opbot/go.mod * contrib/screener-api/go.mod * services/rfq/go.mod * contrib/promexporter/go.mod * ethergo/go.mod * core/go.mod * services/explorer/go.mod * services/scribe/go.mod </details> <details> <summary>✅ Files skipped from review due to trivial changes (20)</summary> * .golangci-version * packages/sdk-router/package.json * packages/sdk-router/CHANGELOG.md * services/rfq/relayer/inventory/circle.go * packages/synapse-constants/package.json * services/rfq/api/db/api_db.go * services/rfq/relayer/quoter/quoter.go * docs/bridge/package.json * contrib/golang-ci-lint/.codecov.yml * packages/explorer-ui/package.json * docs/bridge/CHANGELOG.md * contrib/golang-ci-lint/go.mod * packages/synapse-interface/package.json * packages/synapse-constants/CHANGELOG.md * packages/rest-api/CHANGELOG.md * packages/rest-api/package.json * packages/synapse-interface/public/blacklist.json * packages/widget/CHANGELOG.md * packages/explorer-ui/CHANGELOG.md * packages/synapse-interface/CHANGELOG.md </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🪛 Biome (1.9.4)</summary> <details> <summary>packages/sdk-router/src/constants/chainIds.ts</summary> [error] 46-46: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead. See the MDN documentation for more details. Unsafe fix: Use Number.isNaN instead. (lint/suspicious/noGlobalIsNan) </details> </details> <details> <summary>🪛 LanguageTool</summary> <details> <summary>contrib/golang-ci-lint/README.md</summary> [grammar] ~99-~99: The operating system from Apple is written “macOS”. Context: ...amd64, darwin-arm64, windows-amd64 - MacOS (/private/var) symlink handling: -... (MAC_OS) --- [grammar] ~109-~109: The operating system from Apple is written “macOS”. Context: ... - Linux: Standard path resolution - MacOS: Handles /private/var symlinks and temp... (MAC_OS) </details> </details> <details> <summary>🪛 Markdownlint (0.37.0)</summary> <details> <summary>contrib/golang-ci-lint/README.md</summary> 51-51: Column: 1 Hard tabs (MD010, no-hard-tabs) --- 29-29: null Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> <details> <summary>🪛 actionlint (1.7.4)</summary> <details> <summary>.github/workflows/go.yml</summary> 342-342: shellcheck reported issue in this script: SC2086:info:1:45: Double quote to prevent globbing and word splitting (shellcheck) </details> </details> </details> <details> <summary>🔇 Additional comments (73)</summary> <details> <summary>services/rfq/relayer/inventory/synapse.go (1)</summary> `166-167`: **Confidence in chain ID usage.** These additions suitably clarify the reasoning for ignoring security lint checks by stating that the Chain ID values are guaranteed to be positive and within the `uint64` range. To maintain robust safety measures, it would still be prudent to confirm that these assumptions are validated upstream (e.g., ensuring chain IDs are derived from trusted sources). Otherwise, this straightforward casting approach is acceptable. </details> <details> <summary>packages/widget/package.json (2)</summary> `71-71`: **Verify SDK Router version contains Boba network changes.** The sdk-router dependency update (0.11.8 → 0.11.9) should contain the changes for preventing Boba network bridging. Let's verify the SDK changes: --- `4-4`: **Version bump looks appropriate.** The patch version increment (0.9.9 → 0.9.10) aligns with the scope of changes for blocking Boba network bridging functionality. Let's verify the version bump strategy: <details> <summary>✅ Verification successful</summary> **Version bump follows established pattern** The patch version increment (0.9.9 → 0.9.10) is consistent with the project's versioning strategy as evidenced by: - The changelog shows regular patch version increments for non-breaking changes - The version bump aligns with the related SDK router dependency update (0.11.8 → 0.11.9) - The changelog entry indicates "Version bump only" which is appropriate for the scope of Boba network blocking changes </details> <details> <summary>🏁 Scripts executed</summary> The following scripts were executed for the analysis: Script: ```shell #!/bin/bash # Check if this is the only package being bumped fd package.json | xargs rg "0.9.9" # Check commit history for version bumps to understand the versioning pattern git log -n 5 --oneline -- "$(git rev-parse --show-toplevel)/packages/widget/package.json"Length of output: 34
Script:
#!/bin/bash # Let's try a different approach to check version patterns # First, let's see if there are any other occurrences of the old version rg "0\.9\.9" --type json # Then check for any version-related patterns in package.json rg "\"version\":" --type json # Also check for any changelog entries that might help understand versioning fd CHANGELOG -e md | xargs catLength of output: 77882
Script:
#!/bin/bash # Let's check for version patterns in package.json files rg "\"version\":" packages/widget/package.json -A 1 -B 1 # Check for any changelog files in the widget package fd CHANGELOG packages/widget/ # Check for any version-related commits in widget package git log -n 3 --oneline packages/widget/package.jsonLength of output: 237
Script:
#!/bin/bash # Let's check the changelog content for version history cat packages/widget/CHANGELOG.md # Also check for any related changes in the SDK router package rg "\"version\":" packages/sdk-router/package.json -A 1 -B 1Length of output: 20848
services/rfq/api/db/activequoterequeststatus_string.go (1)
19-19: Confirm the adjusted substring range matches the status name length.
The updated index array truncates the substring for the "Closed" status at index 28 instead of 31. Verify that the length matches the actual status name ("Closed") to avoid off-by-one errors.
services/rfq/api/rest/rfq.go (6)
20-23: Good introduction of a base10 constant.
Using a constant for string-to-integer conversion improves readability and maintainability.
173-174: Handle potential parsing errors for aAmount and bAmount.
The _ in SetString ignores parsing failures. Consider error handling in scenarios where invalid numeric strings could be passed.
194-194: Recheck error handling for resp.DestAmount parsing.
Again, we see _ ignoring the parsing success check. If resp.DestAmount is malformed, it might cause subtle issues downstream.
228-228: Check if chain ID validation stands for negative or unrealistic values.
Although the code comment says chain IDs are validated, consider explicitly confirming valid chain ID ranges to protect from edge cases.
244-244: Validate originAmount parse success.
If request.Data.OriginAmountExact is invalid, ignoring the parse result can lead to unexpected behavior.
251-251: Ensure quoteOriginAmount parse result is handled.
When looping over quotes, an invalid numeric string could disrupt the comparison logic and cause silent failures.
services/rfq/api/rest/rfq_test.go (4)
18-25: New validateChainID function.
Helpful utility to guard against negative chain IDs. This is a good practice for ensuring valid input at the test level as well.
73-75: Usage of validateChainID for verifying request data.
This enforces trouble-free chain ID conversions and prevents potential type overflows. Good job.
269-271: Validation approach for origin/destination chain IDs in quotes.
Consistent usage of validateChainID ensures safer data handling.
348-350: Continued chain ID validation in passive quotes.
Maintains consistency and reliability in all test utilities referencing chain IDs.
services/rfq/api/docs/docs.go (3)
257-257: Enhanced description for Active RFQ endpoint.
Reads clearer and better explains the purpose of initiating an Active Request-For-Quote.
270-270: Refined parameter description.
Replacing the old text clarifies the user intent, aligning with the changes in endpoint functionality.
297-297: Improved WebSocket endpoint description.
Explicitly highlighting “streaming active quote requests” clarifies the real-time nature of this API route.
services/rfq/api/rest/server.go (1)
568-568: Use of base10 enhances maintainability
This change clarifies the base consistently across the code, reducing the chance of mistakes when converting from string to big.Int. Ensure base10 remains set to 10 and is well-documented.
services/rfq/api/rest/server_test.go (3)
401-403: Validation method usage is a step in the right direction
Replacing direct assignments with validateChainID(c.originChainID) and validateChainID(c.destChainID) helps safeguard against invalid or negative chain IDs. Confirm that validateChainID handles edge cases (e.g., zero or negative IDs) gracefully by adding corresponding tests.
412-414: Consistent chain ID validation
Again, good practice to funnel chain ID logic through a single validation function. Double-check usage scenarios to confirm all references to c.originChainID and c.destChainID are validated.
423-425: Unified approach to chain ID validation
Centralizing chain ID validation prevents subtle mismatches. If validateChainID can panic on invalid input, ensure thorough test coverage.
services/rfq/relayer/relconfig/getters.go (2)
16-19: Explicit constant extracts magic number
Defining tokenDecimalsBase = 10 improves readability and maintainability, avoiding magic numbers scattered throughout the code.
785-785: Refactoring exponent usage
Adopting tokenDecimalsBase enhances consistency when computing token decimal factors. Ensure that the token decimals align with contract standards for all networks in scope.
services/rfq/api/docs/swagger.yaml (3)
20-20: Renamed property for clarity
Switching from origin_amount to origin_amount_exact clarifies that the value is precise and not an estimate.
139-139: Consistent naming across models
Retaining the same property name (origin_amount_exact) in both request and response models avoids confusion. Good for ensuring uniformity.
338-339: Improved clarity in WebSocket endpoint
The new description clarifies that it streams active quote requests. This helps consumers understand expected behavior.
packages/synapse-interface/constants/tokens/poolMaster.ts (2)
477-477: Confirm the removal of incentives for CANTO_POOL_SWAP_TOKEN.
This switch from true to false directly impacts user rewards. Ensure this aligns with the updated bridging constraints and business strategy.
526-526: Confirm the removal of incentives for CANTO_WRAPPER_POOL_SWAP_TOKEN.
This change might affect user adoption. Double-check that this is intended within the broader context of restricting bridging.
packages/synapse-constants/src/constants/tokens/poolMaster.ts (2)
476-476: Validate the updated incentive structure for CANTO_POOL_SWAP_TOKEN.
Switching from incentivized to non-incentivized could reduce liquidity. Confirm adherence to your bridging restrictions and incentive strategy.
525-525: Validate the updated incentive structure for CANTO_WRAPPER_POOL_SWAP_TOKEN.
Removing incentives can help limit bridging usage. Confirm that this change is intentional and consistent with the new bridging rules.
contrib/golang-ci-lint/main.go (1)
575-602: HTTP client security configuration looks solid!
Your HTTP client has a well-defined timeout, disables compression, sets a minimum TLS version, and includes secure cipher suites. This is a good practice for secure connections.
contrib/golang-ci-lint/permissions/permissions_windows.go (1)
14-17: No-op umask on Windows is acceptable.
Returning a no-op closure to maintain compatibility is a practical approach. This design is clear and straightforward.
contrib/golang-ci-lint/permissions/permissions_unix.go (2)
11-19: Unix permissions logic is appropriate.
Applying syscall.Umask then reverting it ensures secure permissions. The error handling is correct and minimal.
21-27: Umask closure is straightforward.
The returning closure that restores the old umask is a clean approach.
contrib/golang-ci-lint/permissions/permissions.go (1)
21-27: Platform detection for umask is well-handled.
Falling back to a no-op on Windows while calling setUnixUmask on Unix is a good approach to cross-platform compatibility.
make/go.Makefile (1)
31-31: Lint command streamlined well.
Running golang-ci-lint via go run referencing the main.go file is convenient and avoids extra steps. This is a clear improvement over changing directories multiple times.
contrib/golang-ci-lint/main_test.go (1)
10-21: Good coverage of test cases in the struct.
This test suite provides various scenarios (including OS-specific behavior) and is neatly organized with descriptive names. Ensuring coverage for paths outside allowed directories and MacOS-only paths is commendable.
tools/abigen/internal/etherscan/etherscan.go (2)
31-36: Graceful fallback to a generic ETHERSCAN_KEY.
This logic properly checks for a chain-specific key first, then uses the generic one if none is found. This offers flexibility while minimizing code duplication.
43-44: Verbose logging enabled—potential overhead or sensitive data leak.
Setting Verbose: true is helpful for debugging, but consider the potential overhead or sensitive data in logs.
packages/rest-api/src/controllers/bridgeController.ts (1)
56-68: Conditionally adding callData only if both addresses are provided.
This ensures the bridging logic is invoked only when both destAddress and originUserAddress are present. The approach is correct and reduces potential bridging errors.
packages/rest-api/src/tests/bridgeRoute.test.ts (3)
177-185: Positive test for callData generation.
The test confirms callData is generated when both originUserAddress and destAddress are provided. This aligns with the updated controller logic.
204-211: Testing missing destAddress scenario.
Verifying that callData is null when only originUserAddress is given is a solid negative test, ensuring the bridging logic remains consistent.
213-220: Testing missing originUserAddress scenario.
This negative test checks that callData remains null when only destAddress is provided. It completes coverage for all variations.
packages/rest-api/src/routes/bridgeRoute.ts (2)
59-59: Clarification on parameter usage.
Although originUserAddress is marked optional, the description states it is "required to generate callData." This might confuse integrators. Consider either making it truly optional or enforcing it in validation.
65-65: Clarification on parameter usage.
Similar to originUserAddress, destAddress is labeled as optional yet is described as "required to generate callData." Ensure that the code and documentation remain consistent to avoid confusion.
contrib/promexporter/internal/gql/explorer/models.gen.go (2)
55-60: New field addition is consistent with bridging logic.
Adding BridgeModule to BridgeTransaction looks seamless. Ensure that the accompanying logic in your resolvers or controllers properly populates this field for accurate reporting.
65-70: Align new field usage with existing data flows.
The BridgeModule addition in BridgeWatcherTx aligns with the rest of the bridging process. Confirm that any place referencing BridgeWatcherTx handles BridgeModule correctly to avoid nil references or incomplete data in logs or analytics.
services/explorer/graphql/server/graph/queryutils.go (2)
1887-1887: Ensure event coverage.
Handling events < ModuleSynapseCCTP is labeled "SynapseBridge" which is correct. Just confirm that no new eventType values < 10 will be introduced that differ from standard bridging. Keep an eye on backward compatibility.
1889-1889: Clear identification of CCTP events.
Using eventType == ModuleSynapseCCTP properly identifies CCTP events. This is a straightforward approach, and the code is readable.
docker/golang-ci-lint.Dockerfile (4)
1-1: Lean base image choice.
Using distroless/static:latest is a secure and minimal base. This is aligned with best practices for production Docker images.
10-10: Non-root user enforcement.
Running as a non-root user boosts security. Good job following container hardening best practices.
12-13: Ownership alignment for copied files.
Copying the binary with --chown=nonroot:nonroot ensures correct permissions matching the specified user. This helps avoid permission issues at runtime.
15-15: Minimal and direct container entrypoint.
Directly invoking the binary simplifies container usage. Ensure any required environment variables or flags are configured at runtime or in CI steps.
contrib/golang-ci-lint/Makefile (2)
Line range hint 26-29: LGTM: Lint target follows best practices
The lint target properly sets up the environment by running go mod tidy, formatting code, and syncing workspace before linting.
1-1: Verify the existence of the imported Makefile
The imported Makefile path ../../make/go.Makefile should be verified to ensure it exists and is accessible.
contrib/golang-ci-lint/.goreleaser.yml (1)
7-24: LGTM: Build configuration follows security best practices
The build configuration properly:
- Enables static linking
- Disables CGO
- Uses trimpath for reproducible builds
- Includes necessary security flags
.codecov.yml (1)
33-40: LGTM: Coverage flags properly configured
The new flags for golang-ci-lint and opbot are correctly configured with appropriate paths and carryforward settings, maintaining consistency with other monorepo components.
.golangci.yml (1)
143-143: LGTM! Updated linter exclusion for signoz directory.
The change from 'mnd' to 'gomnd' aligns with the current golangci-lint naming convention for the magic number detection linter.
docs/bridge/docs/02-Bridge/02-REST-API.md (1)
29-30: LGTM! Documentation update is clear and consistent.
The new entry follows the existing format and clearly communicates the API change requiring both originUserAddress and destAddress parameters.
CONTRIBUTING.md (2)
74-75: LGTM! Clear explanation of automated linting process.
The documentation clearly explains that golangci-lint version is now managed automatically through tooling.
76-86: Well-structured documentation for the new Golang-CI-Lint module.
The section provides clear instructions for building and testing, with appropriate references to additional documentation.
README.md (2)
98-105: LGTM! Clear Go development setup instructions.
The setup instructions are concise and provide all necessary information for developers to get started with the correct golangci-lint configuration.
106-113: Well-documented version management features.
The key features section effectively communicates the benefits of the automated linting system.
.github/workflows/go.yml (2)
68-68: LGTM: Added version file to trigger linting.
The addition of .golangci-version to the file watch list ensures that linting is triggered when the version configuration changes.
348-348: LGTM: Dynamic version configuration.
Using the version from .golangci-version file makes the linter version management more maintainable.
Also applies to: 354-354
packages/rest-api/swagger.json (1)
204-204: Verify impact of making destAddress optional.
Making destAddress optional in the /bridge endpoint could affect existing integrations. Please ensure:
- Backend code handles the case when
destAddressis not provided - Documentation is updated to reflect this change
- Clients are notified about this breaking change
Run the following script to check for direct usage of this parameter:
✅ Verification successful
Making destAddress optional is properly handled in the codebase
Based on the code analysis, the change to make destAddress optional in the /bridge endpoint is well-handled:
-
The backend code in
bridgeController.tsproperly handles both cases:- When
destAddressis provided (withoriginUserAddress), it generates bridge call data - When
destAddressis not provided, it still returns quotes without call data
- When
-
The change is documented in the OpenAPI spec (swagger.json) and route documentation:
// From bridgeRoute.ts * @param {string} destAddress - The destination address for the bridge transaction (required to generate callData)
-
Test coverage confirms the behavior:
should return bridge quotes with callData when originUserAddress and destAddress are providedshould return bridge quotes without callData when destAddress is not providedshould return 400 for invalid destAddress
-
Input validation is properly implemented:
check('destAddress') .optional() .custom((value) => isAddress(value)) .withMessage('Invalid destAddress')
Note: This is not a breaking change as it makes a previously required parameter optional, maintaining backward compatibility.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for direct usage of destAddress parameter
# Look for direct references to destAddress in controllers and tests
rg -A 5 "destAddress" --type ts --type js
Length of output: 14564
packages/sdk-router/src/constants/testValues.ts (1)
34-35: Add inline comment to maintain consistency.
Everything looks correct. If you want to keep it consistent with other token addresses, consider adding an explanatory comment or using the same naming scheme as other tokens.
packages/sdk-router/src/constants/chainIds.ts (4)
36-40: Implementation of paused chain IDs looks good.
The introduction of PAUSED_CHAIN_IDS array is consistent. This will help ensure the chain is excluded from bridging tasks.
48-48: Ensure correct filtering of paused chains.
The line correctly excludes paused chain IDs. Please confirm there's no scenario where a chain ID might be erroneously type-cast, e.g., from an environment variable or external data source.
63-63: Check for consistent usage in other constants.
This approach is aligned with the patterns in SUPPORTED_CHAIN_IDS. Ensure all relevant arrays filter out paused chain IDs the same way for consistency.
79-79: Validate that paused chain logic is handled appropriately.
Good to see the paused chain IDs included in the filtering step here as well, ensuring no bridging or RFQ is performed on paused chains.
packages/sdk-router/src/sdk.test.ts (1)
1822-1871: Comprehensive Paused Chain Tests.
The new test suite effectively validates behavior when bridging from or to paused chains. This coverage is beneficial for preventing accidental usage of restricted networks. Great job ensuring that both the bridgeQuote and allBridgeQuotes calls properly fail in these scenarios.
packages/synapse-interface/public/pauses/v1/paused-chains.json (1)
52-58: Updated configuration for Boba chain pause.
The revised pause settings for the Boba chain align with the PR's intent. Switching pauseSwap to false while keeping pauseBridge set to true ensures that bridging remains disabled, while swaps may eventually be allowed. This step is consistent with the paused logic introduced in chainIds.ts.
| "parameters": [ | ||
| { | ||
| "type": "integer", | ||
| "description": "origin chain id to filter quotes by", | ||
| "name": "originChainID", | ||
| "in": "path" | ||
| }, | ||
| { | ||
| "type": "string", | ||
| "description": "origin chain id to filter quotes by", | ||
| "name": "originTokenAddr", | ||
| "in": "path" | ||
| }, | ||
| { | ||
| "type": "integer", | ||
| "description": "destination chain id to filter quotes by", | ||
| "name": "destChainID", | ||
| "in": "path" | ||
| }, | ||
| { | ||
| "type": "string", | ||
| "description": "destination token address to filter quotes by", | ||
| "name": "destTokenAddr", | ||
| "in": "path" | ||
| }, | ||
| { | ||
| "type": "string", | ||
| "description": "relayer address to filter quotes by", | ||
| "name": "relayerAddr", | ||
| "in": "path" | ||
| } |
There was a problem hiding this comment.
Add chain validation to prevent Boba network bridging
According to the PR objectives, users should be prevented from bridging to/from the Boba network. However, the /quotes endpoint accepts any chain ID without validation.
Add chain validation in the schema:
"parameters": [
{
"type": "integer",
"description": "origin chain id to filter quotes by",
"name": "originChainID",
- "in": "path"
+ "in": "path",
+ "enum": [1, 137, 42161], // Example: Only allow Ethereum, Polygon, and Arbitrum
+ "x-excluded-chains": [288] // Boba Network chain ID
},
{
"type": "integer",
"description": "destination chain id to filter quotes by",
"name": "destChainID",
- "in": "path"
+ "in": "path",
+ "enum": [1, 137, 42161], // Example: Only allow Ethereum, Polygon, and Arbitrum
+ "x-excluded-chains": [288] // Boba Network chain ID
}
]Committable suggestion skipped: line range outside the PR's diff.
| "/rfq_stream": { | ||
| "get": { | ||
| "description": "Establish a WebSocket connection to listen for streaming active quote requests.", | ||
| "produces": [ | ||
| "application/json" | ||
| ], | ||
| "tags": [ | ||
| "quotes" | ||
| ], | ||
| "summary": "Listen for Active RFQs", | ||
| "responses": { | ||
| "101": { | ||
| "description": "Switching Protocols", | ||
| "schema": { | ||
| "type": "string" | ||
| }, | ||
| "headers": { | ||
| "X-Api-Version": { | ||
| "type": "string", | ||
| "description": "API Version Number - See docs for more info" | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Add security scheme for WebSocket endpoint
The /rfq_stream WebSocket endpoint lacks security definitions, potentially allowing unauthorized access to quote streams.
Add security scheme to the API:
"swagger": "2.0",
+"securityDefinitions": {
+ "ApiKeyAuth": {
+ "type": "apiKey",
+ "in": "header",
+ "name": "X-API-Key"
+ }
+},
+"security": [
+ {
+ "ApiKeyAuth": []
+ }
+],
"paths": {
"/rfq_stream": {
"get": {
+ "security": [
+ {
+ "ApiKeyAuth": []
+ }
+ ],
"description": "Establish a WebSocket connection to listen for streaming active quote requests.",Committable suggestion skipped: line range outside the PR's diff.
| "model.QuoteData": { | ||
| "type": "object", | ||
| "properties": { | ||
| "dest_amount": { | ||
| "type": "string" | ||
| }, | ||
| "dest_chain_id": { | ||
| "type": "integer" | ||
| }, | ||
| "dest_token_addr": { | ||
| "type": "string" | ||
| }, | ||
| "expiration_window": { | ||
| "type": "integer" | ||
| }, | ||
| "origin_amount_exact": { | ||
| "type": "string" | ||
| }, | ||
| "origin_chain_id": { | ||
| "type": "integer" | ||
| }, | ||
| "origin_token_addr": { | ||
| "type": "string" | ||
| }, | ||
| "quote_id": { | ||
| "type": "string" | ||
| }, | ||
| "relayer_address": { | ||
| "type": "string" | ||
| } | ||
| } |
There was a problem hiding this comment.
Enhance QuoteData model with chain validation
The QuoteData model should enforce chain validation to prevent Boba network bridging at the model level.
Add validation annotations to the chain ID fields:
"model.QuoteData": {
"type": "object",
"properties": {
"dest_chain_id": {
"type": "integer",
+ "enum": [1, 137, 42161],
+ "x-excluded-chains": [288],
+ "description": "Destination chain ID (Boba Network not supported)"
},
"origin_chain_id": {
"type": "integer",
+ "enum": [1, 137, 42161],
+ "x-excluded-chains": [288],
+ "description": "Origin chain ID (Boba Network not supported)"
}
},
+ "x-chain-validation": {
+ "description": "Validates that neither origin nor destination is Boba Network",
+ "implementation-notes": "Backend must implement chain validation logic"
+ }
}📝 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.
| "model.QuoteData": { | |
| "type": "object", | |
| "properties": { | |
| "dest_amount": { | |
| "type": "string" | |
| }, | |
| "dest_chain_id": { | |
| "type": "integer" | |
| }, | |
| "dest_token_addr": { | |
| "type": "string" | |
| }, | |
| "expiration_window": { | |
| "type": "integer" | |
| }, | |
| "origin_amount_exact": { | |
| "type": "string" | |
| }, | |
| "origin_chain_id": { | |
| "type": "integer" | |
| }, | |
| "origin_token_addr": { | |
| "type": "string" | |
| }, | |
| "quote_id": { | |
| "type": "string" | |
| }, | |
| "relayer_address": { | |
| "type": "string" | |
| } | |
| } | |
| "model.QuoteData": { | |
| "type": "object", | |
| "properties": { | |
| "dest_amount": { | |
| "type": "string" | |
| }, | |
| "dest_chain_id": { | |
| "type": "integer", | |
| "enum": [1, 137, 42161], | |
| "x-excluded-chains": [288], | |
| "description": "Destination chain ID (Boba Network not supported)" | |
| }, | |
| "dest_token_addr": { | |
| "type": "string" | |
| }, | |
| "expiration_window": { | |
| "type": "integer" | |
| }, | |
| "origin_amount_exact": { | |
| "type": "string" | |
| }, | |
| "origin_chain_id": { | |
| "type": "integer", | |
| "enum": [1, 137, 42161], | |
| "x-excluded-chains": [288], | |
| "description": "Origin chain ID (Boba Network not supported)" | |
| }, | |
| "origin_token_addr": { | |
| "type": "string" | |
| }, | |
| "quote_id": { | |
| "type": "string" | |
| }, | |
| "relayer_address": { | |
| "type": "string" | |
| } | |
| }, | |
| "x-chain-validation": { | |
| "description": "Validates that neither origin nor destination is Boba Network", | |
| "implementation-notes": "Backend must implement chain validation logic" | |
| } |
| func createAndCopyBinary(tr *tar.Reader, tmpFile string) error { | ||
| var errs []error | ||
|
|
||
| // Validate path before file operations | ||
| if err := validatePath(tmpFile, os.TempDir()); err != nil { | ||
| return fmt.Errorf("invalid temporary file path: %w", err) | ||
| } | ||
|
|
||
| // Safe to use os.OpenFile here as path is validated by validatePath above | ||
| //nolint:gosec // G304: path is validated by validatePath above | ||
| file, err := os.OpenFile(tmpFile, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, execPerms) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to create temporary file: %w", err) | ||
| } | ||
| defer func() { | ||
| if closeErr := file.Close(); closeErr != nil { | ||
| errs = append(errs, fmt.Errorf("failed to close file: %w", closeErr)) | ||
| } | ||
| }() | ||
|
|
||
| // Copy with size limit and verify hash | ||
| hasher := sha256.New() | ||
| reader := io.TeeReader(io.LimitReader(tr, maxDecompressSize), hasher) | ||
| if _, err := io.Copy(file, reader); err != nil { | ||
| if removeErr := os.Remove(tmpFile); removeErr != nil { | ||
| errs = append(errs, fmt.Errorf("failed to remove temporary file: %w", removeErr)) | ||
| } | ||
| errs = append(errs, fmt.Errorf("failed to extract file: %w", err)) | ||
| return errors.Join(errs...) | ||
| } | ||
|
|
||
| // Log hash for verification | ||
| hash := hex.EncodeToString(hasher.Sum(nil)) | ||
| fmt.Printf("Extracted file hash: %s\n", hash) | ||
|
|
There was a problem hiding this comment.
Missing cryptographic validation of downloaded files.
You print the SHA256 hash of the file and the extracted binary, but do not compare it against any known/expected hash. This does not actually verify file authenticity. Consider fetching the official SHA checksum from GolangCI-Lint's GitHub release and verifying it before extraction to fully implement the “cryptographic verification” described in the package doc.
| t.Run(tt.name, func(t *testing.T) { | ||
| if tt.onlyDarwin && runtime.GOOS != "darwin" { | ||
| t.Skip("Skipping MacOS-specific test on non-Darwin system") | ||
| } | ||
|
|
||
| err := validatePath(tt.path, tt.allowedDir...) | ||
| if (err != nil) != tt.wantErr { | ||
| t.Errorf("validatePath() error = %v, wantErr %v", err, tt.wantErr) | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
OS-specific path validation is well-covered but needs Windows test cases
The test suite has good coverage for:
- MacOS-specific paths (3 test cases with
onlyDarwinflag) - Generic paths (temp and cache directories)
- Security cases (directory traversal)
However, there are no explicit test cases for Windows-specific paths. Consider adding test cases for:
- Windows absolute paths (e.g.,
C:\Users\...) - Windows UNC paths (e.g.,
\\server\share\...) - Windows-style directory separators
Example locations to add tests:
contrib/golang-ci-lint/main_test.go: Add Windows-specific test cases withruntime.GOOS == "windows"checks
🔗 Analysis chain
Verify handling of OS-specific paths.
When onlyDarwin is set and the system is non-Darwin, the test is skipped—this aligns with real-world MacOS constraints. Ensure that all Linux- or Windows-specific paths, if present, are also tested.
Run this script to search for validatePath usage outside these tests, to confirm coverage of other OS-specific references:
Let me check the test cases for OS-specific paths.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify references to validatePath in the entire codebase
rg -A 5 "validatePath"
Length of output: 4550
Script:
#!/bin/bash
# Check test cases in main_test.go for OS-specific paths
cat contrib/golang-ci-lint/main_test.go
Length of output: 1961
| - '--label=org.opencontainers.image.revision={{.FullCommit}}' | ||
| - '--label=org.opencontainers.image.version={{.Version}}' | ||
| - '--label=org.opencontainers.image.source={{.GitURL}}' | ||
| dockerfile: ../../docker/golang-ci-lint.Dockerfile |
There was a problem hiding this comment.
💡 Codebase verification
The Dockerfile path needs to be updated
The Dockerfile exists at docker/golang-ci-lint.Dockerfile, but the path in the configuration file is incorrect (../../docker/golang-ci-lint.Dockerfile). The path should be relative to the configuration file's location.
- Update the path in
contrib/golang-ci-lint/.goreleaser.ymlto use the correct relative path:../../docker/golang-ci-lint.Dockerfile→../docker/golang-ci-lint.Dockerfile
🔗 Analysis chain
Verify the existence of the Dockerfile
The referenced Dockerfile path ../../docker/golang-ci-lint.Dockerfile should be verified.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the Dockerfile exists
test -f ../../docker/golang-ci-lint.Dockerfile && echo "Dockerfile exists" || echo "Dockerfile not found"
Length of output: 99
Script:
#!/bin/bash
# Let's search for the Dockerfile in the repository
fd golang-ci-lint.Dockerfile
# Also check if there are any Dockerfiles in the docker directory
fd Dockerfile docker/
Length of output: 620
| - name: Read golangci-lint version | ||
| id: golangci_version | ||
| run: echo "version=v$(cat .golangci-version)" >> $GITHUB_OUTPUT | ||
|
|
There was a problem hiding this comment.
Fix potential word splitting in version reading.
The script that reads the version might be susceptible to word splitting if the version contains spaces.
Apply this diff to fix the potential issue:
- - name: Read golangci-lint version
- id: golangci_version
- run: echo "version=v$(cat .golangci-version)" >> $GITHUB_OUTPUT
+ - name: Read golangci-lint version
+ id: golangci_version
+ run: echo "version=v$(cat ".golangci-version")" >> "$GITHUB_OUTPUT"📝 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.
| - name: Read golangci-lint version | |
| id: golangci_version | |
| run: echo "version=v$(cat .golangci-version)" >> $GITHUB_OUTPUT | |
| - name: Read golangci-lint version | |
| id: golangci_version | |
| run: echo "version=v$(cat ".golangci-version")" >> "$GITHUB_OUTPUT" | |
🧰 Tools
🪛 actionlint (1.7.4)
342-342: shellcheck reported issue in this script: SC2086:info:1:45: Double quote to prevent globbing and word splitting
(shellcheck)
| * List of paused chain IDs | ||
| */ | ||
| export const PAUSED_CHAIN_IDS: SupportedChainId[] = [SupportedChainId.BOBA] | ||
|
|
||
| /** | ||
| * List of supported chain ids, where SynapseBridge is deployed. | ||
| */ | ||
| export const SUPPORTED_CHAIN_IDS: number[] = Object.values(SupportedChainId) | ||
| .map((chainId) => Number(chainId)) | ||
| .filter((chainId) => !isNaN(chainId)) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace global isNaN with Number.isNaN.
Using isNaN(chainId) can result in unexpected type coercions. Consider updating it to Number.isNaN(chainId) to avoid potential pitfalls.
🧰 Tools
🪛 Biome (1.9.4)
[error] 46-46: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (22)
packages/rest-api/src/tests/bridgeRoute.test.ts (1)
177-185: Ensure clarity in test behavior regardingdestAddressrequirement.
The newly introduced test verifies the presence of bothoriginUserAddressanddestAddressto producecallData. This aligns with the updated controller logic. However, consider clarifying the test name or adding a comment so that future maintainers quickly understand the conditions under which call data is returned.packages/rest-api/swagger.json (2)
Line range hint
1-1000: Documentation improvements neededThe API documentation could be enhanced in several areas:
- Add examples showing bridge requests without
destAddress- Document the fallback behavior when
destAddressis not provided- Update the description to explain when
destAddresscan be omittedConsider adding these clarifications to improve the documentation:
"parameters": [ { "in": "query", "name": "destAddress", "required": false, "schema": { "type": "string" }, - "description": "The destination address of the user on the destination chain" + "description": "The destination address of the user on the destination chain. If not provided, the bridge transaction will use the originUserAddress as the destination address." } ],
Line range hint
1-1000: Ensure consistency across deprecated endpointsThe deprecated
/bridgeTxInfoendpoint still showsdestAddressas required. While this endpoint is marked as deprecated, it should maintain consistency with the active endpoints to avoid confusion.Consider updating the deprecated endpoint to match the new parameter requirements:
"/bridgeTxInfo": { "parameters": [ { "in": "query", "name": "destAddress", - "required": true, + "required": false, "schema": { "type": "string" }, - "description": "The destination address for the bridged tokens" + "description": "The destination address for the bridged tokens. If not provided, the bridge transaction will use the originUserAddress as the destination address." } ] }contrib/golang-ci-lint/permissions/permissions.go (2)
1-2: Elevate package-level documentation for better usage clarity.
Consider expanding the package comment to detail how to integrate these permission settings in different environments, or to reference usage examples. This helps future contributors quickly grasp the package's value without scanning through the entire file.
4-8: Consider contextual error handling improvements.
When importing packages likeruntimeandos, it might be beneficial to import a logging library to uniformly handle and log errors across different OS platforms. This can provide richer diagnostic information if something goes wrong.services/rfq/api/rest/rfq.go (2)
194-194: Enhance error handling when parsingDestAmount.Currently, the code logs an error if parsing fails. Consider also returning or handling the parsing failure more gracefully to avoid potential downstream issues.
228-228: Document rationale for nolint.The
//nolint:goseccomment implies chain IDs are validated by the API. Consider adding a reference or docstring pointing to the validation site or function for clarity.services/rfq/api/rest/rfq_test.go (1)
18-25: Robust chain ID validation.
validateChainIDprovides a safe guard against negative chain IDs. Consider returning an error instead of panicking to ensure graceful error handling in production.contrib/golang-ci-lint/main.go (5)
87-102: Consider supporting additional validation for pre-release or custom GitHub URLs.Right now,
validateURLstrictly enforces that the URL must be fromgithub.com/golangci/golangci-lint/releases. While this is correct for most use cases, you might consider allowing or rejecting specific pre-release or nightly builds if required.
104-175: Refactor to reduce cyclomatic complexity.The
validatePathfunction is quite extensive and can be made more modular. Extracting MacOS-specific path normalization and top-level directory checks into helper functions will improve readability and testability.
256-304: Enhance argument parsing clarity.The
processArgsfunction contains multiple responsibilities, including parsing--path, injecting defaults, and replacing$(GIT_ROOT). Splitting these concerns into smaller functions would make the code easier to maintain.
514-528: Graceful shutdown or signal handling might be beneficial.The
mainfunction relies onrun(context.Background())without advanced signal or timeout handling. Consider passing a cancellable context derived from OS signals for a more robust experience if lint runs must be interrupted.
603-641: Potential improvement for bandwidth-limited or unstable networks.
downloadFilestreams the response directly to disk with a 50MB limit. For users on slow or unstable connections, you could consider progressive verification or chunked downloading with retries. This might help avoid partial downloads or having to restart from scratch.tools/abigen/internal/etherscan/etherscan.go (2)
31-36: Ensure that fallback logic is well-documented.The environment variable fallback from chain-specific keys to the generic
ETHERSCAN_KEYis quite useful. However, it would help to add an in-code comment clarifying how these chain-specific environment variables (e.g.,1_KEY,10_KEY) are expected to be defined, to avoid confusion for future maintainers.
43-44: Good addition of verbose logging.Enabling verbose logging can help debug issues, especially when dealing with third-party API requests. However, consider providing a configuration toggle or command-line flag if there's a need to disable verbose logs in production for performance or noise reasons.
contrib/promexporter/internal/gql/explorer/models.gen.go (1)
65-70: Add matching documentation for theBridgeModulefield inBridgeWatcherTx.The new field is consistent with
BridgeTransaction. Ensure references in code or generated docs mention what values can be assigned, so that integrators know how to handle the new data.contrib/golang-ci-lint/Makefile (2)
Line range hint
19-21: Consider adding error handling to the lint target.The lint target runs multiple commands in sequence without checking for errors. Consider adding error handling and making the target fail-fast.
lint: ## lint lints the code with golangci-lint - go mod tidy - go fmt ./... - go work sync - cd $(CURRENT_PATH) && go run $(GIT_ROOT)/contrib/golang-ci-lint/main.go run --fix --config=$(GIT_ROOT)/.golangci.yml + go mod tidy || exit 1 + go fmt ./... || exit 1 + go work sync || exit 1 + cd $(CURRENT_PATH) && go run $(GIT_ROOT)/contrib/golang-ci-lint/main.go run --fix --config=$(GIT_ROOT)/.golangci.yml || exit 1
Line range hint
15-17: Consider a safer docker-clean target.The current docker-clean target stops and removes all containers, which might affect other projects. Consider limiting it to only golang-ci-lint related containers.
docker-clean: ## stops and removes all containers at once - docker ps -aq | xargs docker stop | xargs docker rm + docker ps -a --filter "name=golang-ci-lint" -q | xargs -r docker stop | xargs -r docker rm docker network prunecontrib/golang-ci-lint/.goreleaser.yml (1)
21-24: Consider supporting more architectures.Currently limited to linux/amd64. Consider adding support for arm64 and other common architectures.
goos: - linux goarch: - amd64 + - arm64contrib/golang-ci-lint/README.md (1)
37-38: Consider adding validation for the config flag.The command example uses
--config=.golangci.ymlwithout mentioning config file requirements or validation.Add a note about config file validation or provide a default config template.
CONTRIBUTING.md (1)
76-86: Consider adding examples for common linting scenarios.While the module description is good, it would be helpful to include examples of common linting issues and how to resolve them.
Add a subsection with examples of common linting issues and their solutions.
README.md (1)
106-113: Consider adding troubleshooting section for common setup issues.While the version management documentation is good, it would be helpful to include common issues developers might encounter during setup.
Add a troubleshooting subsection addressing common setup issues and their solutions.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (13)
agents/go.sumis excluded by!**/*.sumcontrib/golang-ci-lint/go.sumis excluded by!**/*.sumcontrib/opbot/go.sumis excluded by!**/*.sumcontrib/promexporter/go.sumis excluded by!**/*.sumcontrib/screener-api/go.sumis excluded by!**/*.sumcore/go.sumis excluded by!**/*.sumethergo/go.sumis excluded by!**/*.sumgo.workis excluded by!**/*.workservices/cctp-relayer/go.sumis excluded by!**/*.sumservices/explorer/go.sumis excluded by!**/*.sumservices/omnirpc/go.sumis excluded by!**/*.sumservices/rfq/go.sumis excluded by!**/*.sumservices/scribe/go.sumis excluded by!**/*.sum
📒 Files selected for processing (76)
.codecov.yml(1 hunks).github/workflows/go.yml(2 hunks).gitignore(1 hunks).golangci-version(1 hunks).golangci.yml(2 hunks)CONTRIBUTING.md(1 hunks)README.md(1 hunks)agents/go.mod(0 hunks)contrib/golang-ci-lint/.codecov.yml(1 hunks)contrib/golang-ci-lint/.goreleaser.yml(1 hunks)contrib/golang-ci-lint/Makefile(1 hunks)contrib/golang-ci-lint/README.md(1 hunks)contrib/golang-ci-lint/go.mod(1 hunks)contrib/golang-ci-lint/main.go(1 hunks)contrib/golang-ci-lint/main_test.go(1 hunks)contrib/golang-ci-lint/permissions/permissions.go(1 hunks)contrib/golang-ci-lint/permissions/permissions_unix.go(1 hunks)contrib/golang-ci-lint/permissions/permissions_windows.go(1 hunks)contrib/opbot/go.mod(0 hunks)contrib/promexporter/go.mod(0 hunks)contrib/promexporter/internal/gql/explorer/models.gen.go(1 hunks)contrib/screener-api/go.mod(0 hunks)core/go.mod(0 hunks)core/metrics/README.md(0 hunks)core/metrics/base.go(0 hunks)core/metrics/internal/const.go(0 hunks)core/metrics/rookout.go(0 hunks)docker/golang-ci-lint.Dockerfile(1 hunks)docs/bridge/CHANGELOG.md(1 hunks)docs/bridge/docs/02-Bridge/02-REST-API.md(1 hunks)docs/bridge/package.json(2 hunks)ethergo/go.mod(0 hunks)make/go.Makefile(1 hunks)packages/explorer-ui/CHANGELOG.md(1 hunks)packages/explorer-ui/package.json(2 hunks)packages/rest-api/CHANGELOG.md(1 hunks)packages/rest-api/package.json(2 hunks)packages/rest-api/src/controllers/bridgeController.ts(1 hunks)packages/rest-api/src/routes/bridgeRoute.ts(1 hunks)packages/rest-api/src/tests/bridgeRoute.test.ts(2 hunks)packages/rest-api/swagger.json(1 hunks)packages/sdk-router/CHANGELOG.md(1 hunks)packages/sdk-router/package.json(1 hunks)packages/sdk-router/src/constants/chainIds.ts(3 hunks)packages/sdk-router/src/constants/testValues.ts(1 hunks)packages/sdk-router/src/sdk.test.ts(2 hunks)packages/synapse-constants/CHANGELOG.md(1 hunks)packages/synapse-constants/package.json(1 hunks)packages/synapse-constants/src/constants/tokens/poolMaster.ts(2 hunks)packages/synapse-interface/CHANGELOG.md(1 hunks)packages/synapse-interface/constants/tokens/poolMaster.ts(2 hunks)packages/synapse-interface/package.json(2 hunks)packages/synapse-interface/public/blacklist.json(1 hunks)packages/synapse-interface/public/pauses/v1/paused-chains.json(1 hunks)packages/widget/CHANGELOG.md(1 hunks)packages/widget/package.json(2 hunks)services/cctp-relayer/go.mod(0 hunks)services/explorer/go.mod(0 hunks)services/explorer/graphql/server/graph/queryutils.go(1 hunks)services/omnirpc/go.mod(0 hunks)services/rfq/api/db/activequoterequeststatus_string.go(1 hunks)services/rfq/api/db/api_db.go(1 hunks)services/rfq/api/docs/docs.go(3 hunks)services/rfq/api/docs/swagger.json(1 hunks)services/rfq/api/docs/swagger.yaml(4 hunks)services/rfq/api/rest/rfq.go(5 hunks)services/rfq/api/rest/rfq_test.go(4 hunks)services/rfq/api/rest/server.go(1 hunks)services/rfq/api/rest/server_test.go(4 hunks)services/rfq/go.mod(0 hunks)services/rfq/relayer/inventory/circle.go(1 hunks)services/rfq/relayer/inventory/synapse.go(1 hunks)services/rfq/relayer/quoter/quoter.go(2 hunks)services/rfq/relayer/relconfig/getters.go(2 hunks)services/scribe/go.mod(0 hunks)tools/abigen/internal/etherscan/etherscan.go(1 hunks)
💤 Files with no reviewable changes (15)
- core/metrics/rookout.go
- core/metrics/internal/const.go
- services/cctp-relayer/go.mod
- core/metrics/README.md
- contrib/screener-api/go.mod
- contrib/opbot/go.mod
- core/metrics/base.go
- services/omnirpc/go.mod
- contrib/promexporter/go.mod
- core/go.mod
- services/rfq/go.mod
- agents/go.mod
- ethergo/go.mod
- services/scribe/go.mod
- services/explorer/go.mod
✅ Files skipped from review due to trivial changes (19)
- .golangci-version
- packages/sdk-router/package.json
- packages/synapse-constants/package.json
- services/rfq/relayer/inventory/circle.go
- services/rfq/api/db/api_db.go
- packages/widget/CHANGELOG.md
- services/rfq/relayer/quoter/quoter.go
- contrib/golang-ci-lint/go.mod
- packages/explorer-ui/package.json
- packages/synapse-interface/package.json
- docs/bridge/package.json
- services/rfq/api/docs/docs.go
- packages/synapse-constants/CHANGELOG.md
- contrib/golang-ci-lint/.codecov.yml
- packages/synapse-interface/CHANGELOG.md
- packages/explorer-ui/CHANGELOG.md
- packages/rest-api/CHANGELOG.md
- docs/bridge/CHANGELOG.md
- packages/sdk-router/CHANGELOG.md
🧰 Additional context used
🪛 Biome (1.9.4)
packages/sdk-router/src/constants/chainIds.ts
[error] 46-46: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
🪛 LanguageTool
contrib/golang-ci-lint/README.md
[grammar] ~99-~99: The operating system from Apple is written “macOS”.
Context: ...amd64, darwin-arm64, windows-amd64 - MacOS (/private/var) symlink handling: -...
(MAC_OS)
[grammar] ~109-~109: The operating system from Apple is written “macOS”.
Context: ... - Linux: Standard path resolution - MacOS: Handles /private/var symlinks and temp...
(MAC_OS)
🪛 Markdownlint (0.37.0)
contrib/golang-ci-lint/README.md
51-51: Column: 1
Hard tabs
(MD010, no-hard-tabs)
29-29: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 actionlint (1.7.4)
.github/workflows/go.yml
342-342: shellcheck reported issue in this script: SC2086:info:1:45: Double quote to prevent globbing and word splitting
(shellcheck)
🔇 Additional comments (61)
packages/synapse-interface/constants/tokens/poolMaster.ts (2)
477-477: Changing the incentive status from true to false.
This update ensures that the CANTO pool is no longer incentivized. Verify that front-end representations and staking logic properly reflect this change.
526-526: Removing incentives from the wrapper pool.
The CANTO wrapper pool is also changed to a non-incentivized status. Confirm that no references assume incentives remain enabled.
packages/synapse-constants/src/constants/tokens/poolMaster.ts (2)
476-476: Consistency check on pool incentivization.
Switching incentivized to false aligns this constant with the interface counterpart. Ensure that nothing else in the codebase depends on the old incentivized setting.
525-525: Disabling wrappers’ incentive for CANTO.
Confirm that any wrapper interactions still function correctly without the incentive.
packages/rest-api/src/tests/bridgeRoute.test.ts (2)
204-211: Test coverage for missing destAddress looks good.
These lines properly validate that if destAddress is omitted, the response object’s callData is null, adhering to the revised logic. This ensures that bridging proceeds without additional call data.
213-220: Sufficient test verification for missing originUserAddress.
Similarly, these lines confirm correct behavior when originUserAddress is excluded; no call data is generated, as expected under the new requirement for both addresses.
packages/rest-api/src/routes/bridgeRoute.ts (1)
59-65: Documentation clarity on required parameters for call data.
The updated descriptions for originUserAddress and destAddress emphasize that both are needed to generate call data. This matches the logic in the controller and the test cases. Good job keeping the docs consistent.
packages/rest-api/src/controllers/bridgeController.ts (1)
56-68: Appropriate conditional check for callData generation.
By verifying that both destAddress and originUserAddress exist before calling Synapse.bridge, you ensure that the bridging flow only proceeds with complete parameters. This is a clear and maintainable approach.
docs/bridge/docs/02-Bridge/02-REST-API.md (1)
29-30: Version notes updated to reflect new bridging requirements.
These lines effectively communicate that both originUserAddress and destAddress must be supplied to generate call data. This is an important piece of documentation to keep users informed of changes.
packages/rest-api/swagger.json (1)
204-204:
Verify the implications of making destAddress optional
Making the destAddress parameter optional in the /bridge endpoint could have security implications and needs careful consideration:
- If
destAddressis not provided, where will the bridged tokens be sent? - How does this change affect the generation of
callDatamentioned in the AI summary?
Let's verify the usage and handling of destAddress:
✅ Verification successful
Making destAddress optional is safe and intentionally designed
Based on the code analysis, making destAddress optional in the /bridge endpoint is a deliberate design choice with proper handling:
- The
bridgeController.tsshows thatdestAddressis used conditionally withoriginUserAddressto generatecallData:
destAddress && originUserAddress
? await Synapse.bridge(
destAddress,
quote.routerAddress,
...
)- The test file
bridgeRoute.test.tsexplicitly verifies both scenarios:
- "should return bridge quotes with callData when originUserAddress and destAddress are provided"
- "should return bridge quotes without callData when destAddress is not provided"
- The OpenAPI documentation in
bridgeRoute.tscorrectly describes this behavior:
description: The destination address for the bridge transaction (required to generate callData)
The endpoint returns quotes regardless of destAddress, but only generates callData when both destAddress and originUserAddress are provided. This allows for quote exploration without requiring a destination address upfront.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for references to destAddress in the codebase
echo "Searching for destAddress usage..."
rg -A 5 "destAddress" --type ts --type js
# Search for bridge-related function implementations
echo "Searching for bridge function implementations..."
ast-grep --pattern 'function $FUNC($$$) {
$$$
destAddress
$$$
}'
Length of output: 14823
packages/rest-api/package.json (1)
3-3: Verify version compatibility and stability
The package version has been bumped along with updates to key dependencies. Let's verify these changes:
Also applies to: 25-26
✅ Verification successful
Version updates verified and stable
The verification confirms that both dependency updates are valid and published:
- @synapsecns/[email protected] exists in the npm registry
- @synapsecns/[email protected] exists in the npm registry
Let's verify if these are breaking changes by checking the version increments:
Version updates are compatible and safe
The verification confirms that all version changes are patch-level updates following semantic versioning:
- @synapsecns/sdk-router: 0.11.8 → 0.11.9 (patch update)
- @synapsecns/synapse-constants: 1.8.5 → 1.8.6 (patch update)
Both the current and previous versions exist in the npm registry, and the increments indicate backward-compatible bug fixes, making these updates safe to apply.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify package versions exist and check for any breaking changes
# Check if the SDK router version exists and get its changelog
echo "Checking @synapsecns/[email protected]..."
npm view @synapsecns/[email protected] version
# Check if synapse-constants version exists
echo "Checking @synapsecns/[email protected]..."
npm view @synapsecns/[email protected] version
# Look for any breaking changes or Boba-related commits
echo "Checking for Boba-related changes..."
gh api repos/synapsecns/sanguine/commits --jq '.[] | select(.commit.message | contains("boba") or contains("Boba"))'
Length of output: 516
Script:
#!/bin/bash
# Check the previous versions to confirm these are non-breaking updates
echo "Checking previous SDK router version..."
npm view @synapsecns/[email protected] version
echo "Checking previous synapse-constants version..."
npm view @synapsecns/[email protected] version
# Look for any breaking changes in package.json history
git log -p --no-merges -10 -- packages/rest-api/package.json
Length of output: 308
contrib/golang-ci-lint/permissions/permissions.go (2)
21-27: Ensure cross-platform consistency via tests.
The SetSecureUmask function correctly defers to a Unix-specific implementation or a no-op on Windows. However, cross-platform tests are important to confirm that the secure umask settings do not inadvertently regress on any supported platforms.
Would you like me to provide test scaffolding for verifying the umask behavior on multiple operating systems?
29-38: Confirm fallback in non-Unix error cases.
When setting file permissions on non-Unix systems, confirm that the fallback behavior is clear. In the rare event of an error on Windows, should the system continue or fail fast? Evaluate whether the operation should propagate the error or attempt retries if secure permissions remain necessary.
services/rfq/api/docs/swagger.yaml (2)
313-314: LGTM! Clear and improved endpoint descriptions
The updated descriptions better clarify the active nature of RFQ operations and the WebSocket connection for streaming, enhancing API documentation quality.
Also applies to: 338-339
20-20: Verify handling of exact amounts across bridge operations
The rename from origin_amount to origin_amount_exact suggests a focus on precision. This change aligns with addressing Boba network bridging issues, but we should ensure consistent handling of exact amounts.
Also applies to: 139-139
✅ Verification successful
Property rename is consistent across the codebase
The rename from origin_amount to origin_amount_exact has been properly implemented across the API models and database schema. The change is consistently applied in:
- API models:
GetOpenQuoteRequestsResponseandQuoteDatamodels in swagger definitions - Database: Column renamed in
api_db.go - Business logic: Updated in quoter service for amount validation
- Client code: Updated in generated client code
The rename reflects a more precise handling of amounts in bridge operations, with the _exact suffix emphasizing precision requirements. The change is complete and consistent.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining instances of origin_amount to ensure complete migration
rg "origin_amount[^_]" --type go --type typescript
# Search for amount handling in bridge-related code
rg -A 5 "amount.*exact" --type go --type typescript
Length of output: 176
Script:
#!/bin/bash
# Search for any remaining instances of origin_amount to ensure complete migration
rg "origin_amount[^_]"
# Search for amount handling in bridge-related code
rg -A 5 "amount.*exact"
# Find all yaml/json files that might contain API specs
fd -e yaml -e json -e yml
# Check content of found swagger files
fd -e yaml -e yml -e json | grep -i "swagger\|openapi" | xargs cat
Length of output: 74855
Script:
#!/bin/bash
# Search for references to origin_amount in the swagger file
cat services/rfq/api/docs/swagger.yaml
# Search for references to amount_exact in the swagger file
cat services/rfq/api/docs/swagger.json
Length of output: 27761
Script:
#!/bin/bash
# Search for references to origin_amount in Go code
rg "origin_amount" --type go -A 2 -B 2
# Search for references to amount_exact in Go code
rg "amount_exact" --type go -A 2 -B 2
Length of output: 11951
services/rfq/api/docs/swagger.json (1)
339-340: LGTM! Consistent documentation across formats
The property rename from origin_amount to origin_amount_exact is consistently applied in both YAML and JSON formats, maintaining documentation integrity.
Also applies to: 503-504
packages/sdk-router/src/constants/testValues.ts (1)
34-35: Added BOBA_USDC constant looks correct.
The address format is the standard 42-character hex string, and the naming convention is consistent with other token constants. This looks good.
packages/sdk-router/src/constants/chainIds.ts (4)
36-40: Clear definition of PAUSED_CHAIN_IDS for Boba.
Adding SupportedChainId.BOBA to a dedicated paused list is a straightforward way to control bridging logic for that chain. This increase in clarity is beneficial for maintainability.
48-48: Proper exclusion of paused chains from SUPPORTED_CHAIN_IDS.
Filtering out the paused chain IDs helps ensure that no bridging routes are incorrectly enabled for these chains. This is consistent with the pause logic.
63-63: Exclusion of paused chains from CCTP_SUPPORTED_CHAIN_IDS is consistent.
Maintaining a similar filter approach for CCTP keeps the logic consistent across all bridging modules.
79-79: RFQ_SUPPORTED_CHAIN_IDS also filters paused chain IDs.
Aligning the RFQ flow with the same filtering approach ensures consistent bridging restrictions.
packages/sdk-router/src/sdk.test.ts (1)
1822-1871: Comprehensive test coverage for paused chain scenarios.
These tests thoroughly check that no quotes are returned when either the origin or destination is paused. This is a solid approach to validating pause logic.
packages/synapse-interface/public/pauses/v1/paused-chains.json (1)
52-58: Duplicate “boba-chain-pause” entry. Please verify intent.
Having two entries for the same id (“boba-chain-pause”) can lead to ambiguity or unintended overrides. Verify that this duplication is expected or whether it should be merged into a single entry.
services/rfq/relayer/inventory/synapse.go (1)
166-167: Double-check chain ID validation.
Even though the comment asserts chain IDs are always positive, consider adding a runtime check or assertion to safeguard against accidental negative values or invalid inputs.
services/rfq/api/rest/rfq.go (4)
20-23: Well-defined base constant usage.
Defining base10 helps improve readability and ensures consistent base usage for string-to-integer conversions.
173-174: Consistent base10 usage for conversion.
Using base10 for conversions creates a standardized approach and avoids hardcoded values.
244-244: Validate originAmount parsing.
If parsing fails, you return an error. This is good; confirm that you properly handle or log all parse failures here to avoid silent issues.
251-251: Check big.Int parsing fallback logic.
Similar to the line above, ensure that any parse failures on MaxOriginAmount do not cause unhandled behaviors.
services/rfq/api/rest/rfq_test.go (3)
73-73: Confirm consistent usage of validation.
You are using validateChainID to confirm OriginChainID and DestChainID. Good practice—double-check that all critical chain ID references call this function.
Also applies to: 75-75
269-269: Validate chain ID on passive quotes too.
Applying validateChainID when upserting passive quotes ensures the same chain ID guarantees and consistency. Good job.
Also applies to: 271-271
348-348: Ensure all test coverage paths are exercised.
Similarly, here validateChainID helps maintain the same validation logic. Make sure tests cover negative chain ID scenarios and confirm the panic or error handling behavior.
Also applies to: 350-350
services/rfq/api/rest/server.go (1)
568-568: Consistent use of a named base constant.
Replacing the hardcoded 10 with base10 clarifies intent and fosters maintainability. Great job.
services/rfq/api/rest/server_test.go (4)
401-403: Validate negative chain ID handling
The use of validateChainID here is a good way to ensure no invalid (e.g., negative) chain IDs proceed. However, please verify that you are testing scenarios where c.originChainID and c.destChainID might be negative to confirm the panic behavior and guarantee full coverage.
412-414: Reinforce chain ID validation tests
As above, confirm that your tests appropriately handle edge cases in which chain IDs could be invalid, ensuring the function's panic behavior is consistent with expectations.
423-425: Maintain consistent use of validateChainID
Using validateChainID consistently helps catch invalid chain IDs early. Just ensure your tests guarantee no unintended panics by covering boundary cases.
434-436: Good practice with chain ID validation
Continuing the consistent usage of validateChainID is beneficial for improved safety. Follow up with robust negative testing to maintain stability and reliability in your test suite.
services/rfq/relayer/relconfig/getters.go (2)
16-18: Clearer use of named constant
Defining tokenDecimalsBase enhances readability, as the magic number is replaced with a well-named constant. This makes the code more maintainable.
785-785: Appropriate usage of tokenDecimalsBase
Replacing the hard-coded base with tokenDecimalsBase is a sensible improvement, ensuring consistency across the code for token decimal exponentiation.
contrib/golang-ci-lint/main.go (2)
1-37: Comprehensive module documentation looks solid.
The high-level overview of security features (e.g., validating file paths, enforcing file permissions, limiting archive size) is thorough and crucial for a tool that handles external downloads.
503-512: Validate GIT_ROOT usage outside of internal scripts.
setupGitRoot sets an environment variable that might be used by other scripts or processes. Ensure these external scripts or processes also handle missing or invalid GIT_ROOT values gracefully and provide an informative error if the repository root cannot be found.
contrib/golang-ci-lint/permissions/permissions_windows.go (2)
9-12: Stub implementation for Windows looks sufficient.
On Windows, Unix-like permissions have limited applicability. The approach of directly calling os.Chmod with the given perm is reasonable for compatibility.
14-17: No-op functions maintain cross-platform consistency.
Providing a no-op setUnixUmask function to match the Unix interface ensures consistent usage across different operating systems.
contrib/golang-ci-lint/permissions/permissions_unix.go (2)
11-19: Ensure safe umask restoration.
The code correctly restores the old umask in a deferred call. This practice prevents potential race conditions if multiple system calls manipulate umask concurrently, though it might still pose concurrency risks in multi-threaded applications.
21-27: Cross-platform fallback logic is well-structured.
Wrapping syscall.Umask in a closure for later restoration is neat and allows for uniform usage in other modules. This helps keep permission handling consistent across the codebase.
services/rfq/api/db/activequoterequeststatus_string.go (1)
19-19: Index array update reflects string length correction.
Reducing the last element from 31 to 28 accurately matches the combined length of the status strings. Verify that any references to this string array are still valid.
make/go.Makefile (1)
31-31: Streamlined lint command.
Moving the lint invocation to a single line using go run with the custom tool is a tidy improvement, ensuring future maintainers can quickly see how lint is orchestrated.
contrib/golang-ci-lint/main_test.go (1)
1-81: Keep up the solid test coverage!
All the newly introduced lines appear to collectively test the validatePath function thoroughly with a variety of scenarios: valid MacOS paths, directory traversal attempts, and checks on allowed vs. disallowed directories. This coverage is helpful for ensuring robust path validation logic.
contrib/promexporter/internal/gql/explorer/models.gen.go (1)
55-60: New BridgeModule field in BridgeTransaction is a helpful addition.
Including a bridgeModule field (with omitempty) allows the UI and downstream consumers to clearly understand which bridging mechanism was used. This can simplify troubleshooting and analytics.
services/explorer/graphql/server/graph/queryutils.go (1)
1875-1884: Constants for module classification improve code clarity.
Defining ModuleSynapseBridge, ModuleSynapseCCTP, and ModuleSynapseRFQ at the top is an excellent way to avoid magic numbers. Great job!
docker/golang-ci-lint.Dockerfile (3)
1-1: LGTM: Using distroless base image enhances security.
Using gcr.io/distroless/static:latest is a good security practice as it minimizes the attack surface.
3-8: LGTM: Well-documented image metadata.
The labels provide comprehensive metadata following the Open Container Initiative (OCI) standards.
13-13: Verify the source of the golang-ci-lint binary.
Ensure the binary is built from a trusted source and its integrity is verified before copying into the image.
✅ Verification successful
The golang-ci-lint binary is built from a trusted source with proper version management
Based on the documentation and Makefile, the golang-ci-lint binary is managed through a secure version management system:
- It's built using the official golang-ci-lint manager in
contrib/golang-ci-lint - The version is pinned in
.golangci-versionfile - The manager verifies binary checksums for security
- The binary is downloaded from official sources with proper architecture support
- The system is integrated into the project's build process via Makefile
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the binary exists and verify its source
if [ -f "golang-ci-lint" ]; then
# Get file info and hash
file golang-ci-lint
sha256sum golang-ci-lint
fi
Length of output: 27
Script:
#!/bin/bash
# Let's check the Dockerfile and related files to understand how golang-ci-lint is obtained
rg -l "golang-ci-lint" --type dockerfile
cat docker/golang-ci-lint.Dockerfile
Length of output: 757
Script:
#!/bin/bash
# Let's check for any build scripts or documentation that might show how the binary is built or obtained
rg -l "golang-ci-lint"
fd golang-ci-lint
cat README.md
Length of output: 18049
Script:
#!/bin/bash
# Let's check the golang-ci-lint specific documentation and build process
cat contrib/golang-ci-lint/README.md
cat make/go.Makefile
Length of output: 4693
contrib/golang-ci-lint/.goreleaser.yml (1)
10-13: LGTM: Secure build configuration.
The build configuration uses appropriate flags for static compilation and security:
-dwarflocationlists=truefor debugging-s -wfor stripping debug information-extldflags '-static'for static linking
.codecov.yml (1)
33-40: LGTM! Configuration follows established patterns.
The additions maintain consistency with other module configurations:
golang-ci-lintflag section properly specifies its path and enables carryforwardopbotflag section follows the same pattern
.gitignore (1)
133-135: LGTM! Appropriate addition to ignore binary file.
The entry correctly ignores the golang-ci-lint binary, which should not be version controlled.
.golangci.yml (2)
107-108: Consider the implications of disabling the goconst linter.
Disabling the goconst linter might lead to missed opportunities for code improvement through constant extraction. Consider enabling it with custom configuration instead of completely disabling it.
Would you like me to help configure goconst with appropriate thresholds to reduce false positives while maintaining its benefits?
143-143: LGTM! Correct update to linter name.
The change from mnd to gomnd reflects the current naming convention for this linter.
CONTRIBUTING.md (1)
74-75: LGTM! Clear documentation of linting setup.
The explanation of golangci-lint usage and version management is clear and accurate.
README.md (1)
98-105: LGTM! Clear and concise setup instructions.
The Go development setup instructions are well-structured and easy to follow.
.github/workflows/go.yml (2)
68-68: LGTM: Added version file to trigger linting.
The addition of .golangci-version to the list of files that trigger GolangCI linting ensures that changes to the linter version are properly handled.
348-348: LGTM: Using dynamic version from file.
Good improvement to use the version from .golangci-version file instead of hardcoding it. The added empty line improves readability.
Also applies to: 354-354
| @@ -1,7 +1,7 @@ | |||
| { | |||
| "name": "@synapsecns/widget", | |||
| "description": "Widget library for interacting with the Synapse Protocol", | |||
| "version": "0.9.9", | |||
| "version": "0.9.10", | |||
There was a problem hiding this comment.
💡 Codebase verification
Version 0.9.10 of the widget package is already published
The verification reveals that:
- The widget package version 0.9.10 is already published to npm
- The sdk-router version ^0.11.9 is consistently used across all dependent packages (rest-api, synapse-interface, and widget)
This suggests a version conflict as the PR is trying to update to a version that already exists in the npm registry. The widget package version needs to be bumped to a new version that hasn't been published yet.
🔗 Analysis chain
Version updates are consistent with rest-api package
The widget package has been updated to use the same SDK router version as the rest-api package, maintaining consistency across the project. The patch version bump (0.9.9 -> 0.9.10) suggests non-breaking changes, which is appropriate for the Boba network updates.
Also applies to: 71-71
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify version consistency across packages
# Check for any other packages that might need the same SDK router version
echo "Checking for other packages using sdk-router..."
fd -t f package.json | xargs rg "@synapsecns/sdk-router"
# Verify the widget package version hasn't been published yet
echo "Checking if widget version 0.9.10 exists..."
npm view @synapsecns/[email protected] version
Length of output: 602
| func createAndCopyBinary(tr *tar.Reader, tmpFile string) error { | ||
| var errs []error | ||
|
|
||
| // Validate path before file operations | ||
| if err := validatePath(tmpFile, os.TempDir()); err != nil { | ||
| return fmt.Errorf("invalid temporary file path: %w", err) | ||
| } | ||
|
|
||
| // Safe to use os.OpenFile here as path is validated by validatePath above | ||
| //nolint:gosec // G304: path is validated by validatePath above | ||
| file, err := os.OpenFile(tmpFile, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, execPerms) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to create temporary file: %w", err) | ||
| } | ||
| defer func() { | ||
| if closeErr := file.Close(); closeErr != nil { | ||
| errs = append(errs, fmt.Errorf("failed to close file: %w", closeErr)) | ||
| } | ||
| }() | ||
|
|
||
| // Copy with size limit and verify hash | ||
| hasher := sha256.New() | ||
| reader := io.TeeReader(io.LimitReader(tr, maxDecompressSize), hasher) | ||
| if _, err := io.Copy(file, reader); err != nil { | ||
| if removeErr := os.Remove(tmpFile); removeErr != nil { | ||
| errs = append(errs, fmt.Errorf("failed to remove temporary file: %w", removeErr)) | ||
| } | ||
| errs = append(errs, fmt.Errorf("failed to extract file: %w", err)) | ||
| return errors.Join(errs...) | ||
| } | ||
|
|
||
| // Log hash for verification | ||
| hash := hex.EncodeToString(hasher.Sum(nil)) | ||
| fmt.Printf("Extracted file hash: %s\n", hash) | ||
|
|
||
| if len(errs) > 0 { | ||
| return errors.Join(errs...) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Add actual checksum comparison.
Although the code computes a SHA-256 hash for the extracted binary, it never compares this hash against a known expected value. This effectively nullifies the intended cryptographic verification since simply printing the hash does not mitigate potential tampering.
As a follow-up, you might incorporate a known or fetched trusted checksum for this release and compare it in the code to validate authenticity before usage.
| case eventType < ModuleSynapseCCTP: | ||
| return "SynapseBridge" | ||
| case eventType == 10: | ||
| case eventType == ModuleSynapseCCTP: | ||
| return "SynapseCCTP" | ||
| case eventType == 12: | ||
| case eventType == ModuleSynapseRFQ: |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Map unknown event types to a safe default or add logging.
In case the event type does not match any known module, returning an empty string might obscure potential new or unsupported event types. Consider logging or defaulting to a known fallback module for better observability.
| 1. **Binary Not Found** | ||
| ```bash | ||
| # Clear the cache and redownload | ||
| rm -rf ~/.cache/golangci-lint | ||
| golang-ci-lint run | ||
| ``` |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance security considerations for cache clearing.
The troubleshooting section suggests removing the cache directory without addressing potential security implications.
Add a warning about verifying the cache location before removal and consider adding a safer alternative:
```bash
# Clear the cache and redownload
- rm -rf ~/.cache/golangci-lint
+ # First, verify the cache location
+ echo "Cache location: ~/.cache/golangci-lint"
+ # Then, remove only the specific version's cache
+ rm -rf ~/.cache/golangci-lint/$(cat .golangci-version)
golang-ci-lint run
<!-- This is an auto-generated comment by CodeRabbit -->
| go install github.com/synapsecns/sanguine/contrib/golang-ci-lint@latest | ||
| ``` |
There was a problem hiding this comment.
💡 Codebase verification
Update installation command to use versioned module path
The repository has established versioning with tags (tools/v1.19.1 being the latest). The installation command should be updated to:
go install github.com/synapsecns/sanguine/contrib/golang-ci-lint@tools/v1.19.1
- The module exists and is properly defined in go.mod
- The repository uses the
tools/v*tag format for versioning - Using a specific version ensures reproducible installations
🔗 Analysis chain
Verify the module path in installation instructions.
The installation command uses an absolute GitHub path. Consider using a versioned import path or documenting version requirements.
Let me decode the go.mod content to verify the module path and requirements.
Let me try one more command to get the raw content of go.mod directly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify if the module exists and is public
gh api repos/synapsecns/sanguine/contents/contrib/golang-ci-lint/go.mod
Length of output: 1187
Script:
#!/bin/bash
# Decode base64 content of go.mod
echo "bW9kdWxlIGdpdGh1Yi5jb20vc3luYXBzZWNucy9zYW5ndWluZS9jb250cmliL2dvbGFuZy1jaS1saW50CgpnbyAxLjIyLjQKCnJlcXVpcmUgZ2l0aHViLmNvbS9pbnRlZ3JhbGlzdC9nby1maW5kcm9vdCB2MC4wLjAtMjAxNjA1MTgxMTQ4MDQtYWM5MDY4MTUyNWRjCg==" | base64 -d
# Check if there are any tags in the repository
gh api repos/synapsecns/sanguine/tags
Length of output: 13607
Script:
#!/bin/bash
# Get raw content of go.mod
curl -s https://raw.githubusercontent.com/synapsecns/sanguine/master/contrib/golang-ci-lint/go.mod
Length of output: 251
| - name: Read golangci-lint version | ||
| id: golangci_version | ||
| run: echo "version=v$(cat .golangci-version)" >> $GITHUB_OUTPUT | ||
|
|
There was a problem hiding this comment.
Fix potential word splitting issue in shell script.
The shell command that reads the version could be vulnerable to word splitting if the version contains spaces.
Apply this diff to fix the issue:
- - name: Read golangci-lint version
- id: golangci_version
- run: echo "version=v$(cat .golangci-version)" >> $GITHUB_OUTPUT
+ - name: Read golangci-lint version
+ id: golangci_version
+ run: echo "version=v$(cat ".golangci-version")" >> "$GITHUB_OUTPUT"📝 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.
| - name: Read golangci-lint version | |
| id: golangci_version | |
| run: echo "version=v$(cat .golangci-version)" >> $GITHUB_OUTPUT | |
| - name: Read golangci-lint version | |
| id: golangci_version | |
| run: echo "version=v$(cat ".golangci-version")" >> "$GITHUB_OUTPUT" | |
🧰 Tools
🪛 actionlint (1.7.4)
342-342: shellcheck reported issue in this script: SC2086:info:1:45: Double quote to prevent globbing and word splitting
(shellcheck)
| "0x9c13354491Ea42655C36A19B5B23a3291e690Db4", | ||
| "0x41076ed38619e9B5d92069765F082ebe971688DA" |
There was a problem hiding this comment.
💡 Codebase verification
Duplicate addresses found in blacklist.json
The verification revealed that while the newly added addresses are not duplicates (each appears exactly once), there are 28 existing duplicate addresses in the blacklist. This needs to be addressed as duplicate entries in the blacklist are unnecessary and may impact performance.
- Existing duplicate addresses found (showing in lowercase):
0x033a41ddb54e5bc1672e85657c05c086c1814e31 0x0605edee6a8b8b553cae09abe83b2ebeb75516ec ... (and 26 more)
🔗 Analysis chain
Verify uniqueness of added addresses.
The addresses follow the correct Ethereum address format, but let's verify they aren't duplicates.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for duplicate addresses in blacklist.json
# Convert to lowercase and check for duplicates
jq -r '.[]' packages/synapse-interface/public/blacklist.json | tr '[:upper:]' '[:lower:]' | sort | uniq -d
# Count occurrences of the new addresses (case-insensitive)
echo "Checking new addresses for duplicates:"
for addr in "0x9c13354491ea42655c36a19b5b23a3291e690db4" "0x41076ed38619e9b5d92069765f082ebe971688da"; do
echo "Address: $addr"
grep -i "$addr" packages/synapse-interface/public/blacklist.json | wc -l
done
Length of output: 2057
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (3)
contrib/golang-ci-lint/Makefile (1)
Line range hint
19-21: Add safety measures to docker-clean target.The docker-clean target could accidentally stop important containers. Consider adding confirmation prompt and scope limitation.
docker-clean: ## stops and removes all containers at once - docker ps -aq | xargs docker stop | xargs docker rm + @echo "This will stop and remove ALL Docker containers." + @read -p "Are you sure? [y/N] " confirm && [ $$confirm = "y" ] + docker ps -aq --filter "label=org.opencontainers.image.source=https://github.com/synapsecns/sanguine" | xargs -r docker stop | xargs -r docker rmpackages/rest-api/swagger.json (1)
Line range hint
1-4: Enhance security documentationConsider adding a security section in the API documentation that covers:
- Input validation requirements for addresses and amounts
- Rate limiting policies
- Authentication/Authorization mechanisms
- Chain ID validation rules
CONTRIBUTING.md (1)
Line range hint
100-100: Incomplete code block at the end of the fileThe file ends with an incomplete code block that needs to be properly closed.
+```
🧹 Nitpick comments (28)
services/rfq/api/rest/rfq.go (1)
194-194: Careful with malformed quote responses
Parsing theresp.DestAmountviabase10is valid, but consider returning an error if it fails instead of relying on an external check.services/rfq/api/rest/rfq_test.go (1)
18-25: Ensure graceful handling of invalid chain IDs
ThevalidateChainIDfunction prevents negative chain IDs but panics instead of returning an error. Consider whether returning an error is better for production stability.services/rfq/api/docs/swagger.json (2)
3-4: Enhance API documentation metadataThe
infosection is missing essential fields that would improve API discoverability and usage:
title: Name of the APIversion: API version numberdescription: Overview of the API's purpose"info": { + "title": "RFQ Service API", + "version": "1.0.0", + "description": "API for managing quote requests and bridging operations", "contact": {} }
231-241: Enhance response documentationAdd error responses and examples to improve API integration experience:
"responses": { "200": { "description": "OK", + "examples": { + "application/json": { + "success": true, + "quote_id": "0x123...", + "dest_amount": "1000000000000000000" + } + }, "headers": { "X-Api-Version": { "type": "string", "description": "API Version Number - See docs for more info" } } - } + }, + "400": { + "description": "Bad Request - Invalid parameters", + "schema": { + "type": "object", + "properties": { + "error": { + "type": "string" + } + } + } + }, + "429": { + "description": "Too Many Requests" + } }Also applies to: 268-281
contrib/golang-ci-lint/main.go (3)
93-95: Consider expanding host validation to include optional subdomains or ports.Currently,
u.Scheme != "https" || u.Host != "github.com"might fail if future GitHub policy changes involve subdomains (e.g., "api.github.com") or appended ports (e.g., "github.com:443"). If strict blocking of subdomains/ports is intended, you can ignore this suggestion; otherwise, consider more flexible host matching.
140-142: Review directory traversal check.Relying on
strings.Contains(realPath, "..")can mistakenly flag legitimate paths containing..as substrings. The prefix-based validation is mostly sufficient to prevent attacks, but consider removing this additional substring check or updating it to detect actual path traversals.
104-175: Consider reducing cyclomatic complexity invalidatePath.The
// nolint: cyclopindicates intentional skipping of complexity checks. Breaking this function into smaller helpers might improve readability and maintainability.contrib/golang-ci-lint/permissions/permissions_unix.go (1)
13-18: Validate external filesystems that might ignore umask.Some network or mounted filesystems do not always respect the umask or permission bits. Ensure the environment where this runs consistently supports traditional Unix permissions.
contrib/golang-ci-lint/permissions/permissions.go (1)
23-27: Ensure no-op on Windows is desired.
SetSecureUmask()returns a no-op on Windows, which may be acceptable. However, if stricter ACL handling is required, consider implementing a Windows-specific secure setup.contrib/golang-ci-lint/main_test.go (4)
10-14: Consider using t.TempDir for safer cleanup.You're manually using
os.TempDir(), but the testing package providest.TempDir()which automatically ensures the directory is cleaned up, even on test failures.
23-35: Improve cross-platform clarity.It might be helpful to describe in a comment the difference between typical MacOS and Linux path structures. The test intentionally covers MacOS's
/private/varpaths, so clarifying that could reduce confusion for new contributors.
37-41: Validate potential directory traversal on Windows.While these tests check
../outsideon Unix systems, consider adding a Windows-specific test if the tool is expected to run on Windows, since path separators (\vs/) differ.
75-79: Log actual path in error messages.You might consider including the validated path in the error message to troubleshoot issues more easily.
tools/abigen/internal/etherscan/etherscan.go (1)
43-44: Guard verbose logging with a toggle.Currently,
Verbose: trueis hard-coded. Consider having a configuration or environment variable to enable/disable verbose logging for better control in production environments.contrib/promexporter/internal/gql/explorer/models.gen.go (1)
65-70: Maintain consistency betweenBridgeTransactionandBridgeWatcherTx.Both now include
BridgeModule. Verify they are used consistently in code and queries, ensuring no duplication or confusion in GraphQL schemas or resolvers.services/explorer/graphql/server/graph/queryutils.go (1)
1887-1891: Improve maintainability with extended enumeration.Using a
switchstatement here is valid. Consider future-proofing by either enumerating all known event types or clarifying fallback cases to ensure you can cleanly handle newly added modules or event types.docker/golang-ci-lint.Dockerfile (1)
1-1: Consider pinning the base image version.Using
latesttag for the base image may lead to unexpected changes. Consider pinning to a specific version for better reproducibility.-FROM gcr.io/distroless/static:latest +FROM gcr.io/distroless/static:nonroot@sha256:24a7c8f1b70f1486a7b0ac92312c6b8eb2ed3b4f546e3d0ef6a9df0f27011aacontrib/golang-ci-lint/Makefile (1)
Line range hint
23-26: Add PHONY targets declaration.Declare PHONY targets to prevent conflicts with files of the same name.
+.PHONY: default help docker-clean lint + lint: ## lint lints the code with golangci-lintcontrib/golang-ci-lint/.goreleaser.yml (1)
26-41: Consider adding Docker content trust.Enable Docker Content Trust for signed container images.
dockers: - goos: linux goarch: amd64 + docker_manifest_templates: + - 'ghcr.io/synapsecns/sanguine/golang-ci-lint:latest' + docker_signs: + - cmd: cosign + args: + - 'sign' + - '${artifact}' + artifacts: all.codecov.yml (1)
33-40: LGTM! Consider organizing flags alphabeticallyThe new sections for
golang-ci-lintandopbotare properly configured with correct paths and carryforward settings. However, to improve maintainability, consider organizing all flag sections alphabetically.packages/sdk-router/src/constants/testValues.ts (1)
34-35: Add clarity or reference regarding usage.The newly introduced
BOBA_USDCconstant is aligned with the existing naming convention and usage pattern. However, consider adding a brief comment or reference in the file about how or when this token address might be used in tests, so future developers are aware of its purpose and usage context.packages/rest-api/src/routes/bridgeRoute.ts (1)
59-65: Clarify optional parameters in OpenAPI docs.Updating the descriptions to highlight that both fields are needed to generate
callDatais excellent. However, consider also clarifying that even though these parameters are optional, passing one without the other results incallData: null. This will help consumers of the API better understand the partial usage scenarios.packages/sdk-router/src/constants/chainIds.ts (1)
45-48: Use Number.isNaN instead of isNaN
Static analysis warns about globalisNaNbeing unsafe due to type coercion. Consider replacingisNaNwithNumber.isNaNto avoid unexpected behavior.- .filter((chainId) => !isNaN(chainId)) + .filter((chainId) => !Number.isNaN(chainId))🧰 Tools
🪛 Biome (1.9.4)
[error] 46-46: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
packages/rest-api/swagger.json (2)
Line range hint
326-327: Consider adding deprecation timelineWhile the endpoints are properly marked as deprecated and their replacements are documented, consider adding:
- A specific timeline for removal
- Migration guide reference
- API version where these endpoints will be removed
Line range hint
2-7: Document API versioning strategyWhile version information is returned in headers, consider enhancing version documentation:
- Include version compatibility matrix
- Document breaking vs. non-breaking changes
- Add version lifecycle information
contrib/golang-ci-lint/README.md (3)
99-99: Fix macOS naming conventionThe operating system name should be "macOS" instead of "MacOS" for consistency with Apple's branding.
- - MacOS (/private/var) symlink handling: + - macOS (/private/var) symlink handling: - - MacOS: Handles /private/var symlinks and temp directories + - macOS: Handles /private/var symlinks and temp directoriesAlso applies to: 109-109
🧰 Tools
🪛 LanguageTool
[grammar] ~99-~99: The operating system from Apple is written “macOS”.
Context: ...amd64, darwin-arm64, windows-amd64 - MacOS (/private/var) symlink handling: -...(MAC_OS)
29-31: Add language specifier to code blockThe code block should specify the version number format, e.g., 'version' or 'text'.
-``` +```version 1.61.0<details> <summary>🧰 Tools</summary> <details> <summary>🪛 Markdownlint (0.37.0)</summary> 29-29: null Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> --- `51-51`: **Replace hard tab with spaces in Makefile example** The Makefile example contains a hard tab which should be replaced with spaces for consistent formatting. ```diff - go run github.com/synapsecns/sanguine/contrib/golang-ci-lint run --fix --config=.golangci.yml + go run github.com/synapsecns/sanguine/contrib/golang-ci-lint run --fix --config=.golangci.yml🧰 Tools
🪛 Markdownlint (0.37.0)
51-51: Column: 1
Hard tabs(MD010, no-hard-tabs)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (13)
agents/go.sumis excluded by!**/*.sumcontrib/golang-ci-lint/go.sumis excluded by!**/*.sumcontrib/opbot/go.sumis excluded by!**/*.sumcontrib/promexporter/go.sumis excluded by!**/*.sumcontrib/screener-api/go.sumis excluded by!**/*.sumcore/go.sumis excluded by!**/*.sumethergo/go.sumis excluded by!**/*.sumgo.workis excluded by!**/*.workservices/cctp-relayer/go.sumis excluded by!**/*.sumservices/explorer/go.sumis excluded by!**/*.sumservices/omnirpc/go.sumis excluded by!**/*.sumservices/rfq/go.sumis excluded by!**/*.sumservices/scribe/go.sumis excluded by!**/*.sum
📒 Files selected for processing (76)
.codecov.yml(1 hunks).github/workflows/go.yml(2 hunks).gitignore(1 hunks).golangci-version(1 hunks).golangci.yml(2 hunks)CONTRIBUTING.md(1 hunks)README.md(1 hunks)agents/go.mod(0 hunks)contrib/golang-ci-lint/.codecov.yml(1 hunks)contrib/golang-ci-lint/.goreleaser.yml(1 hunks)contrib/golang-ci-lint/Makefile(1 hunks)contrib/golang-ci-lint/README.md(1 hunks)contrib/golang-ci-lint/go.mod(1 hunks)contrib/golang-ci-lint/main.go(1 hunks)contrib/golang-ci-lint/main_test.go(1 hunks)contrib/golang-ci-lint/permissions/permissions.go(1 hunks)contrib/golang-ci-lint/permissions/permissions_unix.go(1 hunks)contrib/golang-ci-lint/permissions/permissions_windows.go(1 hunks)contrib/opbot/go.mod(0 hunks)contrib/promexporter/go.mod(0 hunks)contrib/promexporter/internal/gql/explorer/models.gen.go(1 hunks)contrib/screener-api/go.mod(0 hunks)core/go.mod(0 hunks)core/metrics/README.md(0 hunks)core/metrics/base.go(0 hunks)core/metrics/internal/const.go(0 hunks)core/metrics/rookout.go(0 hunks)docker/golang-ci-lint.Dockerfile(1 hunks)docs/bridge/CHANGELOG.md(1 hunks)docs/bridge/docs/02-Bridge/02-REST-API.md(1 hunks)docs/bridge/package.json(2 hunks)ethergo/go.mod(0 hunks)make/go.Makefile(1 hunks)packages/explorer-ui/CHANGELOG.md(1 hunks)packages/explorer-ui/package.json(2 hunks)packages/rest-api/CHANGELOG.md(1 hunks)packages/rest-api/package.json(2 hunks)packages/rest-api/src/controllers/bridgeController.ts(1 hunks)packages/rest-api/src/routes/bridgeRoute.ts(1 hunks)packages/rest-api/src/tests/bridgeRoute.test.ts(2 hunks)packages/rest-api/swagger.json(1 hunks)packages/sdk-router/CHANGELOG.md(1 hunks)packages/sdk-router/package.json(1 hunks)packages/sdk-router/src/constants/chainIds.ts(3 hunks)packages/sdk-router/src/constants/testValues.ts(1 hunks)packages/sdk-router/src/sdk.test.ts(2 hunks)packages/synapse-constants/CHANGELOG.md(1 hunks)packages/synapse-constants/package.json(1 hunks)packages/synapse-constants/src/constants/tokens/poolMaster.ts(2 hunks)packages/synapse-interface/CHANGELOG.md(1 hunks)packages/synapse-interface/constants/tokens/poolMaster.ts(2 hunks)packages/synapse-interface/package.json(2 hunks)packages/synapse-interface/public/blacklist.json(1 hunks)packages/synapse-interface/public/pauses/v1/paused-chains.json(1 hunks)packages/widget/CHANGELOG.md(1 hunks)packages/widget/package.json(2 hunks)services/cctp-relayer/go.mod(0 hunks)services/explorer/go.mod(0 hunks)services/explorer/graphql/server/graph/queryutils.go(1 hunks)services/omnirpc/go.mod(0 hunks)services/rfq/api/db/activequoterequeststatus_string.go(1 hunks)services/rfq/api/db/api_db.go(1 hunks)services/rfq/api/docs/docs.go(3 hunks)services/rfq/api/docs/swagger.json(1 hunks)services/rfq/api/docs/swagger.yaml(4 hunks)services/rfq/api/rest/rfq.go(5 hunks)services/rfq/api/rest/rfq_test.go(4 hunks)services/rfq/api/rest/server.go(1 hunks)services/rfq/api/rest/server_test.go(4 hunks)services/rfq/go.mod(0 hunks)services/rfq/relayer/inventory/circle.go(1 hunks)services/rfq/relayer/inventory/synapse.go(1 hunks)services/rfq/relayer/quoter/quoter.go(2 hunks)services/rfq/relayer/relconfig/getters.go(2 hunks)services/scribe/go.mod(0 hunks)tools/abigen/internal/etherscan/etherscan.go(1 hunks)
💤 Files with no reviewable changes (15)
- core/metrics/README.md
- core/metrics/internal/const.go
- core/metrics/rookout.go
- core/metrics/base.go
- services/explorer/go.mod
- agents/go.mod
- services/cctp-relayer/go.mod
- contrib/opbot/go.mod
- services/rfq/go.mod
- ethergo/go.mod
- contrib/screener-api/go.mod
- services/omnirpc/go.mod
- core/go.mod
- services/scribe/go.mod
- contrib/promexporter/go.mod
✅ Files skipped from review due to trivial changes (19)
- packages/synapse-constants/package.json
- packages/widget/CHANGELOG.md
- .golangci-version
- packages/sdk-router/package.json
- packages/explorer-ui/CHANGELOG.md
- services/rfq/api/db/api_db.go
- services/rfq/api/docs/docs.go
- packages/synapse-interface/package.json
- packages/rest-api/CHANGELOG.md
- docs/bridge/package.json
- packages/explorer-ui/package.json
- contrib/golang-ci-lint/.codecov.yml
- services/rfq/relayer/quoter/quoter.go
- services/rfq/relayer/inventory/synapse.go
- packages/sdk-router/CHANGELOG.md
- packages/synapse-interface/CHANGELOG.md
- docs/bridge/CHANGELOG.md
- packages/synapse-constants/CHANGELOG.md
- contrib/golang-ci-lint/go.mod
🧰 Additional context used
🪛 Biome (1.9.4)
packages/sdk-router/src/constants/chainIds.ts
[error] 46-46: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
🪛 actionlint (1.7.4)
.github/workflows/go.yml
342-342: shellcheck reported issue in this script: SC2086:info:1:45: Double quote to prevent globbing and word splitting
(shellcheck)
🪛 LanguageTool
contrib/golang-ci-lint/README.md
[grammar] ~99-~99: The operating system from Apple is written “macOS”.
Context: ...amd64, darwin-arm64, windows-amd64 - MacOS (/private/var) symlink handling: -...
(MAC_OS)
[grammar] ~109-~109: The operating system from Apple is written “macOS”.
Context: ... - Linux: Standard path resolution - MacOS: Handles /private/var symlinks and temp...
(MAC_OS)
🪛 Markdownlint (0.37.0)
contrib/golang-ci-lint/README.md
51-51: Column: 1
Hard tabs
(MD010, no-hard-tabs)
29-29: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (63)
services/rfq/api/db/activequoterequeststatus_string.go (1)
19-19: Confirm newly introduced index boundary
By changing the upper boundary to 28, ensure that the substring length in the _ActiveQuoteRequestStatus_name constant matches the intended string sequence. An off-by-one error here could produce partial or unintended strings.
services/rfq/api/rest/rfq.go (5)
20-23: Good introduction of the base10 constant
This improvement avoids repeating the numeric literal and helps maintain consistent integer parsing throughout the file.
173-174: Enhanced clarity in big.Int parsing
Using base10 constant clarifies the numeric base and maintains consistency. Ensure to handle any error if parsing fails.
228-228: Validate usage of chain IDs
While the comment indicates the chain IDs are validated by the API, ensure upstream calls or data manipulations also respect this validation.
244-244: Properly parse user input in getPassiveQuote
Parsing request.Data.OriginAmountExact as base 10 is consistent with the rest of the code. Validate and handle any potential parsing failures if the user input is malformed.
251-251: Consistent numeric parsing for MaxOriginAmount
Good use of the shared base10 constant. Be sure that any error during parsing is handled in case the stored data is invalid.
services/rfq/api/rest/rfq_test.go (3)
73-75: Proper usage of validateChainID
These checks help ensure database records always store a valid (non-negative) chain ID.
269-271: Consistency with chain ID validation
Same approach as before. The usage of validateChainID here keeps chain ID usage safe.
348-350: Reusing validation logic
Again, usage of validateChainID shows a consistent approach to guaranteeing valid IDs across tests.
services/rfq/api/rest/server.go (1)
568-568: Uniform numeric base usage
Replacing the literal 10 with the base10 constant helps make the code more readable and consistent.
services/rfq/api/rest/server_test.go (4)
401-403: Good use of chain ID validation.
The newly introduced calls to validateChainID add an extra layer of safety and ensure that the chain IDs are properly validated before use. This is helpful for preventing invalid or negative chain IDs.
412-414: Continued chain ID checks enhance robustness.
Ensuring consistent validation of both OriginChainID and DestChainID here helps maintain data integrity and safeguard against invalid IDs.
423-425: Consistent validateChainID usage.
Applying the same validation approach for each relayer quote ensures uniform handling of chain IDs throughout the tests.
434-436: Well-structured validation in passive quotes.
The addition of validateChainID usage is consistent with the code's overall emphasis on robust chain ID checks, reducing risk of incorrect indexing or negative values.
services/rfq/relayer/relconfig/getters.go (2)
16-19: Introduction of a shared constant for token decimals.
Defining tokenDecimalsBase clarifies the base used for token decimal calculations across different functions. This is a good step toward avoiding magic numbers and promoting consistency.
785-785: Refactored exponent base usage.
Using tokenDecimalsBase here improves maintainability by eliminating a hardcoded base and making the code more expressive.
services/rfq/api/docs/swagger.yaml (4)
20-20: Updated parameter name to “origin_amount_exact.”
Renaming the field helps clarify that the provided origin amount is exact, improving readability and consistency with related code updates.
139-139: Consistent field renaming to “origin_amount_exact.”
Ensuring uniform naming conventions within data structures helps prevent confusion and maintain clarity in the API schema.
313-316: Enhanced clarity in endpoint description.
These lines now better describe how an Active RFQ is initiated and how a best quote is returned, improving user understanding of the endpoint's purpose.
338-339: Refined WebSocket connection details.
The updated documentation clarifies that this endpoint listens for streaming active quote requests rather than just establishing a basic WebSocket connection.
services/rfq/relayer/inventory/circle.go (1)
245-246: Linter override for chain ID fields.
Using //nolint:gosec here is acceptable if external validation ensures these fields are never user-supplied malicious values. Confirm that upstream validations remain robust.
packages/rest-api/package.json (1)
3-3: Verify that updated dependencies include Boba network blocking
The version updates to sdk-router (0.11.9) and synapse-constants (1.8.6) should contain the changes for blocking Boba network bridging.
Let's verify the changes in these versions:
Also applies to: 25-26
✅ Verification successful
Boba network blocking is confirmed in sdk-router package
The verification shows that Boba network (chain ID 288) is explicitly listed in the PAUSED_CHAIN_IDS array in packages/sdk-router/src/constants/chainIds.ts. This confirms that the version updates include the Boba network blocking functionality:
packages/sdk-router/src/constants/chainIds.ts:export const PAUSED_CHAIN_IDS: SupportedChainId[] = [SupportedChainId.BOBA]- Test files also confirm this by including test cases for the paused chain behavior
The version bump follows semantic versioning with patch updates (0.11.8 → 0.11.9 and 1.8.5 → 1.8.6) which is appropriate for this type of change.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the changelog entries and commits for Boba network blocking changes
# Check for Boba-related changes in recent commits
git log -n 20 --oneline | grep -i "boba"
# Look for changelog entries
fd -g "CHANGELOG.md" | xargs rg -i "0\.11\.9|1\.8\.6.*boba"
Length of output: 227
Script:
#!/bin/bash
# Let's check the actual content of the changelog entry and related files for Boba network blocking changes
# Check the specific changelog entry content
rg -A 10 "0\.11\.9.*2024-12-18" packages/sdk-router/CHANGELOG.md
# Search for Boba-related changes in router configuration or network files
rg -i "boba" packages/sdk-router/src/
rg -i "boba" packages/synapse-constants/src/
# Look for network blocking or disabled networks configuration
ast-grep --pattern 'const $_networks = {
$$$
}'
ast-grep --pattern 'export const $_networks = {
$$$
}'
Length of output: 7237
packages/widget/package.json (2)
3-4: LGTM: Version updates are consistent across packages
The version updates are coordinated across packages:
- rest-api: 1.8.15 with sdk-router 0.11.9
- widget: 0.9.10 with sdk-router 0.11.9
This ensures consistent handling of Boba network blocking across the stack.
Also applies to: 25-26, 71-71
4-4: Verify UI handling of blocked Boba network
The widget version update (0.9.10) and sdk-router dependency update (0.11.9) should include UI changes to handle Boba network blocking gracefully.
Let's verify the UI implementation:
Also applies to: 71-71
✅ Verification successful
Let me try another search strategy to find UI-related changes for network blocking.
Let me try one final search to specifically look for Boba network handling.
Boba network blocking is properly implemented in the UI
Based on the code analysis, the Boba network blocking is properly handled in the UI through several mechanisms:
- The Boba chain is marked as paused in
sdk-router/src/constants/chainIds.ts:PAUSED_CHAIN_IDS: SupportedChainId[] = [SupportedChainId.BOBA] - The UI shows appropriate warnings when connected to unsupported networks in
StandardPageContainer.tsx:const unsupported = AcceptedChainId[connectedChainId] ? false : true // Shows toast message: "Connected to an unsupported network; Please switch networks"
- The SDK throws errors for any bridge/swap operations involving the Boba network as shown in
sdk.test.ts - The widget package's version update (0.9.10) correctly incorporates these blocking mechanisms through the updated sdk-router dependency
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for UI components and error handling related to Boba network
# Look for Boba-related UI components or network handling
ast-grep --pattern 'const $networkName = "Boba"'
# Check for network blocking related error messages
rg -i "network.*(?:block|disable|unavailable)" --type ts
Length of output: 1218
Script:
#!/bin/bash
# Search for network-related error handling and UI components
rg -i "(unsupported|disabled|blocked).*(chain|network)" --type ts -A 2
# Look for network status or availability checks
ast-grep --pattern 'network.$_ === $_'
# Search for error messages or UI text
rg -i "network (not supported|unavailable|disabled)" --type ts
Length of output: 16409
Script:
#!/bin/bash
# Search for Boba network related configurations and handling
rg -i "boba" --type ts -A 2
# Look for chain blocking or network disabling logic
ast-grep --pattern 'const $_ = {
$$$
Boba: $_,
$$$
}'
Length of output: 41751
contrib/golang-ci-lint/main.go (2)
68-68: Good job limiting archive size to mitigate decompression bombs.
Having maxDecompressSize set to 50MB is an effective measure for preventing excessively large or malicious archives from consuming resources.
256-304: Verify that additional arguments are safely handled.
The processArgs function merges user-supplied arguments. If a user supplies --path maliciously, ensure you trust or sanitize the provided path. The code does call validatePath later, which is good, but keep an eye on any indirect usage of workDir.
contrib/golang-ci-lint/permissions/permissions_windows.go (1)
10-12: Confirm whether the Windows stub aligns with your security requirements.
On Windows, os.Chmod does not fully replicate Unix-style permissions. If more granular ACL changes are needed, consider using Windows-specific APIs or referencing the Win32 ACL model.
make/go.Makefile (1)
31-31: Confirm capturing exit codes when running linter.
You are directly running go run .... If this process fails, ensure make appropriately surfaces the linter’s exit code. Otherwise, you might miss linting errors in CI.
contrib/golang-ci-lint/main_test.go (1)
15-21: Ensure thorough coverage for edge cases.
Your table-driven tests are good. Double-check that you include scenarios such as empty allowedDir with an empty path or paths that are fully inside an allowed directory with deeper subdirectories.
tools/abigen/internal/etherscan/etherscan.go (1)
31-36: Confirm fallback priority for API key resolution.
The code tries a chain-specific environment variable before ETHERSCAN_KEY. This is correct, but verify it’s the intended fallback order across all relevant chain IDs. Also ensure you handle the scenario where neither is set, possibly returning a more explicit error message.
contrib/promexporter/internal/gql/explorer/models.gen.go (1)
55-60: Ensure test coverage for the new field BridgeModule.
The addition of BridgeModule *string in BridgeTransaction is straightforward. However, confirm your tests cover scenarios where BridgeModule is set vs. left as nil.
services/explorer/graphql/server/graph/queryutils.go (1)
1875-1884: Refactor magic values into clearly named constants.
Great job introducing constants (ModuleSynapseBridge, ModuleSynapseCCTP, ModuleSynapseRFQ). This strongly improves clarity versus using raw integers.
docker/golang-ci-lint.Dockerfile (1)
10-13: LGTM! Good security practices.
Running as nonroot user and setting appropriate workdir permissions are good security practices.
contrib/golang-ci-lint/Makefile (1)
1-1: LGTM! Good modular design.
Including the common Go Makefile promotes code reuse and maintainability.
contrib/golang-ci-lint/.goreleaser.yml (1)
7-24: LGTM! Secure build configuration.
The build configuration follows security best practices with static linking, CGO disabled, and trimpath enabled.
.gitignore (1)
133-135: LGTM! Correctly ignoring the binary file
The addition of the golang-ci-lint binary to .gitignore is appropriate as we don't want to track compiled binaries in version control.
.golangci.yml (2)
143-143: LGTM! Updated to use the correct linter name
The change from mnd to gomnd reflects the correct name of the magic number detection linter.
107-108: Verify the need to disable the goconst linter
The goconst linter helps identify repeated strings that could be constants, improving maintainability. Consider if disabling it is necessary.
✅ Verification successful
Let me run another verification script to check for repeated string literals that could be constants.
Let me try another script to check for repeated string literals with PCRE2 enabled.
Let me try one more script to check for repeated string literals in Go files, focusing on non-contract files.
Based on the analysis of the codebase, I can now provide a response regarding the goconst linter:
Disabling goconst linter is justified
The codebase shows that most string literals that appear multiple times are in specific contexts where converting them to constants would not improve code quality:
- Generated code (e.g., contract ABIs, GraphQL resolvers)
- Test data and configurations
- SQL queries and database connection strings
- Metric names and descriptions
- Log messages and error formats
In these cases, string literals are:
- Either already well-organized (like in configuration files)
- Part of generated code where constants wouldn't be maintained
- Used in test files where readability is more important than reusability
- Context-specific where extracting them as constants would reduce code clarity
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for potential constant strings that would be flagged by goconst
rg --type go -l '".+".*".+".*".+"' | while read -r file; do
echo "=== $file ==="
rg --type go '".+".*".+".*".+"' "$file"
done
Length of output: 131070
Script:
#!/bin/bash
# Check for repeated string literals in Go files
rg --type go -l '".+".*".+".*".+"' | while read -r file; do
echo "=== $file ==="
rg --type go -U '("[^"]+").*\1' "$file"
done
Length of output: 54042
Script:
#!/bin/bash
# Check for repeated string literals in Go files using PCRE2
rg --type go --pcre2 '("[^"]+").*\1' | while read -r line; do
echo "$line"
done
Length of output: 131070
Script:
#!/bin/bash
# Check for repeated string literals in Go files, excluding contract files
rg --type go -g '!*.abigen.go' '".+".*".+".*".+"' | while read -r line; do
echo "$line"
done
Length of output: 92278
.github/workflows/go.yml (2)
68-68: LGTM: Added version file to trigger full lint.
The change ensures that modifications to the lint version trigger a full lint run, maintaining code quality standards.
348-348: LGTM: Dynamic version management implemented.
Good improvement to use a dynamic version from the .golangci-version file instead of hardcoding it. The added empty line improves readability.
Also applies to: 354-354
packages/synapse-interface/public/blacklist.json (1)
616-617: Verify Boba network addresses.
The added addresses need verification to confirm they are indeed related to Boba network bridging functionality. Please ensure:
- These addresses are documented as Boba network contracts
- The impact of blacklisting these addresses is well understood
Run the following script to search for references to these addresses in the codebase:
✅ Verification successful
Let me gather more information about these addresses by searching for any documentation or related configuration files.
Let me check the bridge configuration files for any references to these addresses or Boba network.
Let me check the Boba network related configuration files we found to verify these addresses.
Blacklisting of Boba network bridge addresses confirmed
The verification confirms these addresses should be blacklisted:
- The Boba network (chain ID: 288) is currently under a planned pause due to chain upgrades, as shown in the paused-chains.json configuration
- The addresses being blacklisted align with the network-wide pause of Boba bridging functionality
- The configuration shows both inbound and outbound bridging for Boba network (chain ID: 288) is intentionally paused
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for references to the blacklisted addresses
# Search for address documentation or usage
for addr in "0x9c13354491Ea42655C36A19B5B23a3291e690Db4" "0x41076ed38619e9B5d92069765F082ebe971688DA"; do
echo "Searching for references to $addr..."
rg -i --type ts --type js --type go --type sol "$addr"
done
# Look for related test files
fd -t f -e test.ts -e test.js -e test.go
Length of output: 3042
Script:
#!/bin/bash
# Search for documentation or configuration files that might reference Boba network
echo "Searching for Boba network related files and documentation..."
rg -i "boba" --type md --type json --type yaml --type yml -A 3 -B 3
# Search for bridge or network configuration files
echo -e "\nSearching for bridge configuration files..."
fd -e json -e yaml -e yml | grep -i "bridge\|network"
Length of output: 3352
Script:
#!/bin/bash
# Check bridge configuration files for Boba network references
echo "Checking FastBridge configurations..."
cat packages/contracts-rfq/configs/global/FastBridge.json
echo -e "\nChecking bridge modules configuration..."
cat packages/synapse-interface/public/pauses/v1/paused-bridge-modules.json
echo -e "\nChecking for Boba in network configurations..."
find . -type f -name "*.json" -exec grep -l "boba" {} \;
Length of output: 958
Script:
#!/bin/bash
# Check Boba network related configuration files
echo "Checking Boba network configurations..."
cat packages/synapse-constants/src/scripts/data/providers.json
cat packages/synapse-interface/public/pauses/v1/paused-chains.json
# Check commit message and PR description for context
echo -e "\nChecking git commit message..."
git log -n 1 --pretty=format:"%B"
Length of output: 3215
packages/rest-api/src/controllers/bridgeController.ts (1)
56-68: Ensure bridging logic remains consistent with paused chain restrictions.
The new check requiring both destAddress and originUserAddress to generate callData is valid. However, since the PR’s stated purpose is to block bridging on specific chains (e.g., BOBA), confirm that these lines of code integrate properly with your chain-blocking logic. If you rely on a separate step for blocking the chain, ensure that the bridging call here never executes for disallowed chain IDs.
packages/rest-api/src/tests/bridgeRoute.test.ts (3)
177-185: Improve test clarity by verifying paused chain scenarios.
This new test ensures that callData is returned if both originUserAddress and destAddress are present. It might be useful to add another scenario specifically testing blocked chain IDs to confirm no bridging calls are made in those cases, aligning with the PR objective of blocking bridging to and from the BOBA network.
Do you want me to generate an additional test case to verify blocking on BOBA chain?
204-211: Well-defined behavior when destAddress is missing.
The test coverage ensures no callData is generated if destAddress is absent, which is correct according to the new bridging logic. This clarifies handling of partial input. Good work!
213-220: Well-defined behavior when originUserAddress is missing.
This test is consistent with the bridging logic changes, ensuring no callData is generated if originUserAddress is not provided. This step further clarifies partial input handling.
packages/sdk-router/src/sdk.test.ts (2)
22-22: BOBA_USDC addition
This line introduces the BOBA_USDC constant for usage in tests. The reference appears consistent with the other token definitions.
1822-1871: Comprehensive paused chain tests
These new "Paused Chain Tests" comprehensively validate the SDK behavior when dealing with a paused chain, ensuring no routes are found if either origin or destination is paused. The negative test cases confirm system robustness for restricted scenarios. Overall, excellent coverage.
packages/synapse-interface/public/pauses/v1/paused-chains.json (1)
52-58: Possible duplication of entries for "boba-chain-pause"
There are now two entries with the same "id": "boba-chain-pause", but with differing properties (pauseSwap is true in the first and false in the second). This may cause confusion or unintended overrides. Verify if the second entry should merge or replace the first.
packages/sdk-router/src/constants/chainIds.ts (4)
36-40: Introduction of PAUSED_CHAIN_IDS
Defining BOBA within a dedicated paused chain list is clear and adheres to the new logic for excluding paused networks.
48-48: Excluding paused chains
Using .filter((chainId) => !PAUSED_CHAIN_IDS.includes(chainId)) correctly enforces pause restrictions by omitting chain IDs in the paused list.
63-63: CCTP_SUPPORTED_CHAIN_IDS filter
Likewise, excluding paused chains helps ensure no routes are found on networks that are paused.
79-79: RFQ_SUPPORTED_CHAIN_IDS filter
This change maintains consistency with other modules by removing paused chains from fast-bridge (RFQ) routes.
docs/bridge/docs/02-Bridge/02-REST-API.md (1)
29-30: Updated documentation for /bridge endpoint
The note clarifying that originUserAddress and destAddress are now mandatory parameters improves API clarity and reduces potential usage confusion.
packages/rest-api/swagger.json (1)
Line range hint 204-209: Verify the impact of making destAddress optional
The change to make destAddress optional aligns with preventing bridging to specific networks. However, ensure that:
- The backend validates the
destAddresswhen provided - The quote generation logic handles the absence of
destAddressgracefully - Client applications are updated to handle this change
contrib/golang-ci-lint/README.md (1)
1-116: Well-structured documentation for the golang-ci-lint tool
The documentation is comprehensive and covers all necessary aspects including installation, usage, troubleshooting, and CI/CD integration. The structure is clear and the examples are helpful.
🧰 Tools
🪛 LanguageTool
[grammar] ~99-~99: The operating system from Apple is written “macOS”.
Context: ...amd64, darwin-arm64, windows-amd64 - MacOS (/private/var) symlink handling: -...
(MAC_OS)
[grammar] ~109-~109: The operating system from Apple is written “macOS”.
Context: ... - Linux: Standard path resolution - MacOS: Handles /private/var symlinks and temp...
(MAC_OS)
🪛 Markdownlint (0.37.0)
51-51: Column: 1
Hard tabs
(MD010, no-hard-tabs)
29-29: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
CONTRIBUTING.md (2)
74-76: Clear and concise linting documentation
The updated linting documentation clearly explains the automated version management through the .golangci-version file.
76-86: Well-documented golang-ci-lint module section
The new section provides clear instructions for building and testing the module.
README.md (2)
98-105: Clear Go development setup instructions
The setup instructions are clear and concise, properly explaining the automated golangci-lint management.
106-113: Well-documented version management features
The version management section clearly outlines the key features and benefits of the automated system.
packages/synapse-interface/constants/tokens/poolMaster.ts (2)
526-526: Confirm impacts of disabling incentives for CANTO_WRAPPER_POOL_SWAP_TOKEN.
Similar to CANTO_POOL_SWAP_TOKEN, setting incentivized to false will also disable any yield or reward distributions for this token. Verify the call sites or reward logic that might rely on incentives being active.
Use the following script to check for references to CANTO_WRAPPER_POOL_SWAP_TOKEN:
✅ Verification successful
Let me search for any reward or incentive-related logic that might be impacted.
Setting incentivized to false for CANTO_WRAPPER_POOL_SWAP_TOKEN is safe
Based on the codebase analysis, setting incentivized: false for CANTO_WRAPPER_POOL_SWAP_TOKEN is safe and consistent with other Canto pool tokens. The impact is well-defined:
- The pool will be displayed in the "Unincentivized Pools" section of the UI instead of "Incentivized Pools"
- No reward distributions will be calculated or displayed for this pool
- The deposit button in the pool header will not be shown (as seen in PoolHeader.tsx)
- APY calculations will only show base rates without reward components
This matches the behavior of other Canto pools like CANTO_POOL_SWAP_TOKEN and CANTO_WETH_SWAP_TOKEN which are also set to incentivized: false.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Searching for references to CANTO_WRAPPER_POOL_SWAP_TOKEN across the codebase
rg -A 5 "CANTO_WRAPPER_POOL_SWAP_TOKEN"
Length of output: 1164
Script:
#!/bin/bash
# Search for files containing reward/incentive logic related to pool tokens
rg -A 5 "incentivized|rewards?" --type ts
# Also check for any imports of the CANTO_WRAPPER_POOL_SWAP_TOKEN
rg "import.*CANTO_WRAPPER_POOL_SWAP_TOKEN" --type ts
Length of output: 57949
477-477: Confirm impacts of disabling incentives for CANTO_POOL_SWAP_TOKEN.
Changing incentivized to false means that users of this pool will no longer receive any liquidity mining or staking rewards. Ensure that all references and reward distribution logic tied to this token are updated accordingly.
Below is a script you can run to locate references to CANTO_POOL_SWAP_TOKEN and confirm that no incentive logic depends on the old setting:
✅ Verification successful
Let me search for any incentive-related logic that might be affected.
Let me check the specific implementation of these pool master files to understand the incentive structure better.
Let me check how the incentivized flag is used in the UI to understand its impact.
Setting incentivized to false for CANTO_POOL_SWAP_TOKEN is safe and consistent with the codebase
Based on the code analysis, this change is safe because:
-
The
incentivizedflag is used to:- Determine if a pool shows up in the "Incentivized Pools" or "Unincentivized Pools" section of the UI
- Control whether staking rewards (liquidity mining) are available for the pool
- Affect UI elements like deposit buttons and reward displays
-
The CANTO_POOL_SWAP_TOKEN was already not providing rewards as evidenced by:
- It was already set to
incentivized: false - The pool uses the same miniChefAddress as other unincentivized Canto pools
- No special reward token is configured (no
customRewardTokenproperty)
- It was already set to
-
This change in the duplicate file maintains consistency with the existing configuration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Searching for references to CANTO_POOL_SWAP_TOKEN across the codebase
rg -A 5 "CANTO_POOL_SWAP_TOKEN"
Length of output: 1140
Script:
#!/bin/bash
# Search for files containing both CANTO_POOL_SWAP_TOKEN and incentive-related terms
rg -l "CANTO_POOL_SWAP_TOKEN" | xargs rg -l "incentiv|reward|staking"
# Search for any reward distribution or staking logic related to Canto pools
rg -A 5 "(?i)(reward|staking|incentiv).*canto"
Length of output: 238
Script:
#!/bin/bash
# Check the content of pool master files
cat packages/synapse-interface/constants/tokens/poolMaster.ts
cat packages/synapse-constants/src/constants/tokens/poolMaster.ts
# Search for any usage of incentivized flag in the codebase
rg -A 5 "incentivized.*:"
Length of output: 76463
Script:
#!/bin/bash
# Search for UI components that use the incentivized flag
rg -A 5 "\.incentivized"
# Search for any staking or reward-related components
rg -A 5 "(?i)(staking|reward|minichef)"
Length of output: 88194
packages/synapse-constants/src/constants/tokens/poolMaster.ts (2)
525-525: Ensure no stale references remain for CANTO_WRAPPER_POOL_SWAP_TOKEN incentives.
Disabling incentives for this token aligns with the changes in synapse-interface. Confirm that any reward-related logic or UI references are also updated to reflect this new status.
Below is a sample script to verify references to CANTO_WRAPPER_POOL_SWAP_TOKEN:
✅ Verification successful
Let me search for any reward or incentive-related references to ensure we haven't missed anything.
No stale references found for CANTO_WRAPPER_POOL_SWAP_TOKEN incentives
The verification shows that the incentive status is consistently set to false for CANTO_WRAPPER_POOL_SWAP_TOKEN across both packages:
- packages/synapse-interface/constants/tokens/poolMaster.ts
- packages/synapse-constants/src/constants/tokens/poolMaster.ts
The token is properly configured with incentivized: false and has the correct miniChefAddress reference. No other reward-related logic or UI references were found that would need updating.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Searching for references to CANTO_WRAPPER_POOL_SWAP_TOKEN across the codebase
rg -A 5 "CANTO_WRAPPER_POOL_SWAP_TOKEN"
Length of output: 1164
Script:
#!/bin/bash
# Search for files containing both CANTO_WRAPPER_POOL_SWAP_TOKEN and incentive-related terms
rg -l "CANTO_WRAPPER_POOL_SWAP_TOKEN" | xargs rg -A 5 "incentiv|reward"
# Also search for any UI components that might reference this token
rg -A 5 "0x0271984e4cfA2A0f02664baACD551CcFCC9920E8"
Length of output: 35730
476-476: Confirm consistency and potential impact of removing Canto pool incentives here too.
Mirroring the update from true to false ensures uniformity across packages. Confirm that any external scripts or services referencing the old incentive structure in synapse-constants are aligned with this change.
Run this script to check for references to CANTO_POOL_SWAP_TOKEN in the repository:
✅ Verification successful
Let me gather more information about the incentive structure and its usage.
Let me try a different search pattern to find related incentive configurations.
Setting Canto pool incentives to false is consistent with the UI implementation
The change to set incentivized: false for the Canto pool is consistent with the codebase implementation. The UI properly handles both incentivized and unincentivized pools through:
- Type definitions in both packages that support the
incentivizedboolean flag - UI components that conditionally render based on the incentivization status
- Dedicated sections for displaying incentivized and unincentivized pools
- Proper internationalization support for both pool types
The change will correctly categorize the Canto pool in the "Unincentivized Pools" section of the interface without requiring any additional modifications.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Searching for references to CANTO_POOL_SWAP_TOKEN across the codebase
rg -A 5 "CANTO_POOL_SWAP_TOKEN"
Length of output: 1140
Script:
#!/bin/bash
# Search for references to incentivized pools or Canto incentives
rg -A 5 "incentivized.*Canto|Canto.*incentivized"
# Search for any configuration or references to pool incentives
ast-grep --pattern 'incentivized: $_'
Length of output: 91
Script:
#!/bin/bash
# Search for the pool configuration object containing the incentivized field
ast-grep --pattern '{
$$$
incentivized: $_,
$$$
}'
# Search for any files that might reference pool incentives
rg -i "pool.*incentiv|incentiv.*pool"
Length of output: 2870
| "dest_chain_id": { | ||
| "type": "integer" | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add chain ID validation to prevent unsupported networks
To align with the PR objective of preventing Boba network bridging, add validation for chain IDs:
"dest_chain_id": {
"type": "integer"
+ "description": "Destination chain ID (Boba network not supported)",
+ "enum": [1, 56, 137, 42161], // Example: Ethereum, BSC, Polygon, Arbitrum
+ "x-enum-descriptions": [
+ "Ethereum Mainnet",
+ "BNB Smart Chain",
+ "Polygon",
+ "Arbitrum One"
+ ]
}(Apply similar changes to all chain ID fields)
Also applies to: 342-344, 360-363, 380-383, 462-464, 477-479, 494-496, 506-508
| "produces": [ | ||
| "application/json" | ||
| ], | ||
| "tags": [ | ||
| "quotes" | ||
| ], | ||
| "summary": "Upsert quote", | ||
| "parameters": [ | ||
| { | ||
| "description": "query params", | ||
| "name": "request", | ||
| "in": "body", | ||
| "required": true, | ||
| "schema": { | ||
| "$ref": "#/definitions/model.PutRelayerQuoteRequest" | ||
| } | ||
| } | ||
| ], | ||
| "responses": { | ||
| "200": { | ||
| "description": "OK", | ||
| "headers": { | ||
| "X-Api-Version": { | ||
| "type": "string", | ||
| "description": "API Version Number - See docs for more info" | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| "origin_fast_bridge_address": { | ||
| "type": "string" | ||
| "/rfq": { | ||
| "put": { | ||
| "description": "Initiate an Active Request-For-Quote and return the best quote available.", | ||
| "consumes": [ | ||
| "application/json" | ||
| ], | ||
| "produces": [ | ||
| "application/json" | ||
| ], | ||
| "tags": [ | ||
| "quotes" | ||
| ], | ||
| "summary": "Initiate an Active RFQ", | ||
| "parameters": [ | ||
| { | ||
| "description": "Initiate an Active Request-For-Quote", | ||
| "name": "request", | ||
| "in": "body", | ||
| "required": true, | ||
| "schema": { | ||
| "$ref": "#/definitions/model.PutRFQRequest" | ||
| } | ||
| } | ||
| ], | ||
| "responses": { | ||
| "200": { | ||
| "description": "OK", | ||
| "schema": { | ||
| "$ref": "#/definitions/model.PutRFQResponse" | ||
| }, | ||
| "headers": { | ||
| "X-Api-Version": { | ||
| "type": "string", | ||
| "description": "API Version Number - See docs for more info" | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| "origin_token_addr": { | ||
| "type": "string" | ||
| "/rfq_stream": { | ||
| "get": { | ||
| "description": "Establish a WebSocket connection to listen for streaming active quote requests.", | ||
| "produces": [ | ||
| "application/json" | ||
| ], | ||
| "tags": [ | ||
| "quotes" | ||
| ], | ||
| "summary": "Listen for Active RFQs", | ||
| "responses": { | ||
| "101": { | ||
| "description": "Switching Protocols", | ||
| "schema": { | ||
| "type": "string" | ||
| }, | ||
| "headers": { | ||
| "X-Api-Version": { | ||
| "type": "string", | ||
| "description": "API Version Number - See docs for more info" | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Add security definitions for API endpoints
The API handles financial operations but lacks security definitions. Consider adding:
- Authentication requirements
- Rate limiting headers
- API key or OAuth2 security schemes
Add security definitions:
{
"swagger": "2.0",
+ "securityDefinitions": {
+ "apiKey": {
+ "type": "apiKey",
+ "name": "X-API-Key",
+ "in": "header"
+ }
+ },Add rate limiting headers to responses:
"headers": {
"X-Api-Version": {
"type": "string",
"description": "API Version Number - See docs for more info"
},
+ "X-RateLimit-Limit": {
+ "type": "integer",
+ "description": "Request limit per hour"
+ },
+ "X-RateLimit-Remaining": {
+ "type": "integer",
+ "description": "Remaining requests in the current time window"
+ }
}Committable suggestion skipped: line range outside the PR's diff.
| { | ||
| "type": "integer", | ||
| "description": "origin chain id to filter quotes by", | ||
| "name": "originChainID", | ||
| "in": "path" | ||
| }, | ||
| { | ||
| "type": "string", | ||
| "description": "origin chain id to filter quotes by", | ||
| "name": "originTokenAddr", | ||
| "in": "path" | ||
| }, | ||
| { | ||
| "type": "integer", | ||
| "description": "destination chain id to filter quotes by", | ||
| "name": "destChainID", | ||
| "in": "path" | ||
| }, | ||
| { | ||
| "type": "string", | ||
| "description": "destination token address to filter quotes by", | ||
| "name": "destTokenAddr", | ||
| "in": "path" | ||
| }, | ||
| { | ||
| "type": "string", | ||
| "description": "relayer address to filter quotes by", | ||
| "name": "relayerAddr", | ||
| "in": "path" | ||
| } |
There was a problem hiding this comment.
Fix parameter location in /quotes endpoint
The parameters are defined with in: "path" but the descriptions and usage suggest they should be query parameters. This mismatch could cause implementation issues.
{
"type": "integer",
"description": "origin chain id to filter quotes by",
"name": "originChainID",
- "in": "path"
+ "in": "query"
},(Apply similar changes to other parameters)
Committable suggestion skipped: line range outside the PR's diff.
| - name: Read golangci-lint version | ||
| id: golangci_version | ||
| run: echo "version=v$(cat .golangci-version)" >> $GITHUB_OUTPUT | ||
|
|
There was a problem hiding this comment.
Fix potential shell injection vulnerability.
The version reading step should properly quote the variable to prevent potential shell injection.
Apply this diff to fix the security issue:
- echo "version=v$(cat .golangci-version)" >> $GITHUB_OUTPUT
+ echo "version=v$(cat .golangci-version)" >> "$GITHUB_OUTPUT"📝 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.
| - name: Read golangci-lint version | |
| id: golangci_version | |
| run: echo "version=v$(cat .golangci-version)" >> $GITHUB_OUTPUT | |
| - name: Read golangci-lint version | |
| id: golangci_version | |
| run: echo "version=v$(cat .golangci-version)" >> "$GITHUB_OUTPUT" | |
🧰 Tools
🪛 actionlint (1.7.4)
342-342: shellcheck reported issue in this script: SC2086:info:1:45: Double quote to prevent globbing and word splitting
(shellcheck)
Merging sdk and fe changes that prevent people from bridging to/from boba
4c16438: docs preview link
4c16438: explorer-ui preview link
4c16438: synapse-interface preview link
Summary by CodeRabbit
Release Notes
New Features
golangci-lintImprovements
Bug Fixes
Dependency Updates
Documentation
Chores