feat(rfq-relayer): support FastBridgeV2 with arbitrary calls [SLT-320] #3258
Conversation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (7)
services/rfq/contracts/fastbridge/status.go (2)
Line range hint
28-36: Implement the missing assertion and review loop condition
The comment mentions an assertion, but there's no actual assertion in the code. Consider adding the assertion to ensure type correctness.
The loop condition
i < len(_BridgeStatus_index)-1might miss the last status. Consider changing it toi < len(_BridgeStatus_index)to include all statuses.The
nolintdirective suppresses linter warnings. While this can be useful, it's generally better to address the underlying issues. Consider removing the directive and addressing any linter warnings directly.Here's a suggested implementation addressing these points:
-//nolint:gosec,intrange func init() { - for i := 0; i < len(_BridgeStatus_index)-1; i++ { + for i := 0; i < len(_BridgeStatus_index); i++ { contractType := BridgeStatus(i) allBridgeStatuses = append(allBridgeStatuses, contractType) - // assert type is correct + // Assert type is correct + if contractType.String() == _BridgeStatus_name[_BridgeStatus_index[i]:_BridgeStatus_index[i+1]] { + panic(fmt.Sprintf("Unexpected BridgeStatus: %v", contractType)) + } } }This implementation adds the missing assertion, corrects the loop condition, and removes the
nolintdirective. Please review and adjust as needed.
Line range hint
38-40: Update comment to match implementation or add missing functionalityThe comment for
allBridgeStatusesmentions a panic condition that's not implemented in the code:// In order to make sure stringer is updated, we panic on // any method called where the index is higher than the stringer array length.Either update the comment to accurately reflect the current implementation or add the described panic condition to ensure consistency between the documentation and the code.
If you decide to implement the panic condition, consider adding a check in the
init()function or in methods that useallBridgeStatuses. For example:func init() { // ... existing code ... // Add panic condition if len(allBridgeStatuses) > len(_BridgeStatus_index) { panic("allBridgeStatuses length exceeds _BridgeStatus_index length") } }Please review and adjust as needed.
services/rfq/testutil/contracttype.go (3)
9-9: LGTM! Consider grouping related imports.The new import statements are correctly added to support the new contract types. However, for better readability, consider grouping related imports together.
You could group the imports like this:
import ( // ... other imports ... "github.com/synapsecns/sanguine/services/rfq/contracts/testcontracts/dai" "github.com/synapsecns/sanguine/services/rfq/contracts/testcontracts/fastbridgemockv2" "github.com/synapsecns/sanguine/services/rfq/contracts/testcontracts/mockerc20" "github.com/synapsecns/sanguine/services/rfq/contracts/testcontracts/recipientmock" "github.com/synapsecns/sanguine/services/rfq/contracts/testcontracts/usdc" "github.com/synapsecns/sanguine/services/rfq/contracts/testcontracts/weth9" )Also applies to: 11-11
56-57: LGTM! Consider creating an issue for the TODO.The TODO comment is a good reminder for future work. To ensure it's not forgotten, consider creating a GitHub issue to track this task.
Would you like me to create a GitHub issue for renaming the contract to MockFastBridge?
100-102: LGTM! Consider standardizing contract file paths.The new cases for
FastBridgeMockTypeandRecipientMockTypeare correctly implemented. However, for consistency, consider standardizing the contract file paths across all cases.Some cases use "/solidity/..." while others use "solidity/...". Standardize this across all cases for better consistency. For example:
case FastBridgeMockType: return fastbridgemockv2.Contracts["solidity/FastBridgeMock.sol:FastBridgeMock"] case RecipientMockType: return recipientmock.Contracts["solidity/RecipientMock.sol:RecipientMock"] // Update other cases to use the same formatservices/rfq/e2e/rfq_test.go (1)
397-512: Good addition of TestArbitraryCall, consider enhancing assertionsThe new TestArbitraryCall function effectively tests the arbitrary call functionality in FastBridgeV2. It follows a similar structure to other test functions, which is good for consistency. However, consider the following improvements:
- Add more specific assertions to verify the outcome of the arbitrary call. Currently, it only checks if the destination USDC balance is greater than zero.
- Verify that the CallParams ("Hello, world!") were correctly processed on the destination chain.
- Consider adding a check for the CallValue (1_337_420) to ensure it was correctly handled.
To improve the test coverage, you could add assertions like this after line 510:
i.Greater(destUSDCBalance.Cmp(big.NewInt(0)), 0) // Add more specific assertions here, e.g.: // Check if CallParams were processed correctly callResult, err := destRecipient.GetLastCallParams(&bind.CallOpts{Context: i.GetTestContext()}) i.NoError(err) i.Equal([]byte("Hello, world!"), callResult) // Check if CallValue was handled correctly callValue, err := destRecipient.GetLastCallValue(&bind.CallOpts{Context: i.GetTestContext()}) i.NoError(err) i.Equal(big.NewInt(1_337_420), callValue)Note: This assumes that the RecipientMock contract has methods to retrieve the last call parameters and value. If not, you may need to add these methods to the mock contract for testing purposes.
services/rfq/relayer/reldb/base/model.go (1)
126-127: Explain the reason for suppressing thegoseclinter warningIt's good practice to add a comment explaining why the
//nolint:gosecdirective is used here, to provide context for future maintainers.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (12)
- services/rfq/contracts/fastbridge/status.go (1 hunks)
- services/rfq/contracts/fastbridgev2/generate.go (1 hunks)
- services/rfq/contracts/fastbridgev2/parser.go (1 hunks)
- services/rfq/contracts/fastbridgev2/status.go (1 hunks)
- services/rfq/contracts/testcontracts/fastbridgemockv2/generate.go (1 hunks)
- services/rfq/contracts/testcontracts/recipientmock/generate.go (1 hunks)
- services/rfq/e2e/rfq_test.go (7 hunks)
- services/rfq/relayer/reldb/base/model.go (5 hunks)
- services/rfq/relayer/service/handlers.go (11 hunks)
- services/rfq/relayer/service/suite_test.go (5 hunks)
- services/rfq/testutil/contracttype.go (3 hunks)
- services/rfq/testutil/typecast.go (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- services/rfq/contracts/fastbridgev2/generate.go
- services/rfq/contracts/fastbridgev2/parser.go
- services/rfq/contracts/fastbridgev2/status.go
- services/rfq/contracts/testcontracts/fastbridgemockv2/generate.go
- services/rfq/contracts/testcontracts/recipientmock/generate.go
- services/rfq/relayer/service/suite_test.go
- services/rfq/testutil/typecast.go
🧰 Additional context used
🔇 Additional comments (15)
services/rfq/testutil/contracttype.go (2)
59-60: LGTM! New constant added correctly.The
RecipientMockTypeconstant is correctly added and follows the existing pattern for contract type declarations.
Line range hint
1-114: Overall, the changes look good and achieve the PR objectives.The modifications to this file successfully introduce support for the new
RecipientMockTypeand update theFastBridgeMockType. These changes align with the PR's objective of supporting FastBridgeV2 with arbitrary calls. The new contract types are consistently integrated into the existing structure, maintaining the file's overall organization and functionality.A few minor suggestions have been made for improved consistency and maintainability:
- Grouping related imports
- Creating a GitHub issue for the TODO comment
- Standardizing contract file paths in the
ContractInfomethodThese small improvements would enhance the code's readability and maintainability without affecting its functionality.
services/rfq/e2e/rfq_test.go (5)
Line range hint
325-335: Approve consistent changes in TestETHtoETH for FastBridgeV2 supportThe updates to the TestETHtoETH function correctly implement the new FastBridgeV2 interface, consistent with the changes made in TestUSDCtoUSDC. The switch to fastbridgev2.IFastBridgeBridgeParams and the addition of the Sender field are appropriate for the new contract version.
These changes maintain consistency across test functions, which is good for code maintainability and readability.
Line range hint
554-564: Approve consistent changes in TestDispute for FastBridgeV2 supportThe updates to the TestDispute function correctly implement the new FastBridgeV2 interface, maintaining consistency with the changes made in other test functions. The switch to fastbridgev2.IFastBridgeBridgeParams and the addition of the Sender field are appropriate for the new contract version.
The consistency in these changes across different test functions is commendable, as it helps maintain the overall structure and readability of the test suite.
Line range hint
684-694: Approve consistent changes in TestConcurrentBridges for FastBridgeV2 supportThe updates to the TestConcurrentBridges function, specifically within the sendBridgeReq function, correctly implement the new FastBridgeV2 interface. The switch to fastbridgev2.IFastBridgeBridgeParams and the addition of the Sender field are consistent with changes made in other test functions and appropriate for the new contract version.
The consistency in these changes across different test functions, including this concurrent test, is excellent. It ensures that all aspects of the FastBridgeV2 functionality are being tested under various conditions, including concurrent operations.
Line range hint
1-784: Comprehensive update for FastBridgeV2 support with good test coverageThe changes in this file demonstrate a thorough and consistent update to support FastBridgeV2 across all test functions. Key observations:
- All Bridge calls have been updated to use fastbridgev2.IFastBridgeBridgeParams.
- The Sender field has been consistently added to all Bridge calls.
- The new TestArbitraryCall function adds coverage for the new arbitrary call functionality.
- Existing test structure and coverage have been maintained while incorporating the new features.
These changes ensure that the test suite comprehensively covers the new FastBridgeV2 functionality while maintaining the integrity of existing tests. The consistency in implementation across different test scenarios (USDC, ETH, disputes, concurrent bridges) is commendable.
To further enhance the test suite, consider:
- Adding more specific assertions in the TestArbitraryCall function, as suggested earlier.
- Reviewing if any additional edge cases specific to FastBridgeV2 need to be tested.
- Ensuring that any documentation or comments related to these tests are updated to reflect the new FastBridgeV2 functionality.
Line range hint
170-180: Approve changes to TestUSDCtoUSDC for FastBridgeV2 supportThe updates to the TestUSDCtoUSDC function correctly implement the new FastBridgeV2 interface. The change from fastbridge.IFastBridgeBridgeParams to fastbridgev2.IFastBridgeBridgeParams and the addition of the Sender field are appropriate for the new contract version.
To ensure these changes are consistent across the codebase, run the following script:
This script will help identify any places where the old interface might still be in use or where the Sender field might be missing.
✅ Verification successful
All Bridge calls correctly utilize
fastbridgev2.IFastBridgeBridgeParamswith theSenderfieldThe verification confirms that all Bridge function calls have been updated to use
fastbridgev2.IFastBridgeBridgeParamsand include theSenderfield. No instances of the oldfastbridge.IFastBridgeBridgeParamsremain in the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that all Bridge calls use fastbridgev2.IFastBridgeBridgeParams and include the Sender field # Search for Bridge function calls rg --type go 'Bridge\s*\(' -A 10 | grep -E '(fastbridgev2\.IFastBridgeBridgeParams|Sender\s*:)' # Search for any remaining uses of the old fastbridge.IFastBridgeBridgeParams rg --type go 'fastbridge\.IFastBridgeBridgeParams'Length of output: 1060
services/rfq/relayer/reldb/base/model.go (4)
139-140: Potential nil pointer dereference when accessingTransactionfieldsEnsure that
request.Transactionand its fieldsExclusivityRelayerandExclusivityEndTimeare not nil before accessing them to prevent runtime nil pointer dereference errors.
148-149: Correct decimal scaling when converting amountsThe use of
decimal.NewFromBigIntwith a positive exponent may lead to incorrect scaling of amounts. According to thedecimalpackage, the exponent should be negative for decimal places. This ensures accurate representation of token amounts based on their decimals.
148-153: Check for nil pointers when accessingTransactionandTransactionV1When accessing
request.Transactionandrequest.TransactionV1, ensure they are not nil to prevent nil pointer dereference errors.
237-240: Ensure correct conversion of amounts tobig.IntDividing the
decimal.Decimalamounts by10^{tokenDecimals}may lead to precision loss, especially if the amounts have fractional parts. Instead, consider shifting the decimal point to accurately convert the amounts back tobig.Int.services/rfq/relayer/service/handlers.go (4)
15-17: Imports updated correctlyThe imports are updated to include the necessary packages for
fastbridgev2and other required functionalities.
395-395: Updated function to handleFastBridgeV2BridgeRelayedeventsThe function signature has been updated to handle
fastbridgev2.FastBridgeV2BridgeRelayedevents, ensuring compatibility with the new bridge version.
497-499: Parser creation for event processing is correctThe parser is correctly instantiated using
fastbridgev2.NewParserto handle event parsing for the new contract.
524-524: Updated function to handleFastBridgeV2BridgeProofProvidedeventsThe function
handleProofProvidedhas been updated to handlefastbridgev2.FastBridgeV2BridgeProofProvidedevents, aligning with the new contract definitions.
| // | ||
| //nolint:cyclop | ||
| func (r *Relayer) handleBridgeRequestedLog(parentCtx context.Context, req *fastbridgev2.FastBridgeV2BridgeRequested, chainID uint64) (err error) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring to reduce cyclomatic complexity
The //nolint:cyclop directive disables the cyclomatic complexity check. To improve maintainability and readability, consider refactoring handleBridgeRequestedLog into smaller, more focused functions.
| span.AddEvent("status_check", trace.WithAttributes(attribute.String("chain_bridge_status", fastbridgev2.BridgeStatus(bs).String()))) | ||
|
|
||
| // sanity check to make sure it's still requested. | ||
| if bs != fastbridge.REQUESTED.Int() { | ||
| if bs != fastbridgev2.REQUESTED.Int() { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Log unexpected bridge statuses for better debugging
In handleCommitPending, when the bridge status bs is not REQUESTED, the function returns nil without logging. Consider adding a log statement to capture unexpected statuses, which will aid in debugging and monitoring.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
services/rfq/testutil/deployers.go (1)
31-31: LGTM: Added RecipientMockDeployer to NewDeployManagerThe
NewRecipientMockDeployerhas been correctly added to theNewDeployManagerfunction, which is consistent with the new import and supports the PR objective.Consider breaking this long line into multiple lines for better readability, especially if more deployers are added in the future. For example:
parentManager := manager.NewDeployerManager(t, NewFastBridgeDeployer, NewMockERC20Deployer, NewMockFastBridgeDeployer, NewRecipientMockDeployer, NewWETH9Deployer, NewUSDTDeployer, NewUSDCDeployer, NewDAIDeployer, )
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- services/rfq/testutil/deployers.go (4 hunks)
🧰 Additional context used
🔇 Additional comments (4)
services/rfq/testutil/deployers.go (4)
17-19: LGTM: Updated imports for new contract versionsThe import statements have been correctly updated to use the new versions of the contracts (
fastbridgev2,fastbridgemockv2) and added a new import forrecipientmock. These changes align with the PR objective of supporting FastBridgeV2 with arbitrary calls.
117-119: LGTM: Updated FastBridgeDeployer to use FastBridgeV2The
Deploymethod ofFastBridgeDeployerhas been correctly updated to usefastbridgev2.DeployFastBridgeV2andfastbridgev2.NewFastBridgeV2Ref. These changes are consistent with the PR objective of supporting FastBridgeV2.
138-141: LGTM: Updated MockFastBridgeDeployer to use FastBridgeMockV2The
Deploymethod ofMockFastBridgeDeployerhas been correctly updated to usefastbridgemockv2.DeployFastBridgeMockandfastbridgemockv2.NewFastBridgeMockRef. These changes are consistent with the updates made to the actual FastBridge contract and support the PR objective.
144-161: LGTM: Added RecipientMockDeployer implementationThe new
RecipientMockDeployerhas been implemented correctly, following the same pattern as other deployers in the file. This addition supports the PR objective of handling arbitrary calls, which may require recipient mocking for testing purposes.It's worth noting that the
NewRecipientMockDeployerfunction correctly usesRecipientMockType, addressing the issue mentioned in past review comments about using the wrong contract type.
packages/contracts-rfq/package.json
Outdated
| @@ -23,7 +23,7 @@ | |||
| "lint": "forge fmt && npm run solhint", | |||
| "lint:check": "forge fmt --check && npm run solhint:check", | |||
| "ci:lint": "npm run lint:check", | |||
| "build:go": "./flatten.sh contracts/*.sol test/*.sol", | |||
| "build:go": "./flatten.sh contracts/**/*.sol test/**/*.sol", | |||
There was a problem hiding this comment.
💡 Codebase verification
Build script patterns do not include any Solidity files
The updated build script patterns contracts/**/*.sol and test/**/*.sol do not match any Solidity files in packages/contracts-rfq/contracts/ or packages/contracts-rfq/test/. As a result, no Solidity files will be included in the build process.
- Verify and update the paths in the build script to correctly point to the directories containing Solidity files, such as
packages/contracts-core/contracts/**/*.solandservices/rfq/contracts/**/*.sol.
🔗 Analysis chain
LGTM! Verify the impact of the expanded file pattern.
The change to include all Solidity files in subdirectories of contracts and test is a good improvement. It ensures that no Solidity files are missed in the build process, which is especially helpful as the project grows and potentially adds more nested directories.
To ensure this change doesn't unintentionally include files that shouldn't be part of the build, please run the following script:
This will help you verify that only the intended files are being included in the build process.
Please run the following script to list all Solidity files in the repository. This will help identify the correct paths and ensure that the build process includes the intended files.
Please run the following script to list and count all Solidity files included by the updated build patterns in packages/contracts-rfq/package.json. This will help verify that the expanded patterns correctly include all intended files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: List all Solidity files that will now be included in the build
echo "Newly included Solidity files:"
fd -e sol -d 2 . contracts test | grep -v -E '^(contracts/|test/)[^/]+\.sol$'
echo "Total Solidity files:"
fd -e sol . contracts test | wc -l
Length of output: 538
Script:
#!/bin/bash
# Description: List all Solidity files in the repository
echo "All Solidity files in the repository:"
fd -e sol .
Length of output: 16116
Script:
#!/bin/bash
# Description: List and count Solidity files included by the build script patterns in packages/contracts-rfq/package.json
# Define the base directory relative to the repository root
BASE_DIR="packages/contracts-rfq"
echo "Solidity files matching 'contracts/**/*.sol' within ${BASE_DIR}:"
fd -e sol "$BASE_DIR/contracts" --type f
echo ""
echo "Solidity files matching 'test/**/*.sol' within ${BASE_DIR}:"
fd -e sol "$BASE_DIR/test" --type f
echo ""
echo "Total Solidity files included by the build script:"
fd -e sol "$BASE_DIR/contracts" "$BASE_DIR/test" --type f | wc -l
Length of output: 1967
This reverts commit 4b8dc68.
| @@ -77,7 +77,9 @@ func (c Chain) SubmitRelay(ctx context.Context, request reldb.QuoteRequest) (uin | |||
| // Check to see if ETH should be sent to destination | |||
| if util.IsGasToken(request.Transaction.DestToken) { | |||
| gasAmount = request.Transaction.DestAmount | |||
| } else if request.Transaction.SendChainGas { | |||
| } else if request.Transaction.CallValue != nil { | |||
| gasAmount = request.Transaction.CallValue | |||
There was a problem hiding this comment.
@dwasse for the transactions with non-zero CallValue we also need to update the internal pricing. They involve spending CallValue worth of ETH on destination chain, and CallValue here is a user-controlled value. Do we wanna do this in a separate PR? I believe active quoting will anyway require some adjustments both in terms of CallValue and CallParams.
There was a problem hiding this comment.
Yes let's do this in separate PR.
If I understand correctly, this should be an extra component accounted for in FeePricer according to the extra gas spent in the 'arbitrary call' on destination?
There was a problem hiding this comment.
That's correct! However these two params would require to be treated differently:
- For
CallValuewe know in advance how much native asset are we going to attach to the transaction astransactor.Value- this is a static extra cost to be paid by the relayer. - For
CallParamsthe execution cost itself becomes dynamic, as the recipient could in theory implement a complex on-receive logic. Therefore we would need to estimate the fees for posting a relay transaction using the RPC call (prove & claim estimates are mostly unaffected and could remain static).
| OriginAmount: new(big.Int).Div(r.OriginAmount.BigInt(), big.NewInt(int64(math.Pow10(int(r.OriginTokenDecimals))))), | ||
| DestAmount: new(big.Int).Div(r.DestAmount.BigInt(), big.NewInt(int64(math.Pow10(int(r.DestTokenDecimals))))), | ||
| OriginFeeAmount: new(big.Int).Div(r.OriginFeeAmount.BigInt(), big.NewInt(int64(math.Pow10(int(r.OriginTokenDecimals))))), | ||
| CallValue: new(big.Int).Div(r.CallValue.BigInt(), big.NewInt(int64(math.Pow10(int(r.OriginTokenDecimals))))), |
There was a problem hiding this comment.
CallValue is always expressed in destination chain's native asset, which on the EVM chains is always treated as 18 decimals (ETH, BNB, AVAX, etc).
| OriginFeeAmount: decimal.NewFromBigInt(request.Transaction.OriginFeeAmount, int32(request.OriginTokenDecimals)), | ||
| CallValue: decimal.NewFromBigInt(request.Transaction.CallValue, int32(request.OriginTokenDecimals)), |
There was a problem hiding this comment.
Same here, should not be using OriginTokenDecimals with CallValue
| return nil | ||
| }, | ||
| func(ctx context.Context) error { | ||
| txV2, err = fastBridge.GetBridgeTransactionV2(&bind.CallOpts{Context: ctx}, req.Request) |
There was a problem hiding this comment.
Don't think this affects the Relayer, but could be relevant for the Guard.
GetBridgeTransaction supports both V1 and V2 payloads, and decodes into the V1 struct accornigly.
GetBridgeTransactionV2 on the other hand only supports the V2 payloads, so feeding V1 paylaod will result in a revert.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
services/rfq/relayer/service/handlers.go (4)
35-37: LGTM: Updated function signature, but consider refactoringThe function signature has been correctly updated to use the new
FastBridgeV2BridgeRequestedtype. However, the addition of//nolint:cyclopsuggests high cyclomatic complexity.Consider refactoring this function into smaller, more focused functions to reduce complexity and improve maintainability.
134-165: LGTM: New getBridgeTxs function, but enhance error messagesThe new
getBridgeTxsfunction effectively retrieves both V1 and V2 transactions with proper error handling and retry logic. This approach supports the transition to V2 while maintaining backward compatibility.Consider enhancing the error messages to be more specific about which call failed. For example:
- return txV1, txV2, fmt.Errorf("could not make call: %w", err) + return txV1, txV2, fmt.Errorf("could not get bridge transaction V1: %w", err)And similarly for the V2 call.
352-352: LGTM: Updated bridge status check, consider logging unexpected statusesThe code correctly checks against
fastbridgev2.REQUESTED.Int(), which is necessary for compatibility with the new V2 contract.Consider adding logging for unexpected bridge statuses to aid in debugging. For example:
if bs != fastbridgev2.REQUESTED.Int() { logger.Warnf("Unexpected bridge status: %s", fastbridgev2.BridgeStatus(bs).String()) return nil }
534-534: LGTM: Updated handleProofProvided and bridge status checks, consider handling additional casesThe
handleProofProvidedfunction signature and bridge status checks have been correctly updated to use the V2 types and constants.Consider handling additional bridge statuses or adding a default case in the switch statement to catch unexpected statuses. For example:
switch bs { case fastbridgev2.RelayerProved.Int(): // existing code case fastbridgev2.RelayerClaimed.Int(): // existing code default: logger.Warnf("Unexpected bridge status: %s", fastbridgev2.BridgeStatus(bs).String()) // Handle or log other statuses as needed }Also applies to: 575-577
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- packages/contracts-rfq/package.json (1 hunks)
- services/rfq/relayer/quoter/quoter_test.go (3 hunks)
- services/rfq/relayer/service/handlers.go (11 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/contracts-rfq/package.json
🧰 Additional context used
🔇 Additional comments (11)
services/rfq/relayer/quoter/quoter_test.go (4)
13-13: LGTM: Import statement updated to fastbridgev2The import statement has been correctly updated to use the new
fastbridgev2package. This change aligns with the PR objective of supporting FastBridgeV2.To ensure consistency, let's verify that all references to the old
fastbridgepackage have been updated:#!/bin/bash # Description: Check for any remaining references to the old fastbridge package # Test: Search for any remaining references to the old fastbridge package rg --type go 'fastbridge\.' services/rfq/relayer/quoter/quoter_test.go # Test: Confirm that fastbridgev2 is being used rg --type go 'fastbridgev2\.' services/rfq/relayer/quoter/quoter_test.go
Line range hint
156-164: LGTM: Consistent update of Transaction type to FastBridgeV2The Transaction field has been consistently updated to use the new
fastbridgev2.IFastBridgeV2BridgeTransactionV2type in the TestIsProfitable function. This change maintains consistency with the previous updates.To ensure consistent usage of the new transaction type throughout the test file, let's run the following verification:
#!/bin/bash # Description: Verify consistent usage of the new FastBridgeV2 transaction type # Test: Check for any remaining references to the old transaction type rg --type go 'fastbridge\.IFastBridgeBridgeTransaction' services/rfq/relayer/quoter/quoter_test.go # Test: Confirm that the new transaction type is used consistently rg --type go 'fastbridgev2\.IFastBridgeV2BridgeTransactionV2' services/rfq/relayer/quoter/quoter_test.go
Line range hint
1-424: Summary: Successful transition to FastBridgeV2 in quoter_test.goThe changes in this file successfully implement the transition from FastBridge to FastBridgeV2. The main updates include:
- Updated import statement to use the new
fastbridgev2package.- Consistent replacement of
fastbridge.IFastBridgeBridgeTransactionwithfastbridgev2.IFastBridgeV2BridgeTransactionV2in test functions.These changes align with the PR objective of supporting FastBridgeV2 with arbitrary calls. The test logic remains intact, suggesting that the new interface is backwards compatible.
To ensure a comprehensive transition, let's perform a final verification:
✅ Verification successful
FastBridgeV2 Transition Verified in quoter_test.go
The shell script outputs confirm that all references to the old
fastbridgepackage have been successfully replaced withfastbridgev2inservices/rfq/relayer/quoter/quoter_test.go. No lingering references to the old package were found, and the usage offastbridgev2is consistent throughout the test functions.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Comprehensive check for FastBridgeV2 transition # Test: Verify no references to old fastbridge package remain rg --type go 'fastbridge[^v2]' services/rfq/relayer/quoter/quoter_test.go # Test: Confirm consistent usage of new fastbridgev2 package rg --type go 'fastbridgev2' services/rfq/relayer/quoter/quoter_test.go # Test: Check if any test functions were missed in the update ast-grep --lang go --pattern $'func \(s \*QuoterSuite\) Test.+\{ $$$ fastbridge $$$ \}'Length of output: 780
Line range hint
118-126: LGTM: Updated Transaction type to FastBridgeV2The Transaction field has been correctly updated to use the new
fastbridgev2.IFastBridgeV2BridgeTransactionV2type. This change is consistent with the transition to FastBridgeV2.To ensure the test logic remains valid with the new transaction type, let's verify the structure and usage of the new transaction type:
services/rfq/relayer/service/handlers.go (7)
15-17: LGTM: Updated import for FastBridgeV2The import statement has been correctly updated to use the new
fastbridgev2package, which is necessary for supporting the new version of the FastBridge contract.
79-81: LGTM: Retrieving both V1 and V2 transactionsThe code now retrieves both V1 and V2 transactions using the new
getBridgeTxsfunction. This approach supports backward compatibility while allowing for the new V2 functionality.
349-349: LGTM: Updated bridge status loggingThe code now correctly uses
fastbridgev2.BridgeStatusfor logging, which is consistent with the transition to V2 and provides more informative logging of the bridge status.
405-405: LGTM: Updated handleRelayLog function signatureThe
handleRelayLogfunction signature has been correctly updated to accept*fastbridgev2.FastBridgeV2BridgeRelayed, which is necessary for compatibility with the new V2 contract.
Line range hint
507-521: LGTM: Updated event parsing for V2The code has been correctly updated to use
fastbridgev2.NewParserand check forFastBridgeV2BridgeRelayedevents. These changes ensure proper parsing of V2 events and maintain compatibility with the new contract version.
103-104: 🛠️ Refactor suggestionConsider removing TransactionV1 if not needed
Both
TransactionV1andTransaction(assumed to be V2) are being stored in the database request. If backward compatibility with V1 is no longer required, consider removing it to simplify the data model.If V1 is no longer needed, you can simplify the struct initialization:
dbReq := reldb.QuoteRequest{ // ... other fields ... - TransactionV1: txV1, Transaction: txV2, // ... other fields ... }Likely invalid or redundant comment.
85-85: LGTM: Using V2 transaction for decimals, but verify V1 compatibilityThe code now uses
txV2to retrieve decimals, which is consistent with the move to V2 transactions.Please verify that this change doesn't break compatibility with V1 transactions if they're still supported. Run the following script to check for any remaining V1 transaction handling:
| DestAmount: decimal.NewFromBigInt(request.Transaction.DestAmount, int32(nativeTokenDecimals)), | ||
| OriginFeeAmount: decimal.NewFromBigInt(request.Transaction.OriginFeeAmount, int32(request.OriginTokenDecimals)), | ||
| CallValue: decimal.NewFromBigInt(request.Transaction.CallValue, int32(request.OriginTokenDecimals)), |
trajan0x
changed the title
Summary by CodeRabbit
New Features
BridgeStatusenumeration for tracking on-chain request statuses.RequestForQuotestructure with additional fields for improved transaction handling.Bug Fixes
Documentation