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

proposed mods for feat/guard-v2 [SLT-422] #3364

Merged
merged 11 commits into from
Nov 6, 2024
Merged

proposed mods for feat/guard-v2 [SLT-422] #3364

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
494e841
relabeling & using v2 events/parsing for both versions
parodime Nov 2, 2024
5d9844f
lint
parodime Nov 2, 2024
e4c6e21
typo
parodime Nov 2, 2024
5f162af
addtl nil check on deref
parodime Nov 2, 2024
bce4c82
prove multicalled relays [SLT-422]
parodime Nov 4, 2024
6d58acd
runChainListener tweak. Parse proof relabel
parodime Nov 4, 2024
9ab744a
removing fastbridgev1 BridgeRequested & refs
parodime Nov 4, 2024
c8139b9
test probe - revert guard listener label
parodime Nov 5, 2024
50ac219
restore legacy listener "guard" label. Establish "guardV2" for new ve…
parodime Nov 5, 2024
a4346c4
poke to rerun w/o explorer check
parodime Nov 6, 2024
62320d0
removing dev comments
parodime Nov 6, 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
22 changes: 11 additions & 11 deletions services/rfq/guard/guardconfig/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,10 @@

// ChainConfig represents the configuration for a chain.
type ChainConfig struct {
// RFQAddressV1 is the rfq bridge contract address.
// RFQAddressV1 is the legacy V1 rfq bridge contract address. OPTIONAL. Only populate if also guarding a deprecated V1 contract.
RFQAddressV1 *string `yaml:"rfq_address_v1"`
// RFQAddressV2 is the rfq bridge contract address.
RFQAddressV2 string `yaml:"rfq_address_v2"`
// RFQAddress is the current/latest rfq bridge contract address. REQUIRED.
RFQAddress string `yaml:"rfq_address"`
Comment on lines +37 to +40
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

Update required: RFQAddressV2 references need to be modified

The consolidation of RFQAddress fields requires corresponding updates to the codebase. Several files still contain references to RFQAddressV2:

  • services/rfq/guard/guardconfig/config.go: 3 references including the method GetRFQAddressV2
  • services/rfq/guard/service/handlers.go: 1 reference in bridge request handling
  • services/rfq/guard/service/guard.go: 2 references in contract address retrieval

These references should be updated to use the new consolidated RFQAddress field to maintain consistency with the configuration changes.

🔗 Analysis chain

LGTM! Verify impact of RFQAddress consolidation.

The struct changes improve clarity by marking V1 as legacy/optional and consolidating the primary RFQAddress field. This aligns well with the V2 transition.

Let's verify no direct references to the old RFQAddressV2 field remain:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for any remaining references to RFQAddressV2
rg "RFQAddressV2" --type go

Length of output: 620

Script:

#!/bin/bash
# Let's check the changes in the PR to see if these references are being updated
git diff HEAD^ HEAD -- services/rfq/guard/guardconfig/config.go services/rfq/guard/service/handlers.go services/rfq/guard/service/guard.go

Length of output: 923

Script:

#!/bin/bash
# Let's check if there are any other files in the PR that might be updating these references
git diff --name-only HEAD^ HEAD | grep -i rfq

Length of output: 88

// Confirmations is the number of required confirmations.
Confirmations uint64 `yaml:"confirmations"`
}
Expand Down Expand Up @@ -70,17 +70,17 @@
for chainID := range c.Chains {
addrV1, err := c.GetRFQAddressV1(chainID)
if err != nil {
return fmt.Errorf("could not get rfq address: %w", err)
return fmt.Errorf("could not get v1 rfq address: %w", err)

Check warning on line 73 in services/rfq/guard/guardconfig/config.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/guard/guardconfig/config.go#L73 

Added line #L73 was not covered by tests
}
if addrV1 != nil && !common.IsHexAddress(*addrV1) {
return fmt.Errorf("invalid rfq address: %s", *addrV1)
return fmt.Errorf("invalid rfq v1 address: %s", *addrV1)

Check warning on line 76 in services/rfq/guard/guardconfig/config.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/guard/guardconfig/config.go#L76 

Added line #L76 was not covered by tests
}
addrV2, err := c.GetRFQAddressV2(chainID)
if err != nil {
return fmt.Errorf("could not get rfq address: %w", err)
return fmt.Errorf("could not get v2 rfq address: %w", err)

Check warning on line 80 in services/rfq/guard/guardconfig/config.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/guard/guardconfig/config.go#L80 

Added line #L80 was not covered by tests
}
if !common.IsHexAddress(addrV2) {
return fmt.Errorf("invalid rfq address: %s", addrV2)
return fmt.Errorf("invalid rfq v2 address: %s", addrV2)

Check warning on line 83 in services/rfq/guard/guardconfig/config.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/guard/guardconfig/config.go#L83 

Added line #L83 was not covered by tests
}
}

Expand All @@ -96,7 +96,7 @@
func (c Config) GetRFQAddressV1(chainID int) (*string, error) {
chainCfg, ok := c.Chains[chainID]
if !ok {
return nil, fmt.Errorf("chain config not found for chain %d", chainID)
return nil, fmt.Errorf("v1 chain config not found for chain %d", chainID)

Check warning on line 99 in services/rfq/guard/guardconfig/config.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/guard/guardconfig/config.go#L99 

Added line #L99 was not covered by tests
}
return chainCfg.RFQAddressV1, nil
}
Expand All @@ -105,9 +105,9 @@
func (c Config) GetRFQAddressV2(chainID int) (string, error) {
chainCfg, ok := c.Chains[chainID]
if !ok {
return "", fmt.Errorf("chain config not found for chain %d", chainID)
return "", fmt.Errorf("v2 chain config not found for chain %d", chainID)

Check warning on line 108 in services/rfq/guard/guardconfig/config.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/guard/guardconfig/config.go#L108 

Added line #L108 was not covered by tests
}
return chainCfg.RFQAddressV2, nil
return chainCfg.RFQAddress, nil

Check warning on line 110 in services/rfq/guard/guardconfig/config.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/guard/guardconfig/config.go#L110 

Added line #L110 was not covered by tests
Comment on lines +108 to +110
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

Consider renaming GetRFQAddressV2.

Since this method now returns the main RFQAddress and not a V2-specific address, the method name is misleading.

Consider this change:

-func (c Config) GetRFQAddressV2(chainID int) (string, error) {
+func (c Config) GetRFQAddress(chainID int) (string, error) {

Committable suggestion skipped: line range outside the PR's diff.

}

const defaultDBSelectorIntervalSeconds = 1
Expand All @@ -134,7 +134,7 @@
for chainID, chainCfg := range relayerCfg.GetChains() {
cfg.Chains[chainID] = ChainConfig{
RFQAddressV1: chainCfg.RFQAddressV1,
RFQAddressV2: chainCfg.RFQAddress,
RFQAddress: chainCfg.RFQAddress,

Check warning on line 137 in services/rfq/guard/guardconfig/config.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/guard/guardconfig/config.go#L137 

Added line #L137 was not covered by tests
Confirmations: chainCfg.FinalityConfirmations,
}
}
Expand Down
98 changes: 8 additions & 90 deletions services/rfq/guard/service/guard.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@
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()), metricHandler, listener.WithName("guard"))
chainListener, err := listener.NewChainListener(chainClient, store, common.HexToAddress(*rfqAddrV1), uint64(startBlock.Int64()), metricHandler, listener.WithName("guardListenerLegacy"))

Check warning on line 85 in services/rfq/guard/service/guard.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/guard/service/guard.go#L85 

Added line #L85 was not covered by tests
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure if we need to modify the existing listener name - this might require some additional DB maintenance if implemented. Would defer to @dwasse / @trajan0x here

if err != nil {
return nil, fmt.Errorf("could not get chain listener: %w", err)
}
Expand All @@ -107,7 +107,7 @@
if err != nil {
return nil, fmt.Errorf("could not get deploy block: %w", err)
}
chainListener, err := listener.NewChainListener(chainClient, store, common.HexToAddress(rfqAddrV2), uint64(startBlock.Int64()), metricHandler, listener.WithName("guard"))
chainListener, err := listener.NewChainListener(chainClient, store, common.HexToAddress(rfqAddrV2), uint64(startBlock.Int64()), metricHandler, listener.WithName("guardListener"))

Check warning on line 110 in services/rfq/guard/service/guard.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/guard/service/guard.go#L110 

Added line #L110 was not covered by tests
Copy link
Collaborator

Choose a reason for hiding this comment

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

Might want to do the versioned naming here (e.g. guardV2)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

my thought was this would eventually be "the" listener (ie: the only listener) and will be simpler to remove the "Legacy" one when we're done with it

Copy link
Collaborator

Choose a reason for hiding this comment

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

@parodime the chain listener is using its name to store its unique "latest handled block" in the DB:

sanguine/ethergo/listener/db/store.go

Lines 29 to 59 in 7c5c4f1

// PutLatestBlock upserts the latest block into the database.
func (s *Store) PutLatestBlock(ctx context.Context, chainID, height uint64) error {
tx := s.db.WithContext(ctx).Clauses(clause.OnConflict{
Columns: []clause.Column{{Name: chainIDFieldName}, {Name: listenerNameFieldName}},
DoUpdates: clause.AssignmentColumns([]string{chainIDFieldName, blockNumberFieldName, listenerNameFieldName}),
}).Create(&LastIndexed{
ChainID: chainID,
BlockNumber: int(height),
ListenerName: s.listenerName,
})
if tx.Error != nil {
return fmt.Errorf("could not update latest block: %w", tx.Error)
}
return nil
}
// LatestBlockForChain gets the latest block for a chain.
func (s *Store) LatestBlockForChain(ctx context.Context, chainID uint64) (uint64, error) {
var lastIndexed LastIndexed
tx := s.db.WithContext(ctx).
Where(fmt.Sprintf("%s = ?", chainIDFieldName), chainID).
Where(fmt.Sprintf("%s = ?", listenerNameFieldName), s.listenerName).
First(&lastIndexed)
if tx.Error != nil {
if errors.Is(tx.Error, gorm.ErrRecordNotFound) {
return 0, ErrNoLatestBlockForChainID
}
return 0, fmt.Errorf("could not fetch latest block: %w", tx.Error)
}
return uint64(lastIndexed.BlockNumber), nil
}

So for me it seems that we may want to preserve the old listener name when a new one is added, hence guard and guardV2 suggestions (something that preserves the existing name; and something that will help to preserve the name whenever there's a v3).

if err != nil {
return nil, fmt.Errorf("could not get chain listener: %w", err)
}
Expand Down Expand Up @@ -210,7 +210,7 @@
}
if v1Addr != nil {
group.Go(func() error {
err := g.runChainIndexerV1(ctx, chainID)
err := g.runChainIndexer(ctx, chainID, g.listenersV1[chainID])
if err != nil {
return fmt.Errorf("could not runChainIndexer chain indexer for chain %d [v1]: %w", chainID, err)
}
Expand All @@ -219,7 +219,7 @@
}

group.Go(func() error {
err := g.runChainIndexerV2(ctx, chainID)
err := g.runChainIndexer(ctx, chainID, g.listenersV2[chainID])
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 nil listener before starting V2 chain indexer

The code starts the V2 chain indexer without verifying if the listener exists. If g.listenersV2[chainID] is nil, this could result in a runtime error. Consider adding a check to ensure the listener is initialized before starting the indexer.

You can add a nil check before starting the chain indexer:

if g.listenersV2[chainID] != nil {
    group.Go(func() error {
        err := g.runChainIndexer(ctx, chainID, g.listenersV2[chainID])
        if err != nil {
            return fmt.Errorf("could not runChainIndexer chain indexer for chain %d [v2]: %w", chainID, err)
        }
        return nil
    })
}

if err != nil {
return fmt.Errorf("could not runChainIndexer chain indexer for chain %d [v2]: %w", chainID, err)
}
Expand All @@ -236,66 +236,7 @@
}

//nolint:cyclop
func (g Guard) runChainIndexerV1(ctx context.Context, chainID int) (err error) {
chainListener := g.listenersV1[chainID]

parser, err := fastbridge.NewParser(chainListener.Address())
if err != nil {
return fmt.Errorf("could not parse: %w", err)
}

err = chainListener.Listen(ctx, func(parentCtx context.Context, log types.Log) (err error) {
et, parsedEvent, ok := parser.ParseEvent(log)
// handle unknown event
if !ok {
if len(log.Topics) != 0 {
logger.Warnf("unknown event %s", log.Topics[0])
}
return nil
}

ctx, span := g.metrics.Tracer().Start(parentCtx, fmt.Sprintf("handleLog-%s", et), trace.WithAttributes(
attribute.String(metrics.TxHash, log.TxHash.String()),
attribute.Int(metrics.Origin, chainID),
attribute.String(metrics.Contract, log.Address.String()),
attribute.String("block_hash", log.BlockHash.String()),
attribute.Int64("block_number", int64(log.BlockNumber)),
))

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

switch event := parsedEvent.(type) {
case *fastbridge.FastBridgeBridgeRequested:
err = g.handleBridgeRequestedLog(ctx, event, chainID)
if err != nil {
return fmt.Errorf("could not handle request: %w", err)
}
case *fastbridge.FastBridgeBridgeProofProvided:
err = g.handleProofProvidedLog(ctx, event, chainID)
if err != nil {
return fmt.Errorf("could not handle request: %w", err)
}
case *fastbridge.FastBridgeBridgeProofDisputed:
err = g.handleProofDisputedLog(ctx, event)
if err != nil {
return fmt.Errorf("could not handle request: %w", err)
}
}

return nil
})
if err != nil {
return fmt.Errorf("listener failed: %w", err)
}
return nil
}

//nolint:cyclop,gosec
func (g Guard) runChainIndexerV2(ctx context.Context, chainID int) (err error) {
chainListener := g.listenersV2[chainID]

func (g Guard) runChainIndexer(ctx context.Context, chainID int, chainListener listener.ContractListener) (err error) {
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

Add defensive nil check for chainListener

The function should verify that chainListener is not nil before proceeding to prevent potential runtime panics.

 func (g Guard) runChainIndexer(ctx context.Context, chainID int, chainListener listener.ContractListener) (err error) {
+    if chainListener == nil {
+        return fmt.Errorf("chain listener is nil for chain %d", chainID)
+    }
     parser, err := fastbridgev2.NewParser(chainListener.Address())
📝 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
func (g Guard) runChainIndexer(ctx context.Context, chainID int, chainListener listener.ContractListener) (err error) {
func (g Guard) runChainIndexer(ctx context.Context, chainID int, chainListener listener.ContractListener) (err error) {
if chainListener == nil {
return fmt.Errorf("chain listener is nil for chain %d", chainID)
}
parser, err := fastbridgev2.NewParser(chainListener.Address())
🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 239-239: services/rfq/guard/service/guard.go#L239
Added line #L239 was not covered by tests

parser, err := fastbridgev2.NewParser(chainListener.Address())
if err != nil {
return fmt.Errorf("could not parse: %w", err)
Expand Down Expand Up @@ -325,40 +266,17 @@

switch event := parsedEvent.(type) {
case *fastbridgev2.FastBridgeV2BridgeRequested:
v1Event := &fastbridge.FastBridgeBridgeRequested{
TransactionId: event.TransactionId,
Sender: event.Sender,
Request: event.Request,
DestChainId: event.DestChainId,
OriginToken: event.OriginToken,
DestToken: event.DestToken,
DestAmount: event.DestAmount,
SendChainGas: event.SendChainGas,
Raw: event.Raw,
}
err = g.handleBridgeRequestedLog(ctx, v1Event, chainID)
err = g.handleBridgeRequestedLog(ctx, event, chainID)

Check warning on line 269 in services/rfq/guard/service/guard.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/guard/service/guard.go#L269 

Added line #L269 was not covered by tests
if err != nil {
return fmt.Errorf("could not handle request: %w", err)
}
// following events match ABIs exactly, so no need to differentiate
case *fastbridgev2.FastBridgeV2BridgeProofProvided:
v1Event := &fastbridge.FastBridgeBridgeProofProvided{
TransactionId: event.TransactionId,
Relayer: event.Relayer,
TransactionHash: event.TransactionHash,
Raw: event.Raw,
}
err = g.handleProofProvidedLog(ctx, v1Event, chainID)
err = g.handleProofProvidedLog(ctx, event, chainID)

Check warning on line 274 in services/rfq/guard/service/guard.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/guard/service/guard.go#L274 

Added line #L274 was not covered by tests
if err != nil {
return fmt.Errorf("could not handle request: %w", err)
}
case *fastbridgev2.FastBridgeV2BridgeProofDisputed:
v1Event := &fastbridge.FastBridgeBridgeProofDisputed{
TransactionId: event.TransactionId,
Relayer: event.Relayer,
Raw: event.Raw,
}
err = g.handleProofDisputedLog(ctx, v1Event)
err = g.handleProofDisputedLog(ctx, event)

Check warning on line 279 in services/rfq/guard/service/guard.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/guard/service/guard.go#L279 

Added line #L279 was not covered by tests
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

Inconsistent parameters in handleProofDisputedLog call

Unlike handleBridgeRequestedLog and handleProofProvidedLog, the handleProofDisputedLog function is called without the chainID parameter. If chainID is required in handleProofDisputedLog, consider adding it to maintain consistency and ensure correct processing.

Apply this diff to include chainID in the function call:

-	err = g.handleProofDisputedLog(ctx, event)
+	err = g.handleProofDisputedLog(ctx, event, chainID)

Also, update the handleProofDisputedLog function signature to accept chainID if necessary.

Committable suggestion skipped: line range outside the PR's diff.

🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 279-279: services/rfq/guard/service/guard.go#L279
Added line #L279 was not covered by tests

if err != nil {
return fmt.Errorf("could not handle request: %w", err)
}
Comment on lines +269 to 282
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

Standardize event handler parameters and improve error messages.

The event handling code has two issues:

  1. handleProofDisputedLog is missing the chainID parameter unlike other handlers
  2. Error messages are generic and don't differentiate between event types

Apply this diff:

 case *fastbridgev2.FastBridgeV2BridgeRequested:
     err = g.handleBridgeRequestedLog(ctx, event, chainID)
     if err != nil {
-        return fmt.Errorf("could not handle request: %w", err)
+        return fmt.Errorf("could not handle bridge request event for chain %d: %w", chainID, err)
     }
 case *fastbridgev2.FastBridgeV2BridgeProofProvided:
     err = g.handleProofProvidedLog(ctx, event, chainID)
     if err != nil {
-        return fmt.Errorf("could not handle request: %w", err)
+        return fmt.Errorf("could not handle proof provided event for chain %d: %w", chainID, err)
     }
 case *fastbridgev2.FastBridgeV2BridgeProofDisputed:
-    err = g.handleProofDisputedLog(ctx, event)
+    err = g.handleProofDisputedLog(ctx, event, chainID)
     if err != nil {
-        return fmt.Errorf("could not handle request: %w", err)
+        return fmt.Errorf("could not handle proof disputed event for chain %d: %w", chainID, 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.

Suggested change
err = g.handleBridgeRequestedLog(ctx, event, chainID)
if err != nil {
return fmt.Errorf("could not handle request: %w", err)
}
// following events match ABIs exactly, so no need to differentiate
case *fastbridgev2.FastBridgeV2BridgeProofProvided:
v1Event := &fastbridge.FastBridgeBridgeProofProvided{
TransactionId: event.TransactionId,
Relayer: event.Relayer,
TransactionHash: event.TransactionHash,
Raw: event.Raw,
}
err = g.handleProofProvidedLog(ctx, v1Event, chainID)
err = g.handleProofProvidedLog(ctx, event, chainID)
if err != nil {
return fmt.Errorf("could not handle request: %w", err)
}
case *fastbridgev2.FastBridgeV2BridgeProofDisputed:
v1Event := &fastbridge.FastBridgeBridgeProofDisputed{
TransactionId: event.TransactionId,
Relayer: event.Relayer,
Raw: event.Raw,
}
err = g.handleProofDisputedLog(ctx, v1Event)
err = g.handleProofDisputedLog(ctx, event)
if err != nil {
return fmt.Errorf("could not handle request: %w", err)
}
err = g.handleBridgeRequestedLog(ctx, event, chainID)
if err != nil {
return fmt.Errorf("could not handle bridge request event for chain %d: %w", chainID, err)
}
case *fastbridgev2.FastBridgeV2BridgeProofProvided:
err = g.handleProofProvidedLog(ctx, event, chainID)
if err != nil {
return fmt.Errorf("could not handle proof provided event for chain %d: %w", chainID, err)
}
case *fastbridgev2.FastBridgeV2BridgeProofDisputed:
err = g.handleProofDisputedLog(ctx, event, chainID)
if err != nil {
return fmt.Errorf("could not handle proof disputed event for chain %d: %w", chainID, err)
}
🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 269-269: services/rfq/guard/service/guard.go#L269
Added line #L269 was not covered by tests

[warning] 274-274: services/rfq/guard/service/guard.go#L274
Added line #L274 was not covered by tests

[warning] 279-279: services/rfq/guard/service/guard.go#L279
Added line #L279 was not covered by tests

Expand Down
94 changes: 27 additions & 67 deletions services/rfq/guard/service/handlers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@

var maxRPCRetryTime = 15 * time.Second

func (g *Guard) handleBridgeRequestedLog(parentCtx context.Context, req *fastbridge.FastBridgeBridgeRequested, chainID int) (err error) {
func (g *Guard) handleBridgeRequestedLog(parentCtx context.Context, req *fastbridgev2.FastBridgeV2BridgeRequested, chainID int) (err error) {

Check warning on line 26 in services/rfq/guard/service/handlers.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/guard/service/handlers.go#L26 

Added line #L26 was not covered by tests
ChiTimesChi marked this conversation as resolved.
Show resolved Hide resolved
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

Add test coverage for V2 event handlers

The transition to FastBridgeV2 event types is a critical change that requires thorough testing to ensure correct event handling and maintain system reliability.

Would you like assistance in generating comprehensive test cases for these event handlers?

Also applies to: 70-70, 97-97

🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 25-25: services/rfq/guard/service/handlers.go#L25
Added line #L25 was not covered by tests

ctx, span := g.metrics.Tracer().Start(parentCtx, "handleBridgeRequestedLog-guard", trace.WithAttributes(
attribute.Int(metrics.Origin, chainID),
attribute.String("transaction_id", hexutil.Encode(req.TransactionId[:])),
Expand Down Expand Up @@ -68,7 +68,7 @@
}

//nolint:gosec
func (g *Guard) handleProofProvidedLog(parentCtx context.Context, event *fastbridge.FastBridgeBridgeProofProvided, chainID int) (err error) {
func (g *Guard) handleProofProvidedLog(parentCtx context.Context, event *fastbridgev2.FastBridgeV2BridgeProofProvided, chainID int) (err error) {

Check warning on line 71 in services/rfq/guard/service/handlers.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/guard/service/handlers.go#L71 

Added line #L71 was not covered by tests
ctx, span := g.metrics.Tracer().Start(parentCtx, "handleProofProvidedLog-guard", trace.WithAttributes(
attribute.Int(metrics.Origin, chainID),
attribute.String("transaction_id", hexutil.Encode(event.TransactionId[:])),
Expand All @@ -95,7 +95,7 @@
return nil
}

func (g *Guard) handleProofDisputedLog(parentCtx context.Context, event *fastbridge.FastBridgeBridgeProofDisputed) (err error) {
func (g *Guard) handleProofDisputedLog(parentCtx context.Context, event *fastbridgev2.FastBridgeV2BridgeProofDisputed) (err error) {

Check warning on line 98 in services/rfq/guard/service/handlers.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/guard/service/handlers.go#L98 

Added line #L98 was not covered by tests
ctx, span := g.metrics.Tracer().Start(parentCtx, "handleProofDisputedLog-guard", trace.WithAttributes(
attribute.String("transaction_id", hexutil.Encode(event.TransactionId[:])),
))
Expand Down Expand Up @@ -211,6 +211,7 @@

//nolint:cyclop
func (g *Guard) isProveValid(ctx context.Context, proven *guarddb.PendingProven, bridgeRequest *guarddb.BridgeRequest) (bool, error) {

Check warning on line 214 in services/rfq/guard/service/handlers.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/guard/service/handlers.go#L214 

Added line #L214 was not covered by tests
// get the receipt for this tx on dest chain
chainClient, err := g.client.GetChainClient(ctx, int(bridgeRequest.Transaction.DestChainId))
if err != nil {
Expand All @@ -225,83 +226,38 @@
return false, fmt.Errorf("could not get receipt: %w", err)
}

var valid bool
var rfqContractAddr string

Check warning on line 230 in services/rfq/guard/service/handlers.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/guard/service/handlers.go#L229-L230 

Added lines #L229 - L230 were not covered by tests
if g.isV2Address(int(bridgeRequest.Transaction.OriginChainId), proven.FastBridgeAddress) {
valid, err = g.isProveValidV2(ctx, proven, bridgeRequest, receipt)
rfqContractAddr, err = g.cfg.GetRFQAddressV2(int(bridgeRequest.Transaction.DestChainId))

Check warning on line 232 in services/rfq/guard/service/handlers.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/guard/service/handlers.go#L232 

Added line #L232 was not covered by tests
if err != nil {
return false, fmt.Errorf("could not check prove validity v2: %w", err)
return false, fmt.Errorf("could not get rfq address v2: %w", err)

Check warning on line 234 in services/rfq/guard/service/handlers.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/guard/service/handlers.go#L234 

Added line #L234 was not covered by tests
}
} else {
valid, err = g.isProveValidV1(ctx, proven, bridgeRequest, receipt)
v1addr, err := g.cfg.GetRFQAddressV1(int(bridgeRequest.Transaction.DestChainId))

Check warning on line 237 in services/rfq/guard/service/handlers.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/guard/service/handlers.go#L237 

Added line #L237 was not covered by tests
if err != nil {
return false, fmt.Errorf("could not check prove validity v1: %w", err)
return false, fmt.Errorf("could not get rfq address v1: %w", err)
}
if v1addr == nil {
return false, fmt.Errorf("rfq address v1 is nil")

Check warning on line 242 in services/rfq/guard/service/handlers.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/guard/service/handlers.go#L239-L242 

Added lines #L239 - L242 were not covered by tests
}
rfqContractAddr = *v1addr

Check warning on line 244 in services/rfq/guard/service/handlers.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/guard/service/handlers.go#L244 

Added line #L244 was not covered by tests
}

return valid, nil
}

func (g *Guard) isProveValidV1(ctx context.Context, proven *guarddb.PendingProven, bridgeRequest *guarddb.BridgeRequest, receipt *types.Receipt) (bool, error) {
span := trace.SpanFromContext(ctx)
var valid bool

Check warning on line 247 in services/rfq/guard/service/handlers.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/guard/service/handlers.go#L247 

Added line #L247 was not covered by tests
valid, err = g.parseProvenTransaction(ctx, proven, bridgeRequest, receipt, rfqContractAddr)

rfqAddr, err := g.cfg.GetRFQAddressV1(int(bridgeRequest.Transaction.DestChainId))
if err != nil {
return false, fmt.Errorf("could not get rfq address v1: %w", err)
}
if rfqAddr == nil {
return false, errors.New("rfq address v1 is nil")
}
parser, err := fastbridge.NewParser(common.HexToAddress(*rfqAddr))
if err != nil {
return false, fmt.Errorf("could not get parser: %w", err)
return false, fmt.Errorf("could not parse proven transaction for validity: %w", err)
}

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

if log.Address != common.HexToAddress(*rfqAddr) {
span.AddEvent(fmt.Sprintf("log address %s does not match rfq address %s", log.Address.Hex(), *rfqAddr))
continue
}

event, ok := parsedEvent.(*fastbridge.FastBridgeBridgeRelayed)
if !ok {
span.AddEvent("event is not a BridgeRelayed event")
continue
}

if event.Relayer != proven.RelayerAddress {
span.AddEvent(fmt.Sprintf("relayer address %s does not match prover address %s", event.Relayer.Hex(), proven.RelayerAddress.Hex()))
continue
}

details := relayDetails{
TransactionID: event.TransactionId,
OriginAmount: event.OriginAmount,
DestAmount: event.DestAmount,
OriginChainID: event.OriginChainId,
To: event.To,
OriginToken: event.OriginToken,
DestToken: event.DestToken,
}

return relayMatchesBridgeRequest(details, bridgeRequest), nil
}

return false, nil
return valid, nil

Check warning on line 254 in services/rfq/guard/service/handlers.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/guard/service/handlers.go#L254 

Added line #L254 was not covered by tests
Comment on lines +246 to +253
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

Add test coverage for proof validation logic

The refactored proof validation logic is well-structured but lacks test coverage. This is critical functionality that should be thoroughly tested.

Would you like assistance in generating test cases for the proof validation scenarios?

Also applies to: 257-303

🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 247-247: services/rfq/guard/service/handlers.go#L247
Added line #L247 was not covered by tests

[warning] 254-254: services/rfq/guard/service/handlers.go#L254
Added line #L254 was not covered by tests

}

func (g *Guard) isProveValidV2(ctx context.Context, proven *guarddb.PendingProven, bridgeRequest *guarddb.BridgeRequest, receipt *types.Receipt) (bool, error) {
func (g *Guard) parseProvenTransaction(ctx context.Context, proven *guarddb.PendingProven, bridgeRequest *guarddb.BridgeRequest, receipt *types.Receipt, rfqContractAddr string) (bool, error) {
span := trace.SpanFromContext(ctx)

rfqAddr, err := g.cfg.GetRFQAddressV2(int(bridgeRequest.Transaction.DestChainId))
if err != nil {
return false, fmt.Errorf("could not get rfq address v2: %w", err)
}
parser, err := fastbridgev2.NewParser(common.HexToAddress(rfqAddr))
parser, err := fastbridgev2.NewParser(common.HexToAddress(rfqContractAddr))

Check warning on line 260 in services/rfq/guard/service/handlers.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/guard/service/handlers.go#L260 

Added line #L260 was not covered by tests
if err != nil {
return false, fmt.Errorf("could not get parser: %w", err)
}
Expand All @@ -312,8 +268,8 @@
continue
}

if log.Address != common.HexToAddress(rfqAddr) {
span.AddEvent(fmt.Sprintf("log address %s does not match rfq address %s", log.Address.Hex(), rfqAddr))
if log.Address != common.HexToAddress(rfqContractAddr) {
span.AddEvent(fmt.Sprintf("log address %s does not match rfq address %s", log.Address.Hex(), rfqContractAddr))

Check warning on line 272 in services/rfq/guard/service/handlers.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/guard/service/handlers.go#L271-L272 

Added lines #L271 - L272 were not covered by tests
continue
}

Expand All @@ -338,9 +294,13 @@
DestToken: event.DestToken,
}

return relayMatchesBridgeRequest(details, bridgeRequest), nil
// if we find a relay that matches the bridge, then we can return true. otherwise continue looking through any remaining logs.
if relayMatchesBridgeRequest(details, bridgeRequest) {
return true, nil
}
}

// if we have reached this point, then every log has been examined & none found suitable to validate the proof
return false, nil
}

Expand Down
Loading