Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SLT-234] feat(rfq): dynamic block conf wait + receipt x event quote check w/ RPC consensus #3190

Closed
wants to merge 7 commits into from
Closed

[SLT-234] feat(rfq): dynamic block conf wait + receipt x event quote check w/ RPC consensus #3190

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
4a87e3d
initial
golangisfun123 Sep 24, 2024
0aa10b6
implement rpc checking after confirmation delay
golangisfun123 Sep 25, 2024
3958292
Merge branch 'master' of github.com:synapsecns/sanguine into rpc-conf…
golangisfun123 Sep 30, 2024
7fca370
dynamic confirmation wait based on volume
golangisfun123 Sep 30, 2024
353b63c
better division check
golangisfun123 Sep 30, 2024
bfd1761
divide by 0 check
golangisfun123 Oct 1, 2024
5bc3004
[goreleaser]
golangisfun123 Oct 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion services/rfq/e2e/setup_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -287,6 +287,7 @@ func (i *IntegrationSuite) getRelayerConfig() relconfig.Config {
},
},
NativeToken: "ETH",
VolumeLimit: 10_000,
golangisfun123 marked this conversation as resolved.
Show resolved Hide resolved
},
destBackendChainID: {
RFQAddress: i.manager.Get(i.GetTestContext(), i.destBackend, testutil.FastBridgeType).Address().String(),
Expand All @@ -304,6 +305,7 @@ func (i *IntegrationSuite) getRelayerConfig() relconfig.Config {
},
},
NativeToken: "ETH",
VolumeLimit: 10_000,
},
},
OmniRPCURL: i.omniServer,
Expand All @@ -329,7 +331,6 @@ func (i *IntegrationSuite) getRelayerConfig() relconfig.Config {
TokenPriceCacheTTLSeconds: 60,
},
RebalanceInterval: 0,
VolumeLimit: 10_000,
}
}

Expand Down
143 changes: 124 additions & 19 deletions services/rfq/relayer/limiter/limiter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,16 @@

import (
"context"
"encoding/hex"
"errors"
"fmt"
"math/big"

"github.com/ethereum/go-ethereum"
"github.com/ethereum/go-ethereum/common"
"github.com/synapsecns/sanguine/core/metrics"
"github.com/synapsecns/sanguine/ethergo/client"
"github.com/synapsecns/sanguine/services/rfq/contracts/fastbridge"
"github.com/synapsecns/sanguine/services/rfq/relayer/quoter"
"github.com/synapsecns/sanguine/services/rfq/relayer/relconfig"
"github.com/synapsecns/sanguine/services/rfq/relayer/reldb"
Expand All @@ -34,59 +39,71 @@
quoter quoter.Quoter
cfg relconfig.Config
tokenNames map[string]relconfig.TokenConfig
evmClient client.EVM
}

// NewRateLimiter creates a new Limiter.
// TODO: implement the sliding window: queue up requests and process them in order if cumulative volume is above limit.
func NewRateLimiter(
cfg relconfig.Config,
l LatestBlockFetcher,
q quoter.Quoter,
metricHandler metrics.Handler,
tokens map[string]relconfig.TokenConfig,
evmClient client.EVM,
) Limiter {
return &limiterImpl{
listener: l,
metrics: metricHandler,
quoter: q,
cfg: cfg,
tokenNames: tokens,
evmClient: evmClient,
}
}

// IsAllowed returns true if the request is allowed, false otherwise.
func (l *limiterImpl) IsAllowed(ctx context.Context, request *reldb.QuoteRequest) (_ bool, err error) {
ctx, span := l.metrics.Tracer().Start(
ctx, "limiter.IsAllowed", trace.WithAttributes(util.QuoteRequestToAttributes(*request)...),
ctx, "limiter.IsAllowed", trace.WithAttributes(util.QuoteRequestToAttributes(request)...),
)

defer func() {
metrics.EndSpanWithErr(span, err)
}()

// if enough confirmations, allow because reorgs are rare at this point
hasEnoughConfirmations, err := l.hasEnoughConfirmations(ctx, request)
// if not enough confirmations, check volume. if under limit, wait 0 confirmations.
underVolumeLimit, err := l.isUnderVolumeLimit(ctx, request)
if err != nil {
return false, fmt.Errorf("could not check confirmations: %w", err)
return false, fmt.Errorf("could not check volume limit: %w", err)

Check warning on line 76 in services/rfq/relayer/limiter/limiter.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/relayer/limiter/limiter.go#L76 

Added line #L76 was not covered by tests
}
if hasEnoughConfirmations {
if underVolumeLimit {
return true, nil
}

// if not enough confirmations, check volume
withinSize, err := l.withinSizeLimit(ctx, request)
// if enough confirmations, allow because reorgs are rare at this point.
hasEnoughConfirmations, err := l.hasEnoughConfirmations(ctx, request)
if err != nil {
return false, fmt.Errorf("could not check volume limit: %w", err)
return false, fmt.Errorf("could not check confirmations: %w", err)
}

Check warning on line 86 in services/rfq/relayer/limiter/limiter.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/relayer/limiter/limiter.go#L85-L86 

Added lines #L85 - L86 were not covered by tests
if hasEnoughConfirmations {
// we need to check if the receipt exists, parse the events from it, check for possible reverts,
// and has the correct fields in case of a reorg. then, and only then, we can be sure a reorg will not
// revert this.
receiptFieldsMatch, err := l.checkReceipt(ctx, request)
if err != nil {
return false, fmt.Errorf("could not check receipt: %w", err)
}

Check warning on line 94 in services/rfq/relayer/limiter/limiter.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/relayer/limiter/limiter.go#L93-L94 

Added lines #L93 - L94 were not covered by tests
return receiptFieldsMatch, nil
Comment on lines +73 to +95
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improved IsAllowed method logic

The new logic in the IsAllowed method provides a more comprehensive check for request validation. The sequence of checks (volume limit, confirmations, and receipt verification) enhances the security and reliability of the rate limiting process.

However, consider improving error handling for better debugging:

Consider wrapping errors with more context:

- return false, fmt.Errorf("could not check volume limit: %w", err)
+ return false, fmt.Errorf("IsAllowed failed to check volume limit: %w", err)

- return false, fmt.Errorf("could not check confirmations: %w", err)
+ return false, fmt.Errorf("IsAllowed failed to check confirmations: %w", err)

- return false, fmt.Errorf("could not check receipt: %w", err)
+ return false, fmt.Errorf("IsAllowed failed to check receipt: %w", err)

This change will make it easier to trace the origin of errors in logs.

📝 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.

Suggested change
// if not enough confirmations, check volume. if under limit, wait 0 confirmations.
underVolumeLimit, err := l.isUnderVolumeLimit(ctx, request)
if err != nil {
return false, fmt.Errorf("could not check confirmations: %w", err)
return false, fmt.Errorf("could not check volume limit: %w", err)
}
if hasEnoughConfirmations {
if underVolumeLimit {
return true, nil
}
// if not enough confirmations, check volume
withinSize, err := l.withinSizeLimit(ctx, request)
// if enough confirmations, allow because reorgs are rare at this point.
hasEnoughConfirmations, err := l.hasEnoughConfirmations(ctx, request)
if err != nil {
return false, fmt.Errorf("could not check volume limit: %w", err)
return false, fmt.Errorf("could not check confirmations: %w", err)
}
if hasEnoughConfirmations {
// we need to check if the receipt exists, parse the events from it, check for possible reverts,
// and has the correct fields in case of a reorg. then, and only then, we can be sure a reorg will not
// revert this.
receiptFieldsMatch, err := l.checkReceipt(ctx, request)
if err != nil {
return false, fmt.Errorf("could not check receipt: %w", err)
}
return receiptFieldsMatch, nil
// if not enough confirmations, check volume. if under limit, wait 0 confirmations.
underVolumeLimit, err := l.isUnderVolumeLimit(ctx, request)
if err != nil {
return false, fmt.Errorf("IsAllowed failed to check volume limit: %w", err)
}
if underVolumeLimit {
return true, nil
}
// if enough confirmations, allow because reorgs are rare at this point.
hasEnoughConfirmations, err := l.hasEnoughConfirmations(ctx, request)
if err != nil {
return false, fmt.Errorf("IsAllowed failed to check confirmations: %w", err)
}
if hasEnoughConfirmations {
// we need to check if the receipt exists, parse the events from it, check for possible reverts,
// and has the correct fields in case of a reorg. then, and only then, we can be sure a reorg will not
// revert this.
receiptFieldsMatch, err := l.checkReceipt(ctx, request)
if err != nil {
return false, fmt.Errorf("IsAllowed failed to check receipt: %w", err)
}
return receiptFieldsMatch, nil
}
🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 76-76: services/rfq/relayer/limiter/limiter.go#L76
Added line #L76 was not covered by tests

[warning] 85-86: services/rfq/relayer/limiter/limiter.go#L85-L86
Added lines #L85 - L86 were not covered by tests

[warning] 93-94: services/rfq/relayer/limiter/limiter.go#L93-L94
Added lines #L93 - L94 were not covered by tests

}

span.SetAttributes(
attribute.Bool("has_enough_confirmations", hasEnoughConfirmations),
attribute.Bool("within_size_limit", withinSize),
attribute.Bool("within_size_limit", underVolumeLimit),
)

return withinSize, nil
return false, nil
}

// hasEnoughConfirmations returns true if the request has enough confirmations, false otherwise.
func (l *limiterImpl) hasEnoughConfirmations(ctx context.Context, request *reldb.QuoteRequest) (_ bool, err error) {
_, span := l.metrics.Tracer().Start(ctx, "limiter.hasEnoughConfirmations")
defer func() {
Expand All @@ -98,9 +115,9 @@
return false, fmt.Errorf("could not get block number: %w", err)
}

requiredConfirmations, err := l.cfg.GetFinalityConfirmations(int(request.Transaction.OriginChainId))
requiredConfirmations, err := l.getNumberOfConfirmationsToWait(ctx, request)
if err != nil {
return false, fmt.Errorf("could not get required confirmations from config: %w", err)
return false, fmt.Errorf("could not get number of confirmations to wait: %w", err)

Check warning on line 120 in services/rfq/relayer/limiter/limiter.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/relayer/limiter/limiter.go#L120 

Added line #L120 was not covered by tests
}

actualConfirmations := currentBlockNumber - request.BlockNumber
Expand All @@ -117,8 +134,34 @@
return hasEnoughConfirmations, nil
}

func (l *limiterImpl) withinSizeLimit(ctx context.Context, request *reldb.QuoteRequest) (_ bool, err error) {
ctx, span := l.metrics.Tracer().Start(ctx, "limiter.withinSizeLimit")
// getNumberOfConfirmationsToWait returns the number of confirmations to wait for the request.
// confirmations = requestVolume / volumeLimitForChain, e.g. we wait 1 confirmation per every `volumeLimitForChain` USD.
func (l *limiterImpl) getNumberOfConfirmationsToWait(ctx context.Context, request *reldb.QuoteRequest) (_ uint64, err error) {
ctx, span := l.metrics.Tracer().Start(ctx, "limiter.getNumberOfConfirmationsToWait")

defer func() {
metrics.EndSpanWithErr(span, err)
}()

tokenVolume, err := l.getRequestVolumeOfToken(ctx, request)
if err != nil {
return 0, fmt.Errorf("could not get token volume: %w", err)
}

Check warning on line 149 in services/rfq/relayer/limiter/limiter.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/relayer/limiter/limiter.go#L148-L149 

Added lines #L148 - L149 were not covered by tests

volumeLimitForChain := l.cfg.GetVolumeLimit(int(request.Transaction.OriginChainId), request.Transaction.OriginToken)
if volumeLimitForChain.Cmp(big.NewInt(-1)) == 0 {
return 0, nil
}

return uint64(new(big.Int).Div(
tokenVolume,
volumeLimitForChain,
).Int64()), nil
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

New getNumberOfConfirmationsToWait method looks good, but needs a safeguard

The new getNumberOfConfirmationsToWait method provides a dynamic way to determine the required confirmations based on the request volume and chain-specific volume limit. The logic is sound, including the special case handling for no limit (when volumeLimitForChain is -1).

However, there's a potential issue:

Add a check to prevent division by zero:

 volumeLimitForChain := l.cfg.GetVolumeLimit(int(request.Transaction.OriginChainId), request.Transaction.OriginToken)
 if volumeLimitForChain.Cmp(big.NewInt(-1)) == 0 {
     return 0, nil
 }
+if volumeLimitForChain.Cmp(big.NewInt(0)) == 0 {
+    return 0, fmt.Errorf("volume limit for chain %d and token %s is zero", request.Transaction.OriginChainId, request.Transaction.OriginToken)
+}

 return uint64(new(big.Int).Div(
     tokenVolume,
     volumeLimitForChain,
 ).Int64()), nil

This change will prevent a potential panic if the volume limit is set to zero for a chain/token combination.

📝 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.

Suggested change
// getNumberOfConfirmationsToWait returns the number of confirmations to wait for the request.
// confirmations = requestVolume / volumeLimitForChain, e.g. we wait 1 confirmation per every `volumeLimitForChain` USD.
func (l *limiterImpl) getNumberOfConfirmationsToWait(ctx context.Context, request *reldb.QuoteRequest) (_ uint64, err error) {
ctx, span := l.metrics.Tracer().Start(ctx, "limiter.getNumberOfConfirmationsToWait")
defer func() {
metrics.EndSpanWithErr(span, err)
}()
tokenVolume, err := l.getRequestVolumeOfToken(ctx, request)
if err != nil {
return 0, fmt.Errorf("could not get token volume: %w", err)
}
volumeLimitForChain := l.cfg.GetVolumeLimit(int(request.Transaction.OriginChainId), request.Transaction.OriginToken)
if volumeLimitForChain.Cmp(big.NewInt(-1)) == 0 {
return 0, nil
}
return uint64(new(big.Int).Div(
tokenVolume,
volumeLimitForChain,
).Int64()), nil
}
// getNumberOfConfirmationsToWait returns the number of confirmations to wait for the request.
// confirmations = requestVolume / volumeLimitForChain, e.g. we wait 1 confirmation per every `volumeLimitForChain` USD.
func (l *limiterImpl) getNumberOfConfirmationsToWait(ctx context.Context, request *reldb.QuoteRequest) (_ uint64, err error) {
ctx, span := l.metrics.Tracer().Start(ctx, "limiter.getNumberOfConfirmationsToWait")
defer func() {
metrics.EndSpanWithErr(span, err)
}()
tokenVolume, err := l.getRequestVolumeOfToken(ctx, request)
if err != nil {
return 0, fmt.Errorf("could not get token volume: %w", err)
}
volumeLimitForChain := l.cfg.GetVolumeLimit(int(request.Transaction.OriginChainId), request.Transaction.OriginToken)
if volumeLimitForChain.Cmp(big.NewInt(-1)) == 0 {
return 0, nil
}
if volumeLimitForChain.Cmp(big.NewInt(0)) == 0 {
return 0, fmt.Errorf("volume limit for chain %d and token %s is zero", request.Transaction.OriginChainId, request.Transaction.OriginToken)
}
return uint64(new(big.Int).Div(
tokenVolume,
volumeLimitForChain,
).Int64()), nil
}
🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 148-149: services/rfq/relayer/limiter/limiter.go#L148-L149
Added lines #L148 - L149 were not covered by tests


// isUnderVolumeLimit returns true if the request is under the volume limit, false otherwise.
func (l *limiterImpl) isUnderVolumeLimit(ctx context.Context, request *reldb.QuoteRequest) (_ bool, err error) {
ctx, span := l.metrics.Tracer().Start(ctx, "limiter.isUnderVolumeLimit")
defer func() {
metrics.EndSpanWithErr(span, err)
}()
Expand All @@ -134,14 +177,14 @@
return false, fmt.Errorf("could not get USD amount of token: %w", err)
}

withinSizeLimit := tokenPrice.Cmp(volumeLimit) < 0
underVolumeLimit := tokenPrice.Cmp(volumeLimit) < 0
span.SetAttributes(
attribute.String("volume_limit", volumeLimit.String()),
attribute.String("token_price", tokenPrice.String()),
attribute.Bool("within_size_limit", withinSizeLimit),
attribute.Bool("within_size_limit", underVolumeLimit),
)

return withinSizeLimit, nil
return underVolumeLimit, nil
}

// getRequestVolumeOfToken returns the volume of the token in USD. This value is NOT human readable.
Expand Down Expand Up @@ -183,3 +226,65 @@

return product, nil
}

// checkReceipt checks if the receipt exists and has the correct fields in the case a reorg happened.
func (l *limiterImpl) checkReceipt(ctx context.Context, request *reldb.QuoteRequest) (_ bool, err error) {
// Make sure receipt exists and has the correct fields in case of a reorg.
// Note: https://community.infura.io/t/does-eth-gettransactionreceipt-respond-to-re-orged-transactions/7765
// "You will get a tx receipt back but, as you note, there is a small chance of a reorg.
// In a re-org the shorter side chain (one block usually, two occasionally) will have all its tx’s reverted and
// placed back in the mempool. Calling eth_getTransactionReceipt at this point will return null, until the tx is
// added to a new block and validated."
ctx, span := l.metrics.Tracer().Start(ctx, "limiter.checkReceipt")
defer func() {
metrics.EndSpanWithErr(span, err)
}()

receipt, err := l.evmClient.TransactionReceipt(ctx, request.OriginTxHash)
if err != nil {
if errors.Is(err, ethereum.NotFound) {
return false, nil
}
return false, fmt.Errorf("could not get transaction receipt: %w", err)

Check warning on line 248 in services/rfq/relayer/limiter/limiter.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/relayer/limiter/limiter.go#L245-L248 

Added lines #L245 - L248 were not covered by tests
}

rfqAddr, err := l.cfg.GetRFQAddress(int(request.Transaction.OriginChainId))
if err != nil {
return false, fmt.Errorf("could not get RFQ address: %w", err)
}

Check warning on line 254 in services/rfq/relayer/limiter/limiter.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/relayer/limiter/limiter.go#L253-L254 

Added lines #L253 - L254 were not covered by tests

parser, err := fastbridge.NewParser(rfqAddr)
if err != nil {
return false, fmt.Errorf("could not create parser: %w", err)
}

Check warning on line 259 in services/rfq/relayer/limiter/limiter.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/relayer/limiter/limiter.go#L258-L259 

Added lines #L258 - L259 were not covered by tests

for _, log := range receipt.Logs {
_, parsedEvent, ok := parser.ParseEvent(*log)
if !ok {
continue

Check warning on line 264 in services/rfq/relayer/limiter/limiter.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/relayer/limiter/limiter.go#L264 

Added line #L264 was not covered by tests
}

event, ok := parsedEvent.(*fastbridge.FastBridgeBridgeRequested)
if ok {
return rfqFieldsMatch(request, event) && !log.Removed, nil
}
}
span.SetAttributes(
attribute.String("receipt txHash", receipt.TxHash.Hex()),
attribute.String("receipt log address", receipt.ContractAddress.String()),
attribute.String("receipt data", hex.EncodeToString(receipt.Logs[0].Data)),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Prevent potential index out-of-range panic when accessing receipt logs

Accessing receipt.Logs[0] without checking if receipt.Logs is non-empty can result in a runtime panic if the logs slice is empty. To ensure robustness, add a check to verify that receipt.Logs contains at least one entry before accessing it.

Apply this diff to add the necessary check:

245 + if len(receipt.Logs) > 0 {
245     attribute.String("receipt data", hex.EncodeToString(receipt.Logs[0].Data)),
245 + } else {
245 +     attribute.String("receipt data", "no logs available"),
245 + }
📝 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.

Suggested change
attribute.String("receipt data", hex.EncodeToString(receipt.Logs[0].Data)),
if len(receipt.Logs) > 0 {
attribute.String("receipt data", hex.EncodeToString(receipt.Logs[0].Data)),
} else {
attribute.String("receipt data", "no logs available"),
}

)
Comment on lines +275 to +276
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Check for empty receipt logs before accessing the first element

Accessing receipt.Logs[0] without verifying that receipt.Logs is not empty can lead to an index out-of-range panic if the slice is empty. Before accessing receipt.Logs[0].Data, ensure that there is at least one log entry in receipt.Logs.

Apply this diff to add the necessary check:

 span.SetAttributes(
     attribute.String("receipt txHash", receipt.TxHash.Hex()),
     attribute.String("receipt log address", receipt.ContractAddress.String()),
-    attribute.String("receipt data", hex.EncodeToString(receipt.Logs[0].Data)),
+    attribute.String("receipt data", func() string {
+        if len(receipt.Logs) > 0 {
+            return hex.EncodeToString(receipt.Logs[0].Data)
+        }
+        return "no logs available"
+    }()),
 )
📝 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.

Suggested change
attribute.String("receipt data", hex.EncodeToString(receipt.Logs[0].Data)),
)
span.SetAttributes(
attribute.String("receipt txHash", receipt.TxHash.Hex()),
attribute.String("receipt log address", receipt.ContractAddress.String()),
attribute.String("receipt data", func() string {
if len(receipt.Logs) > 0 {
return hex.EncodeToString(receipt.Logs[0].Data)
}
return "no logs available"
}()),
)


return false, nil

Check warning on line 278 in services/rfq/relayer/limiter/limiter.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/relayer/limiter/limiter.go#L272-L278 

Added lines #L272 - L278 were not covered by tests
}
Comment on lines +230 to +279
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

New checkReceipt method enhances security, but needs a safeguard

The checkReceipt method is a valuable addition that verifies transaction receipts and handles potential reorg scenarios. The event parsing and field matching logic provide a robust check for transaction validity.

However, there's a potential issue:

Add a check before accessing log data to prevent a potential nil pointer dereference:

 span.SetAttributes(
     attribute.String("receipt txHash", receipt.TxHash.Hex()),
     attribute.String("receipt log address", receipt.ContractAddress.String()),
-    attribute.String("receipt data", hex.EncodeToString(receipt.Logs[0].Data)),
+    attribute.String("receipt data", func() string {
+        if len(receipt.Logs) > 0 {
+            return hex.EncodeToString(receipt.Logs[0].Data)
+        }
+        return "no logs available"
+    }()),
 )

This change will prevent a panic if the receipt has no logs, which could happen in certain edge cases.

📝 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.

Suggested change
// checkReceipt checks if the receipt exists and has the correct fields in the case a reorg happened.
func (l *limiterImpl) checkReceipt(ctx context.Context, request *reldb.QuoteRequest) (_ bool, err error) {
// Make sure receipt exists and has the correct fields in case of a reorg.
// Note: https://community.infura.io/t/does-eth-gettransactionreceipt-respond-to-re-orged-transactions/7765
// "You will get a tx receipt back but, as you note, there is a small chance of a reorg.
// In a re-org the shorter side chain (one block usually, two occasionally) will have all its tx’s reverted and
// placed back in the mempool. Calling eth_getTransactionReceipt at this point will return null, until the tx is
// added to a new block and validated."
ctx, span := l.metrics.Tracer().Start(ctx, "limiter.checkReceipt")
defer func() {
metrics.EndSpanWithErr(span, err)
}()
receipt, err := l.evmClient.TransactionReceipt(ctx, request.OriginTxHash)
if err != nil {
if errors.Is(err, ethereum.NotFound) {
return false, nil
}
return false, fmt.Errorf("could not get transaction receipt: %w", err)
}
rfqAddr, err := l.cfg.GetRFQAddress(int(request.Transaction.OriginChainId))
if err != nil {
return false, fmt.Errorf("could not get RFQ address: %w", err)
}
parser, err := fastbridge.NewParser(rfqAddr)
if err != nil {
return false, fmt.Errorf("could not create parser: %w", err)
}
for _, log := range receipt.Logs {
_, parsedEvent, ok := parser.ParseEvent(*log)
if !ok {
continue
}
event, ok := parsedEvent.(*fastbridge.FastBridgeBridgeRequested)
if ok {
return rfqFieldsMatch(request, event) && !log.Removed, nil
}
}
span.SetAttributes(
attribute.String("receipt txHash", receipt.TxHash.Hex()),
attribute.String("receipt log address", receipt.ContractAddress.String()),
attribute.String("receipt data", hex.EncodeToString(receipt.Logs[0].Data)),
)
return false, nil
}
// checkReceipt checks if the receipt exists and has the correct fields in the case a reorg happened.
func (l *limiterImpl) checkReceipt(ctx context.Context, request *reldb.QuoteRequest) (_ bool, err error) {
// Make sure receipt exists and has the correct fields in case of a reorg.
// Note: https://community.infura.io/t/does-eth-gettransactionreceipt-respond-to-re-orged-transactions/7765
// "You will get a tx receipt back but, as you note, there is a small chance of a reorg.
// In a re-org the shorter side chain (one block usually, two occasionally) will have all its tx's reverted and
// placed back in the mempool. Calling eth_getTransactionReceipt at this point will return null, until the tx is
// added to a new block and validated."
ctx, span := l.metrics.Tracer().Start(ctx, "limiter.checkReceipt")
defer func() {
metrics.EndSpanWithErr(span, err)
}()
receipt, err := l.evmClient.TransactionReceipt(ctx, request.OriginTxHash)
if err != nil {
if errors.Is(err, ethereum.NotFound) {
return false, nil
}
return false, fmt.Errorf("could not get transaction receipt: %w", err)
}
rfqAddr, err := l.cfg.GetRFQAddress(int(request.Transaction.OriginChainId))
if err != nil {
return false, fmt.Errorf("could not get RFQ address: %w", err)
}
parser, err := fastbridge.NewParser(rfqAddr)
if err != nil {
return false, fmt.Errorf("could not create parser: %w", err)
}
for _, log := range receipt.Logs {
_, parsedEvent, ok := parser.ParseEvent(*log)
if !ok {
continue
}
event, ok := parsedEvent.(*fastbridge.FastBridgeBridgeRequested)
if ok {
return rfqFieldsMatch(request, event) && !log.Removed, nil
}
}
span.SetAttributes(
attribute.String("receipt txHash", receipt.TxHash.Hex()),
attribute.String("receipt log address", receipt.ContractAddress.String()),
attribute.String("receipt data", func() string {
if len(receipt.Logs) > 0 {
return hex.EncodeToString(receipt.Logs[0].Data)
}
return "no logs available"
}()),
)
return false, nil
}
🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 245-248: services/rfq/relayer/limiter/limiter.go#L245-L248
Added lines #L245 - L248 were not covered by tests

[warning] 253-254: services/rfq/relayer/limiter/limiter.go#L253-L254
Added lines #L253 - L254 were not covered by tests

[warning] 258-259: services/rfq/relayer/limiter/limiter.go#L258-L259
Added lines #L258 - L259 were not covered by tests

[warning] 264-264: services/rfq/relayer/limiter/limiter.go#L264
Added line #L264 was not covered by tests

[warning] 272-278: services/rfq/relayer/limiter/limiter.go#L272-L278
Added lines #L272 - L278 were not covered by tests


// TODO: is this exhaustive?
func rfqFieldsMatch(request *reldb.QuoteRequest, event *fastbridge.FastBridgeBridgeRequested) bool {
transactionIDMatch := request.TransactionID == event.TransactionId
senderMatch := request.Sender.String() == event.Sender.String()
originAmountMatch := request.Transaction.OriginAmount.String() == event.OriginAmount.String()
destAmountMatch := request.Transaction.DestAmount.String() == event.DestAmount.String()
originTokenMatch := request.Transaction.OriginToken.String() == event.OriginToken.String()

return transactionIDMatch && senderMatch && originAmountMatch && destAmountMatch && originTokenMatch
}
Comment on lines +281 to +290
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

rfqFieldsMatch function is not exhaustive

The rfqFieldsMatch function currently verifies transaction ID, sender, origin amount, destination amount, and origin token. However, the BridgeRequested event includes additional fields that should also be matched to ensure comprehensive validation:

  1. Destination Chain ID (destChainId)
  2. Destination Token (destToken)
  3. Send Chain Gas (sendChainGas)

Please update the rfqFieldsMatch function to include these fields to address the TODO comment and enhance the reliability of the verification process.

🔗 Analysis chain

New rfqFieldsMatch function is comprehensive, but needs review

The rfqFieldsMatch function provides a thorough comparison of RFQ fields with event data, which is crucial for verifying the integrity of the transaction. The checks cover all essential fields: transaction ID, sender, amounts, and token.

However, there's an open question:

The TODO comment suggests that the field matching might not be exhaustive. Please review if there are any additional fields that should be checked. Consider adding checks for:

  1. Destination chain ID
  2. Destination token
  3. Recipient address

If these fields are not available in the event data, update the comment to explain why the current checks are sufficient.

This will help ensure that all necessary fields are being verified and improve the robustness of the check.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for the BridgeRequested event definition to verify available fields
ast-grep --lang solidity --pattern 'event BridgeRequested(
  $$$
)'

Length of output: 352

Script:

#!/bin/bash
# Search for the BridgeRequested event definition to verify available fields
ast-grep --lang solidity --pattern 'event BridgeRequested(
  $$$
)'

Length of output: 186

Script:

#!/bin/bash
# Search for the BridgeRequested event definition in Solidity files
rg 'event BridgeRequested\(' --glob '*.sol'

Length of output: 648

Script:

#!/bin/bash
# Extract full definitions of the BridgeRequested event in Solidity files
rg -A 10 'event BridgeRequested\(' --glob '*.sol'

Length of output: 6623

Loading
Loading