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

Jump to bottom

RFQ relayer continues generating quotes if one fails #2264

Merged
merged 2 commits into from
Mar 15, 2024
Merged

RFQ relayer continues generating quotes if one fails #2264

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6d4d33e
Fix: make generateQuote() separate func, continue quoting on err
dwasse Mar 13, 2024
0dca5ef
Update services/rfq/relayer/quoter/quoter.go
trajan0x Mar 14, 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
4 changes: 4 additions & 0 deletions services/rfq/relayer/inventory/manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,10 @@
// the gas token may not be registered in the inventory tokens map,
// but it is always tracked in gasBalances.
if balance == nil && token == chain.EthAddress {
gasBalance, ok := i.gasBalances[chainID]
if !ok || gasBalance == nil {
return nil, fmt.Errorf("could not get gas balance for chain %d", chainID)
}

Check warning on line 90 in services/rfq/relayer/inventory/manager.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/relayer/inventory/manager.go#L87-L90 

Added lines #L87 - L90 were not covered by tests
Comment on lines +87 to +90
Copy link
Contributor

Choose a reason for hiding this comment

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

The added logic for handling gas balances when the gas token is not found in the inventory tokens map is a good improvement. It ensures that gas balances are properly managed, addressing a potential gap in the system's inventory management capabilities. However, it appears these changes are not covered by tests.

Ensuring test coverage for this new logic is crucial for validating its correctness and reliability. Consider adding unit tests that cover scenarios where the gas token is not found in the inventory tokens map, as well as cases where the gas balance is successfully retrieved or when it fails.

balance = i.gasBalances[chainID]
}
return balance, nil
Expand Down
120 changes: 70 additions & 50 deletions services/rfq/relayer/quoter/quoter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,73 +265,93 @@
// Essentially, if we know a destination chain token balance, then we just need to find which tokens are bridgeable to it.
// We can do this by looking at the quotableTokens map, and finding the key that matches the destination chain token.
// Generates quotes for a given chain ID, address, and balance.
func (m *Manager) generateQuotes(ctx context.Context, chainID int, address common.Address, balance *big.Int) ([]model.PutQuoteRequest, error) {
func (m *Manager) generateQuotes(parentCtx context.Context, chainID int, address common.Address, balance *big.Int) (quotes []model.PutQuoteRequest, err error) {
ctx, span := m.metricsHandler.Tracer().Start(parentCtx, "generateQuotes", trace.WithAttributes(
attribute.Int(metrics.Origin, chainID),
attribute.String("address", address.String()),
attribute.String("balance", balance.String()),
))
defer func() {
metrics.EndSpanWithErr(span, err)
}()

destRFQAddr, err := m.config.GetRFQAddress(chainID)
if err != nil {
return nil, fmt.Errorf("error getting destination RFQ address: %w", err)
}

destTokenID := fmt.Sprintf("%d-%s", chainID, address.Hex())

var quotes []model.PutQuoteRequest
quotes = []model.PutQuoteRequest{}
for keyTokenID, itemTokenIDs := range m.quotableTokens {
for _, tokenID := range itemTokenIDs {
//nolint:nestif
if tokenID == destTokenID {
// Parse token info
originStr := strings.Split(keyTokenID, "-")[0]
origin, err := strconv.Atoi(originStr)
if err != nil {
return nil, fmt.Errorf("error converting origin chainID: %w", err)
}
originTokenAddr := common.HexToAddress(strings.Split(keyTokenID, "-")[1])

// Calculate the quote amount for this route
quoteAmount, err := m.getQuoteAmount(ctx, origin, chainID, address, balance)
// don't quote if gas exceeds quote
if errors.Is(err, errMinGasExceedsQuoteAmount) {
quoteAmount = big.NewInt(0)
} else if err != nil {
return nil, err
}

// Calculate the fee for this route
destToken, err := m.config.GetTokenName(uint32(chainID), address.Hex())
if err != nil {
return nil, fmt.Errorf("error getting dest token ID: %w", err)
}
fee, err := m.feePricer.GetTotalFee(ctx, uint32(origin), uint32(chainID), destToken, true)
if err != nil {
return nil, fmt.Errorf("error getting total fee: %w", err)
}
originRFQAddr, err := m.config.GetRFQAddress(origin)
if err != nil {
return nil, fmt.Errorf("error getting RFQ address: %w", err)
}

// Build the quote
destAmount, err := m.getDestAmount(ctx, quoteAmount, chainID)
if err != nil {
return nil, fmt.Errorf("error getting dest amount: %w", err)
}
quote := model.PutQuoteRequest{
OriginChainID: origin,
OriginTokenAddr: originTokenAddr.Hex(),
DestChainID: chainID,
DestTokenAddr: address.Hex(),
DestAmount: destAmount.String(),
MaxOriginAmount: quoteAmount.String(),
FixedFee: fee.String(),
OriginFastBridgeAddress: originRFQAddr,
DestFastBridgeAddress: destRFQAddr,
quote, quoteErr := m.generateQuote(ctx, keyTokenID, chainID, address, balance, destRFQAddr)
if quoteErr != nil {
// continue generating quotes even if one fails
span.AddEvent("error generating quote", trace.WithAttributes(
attribute.String("key_token_id", keyTokenID),
))
continue

Check warning on line 295 in services/rfq/relayer/quoter/quoter.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/relayer/quoter/quoter.go#L291-L295 

Added lines #L291 - L295 were not covered by tests
Comment on lines +291 to +295
Copy link
Contributor

Choose a reason for hiding this comment

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

The loop within generateQuotes that continues generating quotes even if one fails is a crucial improvement. However, the added lines were not covered by tests according to the static analysis hints. Ensuring comprehensive test coverage for this new logic is essential for validating its correctness and reliability.

Would you like assistance in writing tests to cover the new logic introduced in the generateQuotes function? This could help ensure the robustness and reliability of the changes.

}
quotes = append(quotes, quote)
quotes = append(quotes, *quote)
}
}
}
return quotes, nil
}

func (m *Manager) generateQuote(ctx context.Context, keyTokenID string, chainID int, address common.Address, balance *big.Int, destRFQAddr string) (quote *model.PutQuoteRequest, err error) {
// Parse token info
originStr := strings.Split(keyTokenID, "-")[0]
origin, err := strconv.Atoi(originStr)
if err != nil {
return nil, fmt.Errorf("error converting origin chainID: %w", err)
}

Check warning on line 310 in services/rfq/relayer/quoter/quoter.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/relayer/quoter/quoter.go#L309-L310 

Added lines #L309 - L310 were not covered by tests
Comment on lines +310 to +311
Copy link
Contributor

Choose a reason for hiding this comment

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

Several error handling blocks within the generateQuote function are not covered by tests, as indicated by static analysis hints. It's crucial to have tests that cover these scenarios to ensure the function behaves as expected under various error conditions.

I can help create tests that cover the error handling scenarios within the generateQuote function. Ensuring these paths are tested will improve the overall reliability of the quote generation process.

Also applies to: 319-320, 325-326, 329-330, 333-334, 339-340

originTokenAddr := common.HexToAddress(strings.Split(keyTokenID, "-")[1])

// Calculate the quote amount for this route
quoteAmount, err := m.getQuoteAmount(ctx, origin, chainID, address, balance)
// don't quote if gas exceeds quote
if errors.Is(err, errMinGasExceedsQuoteAmount) {
quoteAmount = big.NewInt(0)
} else if err != nil {
return nil, err
}

Check warning on line 320 in services/rfq/relayer/quoter/quoter.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/relayer/quoter/quoter.go#L319-L320 

Added lines #L319 - L320 were not covered by tests

// Calculate the fee for this route
destToken, err := m.config.GetTokenName(uint32(chainID), address.Hex())
if err != nil {
return nil, fmt.Errorf("error getting dest token ID: %w", err)
}

Check warning on line 326 in services/rfq/relayer/quoter/quoter.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/relayer/quoter/quoter.go#L325-L326 

Added lines #L325 - L326 were not covered by tests
fee, err := m.feePricer.GetTotalFee(ctx, uint32(origin), uint32(chainID), destToken, true)
if err != nil {
return nil, fmt.Errorf("error getting total fee: %w", err)
}

Check warning on line 330 in services/rfq/relayer/quoter/quoter.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/relayer/quoter/quoter.go#L329-L330 

Added lines #L329 - L330 were not covered by tests
originRFQAddr, err := m.config.GetRFQAddress(origin)
if err != nil {
return nil, fmt.Errorf("error getting RFQ address: %w", err)
}

Check warning on line 334 in services/rfq/relayer/quoter/quoter.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/relayer/quoter/quoter.go#L333-L334 

Added lines #L333 - L334 were not covered by tests

// Build the quote
destAmount, err := m.getDestAmount(ctx, quoteAmount, chainID)
if err != nil {
return nil, fmt.Errorf("error getting dest amount: %w", err)
}

Check warning on line 340 in services/rfq/relayer/quoter/quoter.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/relayer/quoter/quoter.go#L339-L340 

Added lines #L339 - L340 were not covered by tests
quote = &model.PutQuoteRequest{
OriginChainID: origin,
OriginTokenAddr: originTokenAddr.Hex(),
DestChainID: chainID,
DestTokenAddr: address.Hex(),
DestAmount: destAmount.String(),
MaxOriginAmount: quoteAmount.String(),
FixedFee: fee.String(),
OriginFastBridgeAddress: originRFQAddr,
DestFastBridgeAddress: destRFQAddr,
}
return quote, nil
}
trajan0x marked this conversation as resolved.
Show resolved Hide resolved

// getQuoteAmount calculates the quote amount for a given route.
func (m *Manager) getQuoteAmount(parentCtx context.Context, origin, dest int, address common.Address, balance *big.Int) (quoteAmount *big.Int, err error) {
ctx, span := m.metricsHandler.Tracer().Start(parentCtx, "getQuoteAmount", trace.WithAttributes(
Expand Down
Loading