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: 4
🔭 Outside diff range comments (1)
services/rfq/guard/service/guard.go (1)
Line range hint
319-366: Consider unifying chain indexer implementationsThe V1 and V2 indexer implementations share significant code. Consider creating a generic indexer implementation:
+ func (g Guard) runChainIndexer[T any]( + ctx context.Context, + chainID int, + chainListener listener.ContractListener, + newParser func(common.Address) (T, error), + handleEvent func(context.Context, T, int) error, + ) error { + parser, err := newParser(chainListener.Address()) + if err != nil { + return fmt.Errorf("could not parse: %w", err) + } + + return chainListener.Listen(ctx, func(parentCtx context.Context, log types.Log) error { + // ... common logging and event handling logic + }) + }
♻️ Duplicate comments (1)
services/rfq/guard/service/guard.go (1)
262-262:⚠️ Potential issueFix potential integer overflow in block number conversion
Converting
log.BlockNumberfromuint64toint64may cause integer overflow. Consider using string representation for block numbers in logs.- attribute.Int64("block_number", int64(log.BlockNumber)), + attribute.String("block_number", fmt.Sprintf("%d", log.BlockNumber)),
🧹 Nitpick comments (4)
services/rfq/e2e/rfq_test.go (1)
481-486: Consider parameterizing test valuesThe hardcoded values for
QuoteExclusivitySeconds,ZapData, andZapNativeshould be extracted into constants for better maintainability.services/rfq/e2e/setup_test.go (2)
230-245: Consider enhancing error handling in setupRecipientMock.The error handling could be improved by returning the error instead of using
i.NoError(err), allowing better error propagation and testing flexibility.-func (i *IntegrationSuite) setupRecipientMock() { +func (i *IntegrationSuite) setupRecipientMock() error { testBackends := core.ToSlice(i.originBackend, i.destBackend) for _, b := range testBackends { backend := b err := retry.WithBackoff(i.GetTestContext(), func(_ context.Context) (err error) { handle := i.manager.Get(i.GetTestContext(), backend, testutil.RecipientMockType) err = i.waitForContractDeployment(i.GetTestContext(), backend, handle.Address()) if err != nil { return fmt.Errorf("failed to wait for contract deployment: %w", err) } return nil }, retry.WithMaxTotalTime(30*time.Second)) - i.NoError(err) + if err != nil { + return fmt.Errorf("failed to setup recipient mock for backend %d: %w", backend.GetChainID(), err) + } } + return nil }
536-554: Consider consolidating role granting logic.The role granting logic for V1 and V2 bridges is duplicated. Consider extracting this into a helper function to improve maintainability.
+func (i *IntegrationSuite) grantGuardRole(backend backends.SimulatedTestBackend, metadata contracts.DeployedContract, contract interface { + GUARDROLE(*bind.CallOpts) ([32]byte, error) + GrantRole(*bind.TransactOpts, [32]byte, common.Address) (*types.Transaction, error) +}) error { + txContext := backend.GetTxContext(i.GetTestContext(), metadata.OwnerPtr()) + guardRole, err := contract.GUARDROLE(&bind.CallOpts{Context: i.GetTestContext()}) + if err != nil { + return fmt.Errorf("failed to get guard role: %w", err) + } + tx, err := contract.GrantRole(txContext.TransactOpts, guardRole, i.guardWallet.Address()) + if err != nil { + return fmt.Errorf("failed to grant guard role: %w", err) + } + backend.WaitForConfirmation(i.GetTestContext(), tx) + return nil +}services/rfq/guard/service/guard.go (1)
37-40: Consider grouping related fields using nested structsThe addition of V1 and V2 specific fields creates parallel structures. Consider organizing these fields into version-specific nested structs for better maintainability:
type Guard struct { cfg guardconfig.Config metrics metrics.Handler db guarddb.Service client omniClient.RPCClient - contractsV1 map[int]*fastbridge.FastBridgeRef - contractsV2 map[int]*fastbridgev2.FastBridgeV2Ref - listenersV1 map[int]listener.ContractListener - listenersV2 map[int]listener.ContractListener + v1 struct { + contracts map[int]*fastbridge.FastBridgeRef + listeners map[int]listener.ContractListener + } + v2 struct { + contracts map[int]*fastbridgev2.FastBridgeV2Ref + listeners map[int]listener.ContractListener + } txSubmitter submitter.TransactionSubmitter otelRecorder iOtelRecorder }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
services/rfq/contracts/fastbridgev2/parser.go(1 hunks)services/rfq/e2e/rfq_test.go(10 hunks)services/rfq/e2e/setup_test.go(9 hunks)services/rfq/guard/service/guard.go(9 hunks)services/rfq/relayer/quoter/quoter.go(10 hunks)
🔇 Additional comments (18)
services/rfq/e2e/rfq_test.go (5)
24-36: LGTM: Import changes are appropriate
The new imports support the FastBridgeV2 functionality and are correctly organized.
495-511: Fix error handling in goroutine
Error handling in goroutines using i.NoError(err) is unsafe and may cause race conditions.
172-179: LGTM: Consistent V2 interface migration
The test functions have been correctly updated to use FastBridgeV2 while maintaining the original test logic and assertions.
Also applies to: 325-334
617-710: Reduce code duplication between dispute tests
TestDisputeV2 shares significant setup code with TestDisputeV1. Consider extracting common setup into helper functions.
910-912: Fix range loop in zapData generation
The range loop uses zapDataSize directly which will cause a panic. It should iterate over indices instead.
services/rfq/relayer/quoter/quoter.go (6)
24-24: LGTM: Import for FastBridgeV2 support
The import is correctly added to support the new FastBridgeV2 functionality.
301-302: Verify the channel buffer size
The buffer size of 1000 might be excessive. Consider:
- Monitoring channel utilization in production to tune this value
- Adding a configuration option for this value
- Implementing backpressure mechanisms if the channel fills up
317-325: LGTM: Improved error handling and channel setup
The changes enhance reliability with:
- Proper error handling with metrics
- Correct span management
- Buffered channel to handle load spikes
436-483: Review default values and assumptions
Several potential issues in the implementation:
- Using max uint256 for deadline might be too permissive
- Empty exclusivity relayer address might cause contract validation issues
- Assuming dest amount equals origin amount might not account for token decimals or exchange rates
Run the following script to verify the impact:
#!/bin/bash
# Check for any validation requirements in the FastBridgeV2 contract
ast-grep --pattern $'contract FastBridgeV2 {
$$$
function validateBridgeTransaction($_, $_) {
$$$
}
$$$
}'667-667: LGTM: QuoteInput struct enhancement
The addition of QuoteRequest field properly supports the FastBridgeV2 functionality.
Line range hint 688-692: LGTM: Fee calculation enhancement
The fee calculation now properly integrates with FastBridgeV2 by utilizing the QuoteRequest parameter.
services/rfq/contracts/fastbridgev2/parser.go (5)
28-28: Fix typo in comment: 'contracat' should be 'contract'.
47-47: High cyclomatic complexity in ParseEvent method.
71-75: Rename variable 'requested' to 'relayed' in 'BridgeRelayedEvent' case for clarity.
89-93: Ensure consistent naming between event type and parsing method in 'BridgeDisputeEvent' case.
94-96: Handle unknown event types explicitly.
services/rfq/e2e/setup_test.go (2)
62-69: LGTM: Bridge configuration is properly structured.
The configuration correctly handles both FastBridgeV1 and FastBridgeV2 contracts with appropriate chain IDs and addresses.
272-309: Verify token approvals for both bridge versions.
The Approve method now handles both bridge versions correctly. However, let's verify that all token approvals are properly set.
✅ Verification successful
Let me verify the token approvals for both bridge versions with additional checks.
Based on the code analysis and verification results, I can now provide the final response:
Token approvals are correctly implemented for both bridge versions
The code properly handles token approvals for both FastBridgeV1 and FastBridgeV2:
- Checks existing allowance before approving
- Uses
abi.MaxUint256for unlimited approval amount - Implements retry mechanism with backoff for failed transactions
- Verifies allowance after approval
- Handles each bridge version independently
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify token approvals for both bridge versions
# Search for Approval events in the codebase to ensure consistent implementation
ast-grep --pattern 'function Approve($$$) {
$$$
erc20.$$$
$$$
}'
rg -A 5 'event Approval\('
Length of output: 65638
Script:
#!/bin/bash
# Search for token approval implementations and checks in the codebase
rg -A 5 'func.*Approve' services/rfq/
rg -A 5 'allowance.*=.*0' services/rfq/
Length of output: 93746
| metadataV1, rfqContractV1 := i.manager.GetFastBridge(i.GetTestContext(), backend) | ||
| txContextV1 := backend.GetTxContext(i.GetTestContext(), metadataV1.OwnerPtr()) | ||
|
|
||
| relayerRole, err := rfqContractV1.RELAYERROLE(&bind.CallOpts{Context: i.GetTestContext()}) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to get relayer role: %w", err) | ||
| } | ||
| proverRole, err := rfqContractV1.RELAYERROLE(&bind.CallOpts{Context: i.GetTestContext()}) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to get relayer role: %w", err) | ||
| } | ||
| tx, err := rfqContractV1.GrantRole(txContextV1.TransactOpts, relayerRole, i.relayerWallet.Address()) | ||
| if err != nil { | ||
| return fmt.Errorf("could not grant relayer role: %w", err) | ||
| } | ||
| backend.WaitForConfirmation(i.GetTestContext(), tx) | ||
|
|
||
| metadataV2, rfqContractV2 := i.manager.GetFastBridgeV2(i.GetTestContext(), backend) | ||
| txContextV2 := backend.GetTxContext(i.GetTestContext(), metadataV2.OwnerPtr()) | ||
|
|
||
| txContext := backend.GetTxContext(i.GetTestContext(), metadata.OwnerPtr()) | ||
| relayerRole, err := rfqContract.RELAYERROLE(&bind.CallOpts{Context: i.GetTestContext()}) | ||
| tx, err = rfqContractV2.GrantRole(txContextV2.TransactOpts, proverRole, i.relayerWallet.Address()) | ||
| if err != nil { | ||
| return fmt.Errorf("could not get relayer role: %w", err) | ||
| return fmt.Errorf("could not grant prover role: %w", err) | ||
| } | ||
| backend.WaitForConfirmation(i.GetTestContext(), tx) | ||
|
|
||
| tx, err := rfqContract.GrantRole(txContext.TransactOpts, relayerRole, i.relayerWallet.Address()) | ||
| quoterRole, err := rfqContractV2.QUOTERROLE(&bind.CallOpts{Context: i.GetTestContext()}) | ||
| if err != nil { | ||
| return fmt.Errorf("could not grant role: %w", err) | ||
| return fmt.Errorf("could not get quoter role: %w", err) | ||
| } | ||
| tx, err = rfqContractV2.GrantRole(txContextV2.TransactOpts, quoterRole, i.relayerWallet.Address()) | ||
| if err != nil { | ||
| return fmt.Errorf("could not grant quoter role: %w", err) | ||
| } | ||
| backend.WaitForConfirmation(i.GetTestContext(), tx) |
There was a problem hiding this comment.
Fix ineffectual assignment in setupRelayer.
There's an ineffectual assignment to proverRole that needs to be fixed.
- proverRole, err := rfqContractV1.RELAYERROLE(&bind.CallOpts{Context: i.GetTestContext()})
+ proverRole, err := rfqContractV1.PROVERROLE(&bind.CallOpts{Context: i.GetTestContext()})
if err != nil {
return fmt.Errorf("failed to get relayer role: %w", err)
}Committable suggestion skipped: line range outside the PR's diff.
| // setup v1 | ||
| rfqAddrV1, err := cfg.GetRFQAddressV1(chainID) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not get rfq address: %w", err) | ||
| } | ||
| if rfqAddrV1 != nil { | ||
| chainClient, err := omniClient.GetChainClient(ctx, chainID) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not get chain client: %w", err) | ||
| } | ||
| contract, err := fastbridge.NewFastBridgeRef(common.HexToAddress(*rfqAddrV1), chainClient) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not create fast bridge contract: %w", err) | ||
| } | ||
| startBlock, err := contract.DeployBlock(&bind.CallOpts{Context: ctx}) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not get deploy block: %w", err) | ||
| } | ||
| chainListener, err := listener.NewChainListener(chainClient, store, common.HexToAddress(*rfqAddrV1), uint64(startBlock.Int64()), //nolint:gosec // Acceptable conversion | ||
| metricHandler, listener.WithName("guard")) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not get chain listener: %w", err) | ||
| } | ||
| listenersV1[chainID] = chainListener | ||
| contractsV1[chainID] = contract | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reduce code duplication in contract initialization
The initialization logic for V1 and V2 contracts contains significant duplication. Consider extracting the common initialization logic into a helper method:
+ func (g *Guard) initializeContract[T any](
+ ctx context.Context,
+ chainID int,
+ rfqAddr string,
+ newContract func(common.Address, bind.ContractBackend) (T, error),
+ metricHandler metrics.Handler,
+ listenerName string,
+ ) (T, listener.ContractListener, error) {
+ chainClient, err := g.client.GetChainClient(ctx, chainID)
+ if err != nil {
+ return nil, nil, fmt.Errorf("could not get chain client: %w", err)
+ }
+
+ contract, err := newContract(common.HexToAddress(rfqAddr), chainClient)
+ if err != nil {
+ return nil, nil, fmt.Errorf("could not create contract: %w", err)
+ }
+
+ startBlock, err := contract.DeployBlock(&bind.CallOpts{Context: ctx})
+ if err != nil {
+ return nil, nil, fmt.Errorf("could not get deploy block: %w", err)
+ }
+
+ chainListener, err := listener.NewChainListener(
+ chainClient,
+ g.db,
+ common.HexToAddress(rfqAddr),
+ uint64(startBlock.Int64()),
+ metricHandler,
+ listener.WithName(listenerName),
+ )
+ if err != nil {
+ return nil, nil, fmt.Errorf("could not get chain listener: %w", err)
+ }
+
+ return contract, chainListener, nil
+ }This would simplify the initialization code and make it more maintainable.
Also applies to: 94-117
| v2Event := &fastbridgev2.FastBridgeV2BridgeRequested{ | ||
| TransactionId: event.TransactionId, | ||
| Sender: event.Sender, | ||
| Request: event.Request, | ||
| DestChainId: event.DestChainId, | ||
| OriginToken: event.OriginToken, | ||
| DestToken: event.DestToken, | ||
| OriginAmount: event.OriginAmount, | ||
| DestAmount: event.DestAmount, | ||
| SendChainGas: event.SendChainGas, | ||
| Raw: event.Raw, | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using a type converter utility for event conversions
Manual event conversion between V1 and V2 types is error-prone and could lead to missed fields in future updates. Consider creating a dedicated converter utility:
+ type eventConverter struct{}
+
+ func (c *eventConverter) bridgeRequestedV1ToV2(v1 *fastbridge.FastBridgeBridgeRequested) *fastbridgev2.FastBridgeV2BridgeRequested {
+ return &fastbridgev2.FastBridgeV2BridgeRequested{
+ TransactionId: v1.TransactionId,
+ Sender: v1.Sender,
+ Request: v1.Request,
+ DestChainId: v1.DestChainId,
+ OriginToken: v1.OriginToken,
+ DestToken: v1.DestToken,
+ OriginAmount: v1.OriginAmount,
+ DestAmount: v1.DestAmount,
+ SendChainGas: v1.SendChainGas,
+ Raw: v1.Raw,
+ }
+ }Also applies to: 288-293, 299-303
| func (g *Guard) isV2Address(chainID int, addr common.Address) bool { | ||
| rfqAddr, err := g.cfg.GetRFQAddressV2(chainID) | ||
| if err != nil { | ||
| return false | ||
| } | ||
| return addr == common.HexToAddress(rfqAddr) | ||
| } |
There was a problem hiding this comment.
Improve error handling in isV2Address
The method silently fails by returning false when there's an error. Consider either:
- Returning the error to the caller for proper handling
- Logging the error for debugging purposes
- func (g *Guard) isV2Address(chainID int, addr common.Address) bool {
+ func (g *Guard) isV2Address(chainID int, addr common.Address) (bool, error) {
rfqAddr, err := g.cfg.GetRFQAddressV2(chainID)
if err != nil {
- return false
+ return false, fmt.Errorf("failed to get V2 address: %w", err)
}
- return addr == common.HexToAddress(rfqAddr)
+ return addr == common.HexToAddress(rfqAddr), 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.
| func (g *Guard) isV2Address(chainID int, addr common.Address) bool { | |
| rfqAddr, err := g.cfg.GetRFQAddressV2(chainID) | |
| if err != nil { | |
| return false | |
| } | |
| return addr == common.HexToAddress(rfqAddr) | |
| } | |
| func (g *Guard) isV2Address(chainID int, addr common.Address) (bool, error) { | |
| rfqAddr, err := g.cfg.GetRFQAddressV2(chainID) | |
| if err != nil { | |
| return false, fmt.Errorf("failed to get V2 address: %w", err) | |
| } | |
| return addr == common.HexToAddress(rfqAddr), nil | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
services/rfq/relayer/pricer/fee_pricer.go (2)
264-267: Enhance error message for gas estimation failures.The error message should include context about the destination chain and transaction details for easier debugging.
- return 0, fmt.Errorf("could not estimate gas: %w", err) + return 0, fmt.Errorf("failed to estimate gas on chain %d for transaction %s: %w", destination, quoteRequest.Transaction.Hash, err)
185-216: Consider adding circuit breaker for fee calculations.The
addZapFeesmethod should have upper bounds for fees to prevent excessive charges. Consider adding configurable maximum fee limits.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
services/rfq/relayer/pricer/fee_pricer.go(11 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Go Coverage (1.22.x, services/scribe)
- GitHub Check: Go Coverage (1.22.x, services/explorer)
- GitHub Check: Go Generate (Module Changes) (services/scribe)
- GitHub Check: Go Generate (Module Changes) (services/rfq)
- GitHub Check: Go Generate (Module Changes) (services/omnirpc)
- GitHub Check: Go Generate (Module Changes) (services/explorer)
- GitHub Check: Go Generate (Module Changes) (agents)
🔇 Additional comments (7)
services/rfq/relayer/pricer/fee_pricer.go (7)
33-35: LGTM! Interface changes look good.The updated method signatures in the FeePricer interface properly reflect the new requirements for handling zap transactions.
55-56: Add validation for relayerAddress in constructor.The relayerAddress field is critical for transaction signing and fee estimation. A zero address could cause issues.
Also applies to: 76-76
146-146: Fix potential nil pointer dereference.The code accesses
quoteRequest.Transaction.ZapDatawithout checking ifTransactionis nil.
218-235: Fix data race on fastBridgeV2ABI initialization.The global variable
fastBridgeV2ABIis accessed and modified without synchronization, which could lead to data races in concurrent scenarios.
252-262: Add gas limit to CallMsg for accurate estimation.The gas estimation might be inaccurate without a reasonable gas limit set in the CallMsg struct.
414-414: Handle potential precision loss in fee calculations.The code discards the accuracy flag from the Int() conversion, which could silently ignore precision loss.
201-213: Simplify ZapNative value check.The condition has redundant checks for ZapNative value comparison.
| if util.IsGasToken(quoteRequest.Transaction.DestToken) { | ||
| callMsg.Value = quoteRequest.Transaction.DestAmount | ||
| } else { | ||
| callMsg.Value = quoteRequest.Transaction.ZapNative | ||
| } |
There was a problem hiding this comment.
Add error handling for token validation.
The code should validate that DestToken exists and is properly configured before checking if it's a gas token.
+ destToken, err := f.config.GetToken(int(destination), quoteRequest.Transaction.DestToken)
+ if err != nil {
+ return 0, fmt.Errorf("invalid destination token: %w", err)
+ }
- if util.IsGasToken(quoteRequest.Transaction.DestToken) {
+ if util.IsGasToken(destToken) {Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ethergo/submitter/submitter.go(1 hunks)services/rfq/relayer/quoter/quoter.go(11 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
ethergo/submitter/submitter.go
[warning] 700-700: ethergo/submitter/submitter.go#L700
Added line #L700 was not covered by tests
[warning] 705-708: ethergo/submitter/submitter.go#L705-L708
Added lines #L705 - L708 were not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: run-goreleaser (services/omnirpc)
- GitHub Check: run-goreleaser (services/explorer)
- GitHub Check: run-goreleaser (contrib/promexporter)
- GitHub Check: run-goreleaser (agents)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (4)
services/rfq/relayer/quoter/quoter.go (4)
24-24: LGTM! Import aligns with FastBridgeV2 support.The addition of the fastbridgev2 import is appropriate for implementing the new bridge functionality.
696-698: Remove temporary debug print statements.These debug statements should be removed as they:
- Could expose sensitive information in logs
- Add unnecessary noise to the output
- Are marked with "tmpdebug" comment
Apply this diff to remove the debug statements:
- // tmpdebug - fmt.Printf("Debug Total Fee Amt: %s\n", fee.String()) - - // tmpdebug - fmt.Printf("Debug originRFQAddr: %s\n", originRFQAddr.String()) - - // tmpdebug - fmt.Printf("Debug destAmount: %s\n", destAmount.String())Also applies to: 705-707, 715-717
404-416: Add validation for minimum viable destination amount.The destination amount adjustment could result in a zero amount, which might not be desirable for all scenarios.
Consider adding validation to ensure the amount remains above a minimum viable threshold:
destAmountAdj := new(big.Int).Sub(destAmountBigInt, fixedFeeBigInt) if destAmountAdj.Sign() < 0 { - destAmountAdj = big.NewInt(0) + return nil, fmt.Errorf("adjusted destination amount would be negative: amount=%s, fee=%s", + destAmountBigInt.String(), fixedFeeBigInt.String()) } +minViableAmount := big.NewInt(1000) // Configure this based on your requirements +if destAmountAdj.Cmp(minViableAmount) < 0 { + return nil, fmt.Errorf("adjusted destination amount %s below minimum viable amount %s", + destAmountAdj.String(), minViableAmount.String()) +}
439-485: Review default values and assumptions in quote conversion.Several potential issues in the implementation:
- Using max uint256 for deadline might be too permissive
- Empty exclusivity relayer address might cause contract validation issues
- Assuming dest amount equals origin amount might not account for token decimals or exchange rates
Consider these improvements:
- deadline := new(big.Int).Sub(new(big.Int).Exp(big.NewInt(2), big.NewInt(256), nil), big.NewInt(1)) + deadline := new(big.Int).Add(big.NewInt(time.Now().Unix()), big.NewInt(DefaultQuoteDeadline)) - destAmount := originAmount // assume dest amount same as origin amount for estimation purposes + destAmount := calculateDestAmount(originAmount, quoteData.OriginTokenAddr, quoteData.DestTokenAddr) - exclusivityRelayer := common.HexToAddress("") + exclusivityRelayer := common.HexToAddress(DefaultExclusivityRelayer)
ethergo/submitter/submitter.go
Outdated
|
|
||
| // tmpdebug | ||
| fmt.Printf("Debug Default Gas Estimate: %d\n", t.config.GetGasEstimate(chainID)) | ||
|
|
There was a problem hiding this comment.
Remove temporary debug statement or replace with proper logging.
The debug print statement appears to be temporary and doesn't use proper formatting for the gas estimate value. Consider removing it or replacing it with structured logging using the package's logger.
- // tmpdebug
- fmt.Printf("Debug Default Gas Estimate: %d\n", t.config.GetGasEstimate(chainID))
+ logger.Debugf("falling back to default gas estimate: %d", t.config.GetGasEstimate(chainID))Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 705-708: ethergo/submitter/submitter.go#L705-L708
Added lines #L705 - L708 were not covered by tests
ethergo/submitter/submitter.go
Outdated
| // tmpdebug | ||
| fmt.Printf("Debug Calling EstimateGas") |
There was a problem hiding this comment.
Remove temporary debug statement or replace with proper logging.
The debug print statement appears to be temporary and doesn't follow the established logging patterns. Consider removing it or replacing it with the package's logger.
- // tmpdebug
- fmt.Printf("Debug Calling EstimateGas")
+ logger.Debug("calling EstimateGas")Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 700-700: ethergo/submitter/submitter.go#L700
Added line #L700 was not covered by tests
ethergo/submitter/submitter.go
Outdated
| // tmpdebug | ||
| fmt.Printf("Debug Calling EstimateGas") | ||
|
|
||
| gasEstimate, err = chainClient.EstimateGas(ctx, *call) | ||
| if err != nil { | ||
| span.AddEvent("could not estimate gas", trace.WithAttributes(attribute.String("error", err.Error()))) | ||
|
|
||
| // tmpdebug | ||
| fmt.Printf("Debug Default Gas Estimate: %d\n", t.config.GetGasEstimate(chainID)) | ||
|
|
There was a problem hiding this comment.
💡 Codebase verification
Add test coverage for gas estimation error handling in submitter package
The review comment is valid. While there are EstimateGas tests in other packages, the error handling path in ethergo/submitter/submitter.go lacks test coverage. Specifically:
- The error case where gas estimation fails
- The fallback to default gas estimate
- The debug logging behavior
🔗 Analysis chain
Add test coverage for gas estimation error handling.
The added debug statements and error handling path are not covered by tests. Consider adding test cases to verify the behavior when gas estimation fails.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for existing test coverage of gas estimation error handling
rg -A 5 "TestGetGasEstimate.*Error" || echo "No test found for gas estimation error handling"
Length of output: 142
Script:
#!/bin/bash
# Search for test files in submitter package
echo "=== Searching for test files in submitter package ==="
fd ".*_test.go" submitter/
echo "=== Searching for EstimateGas usage in tests ==="
rg -l "EstimateGas.*" --type go | grep "_test.go" || echo "No EstimateGas usage found in test files"
echo "=== Searching for mock chain client ==="
rg -l "type.*Chain.*Client.*interface" --type go || echo "No chain client interface found"
Length of output: 914
Script:
#!/bin/bash
# Find the correct path of submitter.go
echo "=== Finding submitter.go location ==="
fd submitter.go
echo "=== Checking EstimateGas test implementations ==="
rg -A 10 "func.*Test.*EstimateGas" --type go || echo "No EstimateGas test functions found"
echo "=== Checking for error case testing ==="
rg "EstimateGas.*err.*!= nil" --type go || echo "No error handling tests found"
Length of output: 4366
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 700-700: ethergo/submitter/submitter.go#L700
Added line #L700 was not covered by tests
[warning] 705-708: ethergo/submitter/submitter.go#L705-L708
Added lines #L705 - L708 were not covered by tests
|
This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
* +dynamic_gas_unit_add_percentage flag * test fix * [goreleaser] * new simulate approach * [goreleaser] * fix tx.Hash empty [goreleaser] * remove debug noise. nil deref fix... ? * [goreleaser] * check tx call b4 estgas [goreleaser] * +tmp debuggers [goreleaser] * more tmp debug [goreleaser] * tmp debug tx output [goreleaser] * tmp debug submit tweaks [goreleaser] * submitTransaction refactor [goreleaser] * clone transactor [goreleaser] * deepcopy [goreleaser] * [goreleaser] * test getGasEstimate skip * [goreleaser] * try gaslimit 0 [goreleaser] * gaslimit2 [goreleaser] * gaslimit2 pre/post [goreleaser] * tx_forGasEst [goreleaser] * diff nonces [goreleaser] * Prove gas [goreleaser] * print nonce [goreleaser] * low gas test [goreleaser] * getGasEst bump approach [goreleaser] * reproduce [goreleaser] * repro err [goreleaser] * bump approach [goreleaser] * bump approach - arbi test focus [goreleaser] * swap context [goreleaser] * fix transactor_forGasEstimate lock. +QuoteDetails event * quotedetails event fix. +basic print logs * print log impvs [goreleaser] * typo fix * gitignore edit * getGasEstimate code simplify * remove temp tests * remove underscores, FormatError util func, * submitTransaction use common performSignature func for sim & actual
Bundle ReportChanges will increase total bundle size by 16.48MB (133.46%) ⬆️
ℹ️ *Bundle size includes cached data from a previous commit Affected Assets, Files, and Routes:view changes for bundle: synapse-interface-server-cjsAssets Changed:
view changes for bundle: synapse-interface-client-array-pushAssets Changed:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (2)
services/rfq/relayer/pricer/fee_pricer.go (1)
Line range hint
61-77: Add validation for relayerAddress parameter.The constructor should validate that
relayerAddressis not a zero address to prevent potential issues with gas estimation and fee calculations.Add validation at the start of the constructor:
func NewFeePricer(config relconfig.Config, clientFetcher submitter.ClientFetcher, priceFetcher CoingeckoPriceFetcher, handler metrics.Handler, relayerAddress common.Address) FeePricer { + if relayerAddress == (common.Address{}) { + panic("relayerAddress cannot be zero address") + } gasPriceCache := ttlcache.New[uint32, *big.Int](ethergo/submitter/submitter.go (1)
Line range hint
395-425: Handle potential integer overflow in chainID conversion.The code performs unchecked conversions between
uint64andinttypes when handling chainID values, which could lead to integer overflow.Add overflow checks:
+func safeChainIDToInt(chainID *big.Int) (int, error) { + if !chainID.IsUint64() { + return 0, fmt.Errorf("chainID exceeds uint64 max value") + } + chainIDUint64 := chainID.Uint64() + if chainIDUint64 > math.MaxInt32 { + return 0, fmt.Errorf("chainID exceeds int32 max value") + } + return int(chainIDUint64), nil +} func (t *txSubmitterImpl) SubmitTransaction(parentCtx context.Context, chainID *big.Int, call ContractCallType) (nonce uint64, err error) { + chainIDInt, err := safeChainIDToInt(chainID) + if err != nil { + return 0, fmt.Errorf("invalid chainID: %w", err) + }🧰 Tools
🪛 golangci-lint (1.62.2)
396-396: unnecessary leading newline
(whitespace)
🪛 GitHub Check: Lint (ethergo)
[failure] 396-396:
unnecessary leading newline (whitespace)
♻️ Duplicate comments (2)
services/rfq/contracts/fastbridgev2/events.go (1)
89-96:⚠️ Potential issueAvoid returning the address of a loop variable
Returning&eventTypefrom within a loop may cause unexpected behavior, because the loop variable’s address is reused each iteration. Return the value itself or switch to a direct map-based approach for constant-time lookups and safer handling.services/rfq/contracts/fastbridgev2/parser.go (1)
104-104: 🛠️ Refactor suggestionAdd default case in switch statement.
The function should handle unknown event types explicitly by adding a default case.
🧹 Nitpick comments (7)
.gitignore (1)
139-142: LGTM! Well-structured config management.The configuration file management is well-organized:
- Ignores all config variations
- Maintains the template file
- Clear commenting
Consider adding a comment above
!services/rfq/**/config.ymlto clarify that this is the template configuration file:services/rfq/**/config*.yaml services/rfq/**/config*.yml +# Keep the template configuration file !services/rfq/**/config.ymlservices/rfq/contracts/fastbridgev2/events.go (1)
28-72: Evaluate using a more resilient error-handling strategy
Panic calls ininit()can bring down the entire service upon missing events. This is acceptable if failing fast is intentional. Otherwise, consider returning an error or gracefully handling missing events to improve fault tolerance.ethergo/util/util.go (1)
12-12: Add spacing after//to conform to comment style
The static analysis tool points out that comments should have a space after//.- //if an error message contains embedded HTML ... + // if an error message contains embedded HTML ...🧰 Tools
🪛 golangci-lint (1.62.2)
12-12: commentFormatting: put a space between
//and comment text(gocritic)
🪛 GitHub Check: Lint (ethergo)
[failure] 12-12:
commentFormatting: put a space between//and comment text (gocritic)ethergo/submitter/config/config.go (1)
70-72: Add missing period in comment.The comment for
DefaultDynamicGasUnitAddPercentageshould end with a period.Apply this diff:
-// DefaultDynamicGasUnitAddPercentage is the default percentage to bump the gas limit by +// DefaultDynamicGasUnitAddPercentage is the default percentage to bump the gas limit by.services/rfq/relayer/pricer/fee_pricer.go (1)
202-214: Simplify ZapNative value check.The code has redundant checks for ZapNative value. The condition can be simplified.
-if quoteRequest != nil && quoteRequest.Transaction.ZapNative != nil && quoteRequest.Transaction.ZapNative.Cmp(big.NewInt(0)) > 0 && quoteRequest.Transaction.ZapNative.Sign() > 0 { +if quoteRequest != nil && quoteRequest.Transaction.ZapNative != nil && quoteRequest.Transaction.ZapNative.Sign() > 0 {services/rfq/relayer/service/handlers.go (2)
349-352: Log unexpected bridge statuses for better debugging.When the bridge status
bsis notREQUESTED, the function returnsnilwithout logging. Consider adding a log statement to capture unexpected statuses.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 != fastbridgev2.REQUESTED.Int() { + span.AddEvent("unexpected bridge status", trace.WithAttributes( + attribute.String("expected", fastbridgev2.REQUESTED.String()), + attribute.String("actual", fastbridgev2.BridgeStatus(bs).String()), + )) return nil }
474-481: Use a color utility package for better portability.The code uses hardcoded ANSI escape codes for colored output. These might not work in all environments (e.g., Windows) and could cause display issues.
Consider using a color utility package like
fatih/colorthat handles platform compatibility:+import "github.com/fatih/color" +var ( + yellowColor = color.New(color.FgYellow) + magentaColor = color.New(color.FgMagenta) +) -fmt.Printf("TxID 0x%x %7d.%s > %7d.%s : Submitting \033[33mProof\033[0m\n", +fmt.Printf("TxID 0x%x %7d.%s > %7d.%s : Submitting %s\n", request.TransactionID, request.Transaction.OriginChainId, request.Transaction.OriginToken.Hex()[:6], request.Transaction.DestChainId, - request.Transaction.DestToken.Hex()[:6]) + request.Transaction.DestToken.Hex()[:6], + yellowColor.Sprint("Proof"))Also applies to: 628-636
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
.gitignore(1 hunks)docs/bridge/docs/06-Services/04-Submitter.md(1 hunks)ethergo/submitter/config/config.go(3 hunks)ethergo/submitter/config/config_test.go(1 hunks)ethergo/submitter/config/iconfig_generated.go(1 hunks)ethergo/submitter/submitter.go(4 hunks)ethergo/util/util.go(1 hunks)services/rfq/contracts/fastbridgev2/events.go(1 hunks)services/rfq/contracts/fastbridgev2/eventtype_string.go(1 hunks)services/rfq/contracts/fastbridgev2/parser.go(1 hunks)services/rfq/relayer/chain/chain.go(4 hunks)services/rfq/relayer/pricer/fee_pricer.go(11 hunks)services/rfq/relayer/quoter/quoter.go(12 hunks)services/rfq/relayer/service/handlers.go(15 hunks)
✅ Files skipped from review due to trivial changes (1)
- services/rfq/contracts/fastbridgev2/eventtype_string.go
🚧 Files skipped from review as they are similar to previous changes (1)
- services/rfq/relayer/chain/chain.go
🧰 Additional context used
🪛 golangci-lint (1.62.2)
ethergo/util/util.go
12-12: commentFormatting: put a space between // and comment text
(gocritic)
ethergo/submitter/config/config.go
197-197: Comment should end in a period
(godot)
services/rfq/contracts/fastbridgev2/parser.go
26-26: Comment should end in a period
(godot)
ethergo/submitter/submitter.go
396-396: unnecessary leading newline
(whitespace)
[high] 429-429: G115: integer overflow conversion uint64 -> int
(gosec)
[high] 451-451: G115: integer overflow conversion uint64 -> int
(gosec)
[high] 452-452: G115: integer overflow conversion int -> uint64
(gosec)
[high] 430-430: G115: integer overflow conversion uint64 -> int
(gosec)
431-431: unnecessary leading newline
(whitespace)
[high] 727-727: G115: integer overflow conversion uint64 -> int64
(gosec)
711-711: unnecessary leading newline
(whitespace)
[high] 747-747: G115: integer overflow conversion int -> uint64
(gosec)
services/rfq/relayer/pricer/fee_pricer.go
267-267: unnecessary leading newline
(whitespace)
🪛 GitHub Check: Lint (ethergo)
ethergo/util/util.go
[failure] 12-12:
commentFormatting: put a space between // and comment text (gocritic)
ethergo/submitter/config/config.go
[failure] 197-197:
Comment should end in a period (godot)
ethergo/submitter/submitter.go
[failure] 396-396:
unnecessary leading newline (whitespace)
[failure] 429-429:
G115: integer overflow conversion uint64 -> int (gosec)
[failure] 451-451:
G115: integer overflow conversion uint64 -> int (gosec)
[failure] 452-452:
G115: integer overflow conversion int -> uint64 (gosec)
[failure] 430-430:
G115: integer overflow conversion uint64 -> int (gosec)
[failure] 431-431:
unnecessary leading newline (whitespace)
[failure] 727-727:
G115: integer overflow conversion uint64 -> int64 (gosec)
[failure] 747-747:
G115: integer overflow conversion int -> uint64 (gosec)
🪛 GitHub Check: Lint (services/rfq)
services/rfq/contracts/fastbridgev2/parser.go
[failure] 26-26:
Comment should end in a period (godot)
services/rfq/relayer/pricer/fee_pricer.go
[failure] 267-267:
unnecessary leading newline (whitespace)
🪛 GitHub Check: codecov/patch
ethergo/submitter/submitter.go
[warning] 741-741: ethergo/submitter/submitter.go#L741
Added line #L741 was not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: Go Generate (Module Changes) (tools)
- GitHub Check: Go Generate (Module Changes) (services/scribe)
- GitHub Check: Go Coverage (1.22.x, services/scribe)
- GitHub Check: Go Generate (Module Changes) (services/rfq)
- GitHub Check: Go Generate (Module Changes) (services/explorer)
- GitHub Check: Go Generate (Module Changes) (services/cctp-relayer)
- GitHub Check: Go Generate (Module Changes) (ethergo)
- GitHub Check: Go Coverage (1.22.x, services/explorer)
- GitHub Check: Go Generate (Module Changes) (contrib/promexporter)
- GitHub Check: Go Generate (Module Changes) (contrib/opbot)
- GitHub Check: Go Coverage (1.22.x, ethergo)
- GitHub Check: Go Generate (Module Changes) (agents)
- GitHub Check: Deploy to Vercel (docs)
🔇 Additional comments (22)
.gitignore (1)
137-138: LGTM! Good security practice.Adding
*signer*.txtpattern helps prevent accidental commits of sensitive signer-related data.services/rfq/relayer/quoter/quoter.go (6)
24-24: LGTM: FastBridgeV2 import added.The import aligns with the PR objective of supporting FastBridgeV2.
301-302: LGTM: Channel buffer size constant added.The buffer size of 1000 is appropriate for handling high-throughput RFQ messages.
404-416: Add validation for minimum viable destination amount.Setting negative amounts to 0 could hide potential issues. Consider adding validation as previously suggested.
438-485: Review default values and assumptions in quote conversion.The default values and assumptions in the implementation need review as previously suggested.
669-669: LGTM: QuoteRequest field added to QuoteInput.The field addition is well-structured and necessary for FastBridgeV2 support.
690-695: LGTM: Fee calculation updated for FastBridgeV2.The fee calculation logic is properly updated to handle FastBridgeV2 quotes.
services/rfq/contracts/fastbridgev2/events.go (1)
74-85: Avoid creating a new map on each call
A new map is rebuilt on each invocation oftopicMap(), which may be slightly inefficient. A static or cached map could be returned instead for improved performance and to ensure immutability.ethergo/util/util.go (1)
5-17: Logic for stripping embedded HTML looks good
This approach neatly handles HTML noise in error messages. If further sanitization is desired, consider specialized HTML sanitization libraries. Otherwise, this implementation aligns well with its stated goal.🧰 Tools
🪛 golangci-lint (1.62.2)
12-12: commentFormatting: put a space between
//and comment text(gocritic)
🪛 GitHub Check: Lint (ethergo)
[failure] 12-12:
commentFormatting: put a space between//and comment text (gocritic)ethergo/submitter/config/iconfig_generated.go (1)
44-45: Add test coverage for the newly introduced method
The newGetDynamicGasUnitAddPercentagemethod would benefit from unit tests analogous to those for other getters (e.g.,GetBumpInterval, etc.). This ensures its correctness across various chain IDs.Would you like help creating a basic unit test to validate this new method?
ethergo/submitter/config/config_test.go (1)
80-88: LGTM!The test case correctly verifies the parsing and setting of the new
DynamicGasUnitAddPercentageconfiguration parameter.ethergo/submitter/config/config.go (2)
52-54: LGTM!The new configuration field is well-documented and follows the existing pattern.
197-213: LGTM!The getter method follows the same pattern as other configuration getters, providing chain-specific overrides with fallback to global and default values.
🧰 Tools
🪛 golangci-lint (1.62.2)
197-197: Comment should end in a period
(godot)
🪛 GitHub Check: Lint (ethergo)
[failure] 197-197:
Comment should end in a period (godot)services/rfq/contracts/fastbridgev2/parser.go (2)
30-30: Fix typo in comment.The word "contracat" should be "contract".
73-77: Rename variable for clarity.The variable
requestedin theBridgeRelayedEventcase should be renamed torelayedto match the event type.docs/bridge/docs/06-Services/04-Submitter.md (1)
62-64: LGTM!The documentation clearly explains the purpose and behavior of the new
dynamic_gas_unit_add_percentageconfiguration parameter.services/rfq/relayer/pricer/fee_pricer.go (4)
8-22: LGTM!The imports are well-organized and properly support the new FastBridgeV2 functionality.
219-221:⚠️ Potential issueFix potential data race on global variable.
The
fastBridgeV2ABIis a package-level global that's set without synchronization. Concurrent calls togetZapGasEstimatewhenfastBridgeV2ABIisnilcould lead to a data race.Use
sync.Onceto ensure thread-safe initialization:+var ( + fastBridgeV2ABI *abi.ABI + fastBridgeV2ABIOnce sync.Once +) func (f *feePricer) getZapGasEstimate(ctx context.Context, destination uint32, quoteRequest *reldb.QuoteRequest) (gasEstimate uint64, err error) { - if fastBridgeV2ABI == nil { + var parseErr error + fastBridgeV2ABIOnce.Do(func() { parsedABI, err := abi.JSON(strings.NewReader(fastbridgev2.IFastBridgeV2MetaData.ABI)) if err != nil { - return 0, fmt.Errorf("could not parse ABI: %w", err) + parseErr = fmt.Errorf("could not parse ABI: %w", err) return } fastBridgeV2ABI = &parsedABI + }) + if parseErr != nil { + return 0, parseErr }Likely invalid or redundant comment.
147-155:⚠️ Potential issueAdd nil check for Transaction field.
There's a potential nil pointer dereference when accessing
quoteRequest.Transaction.ZapData. Add a nil check for the Transaction field.-if quoteRequest == nil || len(quoteRequest.Transaction.ZapData) == 0 { +if quoteRequest == nil || quoteRequest.Transaction == nil || len(quoteRequest.Transaction.ZapData) == 0 {Likely invalid or redundant comment.
419-420: 🛠️ Refactor suggestionHandle potential precision loss in fee calculations.
The current implementation discards the accuracy flag from
Int()conversion and might silently ignore precision loss.- feeScaled, _ = new(big.Float).Mul(feeDenom, new(big.Float).SetFloat64(multiplier)).Int(nil) + feeScaled, accurate := new(big.Float).Mul(feeDenom, new(big.Float).SetFloat64(multiplier)).Int(nil) + if !accurate { + return nil, fmt.Errorf("fee calculation resulted in precision loss") + }Likely invalid or redundant comment.
services/rfq/relayer/service/handlers.go (1)
79-81:⚠️ Potential issueHandle potential nil values when retrieving bridge transactions.
Ensure that both
txV1andtxV2are properly checked fornilbefore use, in casegetBridgeTxsreturns without initializing them due to an error.Add nil checks after retrieving bridge transactions:
txV1, txV2, err := r.getBridgeTxs(ctx, req, originClient) if err != nil { return fmt.Errorf("could not get bridge txs: %w", err) } +if txV2 == nil { + return fmt.Errorf("txV2 is nil") +}Likely invalid or redundant comment.
ethergo/submitter/submitter.go (1)
Line range hint
711-750: Add test coverage for gas estimation error handling.The error handling path in
getGasEstimatelacks test coverage, particularly for:
- The error case where gas estimation fails
- The fallback to default gas estimate
Run the following script to verify test coverage:
✅ Verification successful
Add test coverage for gas estimation error handling in getGasEstimate
The verification confirms that test coverage is missing for:
- Error case when EstimateGas fails
- Fallback to default gas estimate from config
Key test files to update:
ethergo/submitter/submitter_test.go🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for existing test coverage of gas estimation error handling rg -A 5 "TestGetGasEstimate.*Error" || echo "No test found for gas estimation error handling" echo "=== Searching for test files in submitter package ===" fd ".*_test.go" submitter/ echo "=== Searching for EstimateGas usage in tests ===" rg -l "EstimateGas.*" --type go | grep "_test.go" || echo "No EstimateGas usage found in test files"Length of output: 844
Script:
#!/bin/bash # Find the correct path to submitter package fd submitter echo "=== Searching for submitter test files ===" fd -e go -e test.go . | grep submitter echo "=== Searching for mock implementations ===" rg -l "Mock.*EstimateGas" --type go echo "=== Searching for indirect test coverage ===" rg -A 10 "func Test.*" --type go | grep -B 10 -A 10 "getGasEstimate" || echo "No indirect test coverage found"Length of output: 1775
🧰 Tools
🪛 golangci-lint (1.62.2)
[high] 727-727: G115: integer overflow conversion uint64 -> int64
(gosec)
711-711: unnecessary leading newline
(whitespace)
🪛 GitHub Check: Lint (ethergo)
[failure] 727-727:
G115: integer overflow conversion uint64 -> int64 (gosec)
| // TODO: consider not exporting to avoid accidental mutation. | ||
| var ( | ||
| // BridgeRequestedTopic is the event topic for a bridge request. | ||
| BridgeRequestedTopic common.Hash | ||
| // BridgeRelayedTopic is the topic emitted by a bridge relay. | ||
| BridgeRelayedTopic common.Hash | ||
| // BridgeProofProvidedTopic is the topic emitted by a bridge relay. | ||
| BridgeProofProvidedTopic common.Hash | ||
| // BridgeDepositClaimedTopic is the topic emitted by a bridge relay. | ||
| BridgeDepositClaimedTopic common.Hash | ||
| // BridgeProofDisputedTopic is the topic emitted by a bridge dispute. | ||
| BridgeProofDisputedTopic common.Hash | ||
| // BridgeQuoteDetailsTopic is a secondary topic emitted by a bridge request. | ||
| BridgeQuoteDetailsTopic common.Hash | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Encapsulate or privatize event topics to avoid accidental mutation
The TODO comment explicitly warns about accidental mutation. If these topics must remain immutable, consider making them unexported or constants (if applicable). Otherwise, expose them more safely (e.g., via getter methods) to prevent accidental mutations.
| // if dynamic gas estimation is not enabled, use cfg var gas_estimate as a gas limit default and do not run a pre-flight simulation | ||
| // since we do not need it to determine proper gas units | ||
| if !t.config.GetDynamicGasEstimate(int(chainID.Uint64())) { | ||
| transactor.GasLimit = t.config.GetGasEstimate(int(chainID.Uint64())) | ||
| } else { | ||
|
|
||
| transaction, err = util.CopyTX(transaction, util.WithNonce(newNonce), util.WithTxType(txType)) | ||
| // deepcopy the real transactor so we can use it for simulation | ||
| transactorForGasEstimate := copyTransactOpts(transactor) | ||
|
|
||
| // override the signer func for our simulation/estimation with a version that does not lock the nonce, | ||
| // which would othewrise cause a deadlock with the following *actual* transactor | ||
| transactorForGasEstimate.Signer = func(address common.Address, transaction *types.Transaction) (_ *types.Transaction, err error) { | ||
| return performSignature(address, transaction) | ||
| } | ||
|
|
||
| txForGasEstimate, err := call(transactorForGasEstimate) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not copy tx: %w", err) | ||
| errMsg := util.FormatError(err) | ||
|
|
||
| return 0, fmt.Errorf("err contract call for gas est: %s", errMsg) | ||
| } | ||
|
|
||
| //nolint: wrapcheck | ||
| return parentTransactor.Signer(address, transaction) | ||
| // with our gas limit now obtained from the simulation, apply this limit (plus any configured % modifier) to the | ||
| // gas limit of the actual transactor that is about to prepare the real transaction | ||
| gasLimitAddPercentage := t.config.GetDynamicGasUnitAddPercentage(int(chainID.Uint64())) | ||
| transactor.GasLimit = txForGasEstimate.Gas() + (txForGasEstimate.Gas() * uint64(gasLimitAddPercentage) / 100) |
There was a problem hiding this comment.
Ensure safe gas limit calculations.
The gas limit calculation with percentage adjustments could overflow for large gas values.
Add overflow checks and use safe math:
+func safeGasLimitAdjustment(gasLimit uint64, percentage int) (uint64, error) {
+ if percentage < 0 {
+ return 0, fmt.Errorf("negative percentage not allowed")
+ }
+ adjustment := new(big.Int).Mul(
+ new(big.Int).SetUint64(gasLimit),
+ new(big.Int).SetInt64(int64(percentage)),
+ )
+ adjustment.Div(adjustment, big.NewInt(100))
+ if !adjustment.IsUint64() {
+ return 0, fmt.Errorf("gas limit adjustment overflow")
+ }
+ total := new(big.Int).Add(
+ new(big.Int).SetUint64(gasLimit),
+ adjustment,
+ )
+ if !total.IsUint64() {
+ return 0, fmt.Errorf("final gas limit overflow")
+ }
+ return total.Uint64(), nil
+}
-transactor.GasLimit = txForGasEstimate.Gas() + (txForGasEstimate.Gas() * uint64(gasLimitAddPercentage) / 100)
+transactor.GasLimit, err = safeGasLimitAdjustment(txForGasEstimate.Gas(), gasLimitAddPercentage)
+if err != nil {
+ return 0, fmt.Errorf("could not calculate gas limit: %w", err)
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // if dynamic gas estimation is not enabled, use cfg var gas_estimate as a gas limit default and do not run a pre-flight simulation | |
| // since we do not need it to determine proper gas units | |
| if !t.config.GetDynamicGasEstimate(int(chainID.Uint64())) { | |
| transactor.GasLimit = t.config.GetGasEstimate(int(chainID.Uint64())) | |
| } else { | |
| transaction, err = util.CopyTX(transaction, util.WithNonce(newNonce), util.WithTxType(txType)) | |
| // deepcopy the real transactor so we can use it for simulation | |
| transactorForGasEstimate := copyTransactOpts(transactor) | |
| // override the signer func for our simulation/estimation with a version that does not lock the nonce, | |
| // which would othewrise cause a deadlock with the following *actual* transactor | |
| transactorForGasEstimate.Signer = func(address common.Address, transaction *types.Transaction) (_ *types.Transaction, err error) { | |
| return performSignature(address, transaction) | |
| } | |
| txForGasEstimate, err := call(transactorForGasEstimate) | |
| if err != nil { | |
| return nil, fmt.Errorf("could not copy tx: %w", err) | |
| errMsg := util.FormatError(err) | |
| return 0, fmt.Errorf("err contract call for gas est: %s", errMsg) | |
| } | |
| //nolint: wrapcheck | |
| return parentTransactor.Signer(address, transaction) | |
| // with our gas limit now obtained from the simulation, apply this limit (plus any configured % modifier) to the | |
| // gas limit of the actual transactor that is about to prepare the real transaction | |
| gasLimitAddPercentage := t.config.GetDynamicGasUnitAddPercentage(int(chainID.Uint64())) | |
| transactor.GasLimit = txForGasEstimate.Gas() + (txForGasEstimate.Gas() * uint64(gasLimitAddPercentage) / 100) | |
| // if dynamic gas estimation is not enabled, use cfg var gas_estimate as a gas limit default and do not run a pre-flight simulation | |
| // since we do not need it to determine proper gas units | |
| if !t.config.GetDynamicGasEstimate(int(chainID.Uint64())) { | |
| transactor.GasLimit = t.config.GetGasEstimate(int(chainID.Uint64())) | |
| } else { | |
| // deepcopy the real transactor so we can use it for simulation | |
| transactorForGasEstimate := copyTransactOpts(transactor) | |
| // override the signer func for our simulation/estimation with a version that does not lock the nonce, | |
| // which would othewrise cause a deadlock with the following *actual* transactor | |
| transactorForGasEstimate.Signer = func(address common.Address, transaction *types.Transaction) (_ *types.Transaction, err error) { | |
| return performSignature(address, transaction) | |
| } | |
| txForGasEstimate, err := call(transactorForGasEstimate) | |
| if err != nil { | |
| errMsg := util.FormatError(err) | |
| return 0, fmt.Errorf("err contract call for gas est: %s", errMsg) | |
| } | |
| // with our gas limit now obtained from the simulation, apply this limit (plus any configured % modifier) to the | |
| // gas limit of the actual transactor that is about to prepare the real transaction | |
| gasLimitAddPercentage := t.config.GetDynamicGasUnitAddPercentage(int(chainID.Uint64())) | |
| func safeGasLimitAdjustment(gasLimit uint64, percentage int) (uint64, error) { | |
| if percentage < 0 { | |
| return 0, fmt.Errorf("negative percentage not allowed") | |
| } | |
| adjustment := new(big.Int).Mul( | |
| new(big.Int).SetUint64(gasLimit), | |
| new(big.Int).SetInt64(int64(percentage)), | |
| ) | |
| adjustment.Div(adjustment, big.NewInt(100)) | |
| if !adjustment.IsUint64() { | |
| return 0, fmt.Errorf("gas limit adjustment overflow") | |
| } | |
| total := new(big.Int).Add( | |
| new(big.Int).SetUint64(gasLimit), | |
| adjustment, | |
| ) | |
| if !total.IsUint64() { | |
| return 0, fmt.Errorf("final gas limit overflow") | |
| } | |
| return total.Uint64(), nil | |
| } | |
| transactor.GasLimit, err = safeGasLimitAdjustment(txForGasEstimate.Gas(), gasLimitAddPercentage) | |
| if err != nil { | |
| return 0, fmt.Errorf("could not calculate gas limit: %w", err) | |
| } |
🧰 Tools
🪛 golangci-lint (1.62.2)
[high] 429-429: G115: integer overflow conversion uint64 -> int
(gosec)
[high] 451-451: G115: integer overflow conversion uint64 -> int
(gosec)
[high] 452-452: G115: integer overflow conversion int -> uint64
(gosec)
[high] 430-430: G115: integer overflow conversion uint64 -> int
(gosec)
431-431: unnecessary leading newline
(whitespace)
🪛 GitHub Check: Lint (ethergo)
[failure] 429-429:
G115: integer overflow conversion uint64 -> int (gosec)
[failure] 451-451:
G115: integer overflow conversion uint64 -> int (gosec)
[failure] 452-452:
G115: integer overflow conversion int -> uint64 (gosec)
[failure] 430-430:
G115: integer overflow conversion uint64 -> int (gosec)
[failure] 431-431:
unnecessary leading newline (whitespace)
github-actions
bot
added
needs-go-generate-agents
needs-go-generate-services/omnirpc
and removed
Stale
labels
Summary by CodeRabbit
New Features
fastbridgev2contract across various components.Bug Fixes
Documentation
Tests
fastbridgev2contract and updated fee pricing logic.6986e7b: docs preview link
93636c2: docs preview link