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.

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
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
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
115 changes: 95 additions & 20 deletions services/rfq/relayer/limiter/limiter.go
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,23 +39,25 @@
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,
}
}

Expand All @@ -63,28 +70,37 @@
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, allow.
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
}

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
}

func (l *limiterImpl) hasEnoughConfirmations(ctx context.Context, request *reldb.QuoteRequest) (_ bool, err error) {
Expand All @@ -98,10 +114,7 @@
return false, fmt.Errorf("could not get block number: %w", err)
}

requiredConfirmations, err := l.cfg.GetFinalityConfirmations(int(request.Transaction.OriginChainId))
if err != nil {
return false, fmt.Errorf("could not get required confirmations from config: %w", err)
}
requiredConfirmations := l.cfg.GetLimitConfirmations(int(request.Transaction.OriginChainId))
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

Handle potential errors when retrieving required confirmations

In the hasEnoughConfirmations function, the retrieval of requiredConfirmations does not handle potential errors from l.cfg.GetLimitConfirmations(). If this function can return an error, it should be properly handled to prevent unexpected behavior.

Modify the code to handle the error:

117 - requiredConfirmations := l.cfg.GetLimitConfirmations(int(request.Transaction.OriginChainId))
+ requiredConfirmations, err := l.cfg.GetLimitConfirmations(int(request.Transaction.OriginChainId))
+ if err != nil {
+     return false, fmt.Errorf("could not get required confirmations: %w", err)
+ }

Committable suggestion was skipped due to low confidence.


actualConfirmations := currentBlockNumber - request.BlockNumber
hasEnoughConfirmations := actualConfirmations >= requiredConfirmations
Expand All @@ -117,8 +130,8 @@
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")
func (l *limiterImpl) isUnderVolumeLimit(ctx context.Context, request *reldb.QuoteRequest) (_ bool, err error) {
ctx, span := l.metrics.Tracer().Start(ctx, "limiter.underVolumeLimitLimit")
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

Correct the tracer name to reflect the function accurately

The tracer name in isUnderVolumeLimit is currently set to "limiter.underVolumeLimitLimit", which seems redundant and may lead to confusion. It should be corrected to "limiter.isUnderVolumeLimit" to match the function name and maintain consistency.

Apply this diff to update the tracer name:

 ctx, span := l.metrics.Tracer().Start(ctx, 
-    "limiter.underVolumeLimitLimit")
+    "limiter.isUnderVolumeLimit")
📝 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
ctx, span := l.metrics.Tracer().Start(ctx, "limiter.underVolumeLimitLimit")
ctx, span := l.metrics.Tracer().Start(ctx, "limiter.isUnderVolumeLimit")

defer func() {
metrics.EndSpanWithErr(span, err)
}()
Expand All @@ -134,14 +147,14 @@
return false, fmt.Errorf("could not get USD amount of token: %w", err)
}

withinSizeLimit := tokenPrice.Cmp(volumeLimit) < 0
underVolumeLimitLimit := 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", underVolumeLimitLimit),
)

return withinSizeLimit, nil
return underVolumeLimitLimit, nil
}

// getRequestVolumeOfToken returns the volume of the token in USD. This value is NOT human readable.
Expand Down Expand Up @@ -183,3 +196,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 218 in services/rfq/relayer/limiter/limiter.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/relayer/limiter/limiter.go#L215-L218

Added lines #L215 - L218 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 224 in services/rfq/relayer/limiter/limiter.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/relayer/limiter/limiter.go#L223-L224

Added lines #L223 - L224 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 229 in services/rfq/relayer/limiter/limiter.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/relayer/limiter/limiter.go#L228-L229

Added lines #L228 - L229 were not covered by tests

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

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

View check run for this annotation

Codecov / codecov/patch

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

Added line #L234 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 248 in services/rfq/relayer/limiter/limiter.go

View check run for this annotation

Codecov / codecov/patch

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

Added lines #L242 - L248 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