FE Release 2024-11-11 #3385
FE Release 2024-11-11 #3385
Conversation
* test: ignore console.error in negative API cases * test: better fetch mock to simulate empty list of quotes * test: partially apply Rabbit suggestion * test: retry for flaky tests
* decimals truncation * return errors on improper decimals
* add additional check * return origin and destination contracts [goreleaser] * fixed * [goreleaser] * [goreleaser] * fix lint [goreleaser] * format [goreleaser] * remove 0x [goreleaser] * fix lint
* refactor: relax pragma in interfaces * refactor: relax pragma in libs * refactor: test pragmas
abtestingalpha
requested review from
trajan0x,
bigboydiamonds,
aureliusbtc,
Defi-Moses and
ChiTimesChi
as code owners
WalkthroughThe changes in this pull request primarily involve enhancements to error handling and validation across various components of the codebase. Key modifications include updates to command handling logic in the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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. 🔧 eslint
packages/synapse-interface/components/layouts/LandingPageWrapper/index.tsxOops! Something went wrong! :( ESLint: 8.57.1 Error: Failed to load parser '@babel/eslint-parser' declared in 'packages/synapse-interface/.eslintrc.js': Cannot find module '@babel/core/package.json'
🔧 golangci-lintlevel=warning msg="[config_reader] The configuration option 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
go
abtestingalpha
temporarily deployed
to
github-pages
Deploying sanguine-fe with
|
| Latest commit: |
4019633
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://5c5ff61c.sanguine-fe.pages.dev |
| Branch Preview URL: | https://master.sanguine-fe.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (14)
packages/sdk-router/src/rfq/api.integration.test.ts (2)
5-6: Consider making the retry count configurable.Instead of hardcoding the retry count, consider making it configurable through environment variables. This would allow different CI environments to adjust the retry behavior as needed.
+const MAX_RETRIES = process.env.TEST_MAX_RETRIES ? parseInt(process.env.TEST_MAX_RETRIES) : 3; -jest.retryTimes(3) +jest.retryTimes(MAX_RETRIES)
Line range hint
9-14: Enhance test robustness with proper error handling and cleanup.The integration test could benefit from additional error handling, timeout configuration, and proper cleanup:
- Add timeout to prevent long-running tests
- Add error handling for API failures
- Add cleanup after tests
- Consider adding retry delay
describe('getAllQuotes', () => { + jest.setTimeout(30000); // 30 seconds timeout + + afterAll(async () => { + // Add cleanup if needed + }); + it('Integration test', async () => { - const result = await getAllQuotes() - // console.log('Current quotes: ' + JSON.stringify(result, null, 2)) - expect(result.length).toBeGreaterThan(0) + try { + const result = await getAllQuotes(); + expect(result).toBeDefined(); + expect(Array.isArray(result)).toBe(true); + expect(result.length).toBeGreaterThan(0); + + // Validate quote structure + if (result.length > 0) { + const firstQuote = result[0]; + expect(firstQuote).toHaveProperty('id'); + // Add more specific assertions based on expected quote structure + } + } catch (error) { + console.error('Test failed:', error); + throw error; + } }) })core/bytes.go (1)
29-37: LGTM! Consider enhancing the documentation.The implementation is correct and follows Go best practices. However, the documentation could be more specific about the 32-byte requirement and the error case.
Consider updating the comment to:
-// BytesToArray converts a slice to a 32 length byte array. +// BytesToArray converts a byte slice to a fixed-length [32]byte array. +// Returns an error if the input slice length is not exactly 32 bytes.packages/rest-api/src/app.ts (1)
17-28: Enhance logging for CORS-related debuggingConsider adding CORS-related information to the logging middleware to help debug cross-origin issues in production.
Add origin information to the existing logging:
app.use((req, res, next) => { logger.info({ msg: `Incoming request ${req.path}`, method: req.method, path: req.path, query: req.query, + origin: req.headers.origin, body: req.method === 'POST' || req.method === 'PUT' ? req.body : undefined, }) // ... rest of the logging middleware })packages/sdk-router/src/rfq/api.test.ts (1)
93-93: Consider extracting error logging expectation into a shared helper.While the error assertions are consistent, the repeated
expect(console.error).toHaveBeenCalled()checks could be made more maintainable by extracting them into a shared helper function.+ const expectErrorLogged = () => { + expect(console.error).toHaveBeenCalled(); + }; it('when the response is not ok', async () => { fetchMock.mockResponseOnce(JSON.stringify(quotesAPI), { status: 500 }) const result = await getAllQuotes() expect(result).toEqual([]) - expect(console.error).toHaveBeenCalled() + expectErrorLogged() })Also applies to: 100-100, 109-109
packages/rest-api/src/tests/swapTxInfoRoute.test.ts (1)
93-107: LGTM! The new test case enhances decimal validation.The test case effectively validates the handling of amounts with excessive decimal places for USDC transactions.
Consider these minor improvements:
- }, 10000) + }, 10_000)- amount: '1000.123456789', // Assuming USDC has 6 decimals + amount: '1000.123456789', // USDC has 6 decimals (1000.123456 is max)expect(response.body.error).toHaveProperty( 'message', - expect.stringContaining('Amount has too many decimals') + 'Amount has too many decimals for USDC (max 6)' )packages/rest-api/src/tests/swapRoute.test.ts (2)
137-150: LGTM! Consider these minor improvements.The test case effectively validates the decimal place restrictions. A few suggestions to enhance it:
- Document USDC's decimal places in the test description for clarity
- Use consistent timeout notation (10_000 instead of 10000) to match other tests
- Consider using exact error message matching instead of
stringContaining- it('should return 400 for amount with too many decimals', async () => { + it('should return 400 for amount exceeding USDC\'s 6 decimal places', async () => { const response = await request(app).get('/swap').query({ chain: '1', fromToken: USDC.addresses[1], toToken: DAI.addresses[1], - amount: '1000.123456789', // Assuming USDC has 6 decimals + amount: '1000.123456789', // USDC has 6 decimals }) expect(response.status).toBe(400) expect(response.body.error).toHaveProperty( - 'message', - expect.stringContaining('Amount has too many decimals') + 'message', + 'Amount has too many decimals' ) - }, 10000) + }, 10_000)
136-136: Consider adding test cases for amount edge cases.The test suite would benefit from additional test cases to validate:
- Zero amount handling
- Negative amount handling
it('should return 400 for zero amount', async () => { const response = await request(app).get('/swap').query({ chain: '1', fromToken: USDC.addresses[1], toToken: DAI.addresses[1], amount: '0', }) expect(response.status).toBe(400) expect(response.body.error).toHaveProperty( 'message', 'Amount must be greater than 0' ) }, 10_000) it('should return 400 for negative amount', async () => { const response = await request(app).get('/swap').query({ chain: '1', fromToken: USDC.addresses[1], toToken: DAI.addresses[1], amount: '-100', }) expect(response.status).toBe(400) expect(response.body.error).toHaveProperty( 'message', 'Amount must be greater than 0' ) }, 10_000)packages/rest-api/src/tests/bridgeTxInfoRoute.test.ts (2)
163-178: LGTM! Consider extracting decimal precision to a constant.The test case effectively validates the decimal precision of USDC amounts. However, the magic number '123456789' could be made more maintainable.
Consider extracting USDC's decimal precision to a constant:
+ const USDC_DECIMALS = 6; + const AMOUNT_WITH_EXCESS_DECIMALS = `1000.${'1'.repeat(USDC_DECIMALS + 3)}`; it('should return 400 for amount with too many decimals', async () => { const response = await request(app).get('/bridgeTxInfo').query({ fromChain: '1', toChain: '137', fromToken: USDC.addresses[1], toToken: USDC.addresses[137], - amount: '1000.123456789', // Assuming USDC has 6 decimals + amount: AMOUNT_WITH_EXCESS_DECIMALS, destAddress: '0x742d35Cc6634C0532925a3b844Bc454e4438f44e', })
163-178: Consider adding test cases for edge cases.While the current test covers the "too many decimals" case, consider adding tests for these scenarios:
- Exactly 6 decimals (should pass)
- Empty decimal part (should pass)
- Only decimal part (e.g., ".123456")
Here's a suggested implementation:
it('should accept amount with exactly 6 decimals', async () => { const response = await request(app).get('/bridgeTxInfo').query({ fromChain: '1', toChain: '137', fromToken: USDC.addresses[1], toToken: USDC.addresses[137], amount: '1000.123456', destAddress: '0x742d35Cc6634C0532925a3b844Bc454e4438f44e', }) expect(response.status).toBe(200) }) it('should accept amount with no decimals', async () => { const response = await request(app).get('/bridgeTxInfo').query({ fromChain: '1', toChain: '137', fromToken: USDC.addresses[1], toToken: USDC.addresses[137], amount: '1000', destAddress: '0x742d35Cc6634C0532925a3b844Bc454e4438f44e', }) expect(response.status).toBe(200) }) it('should handle decimal-only amounts', async () => { const response = await request(app).get('/bridgeTxInfo').query({ fromChain: '1', toChain: '137', fromToken: USDC.addresses[1], toToken: USDC.addresses[137], amount: '.123456', destAddress: '0x742d35Cc6634C0532925a3b844Bc454e4438f44e', }) expect(response.status).toBe(200) })packages/rest-api/src/tests/bridgeRoute.test.ts (1)
177-191: Consider enhancing test coverage with edge cases.While the current test validates rejection of excessive decimals, consider adding these cases:
- Maximum allowed decimals (exactly 6 for USDC)
- One decimal place over the limit (7 decimals)
- Different tokens with different decimal limits
Here's a suggested implementation:
it('should return 400 for amount with too many decimals', async () => { const response = await request(app).get('/bridge').query({ fromChain: '1', toChain: '10', fromToken: USDC.addresses[1], toToken: USDC.addresses[10], amount: '1000.123456789', // Assuming USDC has 6 decimals }) expect(response.status).toBe(400) expect(response.body.error).toHaveProperty( 'message', expect.stringContaining('Amount has too many decimals') ) }, 15000) + it('should accept amount with exactly 6 decimals for USDC', async () => { + const response = await request(app).get('/bridge').query({ + fromChain: '1', + toChain: '10', + fromToken: USDC.addresses[1], + toToken: USDC.addresses[10], + amount: '1000.123456', + }) + expect(response.status).toBe(200) + }, 15000) + + it('should reject amount with 7 decimals for USDC', async () => { + const response = await request(app).get('/bridge').query({ + fromChain: '1', + toChain: '10', + fromToken: USDC.addresses[1], + toToken: USDC.addresses[10], + amount: '1000.1234567', + }) + expect(response.status).toBe(400) + expect(response.body.error).toHaveProperty( + 'message', + expect.stringContaining('Amount has too many decimals') + ) + }, 15000)packages/rest-api/src/routes/swapRoute.ts (1)
170-176: Consider caching token informationThe current implementation retrieves token information for every validation. Consider caching the token information to improve performance, especially for high-traffic scenarios.
Example optimization:
// Cache token info at the route level const tokenInfoCache = new Map<string, TokenInfo>(); // In the validation const cacheKey = `${req.query.chain}-${req.query.fromToken}`; let fromTokenInfo = tokenInfoCache.get(cacheKey); if (!fromTokenInfo) { fromTokenInfo = tokenAddressToToken(req.query.chain, req.query.fromToken); tokenInfoCache.set(cacheKey, fromTokenInfo); }packages/contracts-rfq/contracts/libs/BridgeTransactionV2.sol (1)
Line range hint
49-76: Consider adding bounds checking for zapData lengthThe
validateV2function checks for minimum length but doesn't validate the maximum length ofzapData. While not strictly necessary, consider adding a maximum length check to prevent potential gas-related issues with extremely large transactions.function validateV2(bytes calldata encodedTx) internal pure { // Check the minimum length: must at least include all static fields. if (encodedTx.length < OFFSET_ZAP_DATA) revert BridgeTransactionV2__InvalidEncodedTx(); + // Optional: Add maximum length check for zapData + if (encodedTx.length > OFFSET_ZAP_DATA + 32768) revert BridgeTransactionV2__InvalidEncodedTx(); // Once we validated the length, we can be sure that the version field is present. uint16 version_ = version(encodedTx); if (version_ != VERSION) revert BridgeTransactionV2__UnsupportedVersion(version_); }contrib/opbot/botmd/commands.go (1)
374-376: Include the chain ID in the error message for clarity.To enhance debugging and maintain consistency, include the chain ID in the error message when a contract address is not found.
Apply this diff to update the error message:
if !ok { - return nil, fmt.Errorf("no contract address for chain ID") + return nil, fmt.Errorf("no contract address for chain ID %d", chainID) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (43)
contrib/opbot/botmd/commands.go(5 hunks)core/bytes.go(1 hunks)packages/contracts-rfq/CHANGELOG.md(1 hunks)packages/contracts-rfq/contracts/interfaces/IAdmin.sol(1 hunks)packages/contracts-rfq/contracts/interfaces/IFastBridge.sol(1 hunks)packages/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol(1 hunks)packages/contracts-rfq/contracts/interfaces/IFastBridgeV2Errors.sol(1 hunks)packages/contracts-rfq/contracts/interfaces/IMulticallTarget.sol(1 hunks)packages/contracts-rfq/contracts/interfaces/IZapRecipient.sol(1 hunks)packages/contracts-rfq/contracts/libs/BridgeTransactionV2.sol(1 hunks)packages/contracts-rfq/contracts/libs/Errors.sol(1 hunks)packages/contracts-rfq/contracts/utils/MulticallTarget.sol(1 hunks)packages/contracts-rfq/package.json(1 hunks)packages/contracts-rfq/test/MulticallTarget.t.sol(1 hunks)packages/contracts-rfq/test/harnesses/MulticallTargetHarness.sol(1 hunks)packages/contracts-rfq/test/mocks/ExcessiveReturnValueRecipient.sol(1 hunks)packages/contracts-rfq/test/mocks/IncorrectReturnValueRecipient.sol(1 hunks)packages/contracts-rfq/test/mocks/NoOpContract.sol(1 hunks)packages/contracts-rfq/test/mocks/NoReturnValueRecipient.sol(1 hunks)packages/contracts-rfq/test/mocks/NonPayableRecipient.sol(1 hunks)packages/contracts-rfq/test/mocks/RecipientMock.sol(1 hunks)packages/rest-api/CHANGELOG.md(1 hunks)packages/rest-api/package.json(2 hunks)packages/rest-api/src/app.ts(1 hunks)packages/rest-api/src/routes/bridgeRoute.ts(2 hunks)packages/rest-api/src/routes/bridgeTxInfoRoute.ts(2 hunks)packages/rest-api/src/routes/swapRoute.ts(2 hunks)packages/rest-api/src/routes/swapTxInfoRoute.ts(2 hunks)packages/rest-api/src/tests/bridgeRoute.test.ts(1 hunks)packages/rest-api/src/tests/bridgeTxInfoRoute.test.ts(1 hunks)packages/rest-api/src/tests/swapRoute.test.ts(1 hunks)packages/rest-api/src/tests/swapTxInfoRoute.test.ts(1 hunks)packages/rest-api/src/validations/validateDecimals.ts(1 hunks)packages/sdk-router/CHANGELOG.md(1 hunks)packages/sdk-router/package.json(1 hunks)packages/sdk-router/src/rfq/api.integration.test.ts(1 hunks)packages/sdk-router/src/rfq/api.test.ts(2 hunks)packages/sdk-router/src/sdk.test.ts(1 hunks)packages/synapse-interface/CHANGELOG.md(1 hunks)packages/synapse-interface/components/layouts/LandingPageWrapper/index.tsx(1 hunks)packages/synapse-interface/package.json(2 hunks)packages/widget/CHANGELOG.md(1 hunks)packages/widget/package.json(2 hunks)
✅ Files skipped from review due to trivial changes (23)
- packages/contracts-rfq/CHANGELOG.md
- packages/contracts-rfq/contracts/interfaces/IAdmin.sol
- packages/contracts-rfq/contracts/interfaces/IFastBridgeV2Errors.sol
- packages/contracts-rfq/contracts/interfaces/IMulticallTarget.sol
- packages/contracts-rfq/contracts/interfaces/IZapRecipient.sol
- packages/contracts-rfq/contracts/libs/Errors.sol
- packages/contracts-rfq/contracts/utils/MulticallTarget.sol
- packages/contracts-rfq/package.json
- packages/contracts-rfq/test/MulticallTarget.t.sol
- packages/contracts-rfq/test/harnesses/MulticallTargetHarness.sol
- packages/contracts-rfq/test/mocks/ExcessiveReturnValueRecipient.sol
- packages/contracts-rfq/test/mocks/IncorrectReturnValueRecipient.sol
- packages/contracts-rfq/test/mocks/NoOpContract.sol
- packages/contracts-rfq/test/mocks/NoReturnValueRecipient.sol
- packages/contracts-rfq/test/mocks/NonPayableRecipient.sol
- packages/contracts-rfq/test/mocks/RecipientMock.sol
- packages/rest-api/CHANGELOG.md
- packages/rest-api/package.json
- packages/sdk-router/CHANGELOG.md
- packages/sdk-router/package.json
- packages/synapse-interface/CHANGELOG.md
- packages/synapse-interface/package.json
- packages/widget/CHANGELOG.md
🧰 Additional context used
📓 Learnings (3)
packages/contracts-rfq/contracts/interfaces/IFastBridge.sol (1)
Learnt from: ChiTimesChi
PR: synapsecns/sanguine#3284
File: packages/contracts-rfq/contracts/FastBridgeV2.sol:100-118
Timestamp: 2024-11-10T03:44:13.489Z
Learning: When reviewing code in `packages/contracts-rfq/contracts/FastBridgeV2.sol` (Solidity), focus on changes introduced in the PR and avoid commenting on existing functions like `refund` if they are not modified by the PR, as such comments are considered out of scope.
packages/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol (1)
Learnt from: ChiTimesChi
PR: synapsecns/sanguine#3284
File: packages/contracts-rfq/contracts/FastBridgeV2.sol:100-118
Timestamp: 2024-11-10T03:44:13.489Z
Learning: When reviewing code in `packages/contracts-rfq/contracts/FastBridgeV2.sol` (Solidity), focus on changes introduced in the PR and avoid commenting on existing functions like `refund` if they are not modified by the PR, as such comments are considered out of scope.
packages/rest-api/src/routes/swapTxInfoRoute.ts (4)
Learnt from: Defi-Moses
PR: synapsecns/sanguine#3133
File: packages/rest-api/src/controllers/swapTxInfoController.ts:14-14
Timestamp: 2024-11-10T03:44:13.489Z
Learning: In this project, request parameter validation is handled in the route files (e.g., `swapTxInfoRoute.ts`) rather than in the controller files.
Learnt from: Defi-Moses
PR: synapsecns/sanguine#3133
File: packages/rest-api/src/controllers/swapTxInfoController.ts:14-14
Timestamp: 2024-11-10T03:44:09.367Z
Learning: In this project, request parameter validation is handled in the route files (e.g., `swapTxInfoRoute.ts`) rather than in the controller files.
Learnt from: Defi-Moses
PR: synapsecns/sanguine#3133
File: packages/rest-api/src/controllers/swapTxInfoController.ts:14-14
Timestamp: 2024-11-10T03:44:13.489Z
Learning: In this project, request parameter validation is handled in the route files (e.g., `swapTxInfoRoute.ts`) rather than in the controller files.
Learnt from: Defi-Moses
PR: synapsecns/sanguine#3133
File: packages/rest-api/src/controllers/swapTxInfoController.ts:14-14
Timestamp: 2024-11-10T03:44:09.367Z
Learning: In this project, request parameter validation is handled in the route files (e.g., `swapTxInfoRoute.ts`) rather than in the controller files.
🔇 Additional comments (28)
packages/rest-api/src/validations/validateDecimals.ts (1)
1-11: Add unit tests to verify edge cases
Please ensure comprehensive test coverage for the validation function.
Let's check if there are any existing tests:
I can help generate test cases covering:
- Valid inputs (integers, decimals within limit)
- Invalid inputs (null, undefined, empty string)
- Edge cases (scientific notation, multiple decimal points)
- Error conditions (negative expectedDecimals)
Would you like me to generate the test suite?
✅ Verification successful
Let me search for any existing test coverage or usage of this validation function to make a more informed assessment.
Existing test coverage found for decimal validation
The validation function is already being tested through integration tests in all relevant route handlers:
bridgeRoute.test.tsswapRoute.test.tsbridgeTxInfoRoute.test.tsswapTxInfoRoute.test.ts
Each route has a test case specifically checking for "amount with too many decimals" using USDC (6 decimals) as the test case. While these integration tests cover the main validation scenario, it would still be valuable to add unit tests for edge cases, but this isn't as critical given the existing coverage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Look for test files related to validateDecimals
fd -e test.ts -e spec.ts validateDecimals
Length of output: 43
Script:
#!/bin/bash
# Let's try a broader search for test files in the directory containing validateDecimals
cd packages/rest-api/src/validations
ls -la *.test.ts *.spec.ts 2>/dev/null || echo "No test files found in validations directory"
# Let's also check if there are any tests that might reference this validation
rg -l "validateDecimals" ../ --type ts
Length of output: 352
Script:
#!/bin/bash
# Let's check how validateDecimals is used in these routes to understand its criticality
rg -A 5 "validateDecimals" ../routes/bridgeRoute.ts ../routes/bridgeTxInfoRoute.ts ../routes/swapTxInfoRoute.ts ../routes/swapRoute.ts
# Let's also check if there are any test files for these routes that might indirectly test validateDecimals
fd -e test.ts -e spec.ts . ../routes/
Length of output: 540
Script:
#!/bin/bash
# Let's fix the path and try again from the repository root
rg -A 5 "validateDecimals" packages/rest-api/src/routes/
# And look for test files in the correct path
fd -e test.ts -e spec.ts . packages/rest-api/src/routes/
Length of output: 2248
packages/sdk-router/src/rfq/api.integration.test.ts (1)
5-6: LGTM! Good practice to handle flaky integration tests.
Adding retry mechanism for flaky tests is a good practice, especially for integration tests that depend on external services.
packages/widget/package.json (2)
4-4: LGTM: Version bump follows semantic versioning
The increment from 0.9.4 to 0.9.5 is appropriate for a patch release.
71-71: Verify compatibility with the updated SDK Router
The @synapsecns/sdk-router dependency has been updated from ^0.11.5 to ^0.11.6. While this is a patch version bump, it's important to verify compatibility.
✅ Verification successful
Let me gather more information about the changes between versions 0.11.5 and 0.11.6 by checking the CHANGELOG.md content.
Based on the CHANGELOG.md content, I can now provide a final response:
Version 0.11.6 is a safe update with no breaking changes
The update from 0.11.5 to 0.11.6 is a version bump only release with no functional changes, as indicated by the changelog entry: "Version bump only for package @synapsecns/sdk-router". This type of update typically includes only maintenance changes like dependency updates or documentation improvements.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for breaking changes in sdk-router update
# Check if there's a CHANGELOG.md in the sdk-router package
fd -t f "CHANGELOG.md" -d 3 packages/sdk-router
# Look for any breaking changes or deprecation notices in recent commits
git log --grep="BREAKING CHANGE\|deprecat" -i packages/sdk-router
# Check for major interface changes in the sdk-router package
rg -A 5 "export (interface|type|class|function)" packages/sdk-router/src
Length of output: 76803
Script:
#!/bin/bash
# Check the content of CHANGELOG.md to understand changes between versions
cat packages/sdk-router/CHANGELOG.md
Length of output: 21853
packages/rest-api/src/app.ts (2)
19-19:
HTTP methods mismatch in CORS configuration
The CORS configuration only allows GET and OPTIONS methods, but the application appears to handle POST and PUT requests (evidenced by express.json() middleware and logging logic). This will block legitimate POST/PUT requests from browser clients.
Update the allowed methods to match your API's requirements:
- res.setHeader('Access-Control-Allow-Methods', 'GET, OPTIONS')
+ res.setHeader('Access-Control-Allow-Methods', 'GET, POST, PUT, OPTIONS')Let's verify the HTTP methods used by the API routes:
#!/bin/bash
# Check for route definitions to identify used HTTP methods
rg -g '*.{ts,js}' '(get|post|put|delete|patch)\(' ./packages/rest-api/src/routes17-28:
Security: CORS configuration is too permissive
The current CORS implementation has several security concerns:
- Using
'*'forAccess-Control-Allow-Originallows any domain to access your API - Using
'*'forAccess-Control-Allow-Headersis unnecessarily permissive - CORS configuration is hardcoded without environment-specific controls
Consider implementing a more restrictive CORS policy:
+const allowedOrigins = process.env.ALLOWED_ORIGINS?.split(',') || ['http://localhost:3000']
+const allowedHeaders = ['Content-Type', 'Authorization']
app.use((req, res, next) => {
- res.setHeader('Access-Control-Allow-Origin', '*')
+ const origin = req.headers.origin
+ if (origin && allowedOrigins.includes(origin)) {
+ res.setHeader('Access-Control-Allow-Origin', origin)
+ }
res.setHeader('Access-Control-Allow-Methods', 'GET, OPTIONS')
- res.setHeader('Access-Control-Allow-Headers', '*')
+ res.setHeader('Access-Control-Allow-Headers', allowedHeaders.join(', '))
if (req.method === 'OPTIONS') {
res.sendStatus(200)
return
}
next()
})Let's verify if there are any security-related configurations or environment variables already in place:
packages/sdk-router/src/rfq/api.test.ts (1)
79-87: LGTM! Well-structured test setup for error logging.
The console error mocking implementation follows testing best practices with proper setup and cleanup.
packages/rest-api/src/tests/swapTxInfoRoute.test.ts (1)
93-107: Verify decimal validation coverage for other tokens.
While this test case covers USDC's decimal validation, it would be beneficial to ensure similar validation exists for other tokens with different decimal places (e.g., DAI with 18 decimals).
packages/contracts-rfq/contracts/interfaces/IFastBridge.sol (2)
2-2: Consider security implications of version downgrade
While Solidity 0.8.4 includes essential security features, you might miss out on important optimizations and security improvements available in 0.8.20. Please ensure:
- The version change doesn't affect any existing security measures
- No critical features from newer versions are being used elsewhere in the codebase
- The downgrade is compatible with all dependencies
Let's verify the compatibility with dependencies:
#!/bin/bash
# Description: Check dependencies that might require specific Solidity versions
# Expected: No conflicts with required Solidity version
echo "Checking OpenZeppelin contracts version:"
fd -t f "package.json" "packages/contracts-rfq/" --exec cat {} | jq '.dependencies["@openzeppelin/contracts"]'
echo -e "\nChecking for any hardhat configurations specifying Solidity versions:"
fd "hardhat.config" "packages/contracts-rfq/" --exec cat {}2-2: Verify the rationale for Solidity version downgrade
The pragma version has been changed from ^0.8.20 to ^0.8.4. While both versions include important security features like checked arithmetic, this downgrade needs justification as it might affect compiler optimizations and language features.
Let's check if this is part of a broader version standardization:
✅ Verification successful
Solidity version change requires careful consideration
The version change from ^0.8.4 to ^0.8.20 and back to ^0.8.4 in IFastBridge.sol interface appears inconsistent with the codebase. The analysis shows:
-
The implementation contracts use varying versions:
- FastBridgeV2.sol: 0.8.24
- FastBridge.sol: 0.8.20
- Admin.sol: ^0.8.20
- Most interfaces: ^0.8.4
- Legacy contracts: 0.8.17
-
The interface should be compatible with all implementations, and ^0.8.4 provides this backward compatibility while maintaining critical security features like checked arithmetic.
-
The version change aligns with best practices as interfaces should use a lower bound to ensure maximum compatibility with implementations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check Solidity versions across the contracts
# Expected: Consistent version usage across related contracts
echo "Checking Solidity versions in contracts:"
rg -A 1 "pragma solidity" "packages/contracts-rfq/contracts/"
echo -e "\nChecking if package.json specifies a Solidity version:"
fd -t f "package.json" "packages/contracts-rfq/" --exec cat {} | jq '.solidity'
Length of output: 429
Script:
#!/bin/bash
# Let's try a different approach to check Solidity versions
echo "Checking Solidity versions in all Solidity files:"
rg "pragma solidity" --type solidity
echo -e "\nChecking for any hardhat config files:"
fd "hardhat.config" -x cat {}
Length of output: 833
Script:
#!/bin/bash
# Let's try one more time with explicit path and extension
echo "Checking Solidity versions in contracts:"
rg "pragma solidity" packages/contracts-rfq/contracts/
echo -e "\nChecking all .sol files to ensure we don't miss any:"
fd -e sol . packages/contracts-rfq/contracts/ -x cat {}
Length of output: 114868
packages/rest-api/src/tests/bridgeRoute.test.ts (1)
177-191: LGTM! The test case effectively validates decimal precision.
The test appropriately verifies that the API rejects amounts with excessive decimal places for USDC transactions.
packages/rest-api/src/routes/swapTxInfoRoute.ts (2)
12-13: LGTM: Import statements are well-organized
The new utility imports are properly placed and follow the project's import organization pattern.
146-159: Verify consistent amount validation across routes
According to the summary, similar amount validation changes were made to other routes. Let's verify the consistency.
✅ Verification successful
Amount validation is consistent across all route files
The verification shows that all four route files (swapTxInfoRoute.ts, swapRoute.ts, bridgeTxInfoRoute.ts, and bridgeRoute.ts) implement consistent amount validation:
- All use the
check('amount')with.exists(),.isNumeric(), and a custom validator - All consistently import and use
validateDecimalswithfromTokenInfo.decimals - All have the same error message for required amount validation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent amount validation across route files
# Search for amount validation in route files
echo "Checking amount validation patterns across routes:"
rg -A 5 "check\('amount'\)" "packages/rest-api/src/routes/"
# Search for validateDecimals usage
echo -e "\nChecking validateDecimals usage:"
rg "validateDecimals" "packages/rest-api/src/routes/"
Length of output: 3042
packages/rest-api/src/routes/swapRoute.ts (2)
13-14: LGTM! Clean import additions
The new imports are well-organized and follow the project's module organization pattern.
166-179: Verify error handling for token info retrieval
The decimal validation is a good addition, but we should ensure proper error handling if tokenAddressToToken fails to retrieve token information.
Let's verify the error handling in tokenAddressToToken:
✅ Verification successful
Error handling is properly implemented in tokenAddressToToken
Looking at the implementation in packages/rest-api/src/utils/tokenAddressToToken.ts, the function properly handles error cases by returning null when:
- The chain data is not found (
if (!chainData) return null) - The token info is not found (
if (!tokenInfo) return null)
This null return value is appropriately handled in the validation chain:
- If tokenAddressToToken returns null, accessing
fromTokenInfo.decimalswill throw an error - This error will be caught by the express-validator middleware
- The error message "amount is required" will be returned to the user
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for error handling in tokenAddressToToken implementation
ast-grep --pattern 'function tokenAddressToToken($_) {
$$$
throw $$$
$$$
}'
Length of output: 83
Script:
#!/bin/bash
# Let's try to find the implementation of tokenAddressToToken function
rg -A 10 "tokenAddressToToken"
Length of output: 32875
packages/rest-api/src/routes/bridgeTxInfoRoute.ts (2)
13-14: LGTM! Clean import additions
The new imports are well-organized and follow the project's structure.
171-177: Verify the implementation of validation utilities
Let's ensure the validation utilities are properly implemented and tested.
packages/rest-api/src/routes/bridgeRoute.ts (2)
13-14: LGTM! Clean import additions
The new imports are well-organized and follow the project's structure for validation and utility functions.
231-244: ```shell
#!/bin/bash
Let's check the test coverage for amount validation
echo "Checking amount validation tests in bridgeRoute.test.ts..."
rg "amount.*decimal" -B 2 -A 2 packages/rest-api/src/tests/bridgeRoute.test.ts
echo -e "\nChecking amount validation tests in swapRoute.test.ts..."
rg "amount.*decimal" -B 2 -A 2 packages/rest-api/src/tests/swapRoute.test.ts
echo -e "\nChecking amount validation tests in bridgeTxInfoRoute.test.ts..."
rg "amount.*decimal" -B 2 -A 2 packages/rest-api/src/tests/bridgeTxInfoRoute.test.ts
echo -e "\nChecking amount validation tests in swapTxInfoRoute.test.ts..."
rg "amount.*decimal" -B 2 -A 2 packages/rest-api/src/tests/swapTxInfoRoute.test.ts
</details>
<details>
<summary>packages/synapse-interface/components/layouts/LandingPageWrapper/index.tsx (1)</summary>
`65-65`: **Verify if ending the World Chain announcement early is intended.**
The announcement end date is being moved earlier (from Nov 15 to Nov 10). Please confirm if ending the World Chain support announcement before the Nov 11 release is intentional.
</details>
<details>
<summary>packages/contracts-rfq/contracts/libs/BridgeTransactionV2.sol (2)</summary>
Line range hint `18-34`: **Verify offset calculations in assembly code**
The offset calculations are critical for correct data packing/unpacking. While the current implementation looks correct, consider adding explicit tests to verify these offsets match the struct layout.
```shell
#!/bin/bash
# Search for related test files
echo "Looking for test files..."
fd -e sol -e js -e ts "BridgeTransactionV2" "packages/contracts-rfq/"
# Check if there are any failing test cases
echo "Checking for test failures or issues..."
rg -i "test.*fail.*bridge.*transaction|incorrect.*offset" "packages/contracts-rfq/"
Line range hint 78-103: Verify gas optimization in split encoding
The split encoding approach to avoid stack-too-deep is a good practice. However, verify if this is the most gas-efficient approach compared to alternatives like struct packing or using memory pointers.
✅ Verification successful
Let me gather more information about the encoding implementation and gas benchmarks.
Let me check the actual encoding implementation to understand the current approach better.
Based on the implementation review, I can now provide a final response:
Split encoding approach is optimal for this use case
The current implementation is already well-optimized for gas efficiency because:
- Uses assembly for efficient data extraction
- Employs tight packing with precise offsets
- Splits encoding to avoid stack-too-deep while maintaining minimal memory operations
- Uses calldata instead of memory where possible
- Implements direct bit manipulation through shift operations
The split encoding approach is not just for avoiding stack-too-deep, but also provides an efficient way to handle the large struct while minimizing memory operations and maintaining readability.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Look for gas optimization tests
echo "Searching for gas optimization tests..."
rg -i "gas.*test.*bridge|benchmark.*encode" "packages/contracts-rfq/"
# Check for any gas optimization discussions or issues
echo "Checking for gas optimization discussions..."
gh issue list --search "gas optimization bridge transaction"
Length of output: 2059
Script:
#!/bin/bash
# Check the encoding benchmark test implementation
echo "Checking encoding benchmark test..."
cat packages/contracts-rfq/test/FastBridgeV2.GasBench.Encoding.t.sol
# Look for struct packing or memory pointer optimizations
echo "Checking for struct packing..."
rg -A 5 "struct BridgeTransaction" packages/contracts-rfq/
Length of output: 2973
Script:
#!/bin/bash
# Check the encoding implementation
echo "Checking BridgeTransactionV2 encoding implementation..."
cat packages/contracts-rfq/contracts/libs/BridgeTransactionV2.sol
# Look for gas benchmark results
echo "Checking gas benchmark results..."
rg -B 2 -A 5 "gas" packages/contracts-rfq/test/FastBridgeV2.GasBench.Encoding.t.sol
Length of output: 12005
packages/sdk-router/src/sdk.test.ts (2)
51-52: LGTM: Improved fetch mock implementation
The mock now explicitly sets ok: true and returns an empty array, making the behavior more deterministic and predictable.
56-57: LGTM: Added retry mechanism for flaky tests
Adding retry capability for flaky tests is a good practice to improve CI reliability. The retry count of 3 is reasonable and aligns with similar updates in other test files.
packages/contracts-rfq/contracts/interfaces/IFastBridgeV2.sol (1)
2-2:
Potential issues due to Solidity version downgrade
Downgrading the Solidity compiler version from ^0.8.20 to ^0.8.4 may remove access to important features, optimizations, and security fixes introduced in newer versions. This change could introduce incompatibilities or unintended behaviors, especially if the code relies on features added after 0.8.4. Ensure that all contracts remain compatible with the older compiler version and verify that this downgrade is necessary.
Apply this diff to revert the Solidity version pragma:
-pragma solidity ^0.8.4;
+pragma solidity ^0.8.20;contrib/opbot/botmd/commands.go (3)
23-23: Import of core package is appropriate.
The addition of "github.com/synapsecns/sanguine/core" is necessary for the new functionality.
278-279: Repeated suppression of gosec linter warnings without justification.
As previously noted, suppressing gosec warnings may hide important security issues. Please address the security concerns or add a justification for the suppression.
Line range hint 358-385: Refactored makeFastBridge function improves modularity and reusability.
Changing the function signature to accept chainID instead of the entire request object decouples it from specific request structures, enhancing modularity and making it more reusable across different contexts.
| export const validateDecimals = ( | ||
| amount: string, | ||
| expectedDecimals: number | ||
| ): boolean => { | ||
| const parts = amount.split('.') | ||
| if (parts.length === 2) { | ||
| const decimals = parts[1].length | ||
| return decimals <= expectedDecimals | ||
| } | ||
| return true | ||
| } |
There was a problem hiding this comment.
Add input validation and handle edge cases
The current implementation has several potential issues:
- No validation for null/undefined/empty strings
- No validation for non-numeric characters
- Doesn't handle multiple decimal points
- Doesn't validate negative expectedDecimals
- Missing handling of scientific notation
Consider this more robust implementation:
+/**
+ * Validates if a numeric string has decimal places less than or equal to the expected number
+ * @param amount - The numeric string to validate
+ * @param expectedDecimals - The maximum number of decimal places allowed
+ * @returns true if the amount has valid decimals, false otherwise
+ * @throws Error if inputs are invalid
+ */
export const validateDecimals = (
amount: string,
expectedDecimals: number
): boolean => {
+ if (!amount || typeof amount !== 'string') {
+ throw new Error('Amount must be a non-empty string')
+ }
+ if (!Number.isInteger(expectedDecimals) || expectedDecimals < 0) {
+ throw new Error('Expected decimals must be a non-negative integer')
+ }
+
+ // Convert scientific notation and validate numeric string
+ const numericAmount = Number(amount)
+ if (Number.isNaN(numericAmount)) {
+ throw new Error('Amount must be a valid number')
+ }
+
+ const normalizedAmount = numericAmount.toString()
const parts = amount.split('.')
- if (parts.length === 2) {
+ if (parts.length > 2) {
+ throw new Error('Amount cannot have multiple decimal points')
+ } else if (parts.length === 2) {
const decimals = parts[1].length
return decimals <= expectedDecimals
}
return true
}Committable suggestion skipped: line range outside the PR's diff.
| @@ -2,6 +2,9 @@ import { getAllQuotes } from './api' | |||
|
|
|||
| global.fetch = require('node-fetch') | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Configure fetch mock for better test isolation.
The current fetch mock setup might need more configuration to handle network errors and timeouts properly.
-global.fetch = require('node-fetch')
+const nodeFetch = require('node-fetch');
+
+global.fetch = async (...args) => {
+ try {
+ const response = await nodeFetch(...args);
+ if (!response.ok) {
+ throw new Error(`HTTP error! status: ${response.status}`);
+ }
+ return response;
+ } catch (error) {
+ console.error('Fetch failed:', error);
+ throw error;
+ }
+};📝 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.
| global.fetch = require('node-fetch') | |
| const nodeFetch = require('node-fetch'); | |
| global.fetch = async (...args) => { | |
| try { | |
| const response = await nodeFetch(...args); | |
| if (!response.ok) { | |
| throw new Error(`HTTP error! status: ${response.status}`); | |
| } | |
| return response; | |
| } catch (error) { | |
| console.error('Fetch failed:', error); | |
| throw error; | |
| } | |
| }; |
| func BytesToArray(bz []byte) ([32]byte, error) { | ||
| var bytes [32]byte | ||
| if len(bz) != 32 { | ||
| return bytes, fmt.Errorf("invalid length of bytes: %d", len(bz)) | ||
| } | ||
| copy(bytes[:], bz) | ||
| return bytes, nil | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Add unit tests for BytesToArray function
The BytesToArray function currently lacks test coverage. Please add test cases to verify:
- Success case with valid 32-byte input
- Error case with input length ≠ 32 bytes
Example test cases should include:
- Valid 32-byte array conversion
- Input shorter than 32 bytes
- Input longer than 32 bytes
🔗 Analysis chain
Add unit tests to verify error handling.
The new function includes error handling that should be verified with tests, especially the case where input length ≠ 32 bytes.
Let's check if tests exist:
Would you like me to help generate comprehensive test cases for this function?
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for existing tests of BytesToArray function
# Expected: Find test file and verify coverage of error case
# Look for test file
echo "Searching for test file..."
fd "bytes_test.go" core/
# Check for error case testing
echo "Checking for error case testing..."
rg -A 5 "TestBytesToArray.*func" core/
Length of output: 226
Script:
#!/bin/bash
# Let's check the content of the test file to verify test coverage
cat core/bytes_test.go
Length of output: 2405
| check('amount') | ||
| .isNumeric() | ||
| .exists() | ||
| .withMessage('amount is required') | ||
| .custom((value, { req }) => { | ||
| const fromTokenInfo = tokenAddressToToken( | ||
| req.query.chain, | ||
| req.query.fromToken | ||
| ) | ||
| return validateDecimals(value, fromTokenInfo.decimals) | ||
| }) | ||
| .withMessage( | ||
| 'Amount has too many decimals, beyond the maximum allowed for this token' | ||
| ), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance amount validation with additional checks
While the decimal validation is a good addition, consider adding these essential checks:
- Validate that the amount is positive
- Add error handling for token info retrieval
- Add null/undefined checks for token information
Consider updating the validation like this:
check('amount')
.isNumeric()
.exists()
.withMessage('amount is required')
+ .custom((value) => parseFloat(value) > 0)
+ .withMessage('Amount must be greater than 0')
.custom((value, { req }) => {
const fromTokenInfo = tokenAddressToToken(
req.query.chain,
req.query.fromToken
)
+ if (!fromTokenInfo || typeof fromTokenInfo.decimals !== 'number') {
+ throw new Error('Unable to retrieve token information')
+ }
return validateDecimals(value, fromTokenInfo.decimals)
})
.withMessage(
'Amount has too many decimals, beyond the maximum allowed for this token'
),📝 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.
| check('amount') | |
| .isNumeric() | |
| .exists() | |
| .withMessage('amount is required') | |
| .custom((value, { req }) => { | |
| const fromTokenInfo = tokenAddressToToken( | |
| req.query.chain, | |
| req.query.fromToken | |
| ) | |
| return validateDecimals(value, fromTokenInfo.decimals) | |
| }) | |
| .withMessage( | |
| 'Amount has too many decimals, beyond the maximum allowed for this token' | |
| ), | |
| check('amount') | |
| .isNumeric() | |
| .exists() | |
| .withMessage('amount is required') | |
| .custom((value) => parseFloat(value) > 0) | |
| .withMessage('Amount must be greater than 0') | |
| .custom((value, { req }) => { | |
| const fromTokenInfo = tokenAddressToToken( | |
| req.query.chain, | |
| req.query.fromToken | |
| ) | |
| if (!fromTokenInfo || typeof fromTokenInfo.decimals !== 'number') { | |
| throw new Error('Unable to retrieve token information') | |
| } | |
| return validateDecimals(value, fromTokenInfo.decimals) | |
| }) | |
| .withMessage( | |
| 'Amount has too many decimals, beyond the maximum allowed for this token' | |
| ), |
| check('amount') | ||
| .exists() | ||
| .withMessage('amount is required') | ||
| .isNumeric() | ||
| .custom((value, { req }) => { | ||
| const fromTokenInfo = tokenAddressToToken( | ||
| req.query.fromChain, | ||
| req.query.fromToken | ||
| ) | ||
| return validateDecimals(value, fromTokenInfo.decimals) | ||
| }) | ||
| .withMessage( | ||
| 'Amount has too many decimals, beyond the maximum allowed for this token' | ||
| ), |
There was a problem hiding this comment.
Add error handling and type safety to amount validation
While the decimal validation is a good addition, there are several improvements needed:
- The
tokenAddressToTokencall lacks error handling if the token info cannot be retrieved - Query parameters are used without type assertions
- The validation chain could benefit from additional checks for negative values and zero
Consider applying these improvements:
check('amount')
.exists()
.withMessage('amount is required')
.isNumeric()
+ .isFloat({ min: 0 })
+ .withMessage('Amount must be greater than 0')
.custom((value, { req }) => {
+ const fromChain = req.query.fromChain as string
+ const fromToken = req.query.fromToken as string
+
+ try {
const fromTokenInfo = tokenAddressToToken(
- req.query.fromChain,
- req.query.fromToken
+ fromChain,
+ fromToken
)
return validateDecimals(value, fromTokenInfo.decimals)
+ } catch (error) {
+ throw new Error('Failed to validate token decimals. Please try again.')
+ }
})
.withMessage(
'Amount has too many decimals, beyond the maximum allowed for this token'
),📝 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.
| check('amount') | |
| .exists() | |
| .withMessage('amount is required') | |
| .isNumeric() | |
| .custom((value, { req }) => { | |
| const fromTokenInfo = tokenAddressToToken( | |
| req.query.fromChain, | |
| req.query.fromToken | |
| ) | |
| return validateDecimals(value, fromTokenInfo.decimals) | |
| }) | |
| .withMessage( | |
| 'Amount has too many decimals, beyond the maximum allowed for this token' | |
| ), | |
| check('amount') | |
| .exists() | |
| .withMessage('amount is required') | |
| .isNumeric() | |
| .isFloat({ min: 0 }) | |
| .withMessage('Amount must be greater than 0') | |
| .custom((value, { req }) => { | |
| const fromChain = req.query.fromChain as string | |
| const fromToken = req.query.fromToken as string | |
| try { | |
| const fromTokenInfo = tokenAddressToToken( | |
| fromChain, | |
| fromToken | |
| ) | |
| return validateDecimals(value, fromTokenInfo.decimals) | |
| } catch (error) { | |
| throw new Error('Failed to validate token decimals. Please try again.') | |
| } | |
| }) | |
| .withMessage( | |
| 'Amount has too many decimals, beyond the maximum allowed for this token' | |
| ), |
| check('amount') | ||
| .exists() | ||
| .withMessage('amount is required') | ||
| .isNumeric() | ||
| .custom((value, { req }) => { | ||
| const fromTokenInfo = tokenAddressToToken( | ||
| req.query.fromChain, | ||
| req.query.fromToken | ||
| ) | ||
| return validateDecimals(value, fromTokenInfo.decimals) | ||
| }) | ||
| .withMessage( | ||
| 'Amount has too many decimals, beyond the maximum allowed for this token' | ||
| ), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling and performance of amount validation
While the decimal validation is a good addition, there are a few areas for improvement:
- Error Handling: Add explicit error handling for
tokenAddressToTokenfailures - Performance: Consider caching token info to avoid repeated lookups
- Input Validation: Consider adding range checks for amount
Consider this improved implementation:
check('amount')
.exists()
.withMessage('amount is required')
.isNumeric()
+ .isFloat({ min: 0 })
+ .withMessage('Amount must be greater than 0')
.custom((value, { req }) => {
+ try {
const fromTokenInfo = tokenAddressToToken(
req.query.fromChain,
req.query.fromToken
)
+ if (!fromTokenInfo) {
+ throw new Error('Token information not found')
+ }
return validateDecimals(value, fromTokenInfo.decimals)
+ } catch (error) {
+ throw new Error(`Invalid amount: ${error.message}`)
+ }
})
.withMessage(
'Amount has too many decimals, beyond the maximum allowed for this token'
),📝 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.
| check('amount') | |
| .exists() | |
| .withMessage('amount is required') | |
| .isNumeric() | |
| .custom((value, { req }) => { | |
| const fromTokenInfo = tokenAddressToToken( | |
| req.query.fromChain, | |
| req.query.fromToken | |
| ) | |
| return validateDecimals(value, fromTokenInfo.decimals) | |
| }) | |
| .withMessage( | |
| 'Amount has too many decimals, beyond the maximum allowed for this token' | |
| ), | |
| check('amount') | |
| .exists() | |
| .withMessage('amount is required') | |
| .isNumeric() | |
| .isFloat({ min: 0 }) | |
| .withMessage('Amount must be greater than 0') | |
| .custom((value, { req }) => { | |
| try { | |
| const fromTokenInfo = tokenAddressToToken( | |
| req.query.fromChain, | |
| req.query.fromToken | |
| ) | |
| if (!fromTokenInfo) { | |
| throw new Error('Token information not found') | |
| } | |
| return validateDecimals(value, fromTokenInfo.decimals) | |
| } catch (error) { | |
| throw new Error(`Invalid amount: ${error.message}`) | |
| } | |
| }) | |
| .withMessage( | |
| 'Amount has too many decimals, beyond the maximum allowed for this token' | |
| ), |
| @@ -1,5 +1,5 @@ | |||
| // SPDX-License-Identifier: MIT | |||
| pragma solidity ^0.8.20; | |||
| pragma solidity ^0.8.4; | |||
There was a problem hiding this comment.
💡 Codebase verification
Pragma version downgrade introduces compatibility issues with dependent contract
The pragma version downgrade to ^0.8.4 in BridgeTransactionV2.sol creates a version mismatch with its main dependent contract FastBridgeV2.sol (which uses 0.8.24). This could lead to potential compilation issues since the library is used directly in FastBridgeV2 with the statement using BridgeTransactionV2Lib for bytes.
FastBridgeV2.sol(0.8.24) imports and uses BridgeTransactionV2 library (^0.8.4)- The only other dependency is IFastBridgeV2.sol which is already compatible with ^0.8.4
Consider either:
- Upgrading BridgeTransactionV2.sol to match FastBridgeV2.sol's version (0.8.24)
- Or downgrading FastBridgeV2.sol to maintain version alignment
🔗 Analysis chain
Verify compiler version compatibility across dependencies
While downgrading from ^0.8.20 to ^0.8.4 aligns with other contracts, ensure all dependencies and inherited contracts are compatible with this version range.
Consider documenting the rationale for this version alignment in the repository's documentation to help future maintainers understand the decision.
The previous results show varying pragma versions across the codebase, with some contracts using newer versions (0.8.24, 0.8.20) and others using older versions (0.8.4, 0.8.17). Let's verify the dependencies and usage of BridgeTransactionV2 to ensure version compatibility.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check pragma versions across related contracts
echo "Checking pragma versions in related contracts..."
rg -A 1 "pragma solidity" "packages/contracts-rfq/contracts/"
# Check if any newer Solidity features are used
echo "Checking for newer Solidity features usage..."
rg -i "push\s*\[\s*\]|assembly\s*\{\s*basefee|block\.prevrandao|PUSH0" "packages/contracts-rfq/contracts/"
Length of output: 4493
Script:
#!/bin/bash
# Check for imports and dependencies of BridgeTransactionV2.sol
echo "Checking imports in BridgeTransactionV2.sol..."
rg "^import.*;" "packages/contracts-rfq/contracts/libs/BridgeTransactionV2.sol"
# Check which contracts import BridgeTransactionV2
echo "Checking usage of BridgeTransactionV2..."
rg "import.*BridgeTransactionV2" "packages/contracts-rfq/contracts/"
# Check if there are any inheritance or library usage
echo "Checking for library usage of BridgeTransactionV2..."
rg "using BridgeTransactionV2" "packages/contracts-rfq/contracts/"
Length of output: 802
| //nolint: gosec | ||
| fastBridgeContractOrigin, err := b.makeFastBridge(ctx.Context(), uint32(rawRequest.Bridge.OriginChainID)) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid suppressing gosec linter warnings without justification.
Using //nolint: gosec suppresses potential security issues flagged by the linter. It's advisable to address the underlying security concerns or, if suppression is necessary, provide a justification comment explaining why.
| txBz, err := core.BytesToArray(common.Hex2Bytes(rawRequest.Bridge.TransactionID[2:])) | ||
| if err != nil { | ||
| _, err := ctx.Response().Reply("error converting tx id") | ||
| if err != nil { | ||
| log.Println(err) | ||
| } | ||
| return | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use common.FromHex to safely decode hexadecimal strings and avoid manual slicing.
Instead of manually removing the "0x" prefix and using common.Hex2Bytes, it's safer and more idiomatic to use common.FromHex, which handles the prefix automatically and returns an error for invalid input. This prevents potential runtime panics due to incorrect slicing.
Apply this diff to update the code:
- txBz, err := core.BytesToArray(common.Hex2Bytes(rawRequest.Bridge.TransactionID[2:]))
+ txBytes, err := common.FromHex(rawRequest.Bridge.TransactionID)
+ if err != nil {
+ _, err := ctx.Response().Reply("error converting tx id")
+ if err != nil {
+ log.Println(err)
+ }
+ return
+ }
+ txBz, err := core.BytesToArray(txBytes)
if err != nil {
_, err := ctx.Response().Reply("error converting tx bytes")
if err != nil {
log.Println(err)
}
return
}📝 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.
| txBz, err := core.BytesToArray(common.Hex2Bytes(rawRequest.Bridge.TransactionID[2:])) | |
| if err != nil { | |
| _, err := ctx.Response().Reply("error converting tx id") | |
| if err != nil { | |
| log.Println(err) | |
| } | |
| return | |
| } | |
| txBytes, err := common.FromHex(rawRequest.Bridge.TransactionID) | |
| if err != nil { | |
| _, err := ctx.Response().Reply("error converting tx id") | |
| if err != nil { | |
| log.Println(err) | |
| } | |
| return | |
| } | |
| txBz, err := core.BytesToArray(txBytes) | |
| if err != nil { | |
| _, err := ctx.Response().Reply("error converting tx bytes") | |
| if err != nil { | |
| log.Println(err) | |
| } | |
| return | |
| } |
| tx, err = fastBridgeContractOrigin.Refund(transactor, common.Hex2Bytes(rawRequest.Bridge.Request[2:])) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error submitting refund: %w", err) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use common.FromHex for safer hexadecimal decoding of the bridge request.
Consistent with earlier, utilizing common.FromHex enhances safety by automatically handling the "0x" prefix and error checking, reducing the risk of runtime issues.
Apply this diff to update the code:
- tx, err = fastBridgeContractOrigin.Refund(transactor, common.Hex2Bytes(rawRequest.Bridge.Request[2:]))
+ requestBytes, err := common.FromHex(rawRequest.Bridge.Request)
+ if err != nil {
+ return nil, fmt.Errorf("error decoding bridge request: %w", err)
+ }
+ tx, err = fastBridgeContractOrigin.Refund(transactor, requestBytes)
if err != nil {
return nil, fmt.Errorf("error submitting refund: %w", err)
}
return tx, nil📝 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.
| tx, err = fastBridgeContractOrigin.Refund(transactor, common.Hex2Bytes(rawRequest.Bridge.Request[2:])) | |
| if err != nil { | |
| return nil, fmt.Errorf("error submitting refund: %w", err) | |
| requestBytes, err := common.FromHex(rawRequest.Bridge.Request) | |
| if err != nil { | |
| return nil, fmt.Errorf("error decoding bridge request: %w", err) | |
| } | |
| tx, err = fastBridgeContractOrigin.Refund(transactor, requestBytes) | |
| if err != nil { | |
| return nil, fmt.Errorf("error submitting refund: %w", err) | |
| } | |
| return tx, nil |
null
822975d: synapse-interface preview link
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Chores